Skip to content

Period.end_time should be exclusive or Period should provide duration property #9089

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

Closed
malina-kirn opened this issue Dec 15, 2014 · 7 comments
Labels
API Design Period Period data type

Comments

@malina-kirn
Copy link

A user may expect that the end_time of a Period minus the start_time of a Period returns its duration. Unfortunately, the end_time of a Period is inclusive, consequently, subtracting start from end yields an incorrect period duration:

jan = Period('2014-01', freq='M')
(jan.end_time - jan.start_time).days # returns 30, not 31

This can be manually rectified by the user:

jan = Period('2014-01', freq='M')
(Timestamp(jan.end_time.value + 1) - jan.start_time).days # correctly returns 31

However, this is not intuitive.

The end_time should either be exclusive or Period should provide a duration property.

Although a strong argument can be made that the definitions of duration, start_time, and end_time should all be consistent, changing the end_time definition to exclusive is a significant modification of long-standing behavior.

Consequently, I propose the addition of a duration property that is logically inconsistent with subtracting start_time from end_time.

@jreback jreback added API Design Period Period data type labels Dec 17, 2014
@jreback
Copy link
Contributor

jreback commented Dec 17, 2014

I think the start_time,end_time are consistent and intuitive now. Though I can see when you subtract them you might be suprised. I think more surprising would be an end_time which is NOT included in the advertised Period (e..g. in your example returning Period('2014-1',freq='M').end_time == Timestamp('20140201'))

.duration seems reasonable. @shoyer ?

@shoyer
Copy link
Member

shoyer commented Dec 17, 2014

.duration does seem reasonable -- this would be a Timedelta, yes?

@malina-kirn
Copy link
Author

Yes, datetime.timedelta seems the appropriate type.

@malina-kirn
Copy link
Author

Apologies, I hadn't realized Timedelta was introduced in 0.15.0. That would seem to be the better type.

@jbrockmendel
Copy link
Member

@jreback IIRC there was a semi-recent change to subtract 1 ns from end_time for reasons related to this. Closeable?

@jreback
Copy link
Contributor

jreback commented Nov 16, 2018

yeah i think this is closeable. maybe we should also provide a duration though.

@jreback jreback closed this as completed Nov 16, 2018
@jorisvandenbossche
Copy link
Member

IIRC there was a semi-recent change to subtract 1 ns from end_time for reasons related to this.

I don't think this was recent, but already the case when this issue was discussed (at least it is already this behaviour in 0.14).

I don't know if it is important enough to keep the issue open, but the original reported problem is still valid I think?

Although, (jan.end_time - jan.start_time).days now does give 31 instead of 30, but is that a bug?

In [36]: jan = pd.Period('2014-01', freq='M')                                                                                                                                   

In [37]: td = (jan.end_time - jan.start_time)                                                                                                                                   

In [38]: td                                                                                                                                                                     
Out[38]: Timedelta('30 days 23:59:59.999999')

In [39]: td.components                                                                                                                                                          
Out[39]: Components(days=30, hours=23, minutes=59, seconds=59, milliseconds=999, microseconds=999, nanoseconds=999)

In [40]: td.days                                                                                                                                                                
Out[40]: 31

Or is that a purposed change? (this only changed in 0.23)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API Design Period Period data type
Projects
None yet
Development

No branches or pull requests

5 participants