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

Better way of handling parameters #1516

Open
hadley opened this issue Jan 25, 2016 · 3 comments · May be fixed by #6101
Open

Better way of handling parameters #1516

hadley opened this issue Jan 25, 2016 · 3 comments · May be fixed by #6101
Labels
feature a feature request or enhancement internals 🔎

Comments

@hadley
Copy link
Member

hadley commented Jan 25, 2016

Currently the way we determine the parameters of a geom/stat is ugly. For example, the parameters of a geom are mostly imputed from draw_panel and draw_group, but some need to be listed explicitly in extra_args if they're used elsewhere (as in GeomTile$setup_data().

See also #1532 - na.rm also has to be special cased.

@hadley hadley added feature a feature request or enhancement ready labels Jul 29, 2016
@hadley hadley added this to the v2.2.0 milestone Jul 29, 2016
@hadley hadley removed this from the v2.2.0 milestone Sep 23, 2016
@teunbrand
Copy link
Collaborator

Would it make sense to have a default_params field (name to be negotiated) in the ggproto geom/stat class that gets supplemented with user input and stored in Layer${stat/geom}_params?

This would make it more analogous to how default_aes works, and as Claus remarked elsewhere, that serves two purposes by analogy. The names of the default_params field can be used in layer() to mark input as inappropriate, and secondly, to actually set a default value for an argument in the draw_panel/draw_group methods.

I think it would be relatively straightforward to do, but have to think more carefully about how to do this without negatively affecting extensions. Ideally they'd declare such a field, but it seems like quite a bit to ask without any immediate benefit.

@hadley
Copy link
Member Author

hadley commented Jan 27, 2023

Yeah, that sounds reasonable at a high level. I'd want to see a PR implementing a very minimal version though, just to think more about it before we fully commit.

@arcresu
Copy link

arcresu commented Apr 14, 2023

I encountered an issue with the way parameters are determined, specifically the heuristic that they are taken from the formal arguments of draw_panel unless these contain ... in which draw_group is used instead. This heuristic didn't work for my scenario and was confusing to debug.

I was creating a geom that was a small extension to GeomSf that looked like:

GeomSfCustom <- ggplot2::ggproto("GeomSfCustom", ggplot2::GeomSf,
  draw_panel = function(self, data, panel_params, coord, my_param = NULL, ...) {
    # transform data with some function that uses my_param
    print(my_param)
    ggplot2::GeomSf$draw_panel(data, panel_params, coord, ...)
  },
)

I was finding that my_param was always NULL even when it was given a value in layer_sf(params = ). I tried adding it to extra_params which suppressed the unused param warning, but still my_param was always stripped. What's happening is that draw_layer trims off any params that aren't returned by parameters(extra = FALSE) (bypassing extra_params):

params <- params[intersect(names(params), self$parameters())]

and since GeomSfCustom$draw_panel contains a ... argument, parameters instead checks draw_group:

ggplot2/R/geom-.R

Lines 186 to 190 in 2e649bb

parameters = function(self, extra = FALSE) {
# Look first in draw_panel. If it contains ... then look in draw groups
panel_args <- names(ggproto_formals(self$draw_panel))
group_args <- names(ggproto_formals(self$draw_group))
args <- if ("..." %in% panel_args) group_args else panel_args

I found a few ways to work around this but none seem ideal:

  1. copy the full argument list of GeomSf$draw_panel to GeomSfCustom$draw_panel and keep in track if ggplot2 updates, or
  2. override parameters to hard-code in the additional param even when extra = FALSE, or
  3. define an empty draw_group with the same formal arguments as draw_panel that's never called, just so that the correct args are there.

@teunbrand teunbrand linked a pull request Sep 12, 2024 that will close this issue
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 internals 🔎
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants