REG: quantile with IntegerArray/FloatingArray #41428
Conversation
pandas/core/array_algos/quantile.py
Outdated
| return _quantile_ea_compat(values, qs, interpolation) | ||
| try: | ||
| out = _quantile_ea_compat(values, qs, interpolation) | ||
| except AbstractMethodError: |
There was a problem hiding this comment.
@jorisvandenbossche catching AbstractMethodError to determine if we need to use the fallback implementation isn't my favorite, but all the other options i can think of involve adding methods/attributes to EA. thoughts?
There was a problem hiding this comment.
what exactly is throwing this? can you test something about the EA itself?
There was a problem hiding this comment.
oh i c, you need to test if _from_factorized is implemented. then can you do that explicty?
There was a problem hiding this comment.
oh i c, you need to test if _from_factorized is implemented. then can you do that explicty?
i think to make that work we'd need to put quantile as an EA method (which id be OK with)
There was a problem hiding this comment.
I think I mentioned this before, but I would personally simply special case our own internal EAs. And then we have two cases: the ones backed by an ndarray (NDArrayBackedExtensionArray where you can access _ndarray), and the masked array ones (BaseMaskedArray where you can access _data and _mask).
(and thus not use _from_factorize at all here, as it was not meant for this use case)
There was a problem hiding this comment.
And then we have two cases
what about Interval (currently xfailed) and 3rd party?
but I would personally simply special case our own internal EAs
any objection to making a EA.quantile method with a reasonable default implementation? That lets us implement optimized NDArrayBackedExtensionArray and BaseMaskedArray implementations without special-casing internal EAs, which we usually try to avoid
There was a problem hiding this comment.
We can certainly discuss EA.quantile, but some of my concerns: it's not a general method (only relevant for some types, other reductions are also not as methods but through EA_reduce, and there is also no ndarray method equivalent as reason to add it.
(so therefore I think the easier short-term action is to special case it here. And it's not that this code will be lost, it's just a different location, because if we add the method, the code here can be split and moved to the different method overrides)
So personally I am not that fond on adding EA.quantile in itself, but it's certainly true that we still need to devise an interface that can be used for 3rd party EAs as well.
what about Interval (currently xfailed)
What would be the result for Interval? That's not generally a numeric dtype (we don't support other reductions like mean or median for interval)
There was a problem hiding this comment.
but some of my concerns: it's not a general method (only relevant for some types, other reductions are also not as methods but through EA_reduce, and there is also no ndarray method equivalent as reason to add it.
Would you be happier with this if it were EA._quantile?
What would be the result for Interval? That's not generally a numeric dtype (we don't support other reductions like mean or median for interval)
quantile (and median) only depend on ordering, which Interval does implement. So in principle nothing stops us from supporting it.
There was a problem hiding this comment.
ok so i guess let's do what joris is suggesting and then create an issue about adding EA.quantile (which i am +1)
pandas/core/array_algos/quantile.py
Outdated
| return _quantile_ea_compat(values, qs, interpolation) | ||
| try: | ||
| out = _quantile_ea_compat(values, qs, interpolation) | ||
| except AbstractMethodError: |
There was a problem hiding this comment.
what exactly is throwing this? can you test something about the EA itself?
pandas/core/array_algos/quantile.py
Outdated
| return _quantile_ea_compat(values, qs, interpolation) | ||
| try: | ||
| out = _quantile_ea_compat(values, qs, interpolation) | ||
| except AbstractMethodError: |
There was a problem hiding this comment.
oh i c, you need to test if _from_factorized is implemented. then can you do that explicty?
jbrockmendel
added
quantile
|
let's move this off the milestone for now |
|
updated as suggested; since its a regression we should try to get this in for the RC |
|
lgtm. needs a whatsnew note? |
no, the regression didnt make it into any released version |
|
cool, do we have an issue for discussion about this (eg. the api enhancement / consolidation of the ea_compat quantile methods) ? if not can you create one |
Not present in any released version, so no release note needed as long as before 1.3