-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Make sure mapping
is not stateful
#4475
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
Conversation
Sorry if I'm dense. Could you explain in a few more sentences what exactly is being done? Also, if |
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 With this PR I already added a note at the |
There was a problem hiding this 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.
There was a problem hiding this 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!
This PR fixes the "statefulness" of the
mapping
field introduced in #4265 by aligning it with how we now treatgeom_params
andstat_params
. In fact the state has existed since the introduction ofLayerSf
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