-
-
Notifications
You must be signed in to change notification settings - Fork 19.2k
BUG: Timestamp retains frequency of input Timestamps #23503
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
BUG: Timestamp retains frequency of input Timestamps #23503
Conversation
|
Hello @mroeschke! Thanks for submitting the PR.
|
|
LGTM. Do any of the other constructors lose freq? (Eg to_datetime) |
|
|
pandas/_libs/tslibs/timestamps.pyx
Outdated
| return NaT | ||
|
|
||
| if is_string_object(freq): | ||
| if is_string_object(freq) or isinstance(freq, _BaseOffset): |
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.
don't do this as could have an adverse perf impact (this is a python object). use is_offset_object
| if is_string_object(freq): | ||
| freq = to_offset(freq) | ||
| elif not is_offset_object(freq): | ||
| # GH 22311: Try to extract the frequency of a given Timestamp input |
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.
What do we expect it to be in this case? Aside from None I guess
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 can be a Timestamp in which we want to extract its frequency and pass along.
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.
IIUC you're referring to ts_input being a Timestamp. I'm asking what freq is in this case. More specifically, anything other than None seems like it should be invalid. Or am I missing something?
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.
Ah sorry for misunderstanding. You're correct in that this is essentially swallowing any nonsense frequency and just returning None
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.
freq should only ever be None, a offset string, or offset object
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.
Won't this silently pass if freq has invalid type? Couldn't we just say that freq = getattr(ts_input, 'freq', None) if freq is None else freq?
Codecov Report
@@ Coverage Diff @@
## master #23503 +/- ##
=======================================
Coverage 92.25% 92.25%
=======================================
Files 161 161
Lines 51207 51207
=======================================
Hits 47239 47239
Misses 3968 3968
Continue to review full report at Codecov.
|
|
lgtm. can you rebase and also do a quick perf check. |
|
Looks like there's a little hit (I think the asvs make it seem worse than reality), but imo not too bad |
|
thanks @mroeschke |
git diff upstream/master -u -- "*.py" | flake8 --diff