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

Projects
None yet
2 participants
@larmarange
Contributor

larmarange commented Jan 1, 2015

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 from Impriving time_length for 29 Feb (when dealing with date time) to Improving time_length for 29 Feb (when dealing with date time) Jan 1, 2015

@larmarange

This comment has been minimized.

Contributor

larmarange commented Jan 1, 2015

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

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

Contributor

larmarange commented Jan 2, 2015

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 larmarange force-pushed the larmarange:master branch from b4719eb to c580dab Jan 2, 2015

@larmarange

This comment has been minimized.

Contributor

larmarange commented Jan 2, 2015

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 larmarange:master branch 3 times, most recently from 680d4a7 to f1c22f3 Jan 2, 2015

New args for rollback and .month_plus
Two new arguments (roll_to_first and preserve_hms) for rollback and
.month_plus.  time_length for intervals updated. %m++% not needed
anymore.

@larmarange larmarange force-pushed the larmarange:master branch from f1c22f3 to bbf01b5 Jan 4, 2015

@larmarange

This comment has been minimized.

Contributor

larmarange commented Jan 4, 2015

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

@vspinu

This comment has been minimized.

Member

vspinu commented Jan 4, 2015

Great! Thanks.

vspinu added a commit that referenced this pull request Jan 4, 2015

Merge pull request #295 from larmarange/master
Improving time_length for 29 Feb (when dealing with date time)

@vspinu vspinu merged commit 215c91a into tidyverse:master Jan 4, 2015

@vspinu

This comment has been minimized.

Member

vspinu commented on bbf01b5 Apr 28, 2015

I have finally made .month_plus public. It's now called add_with_rollback.

This comment has been minimized.

Contributor

larmarange replied Apr 28, 2015

Great. Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment