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

should `year()` return an integer instead of a double? #361

Closed
wibeasley opened this issue Dec 5, 2015 · 3 comments · Fixed by #814
Labels

Comments

@wibeasley
Copy link
Contributor

@wibeasley wibeasley commented Dec 5, 2015

Is there any advantage/disadvantage to the year() function returning an integer instead of a numeric?

> (y <- lubridate::year(as.Date("2012-05-09")))
[1] 2012
> class(y)
[1] "numeric"

It's certainly not a real problem, but I've noticed that I almost always wrap it in as.integer() because it's (a) a grouping variable in dplyr or (b) being written to a database.

@wibeasley

This comment has been minimized.

Copy link
Contributor Author

@wibeasley wibeasley commented Dec 5, 2015

I poked around a little more and saw that @gregorp did this in his fork: gregorp@2a9ad5a

If you like the functionality change, tell me if you'd like me to write/modify the tests .

@vspinu

This comment has been minimized.

Copy link
Member

@vspinu vspinu commented Dec 5, 2015

The same applies to other accessors, except day which already returns an integer. So if year to be changed, all others (except second) should be changed as well. That also includes Period objects which enforce integers during the construction but don't have integer representation internally. Rather messy.

So the extent of the change is non-trivial and is likely to cause S4 related breakage internally and for dependent packages. Why did you propose the change in the first place? What's your usage pattern?

Also note that the fix shouldn't be with as.integer but rather modifying + 1900, to + 1900L. as.POSIXlt(..)$year returns an integer.

@wibeasley

This comment has been minimized.

Copy link
Contributor Author

@wibeasley wibeasley commented Dec 5, 2015

Thanks for the explanations, @vspinu.

the extent of the change is non-trivial and is likely to cause S4 related breakage internally and for dependent packages

Ok. It may not make sense, but I thought I'd suggest it anyway. I assumed that it wouldn't cause numerical problems because the 32-bit integer would always be implicitly cast to the 64-bit float by R. But maybe I'm missing something, or maybe there's an S4 type problem that I'm unaware of.

Why did you propose the change in the first place? What's your usage pattern?

Because I use year a lot as a grouping variable or in other scenarios where I'm reluctant to use a floating point value in numerical comparisons. And I write a lot to databases, and the smaller data type makes things a tad more efficient. I guess it's a left over from my OO days, and using the most specific data type possible.

Also, I'm not aware of a scenario where I'd want 'year()' to validly return a non-integer. But I certainly could be overlooking an important use case.

note that the fix shouldn't be with as.integer but rather modifying + 1900, to + 1900L.

Cool. Makes sense.

as.POSIXlt(..)$year returns an integer.

Cool. I wasn't aware of that. Maybe that's another argument lubridate::year() should too. ducks

@vspinu vspinu added this to the v1.6.0 milestone Mar 13, 2016
@vspinu vspinu modified the milestone: v1.6.0 Aug 17, 2016
@hadley hadley added reprex feature and removed question labels Nov 19, 2019
hadley added a commit that referenced this issue Nov 19, 2019
Fixes #361
@hadley hadley removed the reprex label Nov 19, 2019
@hadley hadley added the wip label Nov 19, 2019
@vspinu vspinu closed this in #814 Dec 2, 2019
vspinu added a commit that referenced this issue Dec 2, 2019
[Fix #361] Ensure all accessors return integers
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.