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

Allow scale name to be a function. #4313

Open
eliocamp opened this issue Jan 11, 2021 · 13 comments
Open

Allow scale name to be a function. #4313

eliocamp opened this issue Jan 11, 2021 · 13 comments
Labels
feature a feature request or enhancement scales 🐍

Comments

@eliocamp
Copy link
Contributor

I think it might be useful to have the possibility of passing a function to scale names like so:

library(ggplot2)

df <- expand.grid(X1 = 1:10, X2 = 1:10)
df$value <- df$X1 * df$X2
p1 <- ggplot(df, aes(X1, X2)) + geom_tile(aes(fill = value))

p1 + scale_fill_continuous(function(x) base::toupper(x))

# With function shorthand
p1 + scale_fill_continuous(~ base::toupper(.x))

Created on 2021-01-11 by the reprex package (v0.3.0)

Something like this would make code snippets more portable. In my case, I have a function that makes a plot and I had to define the title outside the plot depending on the function arguments. With this, I could've defined a global dictionary which translates column names to actual (human readable) variable names and re-use that all over my report.

To produce the code above I just added this

  if (is.formula(name)) {
    name <- as_function(name)
  }

  if (is.function(name)) {
    make_title <- name
    name <- waiver()
  } else {
    make_title <- super$make_title
  }

before this line:

https://github.com/tidyverse/ggplot2/blob/master/R/scale-.r#L107

And then added make_title = make_title to the ggproto construction.

All tests pass, so changing this should be safe.

@yutannihilation
Copy link
Member

I like the idea. One more thing to consider is how to handle make_sec_title(). I think we can just assign the same as make_title().

ggplot2/R/scale-.r

Lines 515 to 517 in 37eb64d

make_sec_title = function(title) {
title
}

@eliocamp
Copy link
Contributor Author

I saw that but didn't know what it did, so I didn't dare to touch anything. Is that for the second axis, if there is one?

@clauswilke
Copy link
Member

On this topic: For any new function you add, or any related existing function where you figure out what it does, please add a few comments to the code in scale-.r so in the future we won't have these questions about what certain parts of the API actually do.

@yutannihilation
Copy link
Member

Is that for the second axis, if there is one?

Yes, I guess so, but I too don't know well about the Scale-related things. Honestly, I have no idea what is the case when there's no secondary.axis but the plot needs a secondary title...

make_sec_title = function(self, title) {
if (!is.waive(self$secondary.axis)) {
self$secondary.axis$make_title(title)
} else {
ggproto_parent(ScaleContinuous, self)$make_sec_title(title)
}
}

The mechanism was added by #1868, so we might find some clue there.

@clauswilke
Copy link
Member

Well, maybe @thomasp85 remembers what he did there. :-)

@eliocamp
Copy link
Contributor Author

eliocamp commented Jan 12, 2021

I think I got it. I need to add a similar trick here:

ggplot2/R/axis-secondary.R

Lines 122 to 129 in 37eb64d

set_sec_axis <- function(sec.axis, scale) {
if (!is.waive(sec.axis)) {
if (is.formula(sec.axis)) sec.axis <- sec_axis(sec.axis)
if (!is.sec_axis(sec.axis)) abort("Secondary axes must be specified using 'sec_axis()'")
scale$secondary.axis <- sec.axis
}
return(scale)
}

With that change, now it works also for secondary axis.

library(ggplot2)

df <- expand.grid(X1 = 1:10, X2 = 1:10)
df$value <- df$X1 * df$X2
p1 <- ggplot(df, aes(X1, X2)) + geom_tile(aes(fill = value))


p1 + scale_x_continuous(~ base::tolower(.x), 
                        sec.axis = sec_axis(trans = identity, 
                                            name = ~ gsub("x", "xx", .x))) +
  scale_fill_continuous(~ base::toupper(.x))

Created on 2021-01-11 by the reprex package (v0.3.0)

How should I test this new functionality? My default idea is to create visual tests, but there must be a more specific way.

@thomasp85
Copy link
Member

@clauswilke you think too highly of my memory 😄

But I'm sure I had my reasons...

@thomasp85
Copy link
Member

@eliocamp are you in the process of preparing a PR?

@thomasp85
Copy link
Member

and do your changes also work with p + labs(x = ~toupper(.x)), i.e. when setting label names outside of the scale?

@thomasp85 thomasp85 added feature a feature request or enhancement scales 🐍 labels Mar 24, 2021
@eliocamp
Copy link
Contributor Author

and do your changes also work with p + labs(x = ~toupper(.x)), i.e. when setting label names outside of the scale?

It doesn't, and for it to fully work I'd need to change a lot of how labs work, I think.

Right now labs() changes the plot$labels variable, which is a character vector, so it cannot contain a function. A workaround is to apply the function to the label during update_labels() here:

ggplot2/R/labels.r

Lines 13 to 17 in de0b9e3

update_labels <- function(p, labels) {
p <- plot_clone(p)
p$labels <- defaults(labels, p$labels)
p
}

However, if labs() is called before the relevant aesthetic is present, it doesn't work:

library(ggplot2)
df <- expand.grid(X1 = 1:10, X2 = 1:10)
df$lowercase <- df$X1 * df$X2

ggplot(df, aes(X1, X2)) + 
  geom_tile(aes(fill = lowercase)) +
  labs(fill =  ~base::toupper(.x))  

ggplot(df, aes(X1, X2)) + 
  labs(fill =  ~base::toupper(.x))  +
  geom_tile(aes(fill = lowercase)) 

Created on 2021-03-24 by the reprex package (v1.0.0)

To solve this labs() would need to either add a new scale other modify the existing scale, which will probably break a lot of existing behaviour.

@clauswilke
Copy link
Member

I would hold off on touching labs() until we get to #4269. Both @yutannihilation and I had reasonable prototypes for addressing this issue. But that's for a major feature release, not a patch release.

@eliocamp
Copy link
Contributor Author

Any advice on how to add tests for this feature? Should it be a visual test or there's a better way of checking the text of axis and legends?

@hadley
Copy link
Member

hadley commented Mar 14, 2022

Any implementation of this could include a fix for #4631

Sounds like we could ignore labs() for now, and tackle that as a part of a bigger fix.

@eliocamp I think you should be able to find some part of the data structure that exposes the actual name and test that. I don't remember where it is, but if I was writing the test, I would start by doing a View(p) and str(p) and looking for the label.

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 scales 🐍
Projects
None yet
Development

No branches or pull requests

5 participants