Skip to content

BUG: Period.to_timestamp not matching PeriodArray.to_timestamp #34449

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 4 commits into from
Jun 1, 2020
Merged

BUG: Period.to_timestamp not matching PeriodArray.to_timestamp #34449

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
544f3e6
BUG: Period.to_timestamp not matching PeriodArray.to_timestamp
jbrockmendel May 29, 2020
3041779
BUG: end_time
jbrockmendel May 29, 2020
5bd5bf2
Merge branch 'master' of https://github.com/pandas-dev/pandas into re…
jbrockmendel May 30, 2020
cbebada
fix test
jbrockmendel May 30, 2020
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
13 changes: 7 additions & 6 deletions pandas/_libs/tslibs/period.pyx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1702,10 +1702,7 @@ cdef class _Period:

@property
def end_time(self) -> Timestamp:
# freq.n can't be negative or 0
# ordinal = (self + self.freq.n).start_time.value - 1
ordinal = (self + self.freq).start_time.value - 1
return Timestamp(ordinal)
return self.to_timestamp(how="end")

def to_timestamp(self, freq=None, how='start', tz=None) -> Timestamp:
"""
Expand All @@ -1727,18 +1724,22 @@ cdef class _Period:
-------
Timestamp
"""
if freq is not None:
freq = self._maybe_convert_freq(freq)
how = validate_end_alias(how)

end = how == 'E'
if end:
if freq == "B" or self.freq == "B":
# roll forward to ensure we land on B date
adjust = Timedelta(1, "D") - Timedelta(1, "ns")
return self.to_timestamp(how="start") + adjust
endpoint = (self + self.freq).to_timestamp(how='start')
return endpoint - Timedelta(1, 'ns')

if freq is None:
base, mult = get_freq_code(self.freq)
freq = get_to_timestamp_base(base)
else:
freq = self._maybe_convert_freq(freq)

base, mult = get_freq_code(freq)
val = self.asfreq(freq, how)
Expand Down
2 changes: 1 addition & 1 deletion pandas/core/arrays/period.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -430,7 +430,7 @@ def to_timestamp(self, freq=None, how="start"):

end = how == "E"
if end:
if freq == "B":
if freq == "B" or self.freq == "B":
# roll forward to ensure we land on B date
adjust = Timedelta(1, "D") - Timedelta(1, "ns")
return self.to_timestamp(how="start") + adjust
Expand Down
9 changes: 9 additions & 0 deletions pandas/tests/indexes/period/test_scalar_compat.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,3 +17,12 @@ def test_end_time(self):
expected_index = date_range("2016-01-01", end="2016-05-31", freq="M")
expected_index += Timedelta(1, "D") - Timedelta(1, "ns")
tm.assert_index_equal(index.end_time, expected_index)

def test_end_time_business_friday(self):
# GH#34449
pi = period_range("1990-01-05", freq="B", periods=1)
result = pi.end_time

dti = date_range("1990-01-05", freq="D", periods=1)._with_freq(None)
expected = dti + Timedelta(days=1, nanoseconds=-1)
tm.assert_index_equal(result, expected)
17 changes: 17 additions & 0 deletions pandas/tests/scalar/period/test_period.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -589,6 +589,8 @@ def test_to_timestamp(self):
from_lst = ["A", "Q", "M", "W", "B", "D", "H", "Min", "S"]

def _ex(p):
if p.freq == "B":
return p.start_time + Timedelta(days=1, nanoseconds=-1)
return Timestamp((p + p.freq).start_time.value - 1)

for i, fcode in enumerate(from_lst):
Expand Down Expand Up @@ -632,6 +634,13 @@ def _ex(p):
result = p.to_timestamp("5S", how="start")
assert result == expected

def test_to_timestamp_business_end(self):
per = pd.Period("1990-01-05", "B") # Friday
Copy link
Contributor

@jreback jreback May 29, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you test how='B', and is this result correct?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we have tests for the other case, and it is the correct result.

i got this test by going into the PeriodArray.to_timestamp code and computing the same thing element-wise, then asserting that they matched. this is one of the cases that failed

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is correct, 1/6 is a sunday, by-definition not a business day

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you are missing my point, should this not be pd.Timestamp('1990-01-8') - pd.Timedelta(nanoseconds=1), IOW 1 nano less than the next period which starts on 1990-01-8

e.g. (not including this PR)

In [1]: p = pd.Period("1990-01-05", "B")                                                                                                                      

In [2]: p                                                                                                                                                     
Out[2]: Period('1990-01-05', 'B')

In [3]: p.start_time                                                                                                                                          
Out[3]: Timestamp('1990-01-05 00:00:00')

In [4]: p.end_time                                                                                                                                            
Out[4]: Timestamp('1990-01-07 23:59:59.999999999')

In [6]: pd.Timestamp('1990-01-6') - pd.Timedelta(nanoseconds=1)                                                                                               
Out[6]: Timestamp('1990-01-05 23:59:59.999999999')

In [8]: pd.Timestamp('1990-01-8') - pd.Timedelta(nanoseconds=1)                                                                                               
Out[8]: Timestamp('1990-01-07 23:59:59.999999999')

I believe we should be making [4] == [8]

[6] (what you are testing) does not seem right as it doesn't include the weekend. I believe we cover the complete space with the end-times, IOW you can line the periods up and there is NO space in-between.

result = per.to_timestamp("B", how="E")

expected = pd.Timestamp("1990-01-06") - pd.Timedelta(nanoseconds=1)
assert result == expected

# --------------------------------------------------------------
# Rendering: __repr__, strftime, etc

Expand Down Expand Up @@ -786,6 +795,14 @@ def _ex(*args):
xp = _ex(2012, 1, 2, 1)
assert xp == p.end_time

def test_end_time_business_friday(self):
# GH#34449
per = Period("1990-01-05", "B")
result = per.end_time

expected = pd.Timestamp("1990-01-06") - pd.Timedelta(nanoseconds=1)
assert result == expected

def test_anchor_week_end_time(self):
def _ex(*args):
return Timestamp(Timestamp(datetime(*args)).value - 1)
Expand Down