-
-
Notifications
You must be signed in to change notification settings - Fork 17.9k
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
BUG/Internals: maybe_promote #23833
Labels
Comments
gfyoung
added
Dtype Conversions
Unexpected or buggy dtype conversions
Algos
Non-arithmetic algos: value_counts, factorize, sorting, isin, clip, shift, diff
Bug
labels
Nov 21, 2018
Sounds like a plan. Go for it! |
This was referenced Nov 23, 2018
An extract of the bugs found by #23982. They are not closed by that PR, but should be solved in a follow-up.
|
This was referenced Jan 22, 2019
Closed
This was referenced Feb 20, 2019
mroeschke
removed
the
Algos
Non-arithmetic algos: value_counts, factorize, sorting, isin, clip, shift, diff
label
Jun 23, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Seems I found a pretty deep rabbit hole while trying to solve #23823 (while trying to solve #23192 / #23604):
maybe_upcast_putmask
andmaybe_promote
are both completely untested (or at least, their names do not appear anywhere inpandas/tests/
), andmaybe_promote
also does not have a docstring. Side note: ran into a segfault while trying to remove some old numpy compat code from that method in #23796.Aside from missing a docstring and tests, the behaviour is also false, at least regarding integer types:
To me, this should clearly upcast to
int16
instead offloat
(using arrays forfill_value
is correct usage, as done e.g. inmaybe_upcast_putmask
asmaybe_promote(result.dtype, other)
, and has a dedicated code branch inmaybe_promote
).In int-to-int promotion, the question is what to return as an actual
fill_value
though. Of course, this method is being used in pretty central code paths, but the number of uses is not that high (on master; half of the instances are imports/redefinitions).Therefore it might make sense to adapt the private API, e.g. adding a kwarg
must_hold_na
and/orreturn_default_na
. I've inspected all the occurrences of the code above, and this would not be a problem to implement.Once I get around to it, will probably split this into two PRs, one just for adding tests/docstring, and one to change...
The text was updated successfully, but these errors were encountered: