-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
API: require timezone match in DatetimeArray.shift #37299
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: require timezone match in DatetimeArray.shift #37299
Conversation
i guess this is a small api change (e.g. what did this do before)? can you add a whatsnew note (rebase as well) |
…i-shift_value-setitem
…i-shift_value-setitem
…i-shift_value-setitem
@jbrockmendel what's the rationale of requiring matching timezone here? (as long as tz-awareness matches) |
Trying to have some consistency about which methods require matching tz vs just tzawareness-compat. The heuristic is to require matching tz for setitem-like methods. |
Isn't that inconsisten with other operations like aritmetic/comparison ops, which allow it? (like the subtract PR that is also open) Also setitem on Series doesn't actually raise, so on the Series level, those methods are also not fully consistent. |
Right, those methods are not considered "setitem-like". searchsorted is also considered comparison-like as opposed to setitem-like.
The Series is cast to object and the value being set is not cast to the existing timezone. DatetimeBlock._can_hold_element is equivalent to
|
…i-shift_value-setitem
…i-shift_value-setitem
we should rationalize these one way or the other for all ops. I would be +1 on always being strict here, though this would break some existing code / tests. |
…i-shift_value-setitem
I'd be on board with being consistent across all these methods, but I think we would basically have to make it the non-strict version. Otherwise we would break consistency with the stdlib datetime on comparisons (xref #37329). |
thanks @jbrockmendel yeah I agree we need to make this consitent, but this PR itself is fine as its just requires a tz matching fill value. |
black pandas
git diff upstream/master -u -- "*.py" | flake8 --diff