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

Setting date labels loses tz #1718

Closed
hadley opened this issue Aug 17, 2016 · 9 comments
Closed

Setting date labels loses tz #1718

hadley opened this issue Aug 17, 2016 · 9 comments
Assignees
Labels
bug an unexpected problem or unintended behavior
Milestone

Comments

@hadley
Copy link
Member

hadley commented Aug 17, 2016

library(ggplot2)

df <- data.frame(
  time = as.POSIXct(as.Date("2010-01-01"), tz = "America/Chicago") + 1:10 * 3600,
  y = 1:10  
)

ggplot(df, aes(time, y)) + geom_point()
ggplot(df, aes(time, y)) + geom_point() + scale_x_datetime(date_labels = "%H:%M")
@bogdanstate
Copy link

bogdanstate commented Aug 19, 2016

In your example, there seems to be an issue with the timezone not being properly inherited from the Date object when using as.POSIXct. E.g.:

> attr(as.POSIXct(as.Date("2010-01-01"), tz="America/Chicago"), "tzone")
NULL
> attr(as.POSIXct("2010-01-01", tz="America/Chicago"), "tzone")
[1] "America/Chicago"

I can still reproduce the issue with the timezone reverting to UTC with the following example:

library(ggplot2)
library(gridExtra)

df <- data.frame(
  time = as.POSIXct("2010-01-01", tz = "America/Chicago") + 1:10 * 3600,
  y = 1:10  
)

p1 <- ggplot(df, aes(time, y)) + geom_point()
p2 <- ggplot(df, aes(time, y)) + geom_point() + scale_x_datetime(date_labels = "%H:%M %Z")
grid.arrange(p1, p2)

screenshot from 2016-08-18 20-26-52

The issue seems to be related to a call to scales::date_format not inheriting the time zone.

@hadley
Copy link
Member Author

hadley commented Aug 23, 2016

@thomasp85 I think you'll need to take a look at this one. I did a bit of exploration, and the basic problem is that the scale only gets the date time variables once they've been transformed to numeric, so the tzone attribute has been lost.

I think this implies that ScaleContinuousDatetime needs a fairly heavy rewrite. It's currently a thin wrapper around ScaleContinuous, but it is probably actually sufficiently different that it should be it's own beast - it doesn't really need a transform argument, the breaks and labels are specified differently, and the train() method needs to remember first non-NULL tzone attribute

The scale also needs a way to optionally override the tzone used by the data.

@hadley hadley added bug an unexpected problem or unintended behavior ready labels Aug 23, 2016
@hadley hadley added this to the v2.2.0 milestone Aug 23, 2016
@thomasp85 thomasp85 self-assigned this Aug 23, 2016
@thomasp85
Copy link
Member

My god, date/time scales is a mess... :-)

@thomasp85
Copy link
Member

thomasp85 commented Aug 25, 2016

Doesn't it make sense to still inherit from ScaleContinuous, but overwrite more than just the map() method?

It seems that despite the differences there is still a lot of overlaps

@hadley
Copy link
Member Author

hadley commented Aug 25, 2016

Possibly, but it needs a lot of overriding, and at a certain point it's just easier to not inherit behaviour (or pull out duplicated behaviour into other functions/smaller classes, i.e. composition over inheritance)

@thomasp85
Copy link
Member

Is it unthinkable that people for some reason would want to transform a timeseries?

@hadley
Copy link
Member Author

hadley commented Aug 25, 2016

I don't think I've ever seen a custom transformation in the wild, and I think you'd need that currently to (e.g) log transform a time axis. That said, I can imagine a transformation that (e.g.) dropped weekends, but I'm not sure if ggplot2 can handle non-one-to-one transforms.

@thomasp85
Copy link
Member

Couldn't this be solved using the train_complete() addition proposed in #1708, rather than a complete rewrite?

@hadley
Copy link
Member Author

hadley commented Sep 15, 2016

Oh yes, that's quite possible. If you make that change, I'll take a look at this code.

thomasp85 added a commit that referenced this issue Sep 26, 2016
Fixes #1718

* Store and access timezone when making labels and breaks

* Add default timezone argument
@lock lock bot locked as resolved and limited conversation to collaborators Jun 19, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug an unexpected problem or unintended behavior
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants