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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

`duration` ignores class of first argument #462

Closed
klmr opened this Issue Aug 26, 2016 · 5 comments

Comments

Projects
None yet
3 participants
@klmr

klmr commented Aug 26, 2016

See 馃敆 Stack Overflow question.

In a nutshell, the following code

time = now()
five_sec = interval(time, time + 5)
duration(five_sec)
# [1] "5s"
duration(five_sec, units = 'days')
# [1] "432000s (~5 days)"

Yields an unexpected output for the last line because the class of five_sec is ignored, and its unit is overridden to be interpreted as days. This makes sense for a unitless num argument 鈥 but five_sec isn鈥檛 unitless by virtue of being an Interval.

The expected behaviour is that this should be disallowed 鈥斅爄.e. throw an error, or at least signal a stern warning that the interval鈥檚 unit is being ignored.

@Christoph999

This comment has been minimized.

Show comment
Hide comment
@Christoph999

Christoph999 Aug 26, 2016

I thought the result is transferred to the specified unit. In the case above I would have expected 5/(24*3600). In case of years (which can have 365 or 366 days) this feature is really helpful.

Christoph999 commented Aug 26, 2016

I thought the result is transferred to the specified unit. In the case above I would have expected 5/(24*3600). In case of years (which can have 365 or 366 days) this feature is really helpful.

@vspinu vspinu closed this in 415a36d Aug 26, 2016

@vspinu

This comment has been minimized.

Show comment
Hide comment
@vspinu

vspinu Aug 26, 2016

Member

Yes. It should be disallowed. For conversion use as.duration.

Member

vspinu commented Aug 26, 2016

Yes. It should be disallowed. For conversion use as.duration.

@Christoph999

This comment has been minimized.

Show comment
Hide comment
@Christoph999

Christoph999 Aug 26, 2016

Thank you. Perhaps you can add a comment somewhere that for conversion you
need
as.numeric(as.duration(i1), units = "days")
or
as.numeric(i1, units = 'days') # Throwing a message on my computer - I don't know the reason.

I could not find it in the descrition nor in the article "Dates and Times
Made Easy with lubridate"...

Christoph999 commented Aug 26, 2016

Thank you. Perhaps you can add a comment somewhere that for conversion you
need
as.numeric(as.duration(i1), units = "days")
or
as.numeric(i1, units = 'days') # Throwing a message on my computer - I don't know the reason.

I could not find it in the descrition nor in the article "Dates and Times
Made Easy with lubridate"...

@vspinu

This comment has been minimized.

Show comment
Hide comment
@vspinu

vspinu Aug 26, 2016

Member

Hm. I am starting thinking this was a mistake. There might be plenty of derived classes from numeric that are valid inputs into duration constructor. The fact that lubridate's interval and period inherit from numeric was a mistake. It might be changed some day.

I am replacing this change with a much weaker check for "Period" and "Interval" class.

Member

vspinu commented Aug 26, 2016

Hm. I am starting thinking this was a mistake. There might be plenty of derived classes from numeric that are valid inputs into duration constructor. The fact that lubridate's interval and period inherit from numeric was a mistake. It might be changed some day.

I am replacing this change with a much weaker check for "Period" and "Interval" class.

@vspinu vspinu reopened this Aug 26, 2016

@vspinu vspinu closed this in 5a3f057 Aug 26, 2016

@vspinu

This comment has been minimized.

Show comment
Hide comment
@vspinu

vspinu Aug 26, 2016

Member

as.numeric(i1, units = 'days')# Throwing a message on my computer - I don't know the reason.

Not anymore in the development version of lubridate.

Member

vspinu commented Aug 26, 2016

as.numeric(i1, units = 'days')# Throwing a message on my computer - I don't know the reason.

Not anymore in the development version of lubridate.

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