Repository navigation
Convert guide system to ggproto #3329
Description
The guide system is the last remaining major ggplot2 component that has not been converted to ggproto. As a consequence, it doesn't match well with the rest of the ggplot2 code base, and it is difficult to extend by external package maintainers. We should consider converting it over, possibly before building additional components that would use the current system, as in #3322.
Activity
paleolimbot commentedon May 15, 2019
Yes! I was surprised that guides were S3 when I opened up that code. I think it will be less work to do this after #3322, which is to say, after all the guides use the same system. I'd be happy to take this on as well, but would prefer to do #3322 first.
teunbrand commentedon Jun 9, 2022
The ggplot2 book softly discourages extending ggplot's guide system. However, I'm pretty keen on making some guide extensions, so I'd be willing to spend some time experimenting. What are the current thoughts on moving the guide system to ggproto? I've heard on the grapevines that the entire secondary axis mechanism might move to guides, for example. Other than simply wrapping guide code into ggproto methods and making appropriate classes, what would need to change?
thomasp85 commentedon Jun 10, 2022
@teunbrand I can expand a bit on why I've yet to build up the stamina to do this. If it were a simple rewrite of the current guide methods into a ggproto system it would be quite quick and we would have done it years ago. However, I want this to be an opportunity to rethink the guide implementation in a better way. Currently there is so much code duplication between the different legend guides in terms of how to lay out the grobs, calculate ticks etc. One could even go so far as to say that the keys in a colorbar guide is actually an instance of an axis guide and should use that implementation - but can that be generalised to e.g. the legend guide? There are quite a number of questions about how to do this properly in a manner that doesn't constraint future guide development but allows maximum code reuse.
This is not to deter you from taking on the task. I would be happy if you had the stamina for it, and I would love to set off some time to discuss and guide you through it so you end up with a PR that we agree with the direction on.
teunbrand commentedon Jun 10, 2022
Thanks for your thoughts Thomas, they make a lot of sense to me. From an extension POV, it would be useful too if (in particular the drawing) code was more modular in some ways, which would align with the perspective of reducing code duplication if these modules can be re-used in different guides. For example, if you'd like to make ticks for the minor breaks, you wouldn't want to touch the axis lines or labels parts. I'll have a walk through the code at some point and take stock of what is the same/very similar or very different between various guides.
thomasp85 commentedon Jun 10, 2022
yes, exactly. If we are going to do it we are going to do it with thought and intent. If you want to take it on I will be happy to have one or more design meetings to discuss your ideas and give you feedback
teunbrand commentedon Jun 11, 2022
I'd be willing to take a stab at this, but it may take quite some time before we find appropriate solutions. Worst case scenario is we can't figure out an elegant solution, and we're back where we started with some lessons learned along the way. I'd definitely be happy to get some guidance along the way, I don't consider this a small task. Below, I've made some notes going through the guide code trying to understand what some guides share and what is specific to a guide. I think that the shared bits might be modularised in some way.
Notes
General ideas
element_grob()might simplify codeguide_colourbar()/guide_legend()/guide_bins()might share the samelayout code, if we see the colourbar or bins as a 1-key legend.
frame.colour,frame.linewidthetc., are captured as elements in the constructor. Likewise for
ticks.*,axis.*,label.*andtitle.*. Can later be combined with parent themeelements using
ggplot2:::combine_elements()(might be worth exporting).General notes
guide_grid()doesn't seem to follow conventions of other guides. I think it might be worth exposing, but might get hairy implementation-wise.guide_none()generally does nothing and returnszeroGrob()in the end.guide_colourstepsis, with the exception of training, identical toguide_colourbar().Training tasks
Common tasks
Specific tasks
guide_bins()/guide_coloursteps():(n, n]notationguide_colourbar()/guide_coloursteps():Merging tasks
Common tasks
new_guideSpecific tasks
guide_legend()/guide_bins()override.aesguide_none()new_guideinsteadTransform tasks
Common tasks
Specific tasks
guide_axis()guide_geom()tasksCommon tasks
Specific tasks
guide_legend()/guide_bins()Grob generation, i.e. drawing tasks
Common tasks
The follow tasks are common, but implementation is specific
Specific tasks
guide_axis()zeroGrob().axis.label.{x/y}.{position}angleguide_legend()keywidth/keyheightguide_colourbar()barwidth/barheightguide_legend()/guide_bins()guide_legend()/guide_colourbar()/guide_bins()teunbrand commentedon Jun 16, 2022
I do want to spend some time trying to get this to work. How about I just make a
Guideparent + implementation of 1 guide (axis probably being the simplest), put it in a WIP PR and we could course-correct from there?thomasp85 commentedon Jun 16, 2022
That may be a good idea to get the ball rolling
Convert guides to ggproto (#4879)