REF: lazify relativedelta imports by jbrockmendel · Pull Request #52659 · pandas-dev/pandas (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation14 Commits4 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
In most cases we don't need it, so delay importing it until we do.
in io.stata it can be replaced with datetime.timedelta. In the plotting code I think it can be replaced with tot_secs = (dmax-dmin).total_seconds(), need to track down why that wasn't done in the first place.
| @@ -692,6 +691,9 @@ cdef datetime dateutil_parse( |
|---|
| ) from err |
| if res.weekday is not None and not res.day: |
| assert False |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
woops, this wasn't supposed to make it into the PR. looks like we don't get here in the tests. And its a good thing, because L696 should just be relativedelta(...), not relativedelta.relativedelta(...)
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we just deprecate this case since it isnt reached and would raise if it ever were?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This branch is always false because res.day is never 0 right?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just never reached in tests. pd.Timestamp("2023 Sept Thu") will hit this and raise AttributeError: type object 'relativedelta' has no attribute 'relativedelta' in main
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah interesting. I thought res was always a datetime.datetime object here. (Also this doesn't make sense since res.weekday() should be passed instead.)
What's the result if we remove this branch? If it's sensible, we can remove it and call it a bug fix?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ret is a datetime object here, res is a parser.parser._result object
If we disable this check entirely then pd.Timestamp("2023 Sept Thu") behaves like Timestamp("2023-09-01") but I'm pretty sure if i run it tomorrow it will be 2023-09-02. Rather than skip this block entirely, I'd prefer to change it to raise intentionally.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay I think raising is reasonable as this format is a big ambiguous.
Updated to intentionally raise
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1, seems like a net positive
Comment on lines -352 to +351
| delta = relativedelta(dmax, dmin) |
|---|
| num_days = (delta.years * 12.0 + delta.months) * 31.0 + delta.days |
| num_sec = (delta.hours * 60.0 + delta.minutes) * 60.0 + delta.seconds |
| tot_sec = num_days * 86400.0 + num_sec |
| tot_sec = (dmax - dmin).total_seconds() |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this looks much better, and avoids the approximations (e.g. all months have 31 days) from the original
In the plotting code I think it can be replaced with
tot_secs = (dmax-dmin).total_seconds(), need to track down why that wasn't done in the first place.
the original comes from d0d15b0, but there's no context and looks like an initial oversight to me
topper-123 pushed a commit to topper-123/pandas that referenced this pull request
REF: lazify relativedelta imports
API: intentionally raise ValueError
whatsnew
Rylie-W pushed a commit to Rylie-W/pandas that referenced this pull request
REF: lazify relativedelta imports
API: intentionally raise ValueError
whatsnew
Daquisu pushed a commit to Daquisu/pandas that referenced this pull request
REF: lazify relativedelta imports
API: intentionally raise ValueError
whatsnew