-
-
Notifications
You must be signed in to change notification settings - Fork 19.4k
API: Timedelta constructor from keywords give microseconds #63216
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: Timedelta constructor from keywords give microseconds #63216
Conversation
jorisvandenbossche
left a comment
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.
Looks good!
The actual Timedelta constructor with kwargs, is that tested much?
I was checking pandas/tests/scalar/timedelta/test_constructors.py, and there is test_td_constructor_on_nanoseconds, but maybe you could at least add an explicit test for Timedelta(nanoseconds=1000) still having nanoseconds, as well as something like Timedelta(microseconds=1.5) to cover the check whether to convert to microseconds or not?
| + seconds | ||
| + seconds, "ns" | ||
| ) | ||
| except OverflowError as err: |
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.
We should probably create some todo to try to parse to microseconds if this is out of bounds for nanoseconds?
| Timedelta(days=1).as_unit("ns"), | ||
| Timedelta(days=1).as_unit("ns").to_timedelta64(), |
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 was wondering why this would be needed, but I suppose it is because the expected will have ns. But now the array inference PR is merged, this change can be (or needs to be) reverted?
|
@mroeschke the CI failures look unrelated, but im not seeing them on other PRs so im at a loss. any thoughts? (this is the last RC blocker) |
|
We already have in that test, so it seems we already encountered similar issues before. Given that, I think it would be fine to expand that xfail condition on the short term |
|
Yeah looks like we're hitting dateutil/dateutil#197 from dateutil. This exception is hit in |
|
Updated to expand xfail |
|
Green |
|
Thanks @jbrockmendel |
doc/source/whatsnew/vX.X.X.rstfile if fixing a bug or adding a new feature.AGENTS.md.Discussed in today's dev meeting. There is one test that is currently failing but will pass once #63018 is merged