-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
Fix indexing, reindex on all-sparse SparseArray. #35287
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Looks like #34908 built on the buggy functionality. I may need to revert it as well. |
@pytest.fixture(params=[0, np.nan]) | ||
def data_zeros(request): | ||
return SparseArray(np.zeros(100, dtype=int), fill_value=request.param) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this was doing anything on master.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a089991 hopefully has the proper fix.
@@ -399,31 +399,3 @@ def test_item(self, data): | |||
|
|||
with pytest.raises(ValueError, match=msg): | |||
s.item() | |||
|
|||
def test_boolean_mask_frame_fill_value(self, data): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed that these tests were passing regardless of the change, so I'm not sure they were testing what we expected. I've left them removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm. wasn't clear if you had additional changes. merge when ready.
Thanks, I think we're good here. |
Closes #35286.
Also added a regression tests for the issue reported there.