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

Faster second, minute and hour for objects of class ITime #3158

Closed
arunsrinivasan opened this issue Nov 21, 2018 · 3 comments · Fixed by #3782
Labels
Milestone

Comments

@arunsrinivasan
Copy link
Member

@arunsrinivasan arunsrinivasan commented Nov 21, 2018

The functions second, minute and hour currently have a faster method, by bypassing as.POSIXlt(), for POSIXct objectss under UTC. See here.

The faster logic needs to be also included for ITime objects. The fix is quite simple. The if statement in all these functions need an additional || inherits(x, "ITime")

Here's a quick benchmark:

# Current version
second  <- function(x) {
  if (inherits(x,'POSIXct') && identical(attr(x,'tzone'),'UTC')) {
    # if we know the object is in UTC, can calculate the hour much faster
    as.integer(x) %% 60L
  } else {
    as.integer(as.POSIXlt(x)$sec)
  }
}

# new version
fSecond<- function(x) {
  if (inherits(x, "ITime") || (inherits(x,'POSIXct') && identical(attr(x,'tzone'),'UTC'))) {
    # if we know the object is in UTC, can calculate the hour much faster
    as.integer(x) %% 60L
  } else {
    as.integer(as.POSIXlt(x)$sec)
  }
}

require(data.table)
x <- setattr(0:86399, "class", "ITime")
y <- sample(x, 50e7, TRUE)

system.time(a1 <- second(y))
#    user  system elapsed 
#   29.00   10.72   39.78 
system.time(a2 <- fSecond(y))
#    user  system elapsed 
#    3.51    0.56    4.06 
 
identical(a1, a2)
# [1] TRUE
@MichaelChirico

This comment has been minimized.

Copy link
Member

@MichaelChirico MichaelChirico commented Nov 21, 2018

@arunsrinivasan

This comment has been minimized.

Copy link
Member Author

@arunsrinivasan arunsrinivasan commented Nov 24, 2018

I agree it'll make things neat, but there are no other classes to consider here AFAICT. I think just an additional if-clause would be sufficient for now therefore.

@jangorecki

This comment has been minimized.

Copy link
Member

@jangorecki jangorecki commented Nov 24, 2018

There is Dirks nanotime which we already have in suggests. Having if-elses way will be good enough and easy to turn into S3 anyway.

MichaelChirico added a commit that referenced this issue Aug 20, 2019
@mattdowle mattdowle added this to the 1.12.4 milestone Aug 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.