CLN: remove _try_coerce_result altogether #27683
Conversation
jreback
added
Clean
Internals
pandas/core/groupby/ops.py
Outdated
| @@ -452,6 +453,16 @@ def wrapper(*args, **kwargs): | |||
| def _cython_operation(self, kind, values, how, axis, min_count=-1, **kwargs): | |||
| assert kind in ["transform", "aggregate"] | |||
|
|
|||
| if is_datetime64tz_dtype(values): | |||
There was a problem hiding this comment.
you shouldn't do this here and rather handle at a higher level as datetime64 is caught below for some operations.
There was a problem hiding this comment.
later in this function it checks for datetime64 to exclude add/prod/cumsum/cumprod, but there is nowhere else in this module where we handle datetime64tz AFAICT
There was a problem hiding this comment.
my point is that this is not the right place to handle this. if you want to transform the input then let it pass thru or raise a NotImplementedError then that is fine, but the way you are writing it is is very diffferent.
There was a problem hiding this comment.
all aggregate and transform calls go through here, so this is the only place AFAICT that we can do this without having to duplicate it for every calling function. If there is a better place, can you be more specific?
There was a problem hiding this comment.
missing my point. I have no problem handling this in this function. What you wrote returns early, bypassing all of the logic that handles all of the other dtypes. So it needs to be integrated there OR handled at a higher level on a NotImplementedError raise.
This function becomes even harder to understand with this addition. In theory you could also split this function up more logically. There are a number of solutions.
There was a problem hiding this comment.
edited to do what I think you're suggesting.
the method could use a good refactor, agreed. separate pass.
pandas/core/groupby/ops.py
Outdated
| @@ -452,6 +453,16 @@ def wrapper(*args, **kwargs): | |||
| def _cython_operation(self, kind, values, how, axis, min_count=-1, **kwargs): | |||
| assert kind in ["transform", "aggregate"] | |||
|
|
|||
| if is_datetime64tz_dtype(values): | |||
There was a problem hiding this comment.
missing my point. I have no problem handling this in this function. What you wrote returns early, bypassing all of the logic that handles all of the other dtypes. So it needs to be integrated there OR handled at a higher level on a NotImplementedError raise.
This function becomes even harder to understand with this addition. In theory you could also split this function up more logically. There are a number of solutions.
|
@jreback is this closer to what you had in mind? |
| # TODO: possible need to reshape? kludge can be avoided when | ||
| # 2D EA is allowed. | ||
| values = values.view("M8[ns]") | ||
|
|
There was a problem hiding this comment.
can you move the is_datetimelike and is_numeric checks up here and de-duplicate as needed
There was a problem hiding this comment.
done. not much de-duplication available. in another pass we can separate out some of the casting from the everything-else and see if there is de-duplication available in algos or something
There was a problem hiding this comment.
ok fair, that routine likely needs some cleaning at some point
Following #27628, we can now remove _try_coerce_result altogether.
The step after this removes _try_cast_result.