MAINT,ENH: Rename all dispatchers as well as possible #21667
Conversation
Median does not currently support Datetimes. This is because `mean` does not support datetimes. The reason for `mean` not supporting datetimes, is that it is (understandibly) written as `(a + b)/2`, but datetimes cannot be added so that we would have to use a different scheme, such as `a + (b - a) / 2`.
Unlike Median, the interpolation formula used by quantile/percentile does support Datetimes just fine, so this also adds basic tests for that. Due to the integral nature, times suffer round-off errors that seem not ideal. This is an issue, but distinct from the NaT one?
|
@seberg Needs a rebase (already :) |
This is a bit of a call and various decisions could be made. In some cases, I simply duplicated dispatchers to get the rigth name (there is no other way to rename the function). In other cases, I attempted to find a somewhat decent name (without underscore) that at least gives a better idea than having `_dispatcher` in there. I tried to standardize the two most common ones to `unary_operation` and `binary_operation`. It may be possible to auto-copy the function and replace the name in the decorator. In general (if there is a custom decorator), I think using the same name is probably best. I do not have a strong opinion about the cases where dispatchers are used multiple times though. E.g. when to duplicate and when not.
56de6b3 to
e8a110f
Compare
|
Bit from the sidelines, but would it not be simpler to try/except the call to the dispatcher and on error change the name of the function in the exception message? (or even just append a message noting the function that called the dispatcher). As is, I'm not sure that the reduction in code readability is worth the better error message. |
|
Hmmm, I had half discarded that thought, because the inspection seemed a bit annoying. However, we only have to inspect errors originating in the "dispatcher" itself, and those are much less likely to get a bunch of complicated errors their way, so the inspection is a bit less bad. (There could be rare cases where there is a TypeError not originating from the signature mismatch, but probably really rare.) Let me try that, I think you are likely right and that is just good. |
|
OK, lets consider the alternative in gh-21731 and close this. Thanks for the input! |
This is a bit of a call and various decisions could be made.
In some cases, I simply duplicated dispatchers to get the rigth name
(there is no other way to rename the function).
In other cases, I attempted to find a somewhat decent name (without
underscore) that at least gives a better idea than having
_dispatcherin there.I tried to standardize the two most common ones to
unary_operationand
binary_operation.It may be possible to auto-copy the function and replace the name
in the decorator. In general (if there is a custom decorator), I think
using the same name is probably best.
I do not have a strong opinion about the cases where dispatchers are
used multiple times though. E.g. when to duplicate and when not.
As a reminder, this fixes (mostly) the problem that:
gives:
which I think is potentially confusing to users. It thus mostly addreses gh-21647.
@shoyer since this was originally your work, could you have a look/review this?
(I am happy about other ideas, or at least reducing it to only fix places where the dispatcher is defined directly before the function.)
The following symbols have dispatcher names that do not match and are not either
unary_operationorbinary_operation: