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

Feature request: flexibility of date and time transformations w.r.t. numeric input. #298

Closed
teunbrand opened this issue Oct 15, 2020 · 3 comments · Fixed by #336
Closed
Labels
feature a feature request or enhancement help wanted ❤️ we'd love your help!
Milestone

Comments

@teunbrand
Copy link
Contributor

Originally discussed in tidyverse/ggplot2#980 (comment); it seems as there is some need for flexibility of date and time transformations. These transformations are currently harder to program with than plain numerical transformations, as the date/time transformations have strict requirements on the class of the input data.

@yutannihilation made two excellent suggestions which I'll repeat here.

  1. Let the domain part of the transformation be of the same class as the expected input data. This in particular would allow a solution to a longstanding issue in ggplot2.
  2. Let date_trans()$transform() and time_trans()$transform() accept numeric values. This would solve the aforementioned issue, as well as some others.

I understand that the class requirements protect against unintentional mistakes so I'd like to mention a 3rd idea.

  1. Add ... arguments to all trans$transform() arguments and in addition a force argument to the date/time transforms. The logic then could be as follows:
function(x, force = FALSE, ...)  
{
  if (!inherits(x, "Date") & !force) {
    stop("Invalid input: date_trans works with objects of class Date only", 
         call. = FALSE)
  }
  structure(as.numeric(x), names = names(x))
}

This would allow only the transformation of intentional numeric values while still raising the error upon unintentional input.

Thanks for reading!

@teunbrand
Copy link
Contributor Author

There is a case to be made for handling non-finite input to date/time transformations too, see tidyverse/ggplot2#4308 or https://stackoverflow.com/questions/67177555/while-adding-a-semi-transparent-layer-to-background-image-in-r-getting-error-in?noredirect=1.

@hadley hadley added feature a feature request or enhancement help wanted ❤️ we'd love your help! labels Mar 17, 2022
@hadley
Copy link
Member

hadley commented Mar 17, 2022

I think I'd need to see a concrete implementation to understand what you're suggesting here. My main worry is (e.g.) accidentally passing a date to date-time transformer, so you get number of days instead of number of seconds. I don't think calling as.Date() or as.POSIXct() on the input is sufficient since they have rather surprising behaviours (e.g. as.Date(ISOdatetime(2021, 1, 1, 20, 59, 00, tz = "America/Chicago")))

@hadley hadley added this to the v1.2.0 milestone Mar 20, 2022
@hadley
Copy link
Member

hadley commented Mar 21, 2022

After reading the original issue, I think the easiest fix for the original problem is to give the domain the correct type.

hadley added a commit that referenced this issue Mar 21, 2022
hadley added a commit that referenced this issue Mar 24, 2022
And fix partial match warnings

Fixes #298
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature a feature request or enhancement help wanted ❤️ we'd love your help!
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants