-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
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
@@ -731,8 +732,11 @@ class Timestamp(_Timestamp): | |||
if ts.value == NPY_NAT: | |||
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
@@ -733,6 +734,9 @@ class Timestamp(_Timestamp): | |||
|
|||
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