Skip to content
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

Improving time_length for 29 Feb (when dealing with date time) #295

Merged
merged 1 commit into from
Jan 4, 2015

Conversation

larmarange
Copy link
Contributor

With previous version, we had:

> naiss <- ymd_hms("1992-02-29 12:00:00")
> evt <- ymd_hms("2014-03-01 01:00:00")
> time_length(interval(naiss, evt), "years")
[1] 21.99874

We should consider that the anniversary occurs between 28 Feb at 23:59:59 and 1st March at 00:00:00. Therefore, %m++% should rollback at 00:00:00. In that case:

> time_length(interval(naiss, evt), "years")
[1] 22.00011

@larmarange larmarange changed the title Impriving time_length for 29 Feb (when dealing with date time) Improving time_length for 29 Feb (when dealing with date time) Jan 1, 2015
@larmarange
Copy link
Contributor Author

A quick comment about period computation from intervals:

> naiss <- ymd_hms("1992-02-29 12:00:00")
> evt <- ymd_hms("2014-03-01 01:00:00")
> as.period(interval(naiss, evt))
[1] "22y 0m -1d 13H 0M 0S"

Should we avoid negative days?

@vspinu
Copy link
Member

vspinu commented Jan 1, 2015

This should not happen. It's a bug in as.period.
On Jan 1, 2015 6:04 AM, "Joseph" notifications@github.com wrote:

A quick comment about period computation from intervals:

naiss <- ymd_hms("1992-02-29 12:00:00")> evt <- ymd_hms("2014-03-01 01:00:00")> as.period(interval(naiss, evt))
[1] "22y 0m -1d 13H 0M 0S"

Should we avoid negative days?


Reply to this email directly or view it on GitHub
#295 (comment).

@vspinu
Copy link
Member

vspinu commented Jan 2, 2015

naiss <- ymd_hms("1992-02-29 12:00:00")
evt <- ymd_hms("2014-03-01 01:00:00")
time_length(interval(naiss, evt), "years")
[1] 21.99874

Well. There is a mirror problem with negative intervals that use %m+%:

> time_length(interval(ymd_hms('2000-02-29 12:00:00'), ymd_hms('1999-02-28 20:00:00')), "years")
[1] -0.9990868

So we will have to change %m+% as well, and I am not sure that's a good idea. %m+% will basically make all intraday timestamps for Feb 29 equal, and that's clearly not suitable for intraday analysis.

One easy solution is to add an optional argument to rollback and .month_plus and .month_plus_plus. Something like preserve_hms = TRUE.

BTW. your .rollback_day_one can also be integrated into rallback with an optional argument roll_to_first = FALSE or something similar.

@larmarange
Copy link
Contributor Author

You are right.

We can also add roll_to_first to .month_plus. In that scenario, we don't need %m++ and .month_plus_plus (as it's not exported) and we can use directly .month_plus in time_length.

I will update the pull request.

@larmarange
Copy link
Contributor Author

When we think with time, even for negative intervals we should not use %m-%. In fact, the anniversary should be the 28 Feb at 24:00:00, i.e. the 1st March at 00:00:00.

Therefore, the code is simpler.

Now, we obtain:

> time_length(interval(ymd_hms('2000-02-29 12:00:00'), ymd_hms('1999-02-28 20:00:00')), "years")
[1] -1.000457

@larmarange larmarange force-pushed the master branch 3 times, most recently from 680d4a7 to f1c22f3 Compare January 4, 2015 13:29
Two new arguments (roll_to_first and preserve_hms) for rollback and
.month_plus.  time_length for intervals updated. %m++% not needed
anymore.
@larmarange
Copy link
Contributor Author

Tests and examples added for rollback. Code of time_length updated with ifelse(x@.Data < 0, -1, 1)

@vspinu
Copy link
Member

vspinu commented Jan 4, 2015

Great! Thanks.

vspinu added a commit that referenced this pull request Jan 4, 2015
Improving time_length for 29 Feb (when dealing with date time)
@vspinu vspinu merged commit 215c91a into tidyverse:master Jan 4, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants