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

"stage()" does not work properly when prefixed with "ggplot2::" #6104

Open
HMU-WH opened this issue Sep 14, 2024 · 10 comments · May be fixed by #6107, #6108 or #6110
Open

"stage()" does not work properly when prefixed with "ggplot2::" #6104

HMU-WH opened this issue Sep 14, 2024 · 10 comments · May be fixed by #6107, #6108 or #6110

Comments

@HMU-WH
Copy link

HMU-WH commented Sep 14, 2024

When I use a function from ggplot2 in my own R package, I need to prefix it with "ggplot2::". I found that, in this case, "stage()" does not work properly!

Here is the code to reproduce the bug:
This is the original example in ggplot2. I just changed "stage" to "ggplot2::stage".

ggplot(mpg, aes(class, displ)) +
  geom_violin() +
  stat_summary(
    aes(
      y = ggplot2::stage(displ, after_stat = 8),
      label = after_stat(paste(mean, "±", sd))
    ),
    geom = "text",
    fun.data = ~ round(data.frame(mean = mean(.x), sd = sd(.x)), 2)
  )

Throws the following error:

Error in `stat_summary()`:
! Problem while mapping stat to aesthetics.Error occurred in the 2nd layer.
Caused by error:
! object 'displ' not found
@yutannihilation
Copy link
Member

I need to prefix it with "ggplot2::"

While I agree ggplot2::stage() should work, I think you can import it to avoid the "Undefined global functions or variables" warnings.

#' @importFrom ggplot2 stage

@HMU-WH
Copy link
Author

HMU-WH commented Sep 16, 2024

I need to prefix it with "ggplot2::"

While I agree ggplot2::stage() should work, I think you can import it to avoid the "Undefined global functions or variables" warnings.

#' @importFrom ggplot2 stage

I agree with your suggestion, but I still hope that ggplot2::stage() can work properly, because according to the R language specification, this should not cause an error. After all, this is not a symbolic function.

@teunbrand
Copy link
Collaborator

teunbrand commented Sep 16, 2024

I think what is happening is that the stage() function is masked here to evaluate the after_stat component, but this masking doesn't work due to the ggplot2::-prefix, which uses the original unmasked stage() and thus evaluates the wrong component. I'm not sure yet what would be the best way to fix this.

@HMU-WH
Copy link
Author

HMU-WH commented Sep 16, 2024

I think what is happening is that the stage() function is masked here to evaluate the after_stat component, but this masking doesn't work due to the ggplot2::-prefix, which uses the original unmasked stage() and thus evaluates the wrong component. I'm not sure yet what would be the best way to fix this.

Sorry, with my current ability, I am not capable of understanding the source code you pointed out in a short period of time.

However, it can be confirmed that there are indeed some flaws in the evaluation operation of the stage( ) function.

In addition to the issues I mentioned earlier, I have also noticed the following situations:

library(ggplot2)
library(patchwork)

by <- "cut"
p <- ggplot(diamonds, aes(x = cut, y = price))

p1 <- p + geom_violin(aes(colour = .data[[by]], fill = after_scale(color)), trim = FALSE)
p2 <- p + geom_violin(aes(colour = .data[[by]], fill = stage(!!sym(by), after_scale = color)), trim = FALSE)
p3 <- p + geom_violin(aes(colour = .data[[by]], fill = stage(.data[[by]], after_scale = color)), trim = FALSE)

p1 | p2 | p3

image

The legend of "P3" is inconsistent with "P1" and "P2". Of course, I understand that it can be corrected using p3 + labs(fill = by), but in my opinion, these three methods should produce the same image.

@teunbrand
Copy link
Collaborator

teunbrand commented Sep 16, 2024

I have also noticed the following situations:

This particular situation should already be fixed in the current development version of ggplot2.

@yutannihilation
Copy link
Member

To be comprehensive, I think we have three options about the level of fix.

  1. Not fix. We can say, for example, after_scale() is a syntax rather than a usual function, so qualified form (ggplot2::) is not intended. cf. tidyselect didn't even export where() for a long time (Export where() r-lib/tidyselect#201).
  2. Fix only ggplot2::.
  3. Fix all cases including when stage() is re-exported.

I'd vote for 3 or 1. So, I like #6107 more because the approach of #6108 probably cannot handle the re-export cases.

@teunbrand
Copy link
Collaborator

teunbrand commented Sep 16, 2024

Yeah you make a good point. So if we want this:

library(ggplot2)

ggplot(mpg, aes(displ, hwy)) +
  geom_point(
    aes(fill = stage(drv, after_scale = alpha(fill, 0.3))),
    shape = 21
  )

We cannot do.call() or otherwise construct stage(). Note that the points are not transparent.

ggplot(mpg, aes(displ, hwy)) +
  geom_point(
    aes(fill = do.call("stage", list(start = drv, after_scale = quote(alpha(fill, 0.5))))),
    shape = 21
  ) +
  labs(fill = "drv")

Nor can we use stage wrappers. Again, points are not transparent.

restage <- function(...) stage(...)

ggplot(mpg, aes(displ, hwy)) +
  geom_point(
    aes(fill = restage(drv, after_scale = alpha(fill, 0.3))),
    shape = 21
  ) +
  labs(fill = "drv")

Created on 2024-09-16 with reprex v2.1.1

@thomasp85
Copy link
Member

I personally holds the position that stage(), after_stat(), the old stat(), etc are all decorators, more than actual functions, that instructs aes() how to interpret the mapping.

I know this is a distinction we cannot necessary expect the users to know or understand, but I do think we can make a case for not allowing alternate call constructions

@teunbrand
Copy link
Collaborator

I don't think that is unreasonable. But if these are just syntax instead of functions, then I think the following should also work:

ggplot2::ggplot(ggplot2::mpg, ggplot2::aes(displ, hwy)) +
  ggplot2::geom_point(
    ggplot2::aes(fill = stage(drv, after_scale = scales::alpha(fill, 0.3))),
    shape = 21
  )
#> Error in `ggplot2::geom_point()`:
#> ! Problem while computing aesthetics.
#> ℹ Error occurred in the 1st layer.
#> Caused by error in `stage()`:
#> ! could not find function "stage"

Created on 2024-09-16 with reprex v2.1.1

@thomasp85
Copy link
Member

That I agree on

@teunbrand teunbrand linked a pull request Sep 17, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants