Skip to content

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

Merged
merged 3 commits into from
Nov 17, 2023

Conversation

jorisvandenbossche
Copy link
Member

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. calling df.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 inside nanops.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, the mask always corresponds with NaN values already (so we were actually mutating the calling dataframe, just without any effect).

Comment on lines +794 to +795
if not values.flags.writeable:
values = values.copy()
Copy link
Member Author

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)

Copy link
Member Author

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:

  1. 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

  2. if there are missing values in the FloatArray, we actually already copy the values inside _get_values:

    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)

Copy link
Member

@phofl phofl left a 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

@phofl phofl merged commit 0255ab3 into pandas-dev:main Nov 17, 2023
@phofl
Copy link
Member

phofl commented Nov 17, 2023

thx @jorisvandenbossche

@jorisvandenbossche jorisvandenbossche deleted the cow-bug-nanmedian branch November 27, 2023 11:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants