Skip to content
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

Raise error when datetime64 or timedelta64 values that are outside the valid range for ns precision are converted to ns precision #4454

Merged
merged 10 commits into from
Sep 30, 2020

Conversation

andrewpauling
Copy link
Contributor

Use _possibly_convert_obhects to raise an error when converting datetime64 or timedelta64 objects that are in some units other than ns to ns as per pandas requirements, resulting in dates outside the valid range for ns precision being silently changed to incorrect values.

Call _possibly_convert_objects on datetime64 and timedelta64 objects so that
pandas raises an error if those objects are outside the valid date range for
ns precision. Previously the times would be silently changed to nonsense values
if they fell outside this range. Addresses GH4427.
@max-sixty
Copy link
Collaborator

Thanks @andrewpauling , this looks good.

Could you add a test?

@andrewpauling
Copy link
Contributor Author

Hi @max-sixty, I have just added what I hope is a good test, I haven't written one before, but tried to base it on others in the test_variable file

@max-sixty
Copy link
Collaborator

It looks great! I made one suggestion.

Any thoughts from others?

Copy link
Member

@spencerkclark spencerkclark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! Could you also add a similar test for timedelta64 data?

doc/whats-new.rst Outdated Show resolved Hide resolved
@andrewpauling
Copy link
Contributor Author

Nice! Could you also add a similar test for timedelta64 data?

Hi @spencerkclark, I have been trying to work on a test for timedelta64 data but I don't think I understand timedelta data very well. I can't seem to come up with an example that raises an OutOfBoundsTimedelta error. From what I found online (https://pandas.pydata.org/pandas-docs/stable/user_guide/timedeltas.html), the valid range for nanosecond precision should be about 100000 days. When I try to input an np.timedelta64[D] object of more days than that, it just overflows and gives nonsense without throwing an error. Do you have any suggestions?

@spencerkclark
Copy link
Member

spencerkclark commented Sep 25, 2020

You're absolutely right @andrewpauling -- pandas currently doesn't seem to have overflow protection in the case of casting timedelta64 types. I wrongly assumed that it would! I went ahead and opened an issue there: pandas-dev/pandas#36615. We'll see what they say.

For now maybe you can add the test and attach an expected failure decorator to it?

In researching this, I came across numpy/numpy#16352; if that eventually gets addressed this might all be moot.

@andrewpauling
Copy link
Contributor Author

Thanks @spencerkclark, I have added a test with the decorator you mentioned and edited the formatting in the whats-new file as above. Hopefully it should be good now.

@max-sixty
Copy link
Collaborator

Excellent, thanks @andrewpauling ! LGTM

Copy link
Member

@spencerkclark spencerkclark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @andrewpauling! Sorry for taking a bit to re-review; it looks like there's now a minor merge conflict in the what's new page. Once that's sorted out, I think this should be good to go.

xarray/tests/test_variable.py Outdated Show resolved Hide resolved
@max-sixty
Copy link
Collaborator

Great, let's merge on green!

@andrewpauling
Copy link
Contributor Author

Cool! Thank you all for your help.

@spencerkclark spencerkclark merged commit 5296ed1 into pydata:master Sep 30, 2020
@spencerkclark
Copy link
Member

Thanks again @andrewpauling!

@max-sixty
Copy link
Collaborator

Thanks a lot @andrewpauling !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

assign_coords with datetime64[us] changes dtype to datetime64[ns]
4 participants