REF: lazify relativedelta imports #52659
Conversation
pandas/_libs/tslibs/parsing.pyx
Outdated
| @@ -692,6 +691,9 @@ cdef datetime dateutil_parse( | |||
| ) from err | |||
|
|
|||
| if res.weekday is not None and not res.day: | |||
| assert False | |||
There was a problem hiding this comment.
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(...)
There was a problem hiding this comment.
should we just deprecate this case since it isnt reached and would raise if it ever were?
There was a problem hiding this comment.
This branch is always false because res.day is never 0 right?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Okay I think raising is reasonable as this format is a big ambiguous.
|
Updated to intentionally raise |
| 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() |
There was a problem hiding this comment.
this looks much better, and avoids the approximations (e.g. all months have 31 days) from the original
the original comes from d0d15b0, but there's no context and looks like an initial oversight to me |
|
Thanks @jbrockmendel |
* REF: lazify relativedelta imports * API: intentionally raise ValueError * whatsnew
* REF: lazify relativedelta imports * API: intentionally raise ValueError * whatsnew
* REF: lazify relativedelta imports * API: intentionally raise ValueError * whatsnew
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.