joshuaulrich / xts

Extensible time series class that provides uniform handling of many R time series classes by extending zoo.
http://joshuaulrich.github.io/xts/
GNU General Public License v2.0
220 stars 71 forks source link

'UTC' equality can be extended to other equivalent time zones #319

Closed MichaelChirico closed 4 years ago

MichaelChirico commented 4 years ago

I just filed this at data.table: Rdatatable/data.table#4117

And this fix for base R: https://bugs.r-project.org/bugzilla/show_bug.cgi?id=17674

I notice also several usages of testing UTC value in xts:

xts/R/tzone.R:106:    if (!(tzone(x) %in% c("UTC","GMT")))
xts/R/endpoints.R:133:        Sys.getenv("TZ") %in% c("", "GMT", "UTC"))

If you're amenable, I would file a PR to an equivalent is_utc internal utility & replace the above instances

I also filed a similar issue for lubridate: https://github.com/tidyverse/lubridate/issues/844

joshuaulrich commented 4 years ago

Thanks for the suggestion! As I noted in the R bugzilla tracker, using switch() seems to be about as fast as checking for equality (e.g. tz == "UTC") and can cover all cases. What do you think of that solution?

in_utc <- function(tz) {
  utc_tz <- c("UTC", "GMT", "Etc/UTC", "Etc/GMT", "GMT-0", "GMT+0", "GMT0")
  if (is.null(tz)) tz <- Sys.timezone()
  return(tz %in% utc_tz)
}
is_utc <- function(tz) {
  if (is.null(tz)) {
    tz <- Sys.timezone()
  }
  switch(tz,
         "UTC" = ,
         "GMT" = ,
         "Etc/UTC" = ,
         "Etc/GMT" = ,
         "GMT-0" = ,
         "GMT+0" = ,
         "GMT0" = TRUE,
         FALSE)
}
tzones <- replicate(100, OlsonNames())
system.time(for(tz in tzones) is_utc(tz))  # ~0.035
system.time(for(tz in tzones) in_utc(tz))  # ~0.113
MichaelChirico commented 4 years ago

Looks great! I wonder why it's faster...