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

jbrockmendel · 2020-05-29T01:39:17Z

With this change, I think we can move to share some methods between Period and PeriodArray.

jreback · 2020-05-29T17:24:34Z

pandas/tests/scalar/period/test_period.py

@@ -632,6 +632,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


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

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

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

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.

jorisvandenbossche · 2020-05-29T17:56:48Z

pandas/_libs/tslibs/period.pyx

        how = validate_end_alias(how)

        end = how == 'E'
        if end:
+            if freq == "B":


Are there no other freqs for which you need to do a similar adjustment? (like the other business-related freqs?)

jbrockmendel · 2020-05-29T18:00:02Z

This is the only one that doesn’t “cover the space” so I think this is it

…

On Fri, May 29, 2020 at 10:57 AM Joris Van den Bossche < ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In pandas/_libs/tslibs/period.pyx <#34449 (comment)>: > how = validate_end_alias(how) end = how == 'E' if end: + if freq == "B": Are there no other freqs for which you need to do a similar adjustment? (like the other business-related freqs?) — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#34449 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AB5UM6GDPGRXZ7A4LUILL63RT7ZO7ANCNFSM4NNUBIWA> .

jorisvandenbossche · 2020-05-29T18:05:33Z

Maybe it is not fully related to the changes in this PR, but just since I am trying to understand it now:

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

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

Shouldn't the above also give end of "1990-01-05" ?
So in terms of the code change of your PR, shouldn't the adjustment (also) depend on self.freq instead of the passed freq?

(I am not an avid Period user, to just questions about something that doesn't feel consistent for a novice)

jbrockmendel · 2020-05-29T18:58:48Z

Shouldn't the above also give end of "1990-01-05" ?

yes it should, good catch. will update

jorisvandenbossche · 2020-05-30T07:55:46Z

Is BusinessDay ('B') the only "business" offset that can be used with Periods?

Like pd.period_range("2012", freq=pd.offsets.BusinessDay(), periods=10) works, but pd.period_range("2012", freq=pd.offsets.BusinessMonthEnd(), periods=10) fails

…f-to_timestamp

jbrockmendel · 2020-05-30T14:39:11Z

Is BusinessDay ('B') the only "business" offset that can be used with Periods?

yes.

but pd.period_range("2012", freq=pd.offsets.BusinessMonthEnd(), periods=10) fails

the relationship between DateOffsets and Periods is mostly coincidental, and this has caused confusion in the past. im leaning towards wearning Period off of offsets entirely.

jreback · 2020-05-31T22:43:28Z

pandas/tests/scalar/period/test_period.py

@@ -632,6 +632,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


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

jbrockmendel · 2020-05-31T22:53:13Z

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

>>> ts = pd.Timestamp("1990-01-05")
>>> print(ts.day_name())
'Friday'

So unless they skipped Saturday that week...

jreback · 2020-06-01T13:36:35Z

pandas/tests/scalar/period/test_period.py

@@ -632,6 +632,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


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.

jbrockmendel · 2020-06-01T14:56:54Z

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.

For non-B periods that is how it works, but we're not currently consistent about it for B. So we have to decide what it means. I dont think that Saturday or Sunday are part of the "Business Day" Friday.

In [2]: per = pd.Period("1990-01-05", "B")
In [3]: pa = pd.array([per])

In [4]: per.end_time
Out[4]: Timestamp('1990-01-07 23:59:59.999999999')
In [5]: pa.end_time[0]
Out[5]: Timestamp('1990-01-07 23:59:59.999999999')

In [6]: per.to_timestamp(how="E")                                                                                                                                                                   
Out[6]: Timestamp('1990-01-07 23:59:59.999999999')
In [7]: pa.to_timestamp(how="E")[0]
Out[7]: Timestamp('1990-01-07 23:59:59.999999999')

In [10]: per.to_timestamp("B", how="E")
Out[10]: Timestamp('1990-01-07 23:59:59.999999999')
In [12]: pa.to_timestamp("B", how="E")[0]         # <-- outlier
Out[12]: Timestamp('1990-01-05 23:59:59.999999999')

The commit that changed end_time was added based on Joris pointing out the inconsistency and me agreeing with him. But if that is a sticking point, I can revert that so we can at least get the Period/PeriodArray methods consistent with each other (after which they get to share code) and we can hammer out the end_time thing separately

jreback · 2020-06-01T15:39:35Z

@jbrockmendel

ok maybe that is the issue, its inconcistent! yeah I tend to think that the periods should be contiguous. But we can discuss that later. happy to take this. I think this warrants an issue if you can put detail about when this is inconsistent (either internally e.g vs PA, though i think you are fixing), or against others offsets then that would be good.

jorisvandenbossche · 2020-06-01T15:45:28Z

I tend to think that the periods should be contiguous

I would say that this is the exact feature of "business day" that it is not contiguous? (otherwise you would use the normal "day" frequency?

jreback · 2020-06-01T15:56:45Z

I tend to think that the periods should be contiguous

I would say that this is the exact feature of "business day" that it is not contiguous? (otherwise you would use the normal "day" frequency?

yes Timestamps are non-contiguous, but that is not what we are talking about here. A Period is a span, so you need a specific start & end point. Generally you want to include sat/sunday here for example. Or at least IMHO is the most common operation. Sure I suppose others may want a different behavior.

jorisvandenbossche · 2020-06-01T15:59:23Z

Generally you want to include sat/sunday here for example

But why would you want to include saturday/sunday for a business day ? Then you can use the normal daily frequency?

(now, I never used business days in practice, so I don't really know how it is typically used)

BUG: Period.to_timestamp not matching PeriodArray.to_timestamp

544f3e6

jreback added Period Period data type Datetime Datetime data dtype labels May 29, 2020

jreback reviewed May 29, 2020

View reviewed changes

jorisvandenbossche reviewed May 29, 2020

View reviewed changes

BUG: end_time

3041779

Merge branch 'master' of https://github.com/pandas-dev/pandas into re…

5bd5bf2

…f-to_timestamp

fix test

cbebada

jreback added this to the 1.1 milestone May 31, 2020

jreback requested changes May 31, 2020

View reviewed changes

jreback requested changes Jun 1, 2020

View reviewed changes

jreback approved these changes Jun 1, 2020

View reviewed changes

jreback merged commit 2428cdd into pandas-dev:master Jun 1, 2020

jbrockmendel deleted the ref-to_timestamp branch June 1, 2020 16:34

Uh oh!

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

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

Uh oh!

Conversation

jbrockmendel commented May 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jreback May 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jbrockmendel May 29, 2020

Choose a reason for hiding this comment

Uh oh!

jreback May 31, 2020

Choose a reason for hiding this comment

Uh oh!

jreback Jun 1, 2020

Choose a reason for hiding this comment

Uh oh!

jorisvandenbossche May 29, 2020

Choose a reason for hiding this comment

Uh oh!

jbrockmendel commented May 29, 2020 via email

Uh oh!

jorisvandenbossche commented May 29, 2020

Uh oh!

jbrockmendel commented May 29, 2020

Uh oh!

jorisvandenbossche commented May 30, 2020

Uh oh!

jbrockmendel commented May 30, 2020

Uh oh!

jreback May 31, 2020

Choose a reason for hiding this comment

Uh oh!

jbrockmendel commented May 31, 2020

Uh oh!

jreback Jun 1, 2020

Choose a reason for hiding this comment

Uh oh!

jbrockmendel commented Jun 1, 2020

Uh oh!

jreback commented Jun 1, 2020

Uh oh!

jorisvandenbossche commented Jun 1, 2020

Uh oh!

jreback commented Jun 1, 2020

Uh oh!

jorisvandenbossche commented Jun 1, 2020

Uh oh!

Uh oh!

jbrockmendel commented May 29, 2020 •

edited

Loading

jreback May 29, 2020 •

edited

Loading