search

ropensci / tsbox

tsbox: Class-Agnostic Time Series in R
https://docs.ropensci.org/tsbox
147 stars 12 forks source link

Problem with ts_frequency() when converting to weeks #227

Open mivazq opened 3 weeks ago

mivazq commented 3 weeks ago

Dear Developers

I want to bring to your attention that your ts_frequency() function uses the command data.table::wday() to convert data into weekly frequencies. The problem with this command is that one cannot manually define what the week start is, which is fixed at Sunday. The similar command lubridate::wday() takes the argument week_start allowing the users to define the start of the week. This is useful when working with the ISO week definition, for which Mondays are the first day of the week.

It would be great if you would use lubridate::wday(week_start=7) instead of data.table::wday() such that the default behavious is maintained, while allowing users to manually set the argument week_start.

library(zoo)

# Tests with today's date 2024-09-04, i.e. a Wednesday
data.table::wday(as.Date("2024-09-04")) # returns 4
lubridate::wday(as.Date("2024-09-04")) # returns 4. Here the function uses getOption("lubridate.week.start"). If it's not set, it defaults to 7.
lubridate::wday(as.Date("2024-09-04"), week_start=7) # returns 4. This command is identical to data.table::wday()
lubridate::wday(as.Date("2024-09-04"), week_start=1) # returns 3

If you decide against this change, please at least document this caveat in the helper of ts_frequency() such that users know what to expect when they are transforming into weeks.

Best regards and thanks for your work! Miguel

christophsax commented 3 weeks ago

Thanks, I need a bit of time to look into this.