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

wrong return type in `ceiling_date` when unit = "week" #479

Closed
shrektan opened this issue Sep 27, 2016 · 8 comments
Closed

wrong return type in `ceiling_date` when unit = "week" #479

shrektan opened this issue Sep 27, 2016 · 8 comments

Comments

@shrektan
Copy link
Contributor

@shrektan shrektan commented Sep 27, 2016

Hi, @vspinu, I tried the CRAN version of lubridate and found a bug related to ceiling_date when the unit = "week". It should return a date object instead of a datetime.

Here's the reproducible example:

library(lubridate)

# ceiling date returns a time
ceiling_date(ymd("20160927"), "week")
## [1] "2016-10-02 08:00:00 CST"

# while the floor_date returns a date
floor_date(ymd("20160927"), "week")
## [1] "2016-09-25"
@shrektan
Copy link
Contributor Author

@shrektan shrektan commented Sep 27, 2016

I've filed a PR.

@vspinu
Copy link
Member

@vspinu vspinu commented Sep 27, 2016

It should return a date object instead of a datetime.

Why do you think so?

If it returns date for week, shouldn't it return date for "day", "month" and "year" as well?

@shrektan
Copy link
Contributor Author

@shrektan shrektan commented Sep 27, 2016

It's true that day month and year returns a date. Isn't it?

@shrektan
Copy link
Contributor Author

@shrektan shrektan commented Sep 27, 2016

@vspinu Here's is what's returned in lubridate version 1.6.0. As you can see, only unit = "week" behaves differently.

ceiling_date(ymd("20160927"), "year")
## [1] "2017-01-01"
ceiling_date(ymd("20160927"), "month")
## [1] "2016-10-01"
ceiling_date(ymd("20160927"), "week")
## [1] "2016-10-02 08:00:00 CST"
ceiling_date(ymd("20160927"), "day")
## [1] "2016-09-28"
@vspinu
Copy link
Member

@vspinu vspinu commented Sep 27, 2016

Yes. You are right. I got confused for a second due to recent re-designing of rounding API.

shrektan added a commit to shrektan/lubridate that referenced this issue Sep 27, 2016
@shrektan
Copy link
Contributor Author

@shrektan shrektan commented Sep 27, 2016

It's a very powerful and useful package. Certainly it takes lots of effort to improve. 🍺

shrektan added a commit to shrektan/lubridate that referenced this issue Sep 27, 2016
@vspinu vspinu closed this in #480 Sep 28, 2016
@Henrik-P
Copy link

@Henrik-P Henrik-P commented Jun 12, 2017

Seems like this issue is alive again in lubridate_1.6.0:

ceiling_date(ymd("20160927"), "year")
# [1] "2017-01-01"

ceiling_date(ymd("20160927"), "month")
# [1] "2016-10-01"

ceiling_date(ymd("20160927"), "week")
# [1] "2016-10-02 02:00:00 CEST"

ceiling_date(ymd("20160927"), "day")
# [1] "2016-09-28"

@cderv
Copy link
Contributor

@cderv cderv commented Jun 12, 2017

It is ok in current development version of lubridate 1.6.0.9009.

library(lubridate)
ceiling_date(ymd("20160927"), "year")
#> [1] "2017-01-01"
ceiling_date(ymd("20160927"), "month")
#> [1] "2016-10-01"
ceiling_date(ymd("20160927"), "week")
#> [1] "2016-10-02"
ceiling_date(ymd("20160927"), "day")
#> [1] "2016-09-28"
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants