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

Use Date's tz() when converting with to_posixct() #24

Merged
merged 1 commit into from
Jan 8, 2023

Conversation

AdamSpannbauer
Copy link
Contributor

Issue

Currently, to_posixct() assumes tz = "UTC" when converting a Date type. However, tz() checks for the "tzone" attribute. It is possible to run into dates with the "tzone" attribute and this can lead to unexpected results from to_posixct()

Example of unexpected results when using time_floor():

# Create a date with tzone attribute
x <- as.Date("2020-01-01")
attr(x, "tzone") <- "America/New_York"

packageVersion("timechange")
# [1] ‘0.1.1.9000’

# This version assumes date to be UTC in
# the `date_to_posixct()` conversion
timechange::time_floor(x, "months", 1)
# [1] "2019-12-31"

Proposed fix

Change to_posixct()'s call to date_to_posixct() to use tz(time) instead of assuming UTC. If the Date doesn't have a "tzone" attribute UTC will still be the default.

# after changing to_posixct() usage of date_to_posixct to:
# date_to_posixct(time, tz = tz(time))
timechange::time_floor(x, "months", 1)
# [1] "2020-01-01"

@vspinu
Copy link
Owner

vspinu commented Jan 8, 2023

Makes sense indeed. Thanks!

@vspinu
Copy link
Owner

vspinu commented Jan 8, 2023

Actually this simple solution is not quite enough. First, we don't actually want to end up with as.POSIXlt in as.Date for performance reasons. Second, the tzone argument is not propagated to the end result. Third, there are quite a few other places where this seems to be a problem (pretty much all the places where date is converted to posix). Let me have a closer look

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

Successfully merging this pull request may close these issues.

None yet

2 participants