-
-
Notifications
You must be signed in to change notification settings - Fork 18k
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
API: Series[bool][key] = np.nan -> cast to object #38709
Conversation
…g-block-setitem
…g-block-setitem
…g-block-setitem
…g-block-setitem
…g-block-setitem
…g-block-setitem
…g-block-setitem
…g-block-setitem
…g-block-setitem
…g-block-setitem
…g-block-setitem
…g-block-setitem
…g-block-setitem
…g-block-setitem
yikes, the existing behavior is even worse than i thought:
This fixes that; ill add a test. gentle ping @jreback this is probably the toughest of the outstanding setitem-related PRs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am ok with this change as long as we have a sub-section in api breaking.
pandas/core/array_algos/putmask.py
Outdated
@@ -106,8 +106,11 @@ def putmask_smart(values: np.ndarray, mask: np.ndarray, new) -> np.ndarray: | |||
# preserves dtype if possible | |||
return _putmask_preserve(values, new, mask) | |||
|
|||
# change the dtype if needed | |||
dtype, _ = maybe_promote(new.dtype) | |||
if values.dtype == bool and new.dtype.kind == "f": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm, shouldn't maybe_promote handle this? (`maybe_promote(new.dtype, values.dtype) ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wouldnt find_common_type make more sense?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yah this is nicer, updated
added + green |
thanks, very nice |
way cool. between this and #39163 ill be un-blocked on unifying a bunch of casting behavior i think |
The appears to be a performance regression here https://pandas.pydata.org/speed/pandas/#frame_methods.MaskBool.time_frame_mask_bools There has been some improvement since.
profile
commit before
I guess this is expected? |
Can't comment on the size of the perf hit, but the existence is unsurprising since we're casting to object |
black pandas
git diff upstream/master -u -- "*.py" | flake8 --diff
sits on top of #38688