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

Enhance Supporting Nonstandard Classes in train_continuous #166

Closed
billdenney opened this issue Aug 1, 2018 · 3 comments
Closed

Enhance Supporting Nonstandard Classes in train_continuous #166

billdenney opened this issue Aug 1, 2018 · 3 comments
Labels
feature a feature request or enhancement wip work in progress

Comments

@billdenney
Copy link
Contributor

Could train_continuous also include an if block for the case when existing is NULL to better work with nonstandard classes??

train_continuous <- function(new, existing = NULL) {
  if (is.null(new)) return(existing)
  
  if (is.factor(new) || !typeof(new) %in% c("integer", "double")) {
    stop("Discrete value supplied to continuous scale", call. = FALSE)
  }
  if (is.null(existing)) {
    suppressWarnings(range(new, na.rm = TRUE, finite = TRUE))
  } else {
    suppressWarnings(range(existing, new, na.rm = TRUE, finite = TRUE))
  }
}

This issue relates to r-quantities/units#164.

When trying to use train_continuous with a nonstandard class, it ends up calling range(existing, new) (

suppressWarnings(range(existing, new, na.rm = TRUE, finite = TRUE))
). With classes beyond base classes (in the case listed above, the 'units' class), this causes the class to be lost and yields issues down the line when using classes that cannot be directly compared.

@dpseidel dpseidel added the feature a feature request or enhancement label Aug 1, 2018
@hadley
Copy link
Member

hadley commented Aug 6, 2018

Looks good. Want to do a PR?

@billdenney
Copy link
Contributor Author

PR is coming shortly. FYI, I looked at train_discrete, and it wasn't obvious if it should be updated or not, so I didn't change its behavior.

@hadley
Copy link
Member

hadley commented Nov 5, 2019

I'm reverting your PR because it breaks ggforce, and I see from the other thread in units it doesn't fix the motivating problem.

hadley added a commit that referenced this issue Nov 5, 2019
As it breaks ggforce and doesn't fix the motivating problem described in #166.
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 wip work in progress
Projects
None yet
Development

No branches or pull requests

3 participants