-
-
Notifications
You must be signed in to change notification settings - Fork 18.6k
Min max sparse fillna #41691
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
Min max sparse fillna #41691
Conversation
taytzehao
commented
May 27, 2021
- closes BUG: self.fill_value ignored in SparseArray.min/max #41552
- tests added / passed
- Ensure all linting tests pass, see here for how to run them
- whatsnew entry
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.
Thanks for the pr @taytzehao! Couple quick comments
@@ -1326,6 +1326,8 @@ class TestMinMax: | |||
data_neg = plain_data * (-1) | |||
data_NaN = SparseArray(np.array([0, 1, 2, np.nan, 4])) | |||
data_all_NaN = SparseArray(np.array([np.nan, np.nan, np.nan, np.nan, np.nan])) | |||
data_NA_filled = SparseArray(np.array([np.nan, np.nan, np.nan, np.nan, np.nan])) | |||
data_NA_filled.fill_value = 5 |
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.
Can you just pass fill_value
in the constructor instead?
doc/source/whatsnew/v1.3.0.rst
Outdated
@@ -1081,6 +1081,7 @@ Sparse | |||
- Bug in :meth:`DataFrame.sparse.to_coo` raising ``KeyError`` with columns that are a numeric :class:`Index` without a 0 (:issue:`18414`) | |||
- Bug in :meth:`SparseArray.astype` with ``copy=False`` producing incorrect results when going from integer dtype to floating dtype (:issue:`34456`) | |||
- Implemented :meth:`SparseArray.max` and :meth:`SparseArray.min` (:issue:`40921`) | |||
- Return ``fill_value`` for :meth:`SparseArray.max` and :meth:`SparseArray.min` (:issue:`41552`) |
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.
Think this whatsnew is not needed since min/max was never released with this issue
I am not quite sure what this test failure is pointing towards. It is the same exact failure with my other pull request #41162 |
@jbrockmendel , do I need to change any configuration in order for it to pass? Cause I am not sure why is it so |
no, this looks like the CI being flaky for unrelated reasons. you can ignore it |
thanks @taytzehao |