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

full_seq only checks for tolerance in one direction #657

Closed
ha0ye opened this issue Jun 28, 2019 · 2 comments · Fixed by #690

Comments

@ha0ye
Copy link
Contributor

commented Jun 28, 2019

full_seq() has a tol parameter that is the tolerance for checking periodicity. However, the implementation of the check, which uses the %% operator only checks for tolerances in one direction:

tidyr::full_seq(c(0, 10, 20), 11, tol = 2)
#> Error: `x` is not a regular sequence.

Created on 2019-06-28 by the reprex package (v0.3.0)

Here's an updated full_seq.numeric() that I think solves the issue, and also pads the range if the last element falls short of being k*period more than the first element:

full_seq.numeric.patched <- function(x, period, tol = 1e-6) {
    rng <- range(x, na.rm = TRUE)
    if (any((x - rng[1]) %% period > tol & 
            period - (x - rng[1]) %% period > tol)) {
        stop("`x` is not a regular sequence.", call. = FALSE)
    }
    
    if (period - ((rng[2] - rng[1]) %% period) <= tol)
        rng[2] <- rng[2] + tol
            
    seq(rng[1], rng[2], by = period)
}

full_seq.numeric.patched(c(0, 10, 20), 11, tol = 2)
#> [1]  0 11 22
full_seq.numeric.patched(c(0, 10, 20), 11, tol = 1.8)
#> Error: `x` is not a regular sequence.
full_seq.numeric.patched(c(0, 10, 17), 11, tol = 5)
#> [1]  0 11 22
full_seq.numeric.patched(c(0, 10, 16), 11, tol = 5)
#> [1]  0 11

Created on 2019-06-28 by the reprex package (v0.3.0)

@hadley hadley added the feature label Jul 22, 2019
@hadley

This comment has been minimized.

Copy link
Member

commented Jul 24, 2019

Would you be interested in doing a PR?

@ha0ye

This comment has been minimized.

Copy link
Contributor Author

commented Jul 24, 2019

Would you be interested in doing a PR?

Happy to. I had included my proposed fix and tests in case that was easier for y'all than unsolicited PRs. :)

ha0ye added a commit to ha0ye/tidyr that referenced this issue Jul 25, 2019
…ut still within tolerance

(the %% operator only returns differences in one direction)
closes tidyverse#657
@hadley hadley closed this in #690 Jul 27, 2019
hadley added a commit that referenced this issue Jul 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.