[Request] speed up `as.ITime` for string inputs

[rossholmberg]

I've noticed that as.ITime is surprisingly slow when given character inputs (ie: of the format %H:%M:%S), and discovered that it's actually much faster to convert strings to ITime via an intermediate step. I'll show the time difference first:

# set up some inputs
set.seed( 1 )
x <- as.character( setattr(
    sample( seq_len( 24*60*59 ), 1E5, replace = TRUE ),
    "class",
    "ITime"
) )
head( x )
# [1] "06:15:58" "08:46:56" "13:31:10" "21:26:02" "04:45:35" "21:12:08"

Doing a direct conversion using as.ITime is very slow compared to using an intermediate as.chron.ITime step (the same is true using chron::times first:

microbenchmark::microbenchmark(
    direct = { direct <- as.ITime( x ) },
    twostep = { twostep <- as.ITime( as.chron.ITime( x ) ) },
    times = 10
)
# (I'll just show the median times here to make it easier to read)
# Unit: milliseconds
#     expr     median
#  direct   1808.9899
# twostep    115.9776

# check the output
identical( direct, twostep )
# [1] TRUE

Notice the significant speed increase (~15x), just by adding an intermediate step.

The same can be achieved in a less direct manner, by converting the numeric times values to integer, then converting to ITime with setattr instead of as.ITime. This way ("fivestep") gives basically the same speed increase, so I'm really just adding here as an option.

microbenchmark::microbenchmark(
    twostep = { twostep <- as.ITime( as.chron.ITime( x ) ) },
    fivestep = { 
        fivestep <- as.integer( round( as.chron.ITime( x ) * 86400 ) )
        setattr( fivestep, "class", "ITime" )
    },
    times = 100
)

# Unit: milliseconds
#     expr   median
#   twostep  122.2765
#  fivestep  119.9396

identical( direct, fivestep )
# [1] TRUE

My suggestion would be to build this into as.ITime.character, but my own attempts have failed to maintain the same reliability as the existing function.

NOTE: This does have the complication that is works perfectly when the format is "%H:%M:%S", but I believe the round may need to be floor if it's to maintain consistency when the input format is "%H:%M:%OS". Note round is still more appropriate in my opinion, and would maintain better consistency with chron::times, but as.ITime currently rounds down, so floor would maintain that for input format "%H:%M:%OS". Eg:

data.table::as.ITime( "12:00:00.99" )
# [1] "12:00:00"

chron::times( "12:00:00.99" )
# [1] 12:00:01

data.table::as.ITime( chron::times( "12:00:00.99" ) )
# [1] "12:00:01"

[MichaelChirico]

Possibly related to #1383. I guess as.ITime is overly flexible since IDateTime passes full date strings to as.ITime, whereas chron::times knows to expect H:M:S-type strings. Probably the chron approach is better in the long run.

[rossholmberg]

@MichaelChirico yes, I agree. I personally don't mind the "h:m:s" pickiness of chron::times, so I don't see it as a problem here. I also much prefer the more logical rounding behaviour of times as opposed to the "always round down" behaviour of as.ITime, so I see that difference as a pro.

If those differences are agreeable, then the change in as.ITime.character is trivial. I'd be more than happy to make a pull request with the change if that's the case.

PS: I did see #1383, but figured this was different enough... thanks for linking.

[MichaelChirico]

I mainly linked so we can keep track of how to update one when the other is updated.

I tend to agree with your points -- it may be down to question of API design. What do you think @mattdowle ?

[rossholmberg]

Closing this issue, since conversions have been sped up significantly with v1.11.0 (as per #1392 ). The tests mentioned above still show an intermediate as.chron.ITime() call to speed up the conversions by about 2x over plain as.ITime on its own, but as mentioned above the cost is imposing the h:m:s format, so it's not a zero-sum game:

microbenchmark::microbenchmark(
     direct = { direct <- as.ITime( x ) },
     twostep = { twostep <- as.ITime( as.chron.ITime( x ) ) },
     times = 10
 )

# Unit: milliseconds
# expr       mean
# direct     284.7810
# twostep    103.1194

# check the output
identical( direct, twostep )
# [1] TRUE

I'd personally like to see the rounding of sub-second times to behave differently, such that a time of, for example "12:00:00.99" would evaluate to "12:00:01" rather than "12:00:00", but that's not the key issue here.

[mattdowle]

Thanks for the update @rossholmberg. Could you raise a new issue for the rounding request and link to it here please. I doubt there's any debate that providing that option would be useful. Then it's just a case of what the default rounding should be. A global option could be provided.

[rossholmberg]

No worries Matt. New issue for rounding of sub-second values when converted to ITime is at #2870