Skip to content

Make sure mapping is not stateful#4475

Merged
thomasp85 merged 2 commits intov3.3.4-rcfrom
better-state-in-layers
May 17, 2021
Merged

Make sure mapping is not stateful#4475
thomasp85 merged 2 commits intov3.3.4-rcfrom
better-state-in-layers

Conversation

@thomasp85
Copy link
Copy Markdown
Member

@thomasp85 thomasp85 commented May 11, 2021

This PR fixes the "statefulness" of the mapping field introduced in #4265 by aligning it with how we now treat geom_params and stat_params. In fact the state has existed since the introduction of LayerSf where the mapping would be modified to ensure the geometry was catched.

This clean-up leaves a couple of dangling issues, mainly the fact that default_mapping no longer needs to be passed around. It is kept as-is so we don't change the signature of the guide calculation and break extension packages. We can remove it entirely when we rewrite the guide functionality to ggproto

@thomasp85 thomasp85 added this to the ggplot2 3.3.4 milestone May 11, 2021
@clauswilke
Copy link
Copy Markdown
Member

clauswilke commented May 11, 2021

Sorry if I'm dense. Could you explain in a few more sentences what exactly is being done?

Also, if default_mapping is kept just to maintain the signature, can you point that out in a comment and/or add a check that it's actually a missing argument? Or alternatively, replace with ...?

@thomasp85
Copy link
Copy Markdown
Member Author

thomasp85 commented May 11, 2021

plot-level mappings and layer-level mappings needs to be fused together - earlier this was done all over the place and let to bugs which resulted in us moving the fusing into setup_layer(). This altered the state of the layer since it potentially modified the content of the mapping field. This was already being done in LayerSf where a geometry mapping was added if missing.

With this PR mapping is never altered, but the fused mapping is stored in a computed_mapping field similar to what is being done for computed_geom_params and computed_stat_params. This means that apart from computed_* fields layers remain stateless and are easier to reason about.

I already added a note at the guide_geom generic that default_mapping is unused and should be removed come rewrite

clauswilke
clauswilke approved these changes May 11, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@clauswilke clauswilke left a comment

Choose a reason for hiding this comment

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

Ok, looks good to me.

I notice that there doesn't seem to be a unit test for the inherit.aes argument, so maybe add a very simple one.

yutannihilation
yutannihilation approved these changes May 12, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@yutannihilation yutannihilation left a comment

Choose a reason for hiding this comment

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

Looks good. It seems I was kind of aware about the need to treat the statefulness at the time of #4265 (comment), but couldn't come up with any solutions. Thanks for catching!

@thomasp85 thomasp85 merged commit 1cede18 into v3.3.4-rc May 17, 2021
@cpsievert cpsievert mentioned this pull request Jun 17, 2021
Merged
@thomasp85 thomasp85 mentioned this pull request Apr 14, 2023
Merged
teunbrand added a commit to teunbrand/ggplot2 that referenced this pull request Apr 16, 2023
@teunbrand
Purge default_mapping arguments, see tidyverse#4475
365b3f5
@thomasp85 thomasp85 deleted the better-state-in-layers branch October 6, 2025 11:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@yutannihilation yutannihilation yutannihilation approved these changes

@clauswilke clauswilke clauswilke approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

ggplot2 3.3.4

Development

Successfully merging this pull request may close these issues.

3 participants

@thomasp85 @clauswilke @yutannihilation