-
Notifications
You must be signed in to change notification settings - Fork 172
feat: Warn on incompatible TimeUnit in pandas<2
#3007
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
Conversation
|
In order to make this consistent type wise, I've actually altered the narwhals/v1 type signature for dtypes. This feels like it could be a red-flag depending how type-strict folks our? Essentially, a I'm pretty sure this behaviour doesn't actually change anything for anyone's code, and it definitely doesn't invalidate any existing code, but editing |
Type hints, including stable/v1 type change test fixes
a53eb6c to
817ca9d
Compare
dangotbanned
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.
Thanks for the PR @benrutter!
In order to tell between a default "us" and a specified "us", I've added in a _defaulted object variable to DateTime. This feels a little crufty, but I'm not sure I can see a better option?
What I was trying (and failed π) to suggest in (#2964 (comment)) was intended to only change things in _pandas_like
Fully my bad on that one - hopefully the comments here help explain things better
narwhals/_pandas_like/utils.py
Outdated
| "A time unit other than 'ns' has been specified but is unsupported " | ||
| "in Pandas versions preceding 2.0. To avoid an error, Narwhal's has " | ||
| "fallen back to a time unit of 'ns'." |
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.
In the warning I would like to see:
- Which
time_unitis being ignored? - Why is it being ignored?
- Possibly a link to pandas changelog
- Or mention the required + found versions in the style of
Line 1877 in d318ad3
NotImplementedError: `really_complex_feature` is only available in 'pyarrow>=9000.0.0', found version '20.0.0'.
- How can I fix this?
- Upgrading
pandasis an option - but not the only one and probably not the easiest if they're stuck on a 2+ year old version π
- Upgrading
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.
Thanks, that's really helpful! I've updated the error to:
msg = (
f"The time unit '{dtype.time_unit}' has been specified but is only "
f"available in pandas>2.0, found version {pd.__version__}."
"To avoid an error, Narwhal's has therefore fallen back to using "
"'ns' as a time unit.\n"
"To avoid this warning, consider upgrading pandas or otherwise "
"specifying 'ns' time units."
)
I think that makes things clearer, but let me know if you'd change any of the wording!
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.
Okay with #3098 merged things are looking good
I'll take another look at the error message tomorrow
The main thing standing out is
Narwhal's
Should be
Narwhals
But I'll chew on the rest a bit in the meantime π
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.
This is what I'd suggest, was a bit more involved than a GH suggestion could be π
(docs(suggestion): Tweaks to warning message)
Which comes out as:
UserWarning: `nw.Datetime(time_unit='us')` is only available in 'pandas>=2.0', found version '1.5'.
Narwhals has fallen back to using `time_unit='ns'` to avoid an error.
Hint: to avoid this warning, consider either:
- Upgrading pandas: https://pandas.pydata.org/docs/dev/whatsnew/v2.0.0.html#construction-with-datetime64-or-timedelta64-dtype-with-unsupported-resolution
- Using a bare `nw.Datetime`, if this precision is not important
4c2665a to
da8e594
Compare
Co-authored-by: Dan Redding <[email protected]> Update narwhals/dtypes.py Co-authored-by: Dan Redding <[email protected]> updates
da8e594 to
c5738fc
Compare
TimeUnit in pandas<2
dangotbanned
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.
Thanks for bearing with me on this @benrutter!
|
great feature, thanks all! |

What type of PR is this? (check all applicable)
Related issues
TimeUnitΒ #2964TimeUnitΒ #2964Checklist
If you have comments or can explain your changes, please do so below
In order to tell between a default "us" and a specified "us", I've added in a
_defaultedobject variable to DateTime. This feels a little crufty, but I'm not sure I can see a better option?