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

Boilerplate functions #6143

Open
wants to merge 23 commits into
base: main
Choose a base branch
from
Open

Conversation

teunbrand
Copy link
Collaborator

@teunbrand teunbrand commented Oct 17, 2024

This PR aims to (eventually) fix #6142.

Briefly, it introduces boilerplate() that can take in a ggproto class and spit out a constructor.

This PR is a proof-of-concept because it probably merits some discussion whether we would like to go down this road. As such, the PR only implements boilerplate() for Geoms, but can be expanded in the future for Stats and Scales as well, I think.
The boilerplate() function is used for amenable functions, but not more complex ones. For example, geom_text() or geom_boxplot() perform input checking that we cannot currently easily fit into the boilerplate code.

A few notes about the PR:

  • Because we apply this function for suitable geoms, we had to swap the position of class definition and constructor, as boilerplate() takes the class as input. These are the largest diffs.
  • Some order of arguments are changed, as they are now handled consistently.
  • Many geoms had 'hidden' parameters, i.e. parameters like lineend that would be accepted by the Geom$draw_panel/group() method, but were set as arguments to the constructor. These are now all exposed.
  • In visual tests, I had to accept the changes because draw_key_polygon() now actually gets the line settings (lineend, linejoin, linemitre) from the geom instead of having to default these.
  • I don't have a lot of experience of programatically constructing function bodies. If there is a better way, I'd gladly accept pointers.

@teunbrand teunbrand marked this pull request as draft October 17, 2024 13:19
Copy link
Contributor

@yjunechoe yjunechoe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm super excited to see this develop! (esp. for Stats!)

programatically constructing function bodies

I don't have a perfect solution (not thoroughly tested beyond the example) but I thought I'd just throw in some ideas for that, in case the PR will eventually need to do this before it's ready to merge. (I only looked at constructing the layer() expression - check feels complicated at a more fundamental level?)

R/boilerplates.R Outdated Show resolved Hide resolved
R/boilerplates.R Outdated Show resolved Hide resolved
R/boilerplates.R Outdated Show resolved Hide resolved
R/boilerplates.R Outdated Show resolved Hide resolved
Copy link
Contributor

@yjunechoe yjunechoe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for considering my suggestions! Following up w/ just 2 more things I noticed

R/boilerplates.R Outdated Show resolved Hide resolved
R/boilerplates.R Outdated Show resolved Hide resolved
@mjskay
Copy link
Contributor

mjskay commented Nov 20, 2024

FWIW, very excited to see something like this happen! {ggdist} has its own boilerplate generation for geoms and stats if you are looking for a comparison point: https://github.com/mjskay/ggdist/blob/master/R/abstract_geom.R

Besides reducing maintenance issues I have also moved a lot of documentation into it, so now comprehensive docs of aesthetics and computed variables are generated automatically. Has made my life quite a bit easier.

@teunbrand
Copy link
Collaborator Author

Thanks Matthew, it is cool to see that other people have also run into (and solved!) 'there should be hidden parameters' problem I got stuck on as well ^_^ It is interesting (in the good way) to see you opted for a ggproto approach, but I guess this might help coordinate various tasks at once.

I also recently was pointed to @corybrunson's https://github.com/corybrunson/ordr/blob/main/build-pre/build-layers.r, which performs a similar task as well.

@mjskay
Copy link
Contributor

mjskay commented Nov 20, 2024

Heh oh yeah, I also see your other PR landing on a very similar default_params solution to extra_params as well. I had to fix that because it made it very hard to create a class hierarchy of geoms without constantly copying a bunch of boilerplate param definitions across geoms.

If this gets combined with other changes, depending on how far that goes in the direction of breaking changes (including the potential for introducing breaking changes to solve other longstanding problems), one solution you might consider is introducing an alternative base Geom class people could inherit from to "opt in" to the newer, cleaner interface.

Merge branch 'main' into boilerplates

# Conflicts:
#	R/geom-bin2d.R
#	R/geom-crossbar.R
#	R/geom-errorbar.R
#	R/geom-errorbarh.R
#	R/stat-bin.R
#	R/stat-ellipse.R
#	man/geom_errorbarh.Rd
#	man/geom_histogram.Rd
#	man/geom_linerange.Rd
#	man/ggplot2-ggproto.Rd
@teunbrand
Copy link
Collaborator Author

Alright, stats are covered too now. The method is similar to make_constructor.Geom but fixes the layer(stat) instead of layer(geom) and leaves geom is a user-specified argument rather than stat. Per discussion, the main function is now called make_constructor().

For Stats, I sometimes had to hide variables, which I did through the make_constructor(omit) argument. Mostly for two reasons.

  1. It was a computed parameter in Stat$setup_params() or the parent Stat$compute_*() method. These should not be exposed to users, even though they technically could pass these arguments to ....
  2. It was a parameter that a user could use, like stat_bin_2d(breaks), but was not exposed, similar to Better documentation of xseq for stat_smooth() #5246.

I hide both these cases, but for (1) we might consider a mechanism for explicitly prohibiting these arguments and for (2) whether such parameters should be exposed. This is outside the scope of this PR I think.

I also see your other PR

This doesn't really narrow it down I'm afraid😅

how far that goes in the direction of breaking changes

I don't plan on breaking too much here. This is mostly a convenience function, so I'm looking for a standardised solution. Instead of packing everything in these make_constructor() methods, I aim to pack ~80% of functionality in these methods and keep the current constructors with exotic logic as-is. I don't think it is worth it to code the geom_text() nudge/position logic into the make_constructor() method for example.

@teunbrand teunbrand changed the title POC: Boilerplate functions Boilerplate functions Dec 6, 2024
@teunbrand teunbrand marked this pull request as ready for review December 6, 2024 10:08
@teunbrand
Copy link
Collaborator Author

Marking this as ready for review as we decided we don't want a make_constructor() for scales

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 this pull request may close these issues.

Code duplication
3 participants