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

Why can't you exactly divide a year-long interval by days? #445

Closed
hadley opened this Issue Jul 27, 2016 · 4 comments

Comments

Projects
None yet
3 participants
@hadley
Member

hadley commented Jul 27, 2016

today <- today()
next_year <- today() + years(1)

(today %--% next_year) / days(1)
#> Remainder cannot be expressed as fraction of a period.
#>  Performing %/%.
#> estimate only: convert periods to intervals for accuracy
#> [1] 365

I think the warning "convert periods to intervals for accuracy" is also spurious. What period am I supposed to convert?

@garrettgman

This comment has been minimized.

Show comment
Hide comment
@garrettgman

garrettgman Jul 27, 2016

Member

I think this is a poorly written attempt to warn people away from / with periods (which does not have a precise definition) to %/% which has a precise definition.

/ doesn't have a precise definition because there is no clear rule about how the arithmetic operator should return the remainder in cases like this,

today <- today()
next_yearish <- today() + years(1) + days(1)

(today %--% next_yearish) / months(1)

Perhaps the message should just say something like, "Performing %/%; convert denominator to an interval or duration if you want a remainder."

Member

garrettgman commented Jul 27, 2016

I think this is a poorly written attempt to warn people away from / with periods (which does not have a precise definition) to %/% which has a precise definition.

/ doesn't have a precise definition because there is no clear rule about how the arithmetic operator should return the remainder in cases like this,

today <- today()
next_yearish <- today() + years(1) + days(1)

(today %--% next_yearish) / months(1)

Perhaps the message should just say something like, "Performing %/%; convert denominator to an interval or duration if you want a remainder."

@vspinu

This comment has been minimized.

Show comment
Hide comment
@vspinu

vspinu Jul 28, 2016

Member

I actually started removing those messages here and there. It's more annoying than helpful. The approximate computation with periods is stated in the docs in multiple places, so people who use those should be already aware of pitfalls.

Member

vspinu commented Jul 28, 2016

I actually started removing those messages here and there. It's more annoying than helpful. The approximate computation with periods is stated in the docs in multiple places, so people who use those should be already aware of pitfalls.

@vspinu

This comment has been minimized.

Show comment
Hide comment
@vspinu

vspinu Aug 2, 2016

Member

Given that for a bulk of arithmetic operations with shorter periods is exact and that we currently use surpressWarnings to hide these warnings in a bunch of places, maintaining these warnings appears wrong to me.

So, unless you object I am removing them and adding methods for some arithmetic operations with periods which currently throw errors.

Member

vspinu commented Aug 2, 2016

Given that for a bulk of arithmetic operations with shorter periods is exact and that we currently use surpressWarnings to hide these warnings in a bunch of places, maintaining these warnings appears wrong to me.

So, unless you object I am removing them and adding methods for some arithmetic operations with periods which currently throw errors.

@hadley

This comment has been minimized.

Show comment
Hide comment
@hadley

hadley Aug 3, 2016

Member

Yeah, I think it's a reasonable change.

Member

hadley commented Aug 3, 2016

Yeah, I think it's a reasonable change.

@vspinu vspinu closed this in b95e21d Aug 17, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment