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

Unexpected result tzDiff #23

Closed
anderic1 opened this issue Nov 23, 2017 · 11 comments
Closed

Unexpected result tzDiff #23

anderic1 opened this issue Nov 23, 2017 · 11 comments

Comments

@anderic1
Copy link
Contributor

anderic1 commented Nov 23, 2017

Hi,

Using tzDiff with one of the timezones set to "Europe/London" gives unexpected results. Here is an example:

library(RcppCCTZ)
dt = as.Date("2010-01-01")
fromtz = "Europe/London"
totz="Europe/Berlin"
tzDiff(fromtz, totz, dt, verbose=TRUE)

The last line returns 0, where a 1 hour difference would be expected. verbose=TRUE seems to indicate the "Europe/London" date gets parsed as a +0100 timezone-offset instead of +0000.

As a comparison, both the following lines return a 1h difference:

parseDatetime(format(dt), "%Y-%m-%d", fromtz) - parseDatetime(format(dt), "%Y-%m-%d", totz)
as.POSIXct(format(dt), tz=fromtz) - as.POSIXct(format(dt), tz=totz)
@eddelbuettel
Copy link
Owner

eddelbuettel commented Nov 23, 2017

"Maybe".

All time zone libraries have odd corner cases, it seems. I chases a special case for London for a long time in my [anytime[(https://github.com/eddelbuettel/anytime) package (which uses the corresponding Boost library). In general, I think you want "UTC" instead as "London" implies some weird daylight savings rules that may be off:

Here it is with "UTC" also using a summer date:

library(RcppCCTZ)

fromtz <- "UTC"  #Europe/London"
totz <- "Europe/Berlin"

dt <- as.Date("2010-01-01")
tzDiff(fromtz, totz, dt, verbose=TRUE)

dt <- as.Date("2017-07-01")   # try DST
tzDiff(fromtz, totz, dt, verbose=TRUE)

@eddelbuettel
Copy link
Owner

eddelbuettel commented Nov 23, 2017

Also: your call was wrong in the sense that as.Date() does not return the type requested and expected by the function. You really want

dt <- as.POSIXct("2010-01-01")
# ...
dt <- as.POSIXct("2017-07-01")   # try DST

in which case you get 1 and 2 hours (to UTC) which, as I recall, are the correct answers.

@anderic1
Copy link
Contributor Author

My bad. With a POSIXct date all works as expected.
Thanks

@eddelbuettel
Copy link
Owner

I should test and/or convert though.

@anderic1
Copy link
Contributor Author

I could give it a shot. I also think vectorizing over the dt argument would be useful

@anderic1
Copy link
Contributor Author

Reopening to further discuss whether or not Dates should be supported

As I see it there are basically three options:

  1. Allow dates and shift them at midnight UTC
  2. Allow dates and shift them at midnight of the fromtz timezone
  3. Generate an error if dt is a Date

As mentioned in #24 , from a practical standpoint I think it makes sense to allow Dates. And between options 1. and 2. , either would work for me. 1. is perhaps more what R does (eg. as.POSIXct(Sys.Date())), while option 2. is perhaps more what a typical user of this function would expect(?).
Thoughts?

@anderic1 anderic1 reopened this Nov 24, 2017
@eddelbuettel
Copy link
Owner

My main problem is

R> as.POSIXct(Sys.Date())
[1] "2017-11-23 18:00:00 CST"
R> 

which makes of course total sense once you think it through:

  • Dates have no timezone, hence become UTC
  • I am in Chicago and hence six hours behind

but this is still far from useful.

@anderic1
Copy link
Contributor Author

You have convinced me, making it accept dates requires too many assumptions to be made.

I could modify #24 to only have it accept POSIXct, and error out otherwise. Let me know what you think

@eddelbuettel
Copy link
Owner

I was about to suggest that to. Go ahead and pile another commit (or two, if you need them) onto the PR. It looked good so far.

@anderic1
Copy link
Contributor Author

PR Updated

@eddelbuettel
Copy link
Owner

(Thanks. It's good form to keep it open and then add .... (closes: #23) in the title of the PR closing it. No need to reopen now though.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants