week() does not return actual calendar week #1765

Closed
STATWORX opened this Issue Jul 7, 2016 · 8 comments

Comments

Projects
None yet
4 participants
@ghost

ghost commented Jul 7, 2016

Hi,

the week() function does not return the actual calendar week (what I think most users expect).
Please fix or rename function since a lot of users are expecting a different result.

Thank you!

Example:

### Packages
library(data.table)
library(lubridate)

### Thursday, 7th january 2016, calendar week 1
my_date <- as.Date("2016-01-07", format = "%Y-%m-%d")

### Week from data.table
data.table::week(my_date)
> 2 # not what most people would expect

### Week from lubridate
lubridate::isoweek(my_date)
> 1 # as expected
@MichaelChirico

This comment has been minimized.

Show comment
Hide comment
@MichaelChirico

MichaelChirico Jul 7, 2016

Contributor

please include the output -- I'm not at a machine so I can't tell what you see/expect.

For now, please see the relevant code and I particular the preceding comment section:

https://github.com/Rdatatable/data.table/blob/master/R/IDateTime.R

Contributor

MichaelChirico commented Jul 7, 2016

please include the output -- I'm not at a machine so I can't tell what you see/expect.

For now, please see the relevant code and I particular the preceding comment section:

https://github.com/Rdatatable/data.table/blob/master/R/IDateTime.R

@MichaelChirico

This comment has been minimized.

Show comment
Hide comment
@MichaelChirico

MichaelChirico Jul 7, 2016

Contributor

see also #1658 which i imagine is related

Contributor

MichaelChirico commented Jul 7, 2016

see also #1658 which i imagine is related

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Jul 7, 2016

Updated the example. Actually, the week() produces this: "Weeks is the number of complete seven day periods that have occured between the date and January 1st, plus one..."(see http://stackoverflow.com/questions/27116645/r-week-function-returns-unexpected-values). I think that this is an unexpected return that a lot of users are not aware of.

ghost commented Jul 7, 2016

Updated the example. Actually, the week() produces this: "Weeks is the number of complete seven day periods that have occured between the date and January 1st, plus one..."(see http://stackoverflow.com/questions/27116645/r-week-function-returns-unexpected-values). I think that this is an unexpected return that a lot of users are not aware of.

@DavidArenburg

This comment has been minimized.

Show comment
Hide comment
@DavidArenburg

DavidArenburg Jul 7, 2016

Contributor

According to you, what will most users expect for "2016-01-08"? Both lubridate and data.table will return 2, while according to this for instance, first week is until 2016-01-10. Did you actually look at the source code of data.table::week and lubridate::week? They are almost identical with a small change (which was implied by @MichaelChirico already) - both written in a very simple base R code which you could easily understand exactly what it's doing.

Contributor

DavidArenburg commented Jul 7, 2016

According to you, what will most users expect for "2016-01-08"? Both lubridate and data.table will return 2, while according to this for instance, first week is until 2016-01-10. Did you actually look at the source code of data.table::week and lubridate::week? They are almost identical with a small change (which was implied by @MichaelChirico already) - both written in a very simple base R code which you could easily understand exactly what it's doing.

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Jul 7, 2016

OK, you're right. lubridate:week() and data.table::week() are almost identical. Just checked the source code. Issue updated. We're using the lubridate:isoweek() function now for the actual calendar week.

ghost commented Jul 7, 2016

OK, you're right. lubridate:week() and data.table::week() are almost identical. Just checked the source code. Issue updated. We're using the lubridate:isoweek() function now for the actual calendar week.

@arunsrinivasan

This comment has been minimized.

Show comment
Hide comment
@arunsrinivasan

arunsrinivasan Jul 7, 2016

Member

Thanks @STATWORX for filing the issue (by following up from twitter). Perhaps we'll need an isoweek as well. Good to know.

Member

arunsrinivasan commented Jul 7, 2016

Thanks @STATWORX for filing the issue (by following up from twitter). Perhaps we'll need an isoweek as well. Good to know.

@arunsrinivasan

This comment has been minimized.

Show comment
Hide comment
@arunsrinivasan

arunsrinivasan Aug 27, 2016

Member

@MichaelChirico do you think you've some time to look up into isoweek?

Member

arunsrinivasan commented Aug 27, 2016

@MichaelChirico do you think you've some time to look up into isoweek?

@MichaelChirico

This comment has been minimized.

Show comment
Hide comment
@MichaelChirico

MichaelChirico Aug 28, 2016

Contributor

sure, why not =D

Contributor

MichaelChirico commented Aug 28, 2016

sure, why not =D

MichaelChirico added a commit to MichaelChirico/data.table that referenced this issue Aug 29, 2016

arunsrinivasan added a commit that referenced this issue Aug 29, 2016

Merge pull request #1829 from MichaelChirico/isoweek
Closes #1765 -- new isoweek function produces ISO 8601-consistent week numbering as an integer
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment