Skip to content

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.

Sign up for GitHub

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

Merged
merged 7 commits into from
Nov 7, 2018
Merged

BUG: Timestamp retains frequency of input Timestamps #23503

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
232c29b
BUG: Timestamp retains frequency of input Timestamps
Nov 5, 2018
4f34c01
Be a little more strict with offset checking
Nov 5, 2018
a14776e
Merge remote-tracking branch 'upstream/master' into timestamp_roundtr…
Nov 5, 2018
41fcdd7
use better offest check
Nov 5, 2018
5854b88
Merge remote-tracking branch 'upstream/master' into timestamp_roundtr…
Nov 6, 2018
2dcb1fb
Merge remote-tracking branch 'upstream/master' into timestamp_roundtr…
Nov 7, 2018
218e418
Merge remote-tracking branch 'upstream/master' into timestamp_roundtr…
Nov 7, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions doc/source/whatsnew/v0.24.0.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1114,6 +1114,7 @@ Datetimelike
- Bug in :func:`DataFrame.combine` with datetimelike values raising a TypeError (:issue:`23079`)
- Bug in :func:`date_range` with frequency of ``Day`` or higher where dates sufficiently far in the future could wrap around to the past instead of raising ``OutOfBoundsDatetime`` (:issue:`14187`)
- Bug in :class:`PeriodIndex` with attribute ``freq.n`` greater than 1 where adding a :class:`DateOffset` object would return incorrect results (:issue:`23215`)
- Bug in :class:`Timestamp` constructor which would drop the frequency of an input :class:`Timestamp` (:issue:`22311`)

Timedelta
^^^^^^^^^
Expand Down
6 changes: 5 additions & 1 deletion pandas/_libs/tslibs/timestamps.pyx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ from np_datetime import OutOfBoundsDatetime
from np_datetime cimport (reverse_ops, cmp_scalar, check_dts_bounds,
npy_datetimestruct, dt64_to_dtstruct)
from offsets cimport to_offset
from offsets import _BaseOffset
from timedeltas import Timedelta
from timedeltas cimport delta_to_nanoseconds
from timezones cimport (
Expand Down Expand Up @@ -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):
Copy link
Contributor

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

freq = to_offset(freq)
else:
# GH 22311: Try to extract the frequency of a given Timestamp input
Copy link
Member

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

Copy link
Member Author

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.

Copy link
Member

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?

Copy link
Member Author

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

Copy link
Member Author

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

Copy link
Member

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?

freq = getattr(ts_input, 'freq', None)

return create_timestamp_from_ts(ts.value, ts.dts, ts.tzinfo, freq)

Expand Down
6 changes: 6 additions & 0 deletions pandas/tests/scalar/timestamp/test_timestamp.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -569,6 +569,12 @@ def test_construct_with_different_string_format(self, arg):
expected = Timestamp(datetime(2013, 1, 1), tz=pytz.FixedOffset(540))
assert result == expected

def test_construct_timestamp_preserve_original_frequency(self):
# GH 22311
result = Timestamp(Timestamp('2010-08-08', freq='D')).freq
expected = offsets.Day()
assert result == expected


class TestTimestamp(object):

Expand Down