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

Should ymd() and similar return a date? #391

Closed
hadley opened this Issue Mar 11, 2016 · 14 comments

Comments

Projects
None yet
4 participants
@hadley
Member

hadley commented Mar 11, 2016

Currently, ymd() returns a POSIXct, which forces you to think about time zones. It seems like a reasonable principle to return a date in any situation where the time doesn't matter (this might include certain round_date() options).

@vspinu vspinu added this to the v1.5.6 milestone Mar 13, 2016

@vspinu

This comment has been minimized.

Member

vspinu commented Mar 13, 2016

Indeed. The tz argument to ymd is not meaningful either. I will deprecate it and return date instead.

@vspinu

This comment has been minimized.

Member

vspinu commented Mar 13, 2016

I have implemented this locally and realized that it leads to some ocassionall inconveniences.

When you need a POSIX object with HMS of 00:00:00 in UTC or other timezone, ymd("2013-01-01") was a nice way to achieve that. Now there is no easy way to arrive at that because as.POSIXct.Date ignores TZ:

> as.POSIXct(ymd("2013-01-01"), tz = "UTC")
[1] "2013-01-01 01:00:00 CET"

Our with_tz doesn't help either:

> str(with_tz(ymd("2013-01-01"), tz = "UTC"))
 Date[1:1], format: "2013-01-01"
> 

So you would need:

with_tz(as.POSIXct(ymd("2013-01-01")), tz = "UTC"))

So I am leaving tz argument in place and default it to NULL. When un-specified the returned object is Date. When specified it's POSIXct.

I am also leaving truncation functions alone for now. Till we have a clean and fast way to jump between Date and POSIX it's looks like bringing more hustle than it solves. Maybe we should consider a reverse of new as_date (#355).

@vspinu vspinu closed this in 44f461a Mar 13, 2016

@hadley

This comment has been minimized.

Member

hadley commented Mar 13, 2016

Yeah I realised afterwards that changing rounding wouldn't be a good idea

@hadley

This comment has been minimized.

Member

hadley commented Mar 13, 2016

I also wondered if maybe ymd_hms etc should work even the time is missing. I think I'll file a new issue about that and in general integrating the date parsing capabilities of lubridate and readr and maybe using cctz.

@vspinu

This comment has been minimized.

Member

vspinu commented Mar 13, 2016

ymd_hms etc should work even the time is missing.

It works if you pass the truncation parameter:

> ymd_hms("2013-01-01", truncated = 3)
[1] "2013-01-01 UTC"
@bonzodroog

This comment has been minimized.

bonzodroog commented May 4, 2016

FYI

This change might break existing ggplot visuals that plot according to a date variable and use some kind of scale_*_date().

Before you could get away with ymd("2013-01-01") type entries in your data frame and also in your scale_*_date(), and everything would work as expected. Now if you are plotting dates and want to adjust the scale, both the data frame and the argument to the scale function need a tz argument like ... ymd("2013-01-01", tz = "UTC").

Maybe ggplot2 could be updated to more gracefully handle Date objects (tz not specified, instead of POSIXct) as it effectively was doing before lubridate was updated??

Thanks

@vspinu

This comment has been minimized.

Member

vspinu commented May 4, 2016

Do you have concrete examples when ggplot doesn't handle Date objects "gracefully'? That should be reported on ggplot2 repo.

@vspinu

This comment has been minimized.

Member

vspinu commented May 8, 2016

@hadley this change had an unfortunate impact on comparison operators:

> ymd("2007-01-02") >= ymd_hms("2007-01-01 00:00:00")
[1] FALSE
Warning message:
Incompatible methods ("Ops.Date", "Ops.POSIXt") for ">=" 
> 

I think comparing Date and POSIX objects makes sense. So, I am considering overwriting Date Ops generic. What do you think?

@hadley

This comment has been minimized.

Member

hadley commented May 8, 2016

I'd rather push that towards R-core.

@bonzodroog

This comment has been minimized.

bonzodroog commented May 9, 2016

Thanks for looking in to this!

It looks like you have already nailed down the underlying problem, but here
is a short MWE of ggplot visuals that will fail because of the change to
lubridate. Let me know if I can help with anything else.

require(ggplot2)
require(lubridate)

works for all versions

df <- data.frame(Date = ymd(c(20151231, 20160131, 20160228, 20160331)),
Price = c(100, 101, 102, 103))
p <- ggplot(df) + geom_point(aes(Date, Price))
p

only works with older versions of lubridate

p <- p + scale_x_datetime(breaks = ymd(c("20160115", "20160215",
"20160315")),
labels = c("1/15/16", "2/15/16", "3/1/16"))
p

you will see this error message when using newer versions ...

#Error: Invalid input: time_trans works with objects of class POSIXct only

update required to work with new and old lubridate.

note the tz argument in both the data and the scale_x_datetime() function

df <- data.frame(Date = ymd(c(20151231, 20160131, 20160228, 20160331), tz =
"UTC"),
Price = c(100, 101, 102, 103))
p <- ggplot(df) + geom_point(aes(Date, Price))
p <- p + scale_x_datetime(breaks = ymd(c("20160115", "20160215",
"20160315"), tz = "UTC"),
labels = c("1/15/16", "2/15/16", "3/1/16"))
p

On Sun, May 8, 2016 at 12:03 PM, Hadley Wickham notifications@github.com
wrote:

I'd rather push that towards R-core.


You are receiving this because you commented.
Reply to this email directly or view it on GitHub
#391 (comment)

@vspinu

This comment has been minimized.

Member

vspinu commented Aug 3, 2016

So, I am considering overwriting Date Ops generic. What do you think?

I spent a good part of the night figuring how to do that. Nor S4 Compare group generic, nor S3 Ops worked as advertised. I eventually got all comparisons manually registered. Seem to work as expected now.

Let's wait and see if this has any undesirable effects, and then propose to R-core.

@yeedle

This comment has been minimized.

yeedle commented Aug 30, 2016

@hadley mentioned round_date in the opening post. Should round_date(as.POSIXct("2016-8-30 12:00:00 EDT"), "day"), or any unit greater than a day, return a POSIXct with the tz included? Is "2016-8-30 EDT" meaningful?

@bonzodroog

This comment has been minimized.

bonzodroog commented Aug 30, 2016

I would say no, "2016-08-30 EDT" is not meaningful generally.

On the other hand, with global weather or financial data, the difference
between 2016-08-30 in New York or Tokyo might be informative.

On Tue, Aug 30, 2016 at 1:38 PM, Abraham Neuwirth notifications@github.com
wrote:

@hadley https://github.com/hadley mentioned round_date in the opening
post. Should round_date(as.POSIXct("2016-8-30 12:00:00 EDT"), "day"), or
any unit greater than a day, return a POSIXct with the tz included? Is
"2016-8-30 EDT" meaningful?


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#391 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AQLBXLgpZYn33hKdXdNjNyy-SaE0YVnQks5qlHiIgaJpZM4Huv7b
.

@vspinu

This comment has been minimized.

Member

vspinu commented Aug 30, 2016

All rounding functions return POSIXct and that's very unlikely to change. So yes, rounding functions do preserve TZ because of that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment