BUG: Period.to_timestamp not matching PeriodArray.to_timestamp
State: closed

- [ ] closes #xxxx
- [x] tests added / passed
- [ ] passes `black pandas`
- [ ] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [ ] whatsnew entry

With this change, I _think_ we can move to share some methods between Period and PeriodArray.

Labels:
name: Datetime
description: Datetime data dtype
name: Period
description: Period data type

Comments:
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 <
notifications@github.com> wrote:

> *@jorisvandenbossche* commented on this pull request.
> ------------------------------
>
> In pandas/_libs/tslibs/period.pyx
> <https://github.com/pandas-dev/pandas/pull/34449#discussion_r432647674>:
>
> >          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
> <https://github.com/pandas-dev/pandas/pull/34449#pullrequestreview-421169093>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AB5UM6GDPGRXZ7A4LUILL63RT7ZO7ANCNFSM4NNUBIWA>
> .
>

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)

> Shouldn't the above also give end of "1990-01-05" ?

yes it should, good catch.  will update
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
> 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.
> 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...
> 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
@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.
> 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?
> > 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.
> 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)