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

as.ITime should round sub-second values instead of floor/truncate #2870

Closed
rossholmberg opened this issue May 12, 2018 · 4 comments
Closed

as.ITime should round sub-second values instead of floor/truncate #2870

rossholmberg opened this issue May 12, 2018 · 4 comments

Comments

@rossholmberg
Copy link

@rossholmberg rossholmberg commented May 12, 2018

(referencing closed issue #2156 as requested by Matt)

as.ITime converts time strings to integers, so it makes sense that any inputs with sub-second precision would be rounded to the nearest second. Unfortunately, it always rounds down (probably just truncating the input to discard decimal values), which doesn't seem like the best option to me.

The issue

For example:

> as.ITime( "00:00:10.999" )
[1] "00:00:10"

This isn't just being printed that way, the integer behind the scenes agrees with the printed value:

> as.numeric( as.ITime( "00:00:10.999" ) )
[1] 10

Comparison with chron::times

The chron package uses numeric instead of integer values, so this isn't an issue there:

> chron::times( "00:00:10.999" )
[1] 00:00:11

# multiply here to convert to number of seconds since midnight for comparison
> as.numeric( chron::times( "00:00:10.999" ) ) * 24*60*60
[1] 10.999

Comparison with as.POSIXct

as.POSIXct prints the "truncated" value, but retains the sub-second precision in the numeric value behind the scenes:

> as.POSIXct( "1970-01-01 00:00:10.999", tz = "UTC" )
[1] "1970-01-01 00:00:10 UTC"
> as.numeric( as.POSIXct( "1970-01-01 00:00:10.999", tz = "UTC" ) )
[1] 10.999

Something odd I noticed with as.POSIXct

Oddly, POSIXct seems to truncate sometimes, possibly depending on timezone, and possibly something to do with negative numbers. Not sure why this is, and it's not related to data.table at all, just something I noticed when looking at the above, so you might notice it too:

> as.POSIXct( "1970-01-01 00:00:10.999", tz = "UTC" )
[1] "1970-01-01 00:00:10 UTC"
> as.POSIXct( "1970-01-01 00:00:10.999", tz = "Australia/Melbourne" )
[1] "1970-01-01 00:00:11 AEST"
@MichaelChirico
Copy link
Member

@MichaelChirico MichaelChirico commented May 13, 2018

Regarding the last point, yes, it's to do with negative numbers -- truncation means always bringing the number closer to 0, which in this case means rounding "up" for negative numbers:

as.numeric(s.POSIXct( "1970-01-01 00:00:10.999", tz = "Australia/Melbourne" ))
# [1] -35989.001

structure(-35989, class = 'POSIXct', tzone = 'Australia/Melbourne')
# [1] "1970-01-01 00:00:11 AEST"

@MichaelChirico
Copy link
Member

@MichaelChirico MichaelChirico commented May 13, 2018

The fix is as simple as fixing this line... I'm not immediately sold that we should change the behavior, though...

as.ITime.POSIXlt <- function(x, ...) {
  structure(with(x, as.integer(sec) + min * 60L + hour * 3600L),
        class = "ITime")
}

Anyway, I'll file a PR to start the discussion with a solution in hand

@MichaelChirico
Copy link
Member

@MichaelChirico MichaelChirico commented May 13, 2018

Actually, I think internal behavior is inconsistent at the moment, IIUC:

as.ITime.times <- function(x, ...) {
  x <- unclass(x)
  daypart <- x - floor(x)
  secs <- as.integer(round(daypart * 86400))
  structure(secs,
        class = "ITime")
}

I don't know what a times object is, but it appears to record time in fractional days. And round is being applied, not as.integer...

@MichaelChirico
Copy link
Member

@MichaelChirico MichaelChirico commented May 13, 2018

Confirmed:

as.ITime(structure(10.999/86400, class = 'times'))
# [1] "00:00:11"

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

No branches or pull requests

3 participants