-
-
Notifications
You must be signed in to change notification settings - Fork 18.1k
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: setitem with mixed-resolution dt64s #56419
BUG: setitem with mixed-resolution dt64s #56419
Conversation
what happens after this PR for the cases in #56410 ? would they emit a warning now, and raise in the future? |
The |
This pull request is stale because it has been open for thirty days with no activity. Please update and respond to this comment if you're still interested in working on this. |
@MarcoGorelli gentle ping |
if there are no further comments, i plan to merge this next week |
sorry for the delay, will try to take a look tomorrow |
Sorry this has taken a while, just not sure about something Isn't this going to risk causing overflows? E.g. ser = pd.Series(np.array(['NaT', '3000-01-01', '3000-01-02'], dtype='datetime64[s]'))
item = pd.Timestamp('2020-01-01T00:00:00.123456789')
print(ser.fillna(item)) This raises
Checking the behaviour for nullable integers:
|
Yes, and that is a good thing. In the status quo we silently round that non-nano fill value and give an incorrect result. The pyarrow example looks pretty clearly buggy to me. |
to be honest I'm not totally sure about this - I'm not going to block it, but don't really feel comfortable approving, could you ask someone else please? |
@mroeschke @phofl thoughts? |
I do find the overflow behavior a better behavior than silently truncating. But I also find the exception message in @MarcoGorelli 's #56419 (comment) kinda opaque compared to setting a float into a nullable int ( |
It is a bit tough to tailor the exception message due to where the exception is raised. How about f"Incompatible (high-resolution) value for {self.dtype}. Explicitly cast before operating." |
Yeah I would be good with that |
npdev failure looks unrelated |
Thanks @jbrockmendel |
* BUG: setitem with mixed-resolution dt64s * Move whatsnew to 3.0 * de-xfail * improve exception message
doc/source/whatsnew/vX.X.X.rst
file if fixing a bug or adding a new feature.