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

sec_axis: minor doc + implementation suggestions #3031

Closed
ptoche opened this issue Dec 9, 2018 · 13 comments
Closed

sec_axis: minor doc + implementation suggestions #3031

ptoche opened this issue Dec 9, 2018 · 13 comments

Comments

@ptoche
Copy link

@ptoche ptoche commented Dec 9, 2018

I have used sec_axis for the first time and it worked great. Two minor comments below.

Comment 1: Functionality suggestion.

Consider:

library(ggplot2)
ggplot(mtcars, aes(wt, hp)) +
    geom_point() +
    scale_y_continuous(sec.axis = sec_axis(~. / max(mtcars$hp)))

Suggestion: Allow the scale function to access the variables passed to the aes(), so that this become possible:

ggplot(mtcars, aes(x = wt, y = hp)) +
    geom_point() +
    scale_y_continuous(sec.axis = sec_axis(~. / max(y)))

From the end-user perspective, + scale_y_secondary(~./max(y)) would be even better.

Comment 2: Doc improvement suggestion.

The following is a quote from the docs:

This function is used in conjunction with a position scale to create a secondary axis, positioned opposite of the primary axis. All secondary axes must be based on a one-to-one transformation of the primary axes.

With this wording, I expected to be able to do:

ggplot(mtcars, aes(wt, hp)) +
    geom_point() +
    sec_axis() 

So I would suggest changing "in conjunction with" to "inside a" to make it clear that it's to be used inside. I would also write that it's a function associated with the sec.axis argument of the position scale or something to that effect.

@yutannihilation
Copy link
Member

@yutannihilation yutannihilation commented Dec 12, 2018

For comment 1, I think you can just write

    scale_y_continuous(sec.axis = sec_axis(~ . / max(.)))

Does this help?

@ptoche
Copy link
Author

@ptoche ptoche commented Dec 12, 2018

Thanks yutannihilation! That's a nice trick for the initiated. I've seen the use of the dot before, but it didn't occur to me I could use it here.

Still, max(y) would be more natural. Also, if you were to plot, say, cost and cost per unit, you might want to have max(y/x), which I'm not sure you could do with dots. It's hard enough to remember to have a dot right after the tilde ~.

@yutannihilation
Copy link
Member

@yutannihilation yutannihilation commented Dec 12, 2018

Thanks. Creating functions with ~ and . is a common pattern in purrr package, but I agree those who not familiar with this manner may feel confused. What do you feel about this one? (I'm not sure if this will be possible, though)

    scale_y_continuous(sec.axis = sec_axis(function(y) y / max(y)))

I feel max(y/x) is not possible since a scale only knows their own scale...

@ptoche
Copy link
Author

@ptoche ptoche commented Dec 12, 2018

That would be fantastic. It's easy to read and intuitive. It would make sense to have this particular example listed in the docs too.

If scale_*_* could be trained to detect sec_axis that would be scale_y_continuous(sec_axis(function(y) y / max(y))). Too greedy? :-)

@yutannihilation
Copy link
Member

@yutannihilation yutannihilation commented Dec 14, 2018

@thomasp85 @dpseidel
Are there any reason that sec_axis() cannot accept functions? Excuse me for asking you directly, I couldn't find the discussions behind this design...

@dpseidel
Copy link
Member

@dpseidel dpseidel commented Dec 14, 2018

I'm actually just now working on a big review of the sec.axis framework and the open issues and was going to respond to this in turn. Currently the transformation of the secondary axis is implemented with f_rhs (see line 145) which expects formula notation:

ggplot2/R/axis-secondary.R

Lines 142 to 149 in 92d2777

transform_range = function(self, range) {
range <- new_data_frame(list(. = range))
rlang::eval_tidy(
rlang::f_rhs(self$trans),
data = range,
env = rlang::f_env(self$trans)
)
},

I expect it would take some serious refactoring to accept functions, but I admit I haven't had a chance to look into it yet.

@thomasp85
Copy link
Member

@thomasp85 thomasp85 commented Dec 14, 2018

Basically it just seemed right at the time, and also slightly enforced the use of simple univariate functions which is all that is meaningful for this purpose. Further, anonymous functions as part of ggplot2 calls is pretty uncommon whereas functional notation is at least seen in facets

@yutannihilation
Copy link
Member

@yutannihilation yutannihilation commented Dec 14, 2018

Thanks for prompt replies! The reason I asked is because I felt this part can be roughly replaced with building a function beforehand and applying it on data, like:

fun <- rlang::as_function(self$trans)
fun(range)

(But, I don't have a closer look yet)

anonymous functions as part of ggplot2 calls is pretty uncommon

OK, I thought we are familiar with using anonymous functions for such arguments as labels, but maybe I was wrong.

@dpseidel
Copy link
Member

@dpseidel dpseidel commented Dec 15, 2018

You're not incorrect. Both the breaks and labels arguments to all scale functions accommodate any functions that can take the scale limits and return either a numeric or character vector respectively. Certainly it's not inconceivable that one would then think it natural to use a function to set the transformation of the secondary axis, though my personal bias (probably influenced by comfort with the current set up more than anything) is to think that formula notation is cleaner and more straightforward.

@yutannihilation
Copy link
Member

@yutannihilation yutannihilation commented Dec 15, 2018

#3038 is a PR to explain what I'm thinking. Please ignore this for now; I'll rethink after your refactoring around secondary axis finished. At the moment, I feel the change is not so significant since it's just about how to store the user's input.

@dpseidel
Copy link
Member

@dpseidel dpseidel commented Dec 15, 2018

Great! Yes, my priority right now is trying to refactor break_info to fix the regressions introduced in 3.1 in a thoughtful way but once that is cleared up, I certainly think further consideration and documentation of the sec.axis design is warranted.

@yutannihilation
Copy link
Member

@yutannihilation yutannihilation commented Dec 15, 2018

Thanks. To be clear, I too think formula notation is cleaner and we should promote it. Ultimately, I want to allow both functions and formulas consistently where it's possible, so I expect I'll get urged to make labels and breaks accept formulas next...

@lock
Copy link

@lock lock bot commented Oct 10, 2019

This old issue has been automatically locked. If you believe you have found a related problem, please file a new issue (with reprex) and link to this issue. https://reprex.tidyverse.org/

@lock lock bot locked and limited conversation to collaborators Oct 10, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

4 participants