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

Make round_date() really use week_start= #736

Closed
wants to merge 2 commits into from
Closed

Conversation

xvrdm
Copy link

@xvrdm xvrdm commented Nov 27, 2018

After reading the documentation and many SO questions, I still couldn't really explain the difference between round_date() and ceiling/floor_date when using unit="week" and week_start=1.

I thought it might just be that round_date() ignore week_start=, a bit like floor/ceiling_date back in the #509 days.

Without week_start=, everything looks as expected.

> date <- parse_date_time("November 27 2018 23:45", orders="bdyHM")
> date
[1] "2018-11-27 23:45:00 UTC"
> lubridate::round_date(date, "week")
[1] "2018-11-25 UTC"
> lubridate::floor_date(date, "week")
[1] "2018-11-25 UTC"
> lubridate::ceiling_date(date, "week")
[1] "2018-12-02 UTC"

But if you ask for weeks starting on Mondays (or any other day). Only floor/ceiling_date seem affected:

> lubridate::round_date(date, "week", week_start = 1)
[1] "2018-11-25 UTC"
> lubridate::floor_date(date, "week", week_start = 1)
[1] "2018-11-26 UTC"
> lubridate::ceiling_date(date, "week", week_start = 1)
[1] "2018-12-03 UTC"

Apart from this tiny glitch, thanks for the awesome library: I don't want to use anything else when it comes to dates 👍 !

After reading the documentation and many SO questions, I still couldn't really explain the difference between `round_date()` and `ceiling/floor_date` when using `unit="week"` and `week_start=1`.

I thought it might just be that `round_date()` ignore `week_start=`, a bit like `floor/ceiling_date` back in the tidyverse#509 days.

Without `week_start=`, everything looks as expected. 

```
> date <- parse_date_time("November 27 2018 23:45", orders="bdyHM")
> date
[1] "2018-11-27 23:45:00 UTC"
> lubridate::round_date(date, "week")
[1] "2018-11-25 UTC"
> lubridate::floor_date(date, "week")
[1] "2018-11-25 UTC"
> lubridate::ceiling_date(date, "week")
[1] "2018-12-02 UTC"
```

But if you ask for weeks starting on Mondays (or any other day). Only `floor/ceiling_date` seem affected:

```
> lubridate::round_date(date, "week", week_start = 1)
[1] "2018-11-25 UTC"
> lubridate::floor_date(date, "week", week_start = 1)
[1] "2018-11-26 UTC"
> lubridate::ceiling_date(date, "week", week_start = 1)
[1] "2018-12-03 UTC"
```

Apart from this tiny glitch, thanks for the awesome library: I don't want to use anything else when it comes to dates 👍 !
Make round_date() really use week_start=
@xvrdm
Copy link
Author

xvrdm commented Nov 29, 2018

Closing this for now. I tried to add tests in the style of ceiling_date()/floor_date():

# in test-round.r
  expect_equal(wday(round_date(ct, "week", week_start = 1)), 2)
  expect_equal(wday(round_date(ct, "week", week_start = 2)), 3)
  expect_equal(wday(round_date(ct, "week", week_start = 5)), 6)
  expect_equal(wday(round_date(ct, "week", week_start = 7)), 1)
  expect_equal(wday(round_date(date, "week", week_start = 1)), 2)
  expect_equal(wday(round_date(date, "week", week_start = 2)), 3)
  expect_equal(wday(round_date(date, "week", week_start = 5)), 6)
  expect_equal(wday(round_date(date, "week", week_start = 7)), 1)

  expect_equal(wday(round_date(ct, "week", week_start = 1), week_start = 1), 1)
  expect_equal(wday(round_date(ct, "week", week_start = 2), week_start = 2), 1)
  expect_equal(wday(round_date(ct, "week", week_start = 5), week_start = 5), 1)
  expect_equal(wday(round_date(ct, "week", week_start = 7), week_start = 7), 1)
  expect_equal(wday(round_date(date, "week", week_start = 1), week_start = 1), 1)
  expect_equal(wday(round_date(date, "week", week_start = 2), week_start = 2), 1)
  expect_equal(wday(round_date(date, "week", week_start = 5), week_start = 5), 1)
  expect_equal(wday(round_date(date, "week", week_start = 7), week_start = 7), 1)

Two don't pass yet.

test-round.R:484: failure: round on week respects week_start
wday(round_date(ct, "week", week_start = 5)) not equal to 6.
1/1 mismatches
[1] 1 - 6 == -5

test-round.R:493: failure: round on week respects week_start
wday(round_date(ct, "week", week_start = 5), week_start = 5) not equal to 1.
1/1 mismatches
[1] 3 - 1 == 2

Will try to do more testing.

@xvrdm xvrdm closed this Nov 29, 2018
@vspinu
Copy link
Member

vspinu commented Dec 1, 2018

Only floor/ceiling_date seem affected:

Both ceiling_date and floor_date work as expected in your example. It's really only the issue of round_date, looks like.

@xvrdm
Copy link
Author

xvrdm commented Dec 1, 2018

Thanks, that is what I meant but apparently my brain did not work correctly :)... Corrected now.

@xvrdm
Copy link
Author

xvrdm commented Dec 1, 2018

Actually now I remember. By "affected" I meant that the week_start argument has the expected effect on them, but not on round_date. Sorry, it might be poor english...

vspinu added a commit that referenced this pull request Dec 3, 2018
@vspinu
Copy link
Member

vspinu commented Dec 3, 2018

Fixed in master. Your fix was almost there it's just that week_start is not the 3rd arg in ceiling_date.

@xvrdm
Copy link
Author

xvrdm commented Dec 3, 2018

Thanks! I guess that will teach me to be more careful before searching complicated explanations!!

@StatsRhian
Copy link

Thanks for raising this @xvrdm. Was the PR ever merged? lubridate::round_date(date, "week", week_start = 1) is still ignoringweek_start for me in the latest development version of the package from GitHub.

@xvrdm
Copy link
Author

xvrdm commented Apr 15, 2019

I thought it was because I made a mistake in my PR that was kindly fixed by @vspinu . But I have not tested it in a while.

@vspinu
Copy link
Member

vspinu commented Apr 15, 2019

Yes. It's fixed, but not on CRAN yet.

@khayes888
Copy link

It seems this issue has still not been fixed on CRAN.

round_date(ymd("2019-10-22"),unit="week",week_start = 5)
[1] "2019-10-20"
round_date(ymd("2019-10-22"),unit="week",week_start = 1)
[1] "2019-10-20"
sessionInfo()
R version 3.5.1 (2018-07-02)
Platform: x86_64-w64-mingw32/x64 (64-bit)
Running under: Windows >= 8 x64 (build 9200)

Matrix products: default

locale:
[1] LC_COLLATE=English_United States.1252 LC_CTYPE=English_United States.1252 LC_MONETARY=English_United States.1252
[4] LC_NUMERIC=C LC_TIME=English_United States.1252

attached base packages:
[1] stats graphics grDevices utils datasets methods base

other attached packages:
[1] lubridate_1.7.4

loaded via a namespace (and not attached):
[1] Rcpp_0.12.18 crayon_1.3.4 dplyr_0.7.6 assertthat_0.2.0 R6_2.2.2 magrittr_1.5 pillar_1.3.0 stringi_1.2.4
[9] rlang_0.2.2 rstudioapi_0.7 bindrcpp_0.2.2 tools_3.5.1 stringr_1.3.1 glue_1.3.0 purrr_0.2.5 yaml_2.2.0
[17] compiler_3.5.1 pkgconfig_2.0.2 bindr_0.1.1 knitr_1.20 tidyselect_0.2.4 tibble_1.4.2

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.

4 participants