Convert guides to ggproto #4879

teunbrand · 2022-06-18T21:56:47Z

This is a WIP PR addressing #3329.
Since the aim is a substantial overhaul of the guide system, it would be good to gather some feedback along the way. I was hoping we could align on some vision early on, which would make subsequent steps easier. For now, I made a first step by making a parent Guide class and GuideAxis to get the ball rolling. This step is small enough that it wouldn't matter if we decide on a totally different direction.

A few comments from my side that aren't in the code itself:

The general aim was to modularise code. I hope this can serve the purposes of countering code duplication and making it easier to extend.
The current GuideAxis is visually identical to the previous axis guide.
Also had to implement GuideNone to get this to work.
Two types of test are currently failing (locally):
- Almost all visual tests. This is merely because the order of grobs in the axis has changed, which matters for the svg files, but not for the human eye. I can reverse-engineer the previous order if required.
- One test for 'Using non-position guides for position scales results in an informative error', which should be resolved when guide_legend() is also converted.

The type of feedback I was hoping to get is (not limited to) the following:

General feedback about the design.
The naming of arguments, parameters, functions etc.
The 'statelessness' of ggproto objects. I know this should be pursued, but I couldn't immediately figure out how to transition the previously stateful S3 classes to a stateless ggproto class.
In the draw-method (previously the guide_gengrob() method), I just pass around the entire params list to every function that might use it. I don't know whether it is a bad thing, but if it might be useful if there was some elegant way to pass just the parameters it needs, without resorting to very verbose code.

Some magic github incantations:
Fix #4879, Fix #4364, Fix #4930, Fix #2728, Fix #4650, Fix #4958, Fix #5152

Sync branches

Merge branch 'main' of https://github.com/tidyverse/ggplot2 into tidyverse-main2 # Conflicts: # R/guides-.r # R/guides-axis.r

teunbrand · 2022-07-27T18:27:05Z

Hi @thomasp85, do you think you want to course-correct this WIP-PR at some point (whenever is convenient for you, I know these are busy times with the conference and all) or should I continue with the next guide? I was thinking about guide_legend next.

thomasp85 · 2022-07-27T18:31:30Z

I'll take a look next week as conference gets behind me. Thanks for reminding me

thomasp85

I would like to challenge you to think about a way where we can make the guide implementations themselves stateless. Not for the purity of heart, but more as a way to make the implementation more strict and understandable. Making it stateless requires you to think about the input and output of each function call more clearly and I think this will benefit us going forward. If you'd like to discuss this in more detail let me know and I'll set up a meeting

R/guide-.r

teunbrand · 2022-08-01T11:09:40Z

Thanks for taking a look, Thomas!

make the guide implementations themselves stateless

My intuition here would then be to tinker with the build_guides function to decompose the various states of the gdefs variable to explicitly pass around things like params, key and other parts that make the classes currently stateful. I'd have to figure out how this differs between position and non-position guides to see if we can reduce code duplication. Does that direction sound reasonable to you?

thomasp85 · 2022-08-02T08:46:10Z

Yes exactly. Having a clear expected output of a method makes it so much easier to build up intuition for what it is supposed to do. You may also think about having a Guides class that orchestrates it all and keeps all the stateful information but I'll leave that up to you

Merge branch 'main' of https://github.com/tidyverse/ggplot2 into tidyverse-main # Conflicts: # R/coord-cartesian-.r # R/guides-axis.r # man/ggplot2-ggproto.Rd

thomasp85 · 2023-04-14T08:48:52Z

Maybe instead of testing we should make sure we have a very clear understanding of which values in the guides and guidelist are susceptible to change during a rendering pass, and then make sure that changes in these values does not leak into a new rendering pass...

thomasp85 · 2023-04-14T08:53:06Z

This is the sort of situation I am eager to avoid #4475

teunbrand · 2023-04-16T20:42:50Z

R/guides-.R

+  # `guides` is the only initially mutable field.
+  # It gets populated as a user adds `+ guides(...)` to a plot by the
+  # `Guides$add()` method.
+  guides = list(),


This should be the only mutable field up to the point of rendering

teunbrand · 2023-04-16T20:43:21Z

R/guides-.R

+  # Setup routine for resolving and validating guides based on paired scales.
+  #
+  # The output of the setup is a child `Guides` class with two additional
+  # mutable fields, both of which are parallel to the child's `Guides$guides`
+  # field.
+  #
+  # 1. The child's `Guides$params` manages all parameters of a guide that may
+  # need to be updated during subsequent steps. This ensures that we never need
+  # to update the `Guide` itself and risk reference class shenanigans.
+  #
+  # 2. The child's `Guides$aesthetics` holds the aesthetic name of the scale
+  # that spawned the guide. The `Coord`'s own build methods need this to
+  # correctly pick the primary and secondary guides.


I tried to clarify here a bit that two other fields are also mutable

teunbrand · 2023-04-16T20:44:15Z

R/guides-.R

+    ggproto(
+      NULL, self,
+      guides     = new_guides,
+      # Extract the guide's params to manage separately
+      params     = lapply(new_guides, `[[`, "params"),
+      aesthetics = aesthetics
+    )


Here are these mutable fields added to a child ggproto object and the original guide's parameters are copied. If the user has provided a guide directly, e.g. x = guide_axis() instead of x = "axis", the Guides class has never touched the guide_axis() other than to read out available_aes and such.

teunbrand · 2023-04-16T20:50:27Z

R/guide-.R

+  ggproto(
+    NULL, super,
+    params   = params,
+    elements = elems,
+    available_aes = available_aes
+  )


The guide constructor never updates the class with a user's input. It takes the user's input and fuses it to the guide's defaults, which lives on in a child instead of modifying the parent.

teunbrand · 2023-04-16T20:53:40Z

I've reordered the Guides class a bit so that many methods follow the order in which they are called. I also added some clarifying comments about what the mutable fields are. Because the diff of the last commit is difficult to read, I've pointed towards the relevant bits in the comments above.

thomasp85

Okay let's do this :-)

But maybe wait until Monday with merging just to avoid the "never push to prod on a friday" situation :-)

teunbrand · 2023-04-23T20:51:28Z

Good call to wait after the weekend, I just caught a bug that I should've caught before :)

thomasp85 · 2023-04-24T08:28:57Z

I don't think GitHub picks up fix keywords in comments so you may want to add them to the first message instead

teunbrand added 12 commits January 13, 2021 20:09

Merge pull request #1 from tidyverse/master

59850ac

Sync branches

Merge branch 'tidyverse:master' into master

86787de

Merge branch 'tidyverse:main' into master

1d1a2b0

Merge branch 'tidyverse:main' into master

28eff54

Allow Guide ggproto as valid guide

98a5c33

The parent Guide ggproto

30d7382

Add guide constructor

2d871e8

Small tweaks

afbf086

Convert guide_none to ggproto

3a286f3

Accommodate Guide in cartesian coords

870221c

Bifurcate guides_train (for now)

12fc416

Convert guide_axis to ggproto

23d2f76

teunbrand marked this pull request as draft June 18, 2022 21:57

teunbrand added 2 commits June 19, 2022 16:51

Resolve merge conflicts

ad3aa6c

Merge branch 'main' of https://github.com/tidyverse/ggplot2 into tidyverse-main2 # Conflicts: # R/guides-.r # R/guides-axis.r

Merge branch 'tidyverse-main2' into ggproto_guides

0e67b73

teunbrand mentioned this pull request Jul 26, 2022

Natively Cap the Axis Lines, as featured on the lemon package (feature) #4907

Closed

thomasp85 reviewed Aug 1, 2022

View reviewed changes

R/guide-.r Outdated Show resolved Hide resolved

R/guide-.r Outdated Show resolved Hide resolved

R/guide-.r Outdated Show resolved Hide resolved

teunbrand added 6 commits August 7, 2022 18:02

merge upstream

26eebaf

Merge branch 'main' of https://github.com/tidyverse/ggplot2 into tidyverse-main # Conflicts: # R/coord-cartesian-.r # R/guides-axis.r # man/ggplot2-ggproto.Rd

Merge branch 'tidyverse-main' into ggproto_guides

952b153

revert scale$map() logic

7c12fda

Adress some minor comments

570aba0

Better warning when guide function not found

9c27d3e

move guide position specification from setup to training

058f0cf

teunbrand mentioned this pull request Aug 11, 2022

Unexpected results using guides(x = guide_axis(position = ...)) #4650

Closed

teunbrand added 2 commits August 13, 2022 17:15

Stop if guide has no valid function

fb32308

Make a GuidesList class

9b95e22

teunbrand added 6 commits April 16, 2023 19:49

Purge default_mapping arguments, see tidyverse#4475

365b3f5

Test Guides instead of plain list

a230443

validator also validates output of function name input

5bd9e59

remove unnecessary scale_index field

54b3a6a

Deal with guide_none() once

a1bbe23

Reorganise and clarify

91059d7

teunbrand commented Apr 16, 2023

View reviewed changes

thomasp85 approved these changes Apr 21, 2023

View reviewed changes

This comment was marked as resolved.

Sign in to view

teunbrand and others added 2 commits April 22, 2023 10:27

Merge branch 'main' into ggproto_guides

da5e75d

Fix plot state bug

57b82f7

Add news bullet

700d717

teunbrand merged commit b9f43f2 into tidyverse:main Apr 24, 2023

teunbrand deleted the ggproto_guides branch April 24, 2023 16:16

teunbrand restored the ggproto_guides branch April 24, 2023 16:45

cpsievert added a commit to plotly/plotly.R that referenced this pull request May 4, 2023

Also accomodate for guide changes made in tidyverse/ggplot2#4879

5b73531

cpsievert mentioned this pull request May 4, 2023

Handle recent changes to ggplot2's plot_build() logic plotly/plotly.R#2262

Merged

teunbrand deleted the ggproto_guides branch May 7, 2023 17:54

grantmcdermott mentioned this pull request May 11, 2023

Axis caps #5289

Merged

This was referenced May 16, 2023

Can't load ggiraph with dev ggplot2 davidgohel/ggiraph#267

Closed

{ggplot2} gutted methods in dev branch. Causes {ggiraph} to not install rstudio/shinycoreci#176

Open

teunbrand mentioned this pull request Jan 6, 2024

deal with old and new guides jtlandis/ggside#54

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Convert guides to ggproto #4879

Convert guides to ggproto #4879

teunbrand commented Jun 18, 2022 •

edited

Loading

teunbrand commented Jul 27, 2022

thomasp85 commented Jul 27, 2022

thomasp85 left a comment

teunbrand commented Aug 1, 2022

thomasp85 commented Aug 2, 2022

thomasp85 commented Apr 14, 2023

thomasp85 commented Apr 14, 2023

teunbrand Apr 16, 2023

teunbrand Apr 16, 2023

teunbrand Apr 16, 2023 •

edited

Loading

teunbrand Apr 16, 2023

teunbrand commented Apr 16, 2023

thomasp85 left a comment

This comment was marked as resolved.

teunbrand commented Apr 23, 2023

thomasp85 commented Apr 24, 2023

Convert guides to ggproto #4879

Convert guides to ggproto #4879

Conversation

teunbrand commented Jun 18, 2022 • edited Loading

teunbrand commented Jul 27, 2022

thomasp85 commented Jul 27, 2022

thomasp85 left a comment

Choose a reason for hiding this comment

teunbrand commented Aug 1, 2022

thomasp85 commented Aug 2, 2022

thomasp85 commented Apr 14, 2023

thomasp85 commented Apr 14, 2023

teunbrand Apr 16, 2023

Choose a reason for hiding this comment

teunbrand Apr 16, 2023

Choose a reason for hiding this comment

teunbrand Apr 16, 2023 • edited Loading

Choose a reason for hiding this comment

teunbrand Apr 16, 2023

Choose a reason for hiding this comment

teunbrand commented Apr 16, 2023

thomasp85 left a comment

Choose a reason for hiding this comment

This comment was marked as resolved.

teunbrand commented Apr 23, 2023

thomasp85 commented Apr 24, 2023

teunbrand commented Jun 18, 2022 •

edited

Loading

teunbrand Apr 16, 2023 •

edited

Loading