REGR: Series.nlargest with masked arrays #42838
Conversation
jbrockmendel
commented
Jul 31, 2021
jbrockmendel
commented
- closes BUG: nlargest raises TypeError "No matching signature found" on Float64Dtype Series, versions >1.3.0 #42816
- tests added / passed
- Ensure all linting tests pass, see here for how to run them
- whatsnew entry
jbrockmendel
added
NA - MaskedArrays
Co-authored-by: Simon Hawkins <simonjayhawkins@gmail.com>
| @@ -1255,6 +1259,18 @@ def compute(self, method: str) -> Series: | |||
|
|
|||
| dropped = self.obj.dropna() | |||
|
|
|||
| if is_extension_array_dtype(dropped.dtype): | |||
There was a problem hiding this comment.
why is this prefereable to hanlding on L1281 with the other dtypes?
There was a problem hiding this comment.
bc ensure_data does the wrong thing with MaskedArrays, np.asarray(obj) defaults to object dtype.
we could kludge _ensure_data to work in cases where we dont have any pd.NAs, but doing it here lets us handle cases with NAs too.
There was a problem hiding this comment.
bc ensure_data does the wrong thing with MaskedArrays, np.asarray(obj) defaults to object dtype.
this is very unfortunate. I don't really like this approach here. i suppose its ok for a backport though this is na experimental type and so i don't consider this regression to be a big deal.
There was a problem hiding this comment.
the more i work on it, the more i want to nuke NA from space
There was a problem hiding this comment.
ok what is involved with changing this to a non-recursive formulation though? e.g. ensure_data should be able to handle NA (if not we will have other issues)
There was a problem hiding this comment.
Three options:
- have ensure_data special-case MaskedArray cases with no NAs, in which case they can just use the ndarray. This fixes the regression (cases with NAs didnt use to work IIUC). kluuuuudge
- make algos.kth_smallest support object dtype (and not choke on pd.NA)
- make the EA implement its own nlargest
I decided the approach here was less kludgy than those in part bc this function uses obj.dropna(), so the MaskedArray case actually is much simpler to implement than an arbitrary EA
There was a problem hiding this comment.
bc its special-casing for MaskedArray and special casing for not values.isna().any()
There was a problem hiding this comment.
worse than that, it isnt not values.isna().any() but not values._mask.any()
|
not super thrilled with this, but does fix. |
|
@meeseeksdev backport 1.3.x |
|
Something went wrong ... Please have a look at my logs. |
| @@ -23,6 +23,7 @@ Fixed regressions | |||
| - Fixed regression where :meth:`pandas.read_csv` raised a ``ValueError`` when parameters ``names`` and ``prefix`` were both set to None (:issue:`42387`) | |||
| - Fixed regression in comparisons between :class:`Timestamp` object and ``datetime64`` objects outside the implementation bounds for nanosecond ``datetime64`` (:issue:`42794`) | |||
| - Fixed regression in :meth:`.Styler.highlight_min` and :meth:`.Styler.highlight_max` where ``pandas.NA`` was not successfully ignored (:issue:`42650`) | |||
| - Regression in :meth:`Series.nlargest` and :meth:`Series.nsmallest` with nullable integer or float dtype (:issue:`41816`) | |||
There was a problem hiding this comment.
will need to change to #42816. will do that after backport is merged.
