-
-
Notifications
You must be signed in to change notification settings - Fork 18.5k
BUG: fix nanmedian for CoW without bottleneck #55742
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
if not values.flags.writeable: | ||
values = values.copy() |
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 think that in theory, this copy might not be needed, and we could also ignore the read-only flag instead. The case that could still end up here is the masked Float64Dtype, where you do pass float64 data together with a mask. Although in that case, setting the values under the mask to NaN should I think always be "safe" at the moment (I don't think we already allow sharing data but not mask between FloatingArrays). But that's maybe something we can optimize later for Float64Dtype when we have a better idea of its memory model (and if sharing of data is definitely never possible)
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.
Now, looking a bit more, in practice we don't actually get here for FloatingArray at the moment, for two reasons:
-
we don't yet mark the numpy arrays as read-only when accessing those (FloatingArray
_data
), so it's only for the numpy float64 dtype that we would get read-only data at the moment -
if there are missing values in the FloatArray, we actually already copy the values inside
_get_values
:Lines 312 to 316 in 984d755
if fill_value is not None: if mask.any(): if datetimelike or _na_ok_dtype(dtype): values = values.copy() np.putmask(values, mask, fill_value)
Now, that seems double effort, and something we should fix, but that's for another issue (we are also setting with a fill_value of 0 here, which doesn't seem to make sense in the case of median
, and then in nanmedian itself we either filter with the mask or set NaNs into it ..)
So I can also leave out this if not values.flags.writeable:
check, because right now it will not be covered by any test / case in pandas, I think (unless you would use nanmedian directly)
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.
this looks good to me
Fixing a failing test that turned up in #55732.
This PR specifically fixes
pandas/tests/frame/test_reductions.py::test_reduction_axis_none_returns_scalar[float64-True-median]
, i.e. callingdf.median(axis=None)
, for builds that don't have bottleneck installed.If the underlying data of the DataFrame or Series is float64 data, the
values
insidenanops.nanmedian(..)
will be the original values from the DataFrame, and with CoW set to be read-only (for other dtypes, we cast to float64 inside the function, and so that will always give a copy).And then we do
values[mask] = np.nan
, which raises an error if values is read-only. In practice, this didn't give any problems with mutating the original DataFrame, because if you start with float64 values, themask
always corresponds with NaN values already (so we were actually mutating the calling dataframe, just without any effect).