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

Allow subsecond intervals with date_breaks() #160

Merged
merged 5 commits into from
Jul 25, 2018

Conversation

dpseidel
Copy link
Collaborator

@dpseidel dpseidel commented Jul 24, 2018

Closes #85

This allows support for subsecond width arguments to work with date_breaks()

I also caught a bug whereby the to_time() function in floor_time() was mishandling the timezone of "min" and "sec" intervals, so I have adjusted that to match the tz handling for all other units of time.

Copy link
Member

@hadley hadley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why didn't this work before? Because seq.POSIXct() doesn't support partial seconds? (even though it supports other partial units?)

R/date-time.r Outdated
floor_time <- function(date, time) {
to_time <- function(x) {
force(x)
structure(x, class = c("POSIXt", "POSIXct"))
structure(x, class = c("POSIXt", "POSIXct"),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please indent so each argument to structure() goes on its own line?

R/date-time.r Outdated
floor_time <- function(date, time) {
to_time <- function(x) {
force(x)
structure(x, class = c("POSIXt", "POSIXct"))
structure(x, class = c("POSIXt", "POSIXct"),
tzone = attr(date, "tzone", exact = TRUE) %||% "")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure you need the %||% "" here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did this to match what is being called for other units in L32 but no I did not fully think through whether it's necessary in either place.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah ok, let's keep it consistent then

@dpseidel
Copy link
Collaborator Author

dpseidel commented Jul 24, 2018

seq.POSIXct() doesn't support partial seconds that are represented by character strings. Only numerics.

options("digits.secs" = 2)
times <- as.POSIXct(c("2000-01-01 08:29:58", "2000-01-01 08:30:10"), tz = "UTC")
seq.POSIXt(times[1], times[2], by = "1 sec")
#>  [1] "2000-01-01 08:29:58 UTC" "2000-01-01 08:29:59 UTC"
#>  [3] "2000-01-01 08:30:00 UTC" "2000-01-01 08:30:01 UTC"
#>  [5] "2000-01-01 08:30:02 UTC" "2000-01-01 08:30:03 UTC"
#>  [7] "2000-01-01 08:30:04 UTC" "2000-01-01 08:30:05 UTC"
#>  [9] "2000-01-01 08:30:06 UTC" "2000-01-01 08:30:07 UTC"
#> [11] "2000-01-01 08:30:08 UTC" "2000-01-01 08:30:09 UTC"
#> [13] "2000-01-01 08:30:10 UTC"

seq.POSIXt(times[1], times[2], by = ".5 sec")
#> Error in seq.int(0, to0 - from, by): invalid '(to - from)/by'

seq.POSIXt(times[1], times[2], by = .5)
#>  [1] "2000-01-01 08:29:58.0 UTC" "2000-01-01 08:29:58.5 UTC"
#>  [3] "2000-01-01 08:29:59.0 UTC" "2000-01-01 08:29:59.5 UTC"
#>  [5] "2000-01-01 08:30:00.0 UTC" "2000-01-01 08:30:00.5 UTC"
#>  [7] "2000-01-01 08:30:01.0 UTC" "2000-01-01 08:30:01.5 UTC"
#>  [9] "2000-01-01 08:30:02.0 UTC" "2000-01-01 08:30:02.5 UTC"
#> [11] "2000-01-01 08:30:03.0 UTC" "2000-01-01 08:30:03.5 UTC"
#> [13] "2000-01-01 08:30:04.0 UTC" "2000-01-01 08:30:04.5 UTC"
#> [15] "2000-01-01 08:30:05.0 UTC" "2000-01-01 08:30:05.5 UTC"
#> [17] "2000-01-01 08:30:06.0 UTC" "2000-01-01 08:30:06.5 UTC"
#> [19] "2000-01-01 08:30:07.0 UTC" "2000-01-01 08:30:07.5 UTC"
#> [21] "2000-01-01 08:30:08.0 UTC" "2000-01-01 08:30:08.5 UTC"
#> [23] "2000-01-01 08:30:09.0 UTC" "2000-01-01 08:30:09.5 UTC"
#> [25] "2000-01-01 08:30:10.0 UTC"

@hadley
Copy link
Member

hadley commented Jul 25, 2018

Can you please add a comment explaining that seq() doesn't support partial seconds? Otherwise, is good to merge.

@dpseidel dpseidel merged commit 37668bf into r-lib:master Jul 25, 2018
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.

2 participants