Fix timezone issues in scale_*_datetime

[thomasp85]

Fixes #1718

[thomasp85]

@hadley I think I've been able to fix the datetime scale with minimal implication for surrounding code - can you stress-test it?

There are some points you may not like:

• We reassign the transformation function if a timezone exists - this is needed as the scale constructor has no acces to the data. This should be safe as the user has no access to pass transformation functions to the scale so we know it is a time transformation object.
• We assign a function with access to self as the labeller and postpone creation of the labeller function until it is actually called - this is needed so the labeller function gets the timezone for the same reasons as with the trans object

Example adapted from #1718

df <- data.frame(
  time = as.POSIXct("2010-01-01", tz = "America/Chicago") + 1:10 * 3600,
  y = 1:10  
)
ggplot(df, aes(time, y)) + geom_point() + scale_x_datetime(date_labels = "%H:%M %Z")

[hadley]

I think the default should be "", which means the default local timezone.

[hadley]

And can we add a tz parameter to the constructor to override the default?

[thomasp85]

How should this behave if there is a mismatch between the timezone in the data and the one provided in the constructor? Should the constructor value only kick in if the data does not have a timezone provided?

[hadley]

I think you can drop this warning, as it's not very important.

[hadley]

I'll write up a few unit tests for you too

[hadley]

These tests all look good to me:

base_time <- function(tz = "") {
  as.POSIXct(strptime("2015-06-01", "%Y-%m-%d", tz = tz))
}

df <- data.frame(
  time1 = base_time("") + 0:6 * 3600,
  time2 = base_time("UTC") + 0:6 * 3600,
  time3 = base_time("Australia/Lord_Howe") + (0:6 + 13) * 3600, # has half hour offset
  y = seq_along(x)
)

test_that("inherits timezone from data", {
  # Local time
  p <- ggplot(df, aes(y = y)) + geom_point(aes(time1))
  sc <- layer_scales(p)$x
  expect_equal(sc$timezone, NULL)
  expect_equal(sc$get_labels()[1], "00:00") 

  # UTC
  p <- ggplot(df, aes(y = y)) + geom_point(aes(time2))
  sc <- layer_scales(p)$x
  expect_equal(sc$timezone, "UTC")
  expect_equal(sc$get_labels()[1], "00:00") 
})


test_that("first timezone wins", {
  p <- ggplot(df, aes(y = y)) +
    geom_point(aes(time2)) + 
    geom_point(aes(time3), colour = "red") + 
    scale_x_datetime(date_breaks = "hour", date_labels = "%H:%M")
  sc <- layer_scales(p)$x
  expect_equal(sc$timezone, "UTC")
})

test_that("not cached across calls", {
  scale_x <- scale_x_datetime(date_breaks = "hour", date_labels = "%H:%M")

  p1 <- ggplot(df, aes(y = y)) + geom_point(aes(time2)) + scale_x
  p2 <- ggplot(df, aes(y = y)) + geom_point(aes(time3)) + scale_x

  expect_equal(layer_scales(p1)$x$timezone, "UTC")
  expect_equal(layer_scales(p2)$x$timezone, "Australia/Lord_Howe")
})

[hadley]

LGTM

[thomasp85]

And the timezone default stuff..?

[hadley]

Oh my tests seem to suggest that it all works currently. I certainly couldn't figure out how to elicit the bug

[thomasp85]

I was more referring to your wish for an argument in the constructor to set the timezone

[hadley]

Oh, yeah, please do that!

[thomasp85]

Should the argument be ignored in the presence of one encoded in the data?

[hadley]

No, it should override the data. (i.e. it should default to NULL)

[thomasp85]

So if data has one timezone and the scale has another it should first convert the data to the other timezone, or should the original timezone simply be ignored?

[hadley]

There's no converting - the tzone attribute just controls how the instant in time is displayed.

[thomasp85]

I see, that's easy then

[thomasp85]

You can now do this:

df <- data.frame(
  time = as.POSIXct("2010-01-01", tz = "America/Chicago") + 1:10 * 3600,
  y = 1:10  
)
ggplot(df, aes(time, y)) + geom_point() + 
  scale_x_datetime(date_labels = "%H:%M %Z", timezone = "CET")

[hadley]

Looks good!

[lock]

This old issue has been automatically locked. If you believe you have found a related problem, please file a new issue (with reprex) and link to this issue. https://reprex.tidyverse.org/