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

Cannot Use ggplot2 with units #164

Closed
billdenney opened this issue Aug 1, 2018 · 18 comments · Fixed by #294
Closed

Cannot Use ggplot2 with units #164

billdenney opened this issue Aug 1, 2018 · 18 comments · Fixed by #294

Comments

@billdenney
Copy link
Contributor

billdenney commented Aug 1, 2018

I just tried to plot data using ggplot2 with a units vector, and it yielded an error. Diving into the ggplot2 code, the issue comes down to range(NULL, units_value) returning a numeric vector which cannot be compared with a units class object. Specifically, this occurs in https://github.com/tidyverse/ggplot2/blob/8778b48b37d8b7e41c0f4f213031fb47810e70aa/R/range.r#L21-L25 which involves a call to scales::train_continuous which has either NULL or a numeric vector as the first argument to range.

I don't see a way to fix this without either converting the value I'm plotting to numeric (easy, but losing information and losing an opportunity to make a common pair of libraries more usable together) or converting the units library to an S4 class, but maybe you see a simpler way to do this?

library(units)                         
#> udunits system database from C:/Users/Bill Denney/Documents/R/win-library/3.4/units/share/udunits
library(ggplot2)                       
                                       
d <- data.frame(x=set_units(1:3, m),   
y=1:3)                                 
ggplot(d, aes(x=x, y=y)) + geom_point()
#> Don't know how to automatically pick scale for object of type units. Defaulting to continuous.
#> Error in Ops.units(x, range[1]): both operands of the expression should be "units" objects
@Enchufa2
Copy link
Member

Enchufa2 commented Aug 1, 2018

Was this working with the previous version of ggplot2?

Specifically, this occurs in https://github.com/tidyverse/ggplot2/blob/8778b48b37d8b7e41c0f4f213031fb47810e70aa/R/range.r#L21-L25 which involves a call to scales::train_continuous which has either NULL or a numeric vector as the first argument to range.

That's not the problem, range(NULL, set_units(1:3)) works ok. The problem is that this range is then compared against raw values to determine the limits of the axis, and units does not allow e.g. 3 < set_units(4, m).

The cleanest solution is to provide a ScaleUnits, but this is already included in the ggforce package, and in this way units doesn't need to import ggplot2, which is a quite heavy dependency. In fact, if you load ggforce, both the warning and the error go away.

@billdenney
Copy link
Contributor Author

I'm testing with ggplot2 version 2.2.1 (not 3.0).

If ggforce fixes it, maybe we can do the following (not tested):

scale_type.units <- function(x) {
  if (require(ggforce)) {
    ggforce::scale_type.units(x)
  } else {
    stop("Please install and load the ggforce library to plot units with ggplot2")
  }
}

@billdenney
Copy link
Contributor Author

One more note:

That's not the problem, range(NULL, set_units(1:3)) works ok. The problem is that this range is then compared against raw values to determine the limits of the axis, and units does not allow e.g. 3 < set_units(4, m).

The issue that I was pointing to is that the output of range above is a number without units. That output number is then compared yielding an error. If range returned a value with units for inputs of NULL and units, then ggplot2 would succeed (at least beyond that point).

@Enchufa2
Copy link
Member

Enchufa2 commented Aug 1, 2018

Yes, sorry, I messed up my explanation: the output of that range is numeric; the other operand of the comparison is units. Then the comparison fails.

The thing is that the first argument determines which method is dispatched. The first argument is NULL in this case, and thus range.units is never called, and the output of range cannot be units.

About scale_type.units, unfortunately exporting such a method would collide with ggforce generating an ugly warning. Let me explore whether it is possible to register and unregister it dynamically depending on whether ggforce is present or not, and if present, whether it is loaded or not.

@billdenney
Copy link
Contributor Author

Thanks for the clarification. I'll ask that the order in the call to range in scales::train_continuous be reversed so that it can use the class of the new value. (Or at least, it could ignore NULL.)

@edzer
Copy link
Member

edzer commented Sep 11, 2018

Has this issue been resolved?

@Enchufa2
Copy link
Member

Please, keep this open. This is low priority, because it's not a regression, but I would like to have a further look into this.

@billdenney
Copy link
Contributor Author

The fix to scales went through a few days ago. I’ve not had a chance to test it yet, but it may work with the development version of scales now.

@Enchufa2
Copy link
Member

@billdenney But anyway, I don't think that the error showed in your first message is acceptable. It should work ignoring units, but with a warning, or the error should point to ggforce. Otherwise, the user has no clue about what's happening. Yes, this is in the main vignette, but still...

@Enchufa2
Copy link
Member

Unfortunately, there's not much we can do here without moving ggforce to Imports, and that's not an option.

@billdenney
Copy link
Contributor Author

There is a fix that made it into the scales package that hopefully addresses this. (r-lib/scales#167)

@Enchufa2
Copy link
Member

Unfortunately, it doesn't. It avoids that error just to get into another one.

@billdenney
Copy link
Contributor Author

Shucks.

@krlmlr
Copy link
Contributor

krlmlr commented Dec 26, 2021

It works with ggforce::scale_x_unit() . I have a similar problem in pillar, I added the functionality without adding a strong dependency to ggplot2 or scales: r-lib/pillar#404.

It would be great if it worked out of the box with only units loaded. Perhaps borrow the implementation from ggforce, and adapt similarly to the pillar PR?

@Enchufa2
Copy link
Member

Initially, here we were trying to point to ggforce if it wasn't loaded, but found no way. Instead, I suppose you mean porting ggforce's implementation here, guarding it behind a stopifnot(requireNamespace("ggplot2", quietly=TRUE)), and adding ggplot2 to Suggests, right? It would work, certainly. But this would collide with ggforce (because exporting scale_type.units is unavoidable), and thus keeping that in ggforce would make little sense.

@thomasp85 How would you feel about moving scale_*_unit() here and deprecating ggforce's one? In 2016, this extension was requested in ggforce's repo because there was little experience with ggplot2 in this project, but nowadays I've learned about ggplot2's inner workings and I could maintain that code.

@thomasp85
Copy link

Fine with me

@Enchufa2 Enchufa2 reopened this Dec 26, 2021
@Enchufa2
Copy link
Member

Thanks, @thomasp85. I can send you a PR with deprecation warnings after porting the functionality here if you wish.

@thomasp85
Copy link

Please do 🙂

Enchufa2 added a commit that referenced this issue Dec 27, 2021
@Enchufa2 Enchufa2 linked a pull request Dec 27, 2021 that will close this issue
Enchufa2 added a commit that referenced this issue Dec 30, 2021
* integrate ggplot2 scales, closes #164

* update NEWS

* simplify workflows

* small fix

* raise error if scale_type is called when units is not attached

* complete missing test

* add tests
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 a pull request may close this issue.

5 participants