Skip to content

Provide access to list of positional aesthetics (x_aes and y_aes in ggplot_blobal) #4135

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

Closed
yutannihilation opened this issue Jul 13, 2020 · 14 comments

Comments

@yutannihilation
Copy link
Member

yutannihilation commented Jul 13, 2020

(Originally suggested at #4129 (review))

Now we refer to x_aes and y_aes in ggplot_global to know which is the positional aesthetic variable. This indicates the extension packages might need this to create a custom scale. I'm not sure if the entire ggplot_global should be exported (edit: no) or it's better to create some subset of it, but anyway let's discuss.

@clauswilke @thomasp85 @teunbrand
Do you have any opinion or use case of ggplot_global in your mind?

@yutannihilation
Copy link
Member Author

To be clear, I personally have never felt I needed ggplot_global. I'm still a newbie to create an extension package, especially ones about positional scales.

@clauswilke
Copy link
Member

I think ggplot_global is an internal implementation detail that shouldn't be exposed. However, we can expose some of its contents by writing accessor or setter functions. See e.g. get_element_tree().

@yutannihilation
Copy link
Member Author

Sorry, maybe I took your comment wrong. "Export ggplot_global" seems too large....

@yutannihilation yutannihilation changed the title Export ggplot_global Provide access to list of positional aesthetics (x_aes and y_aes in ggplot_blobal) Jul 13, 2020
@teunbrand
Copy link
Collaborator

It would be nice for me to have read-access to some of the variables in ggplot_global, specifically the all_aesthetics and x_aes / y_aes. The main reason I would like these is standardisation and convenience though.

My current workaround looks like defining the following and then setting aesthetics = .glob$x_aes in the scale constructors.

.glob <- rlang::new_environment(
    list(
        x_aes = c("x", "xmin", "xmax", "xend", "xintercept", "xmin_final",
                  "xmax_final", "xlower", "xmiddle", "xupper", "x0"),
        y_aes = c("y", "ymin", "ymax", "yend", "yintercept", "ymin_final",
                  "ymax_final", "lower", "middle", "upper", "y0")
    )
)

Which works ok and is not that much work, but if ggplot decides to change lower to ylower for example, I also need to update my object. If there was a function get_aesthetic_names() that takes "all", "x" or "y" that would make life a tiny bit easier.

@clauswilke
Copy link
Member

Just a quick remark: I consider any code using all_aesthetics to be problematic, since geoms and stats can make up any new aesthetics they want. ggplot itself isn't using this anywhere important. If you need it in any extension package, it may suggest a design flaw. I would vote against exporting it.

@yutannihilation
Copy link
Member Author

yutannihilation commented Jul 29, 2020

Sorry, I forgot to reply. Thanks for your comments! It seems we all agree to expose x_aes and y_aes

See e.g. get_element_tree().

So, probably will it be something like this?

get_x_aes <- function() {
  ggplot_global$x_aes
}

get_y_aes <- function() {
  ggplot_global$y_aes
}

@clauswilke
Copy link
Member

Yes, I think so. Should it be get_x_aes_names()?

@yutannihilation
Copy link
Member Author

Sounds better!

By the way, I feel we now are only few steps away from providing something like add_x_aes_name(), which registers custom positional aesthetics (though we need to review the internals, e.g. how to subset the aesthetics for discrete scales). Do you think it's too early to consider? Just curious.

@clauswilke
Copy link
Member

I'm not sure about the whole thing (including adding the functions get_*_aes_names()). I'd prefer a system where we don't have hardcoded names and where instead geoms or stats somehow register their specific positional aesthetics. This also goes along with the idea of not having hard-coded x and y position scales, which we also need to address at some point (see #3898).

@yutannihilation
Copy link
Member Author

Thanks. I think I basically agree with you, and I too have no concrete idea about the whole thing. Let's revisit the idea when it's needed. Hope we can find some nice entry point towards #3898 in future...

@thomasp85
Copy link
Member

Should we close this for now in anticipation of better things to come?

@yutannihilation
Copy link
Member Author

I have no strong opinions. Do you feel this is not nice enough to expose at the moment?

@thomasp85
Copy link
Member

It is certainly nice, but if we plan to provide a better setup altogether in the near future I think we should avoid providing a soon-to-be-superseeded feature

@yutannihilation
Copy link
Member Author

Agreed. I expected #3898 didn't seem to happen in the near future, but if it (or another nicer one) will, I think we should close this, unless there's strong need for this.

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