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

ceiling_date behavior should fit the intuition at boundary #390

Closed
shrektan opened this Issue Mar 11, 2016 · 5 comments

Comments

Projects
None yet
3 participants
@shrektan
Contributor

shrektan commented Mar 11, 2016

Issue Description

As discussed in #262, the ceiling_date behaves strangely at the boundary (i.e., the first date of a month or a quarter), as documented in ceiling_date,

By convention the boundary for a month is the first second of the month. Thus floor_date(ymd("2000-03-01"), "month") gives "2000-03-01 UTC".

Current behavior

lubridate::ceiling_date(as.Date("2016-03-01"), unit = "month")
# [1] "2016-03-01"
lubridate::ceiling_date(as.Date("2016-03-02"), unit = "month")
# [1] "2016-04-01"

What users might expect

However, the users (at least myself) would naturally expect the two lines above return the same value 2016-04-01.

@vspinu vspinu added the enhancement label Mar 13, 2016

@vspinu vspinu added this to the v1.5.6 milestone Mar 13, 2016

@vspinu

This comment has been minimized.

Show comment
Hide comment
@vspinu

vspinu Mar 28, 2016

Member

One solution would be to define boundary for all units being uniformly 00 not 00 for some (hour, minute, second) and 01 for others (day, month). In that case yyyy-04-01 will be ceiled to yyyy-05-01.

This would mean that day and month don't have a boundary which implies at least two bad things. First one being that floor(ceiling(...) is not the same as floor, second is that ceiling(ceiling(...)) for a date jumps over two months. This is a major change and might break current code in quite unexpected ways.

So I am thinking to add a new argument to ceiling_date. Maybe ceiling_on_1 or no_boundary? Other ideas? If, TRUE, ceiling behaves as you propose. If FALSE, it's the current behavior. This might be a global option which you can set with options once per session.

Member

vspinu commented Mar 28, 2016

One solution would be to define boundary for all units being uniformly 00 not 00 for some (hour, minute, second) and 01 for others (day, month). In that case yyyy-04-01 will be ceiled to yyyy-05-01.

This would mean that day and month don't have a boundary which implies at least two bad things. First one being that floor(ceiling(...) is not the same as floor, second is that ceiling(ceiling(...)) for a date jumps over two months. This is a major change and might break current code in quite unexpected ways.

So I am thinking to add a new argument to ceiling_date. Maybe ceiling_on_1 or no_boundary? Other ideas? If, TRUE, ceiling behaves as you propose. If FALSE, it's the current behavior. This might be a global option which you can set with options once per session.

@vspinu vspinu closed this in 5cf8a05 Mar 28, 2016

@vspinu

This comment has been minimized.

Show comment
Hide comment
@vspinu

vspinu Mar 28, 2016

Member

I have settled on change_on_boundary (defaults to FALSE). Please let me know if you think the argument name can be better. Thanks.

Member

vspinu commented Mar 28, 2016

I have settled on change_on_boundary (defaults to FALSE). Please let me know if you think the argument name can be better. Thanks.

@shrektan

This comment has been minimized.

Show comment
Hide comment
@shrektan

shrektan Mar 29, 2016

Contributor

@vspinu Thanks. Naming is really hard... I think change_on_boundary is very clear for me, but if I can come up with something more concise I'll let you know 😃

Contributor

shrektan commented Mar 29, 2016

@vspinu Thanks. Naming is really hard... I think change_on_boundary is very clear for me, but if I can come up with something more concise I'll let you know 😃

@mr-teu

This comment has been minimized.

Show comment
Hide comment
@mr-teu

mr-teu Mar 30, 2016

Thanks, ran into this issue literally 5 minutes ago!

mr-teu commented Mar 30, 2016

Thanks, ran into this issue literally 5 minutes ago!

@vspinu

This comment has been minimized.

Show comment
Hide comment
@vspinu

vspinu Aug 2, 2016

Member

Just letting you guys know that there is no global option for change_on_boundary anymore. See #443 for the reference. Just make a wrapper function if you are bothered by typing the option every time.

Member

vspinu commented Aug 2, 2016

Just letting you guys know that there is no global option for change_on_boundary anymore. See #443 for the reference. Just make a wrapper function if you are bothered by typing the option every time.

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