Improve and use shared_attributes for attribute passthrough

[ffreyer]

Description

We have shared_attributes(parent, TargetType) as a function that filters attributes from parent applicable to TargetType but we are not using it. This pr aims to extend the function and use it consistently in recipes. That should help fix a lot of the issues about attributes not being passed along/having no effect in recipes.

Some thoughts:

• Allowing shared_attributes to skip/drop a set of given attribute names means that some valid attributes will not get set. That sounds like something we should not encourage. Edit: It is quite useful in some cases, e.g. if you want to pass strokecolor to a poly but not a scatter or fxaa to a mesh but not lines.
• Renaming attributes is a common and valid task (e.g. strokecolor => color), so shared_attributes should handle that
• Replacing attributes is also common, but typically requires a multiline map (etc). Because of that I think it's preferable to have attr[:name] = map(...) instead of something like obs = map(...); shared_attributes(..., :name => obs).
• There are some attributes that only make sense at the top level, e.g. the inspector ones. These should be automatically cleared.
• Warnings for missing valid attributes might be useful to find further issues with missing attribute passthrough.

First Version

The usage of the first version looked something like this:

# filters attributes applicable to TargetType from parent plot
# remove some top level/local attributes
# renames based on rename list
attr = shared_attributes(parent, TargetType, [:name_in_parent => :name_in_target, ...])

# removing attributes
pop!(attr, :name)

# replacing/adding
attr[:name] = new_attr

This has a couple of problems:

• External pop! and replacing/adding makes "warn_on_unset_attribute" pretty useless since a good chunk of attribute mutation happens afterwards
• Replacements invite mistakes as setindex!(::Attributes, val, name) updates Observables if val is not an Observable. E.g. lines_attr[:fxaa] = false would change the parent attributes fxaa value, which could also be used in another plot where it should remain true.
• The rename array is unintuitive. As a replacement it would be written in reverse order as attr[:name_in_target] = parent[:name_in_parent]

Second Version (c04f450 and after)

attr = shared_attributes(
    parent, TargetType, 
    :name_to_delete, ...; 
    name_to_replace_or_add = value, ...
)

The second version solves the issues above:

• Everything can/should happen in the shared_attributes call so we can track defaulted attributes.
• Replacements (given as kwargs) are handled by shared_attributes to not change values of the parent.
• Renaming doesn't exist explicitly anymore. They are handled via replacements/kwargs, i.e. new_name = parent[:old_name].

Related Issues/prs

• Fixes Visible option does not work with tricontourf! #4458
• Closes attributes_from breaking  #2206 (shared_attributes should be a preferable alternative)
• Fixes Toggling scatterlines #2733
• Closes Recipes do not react to keyword arguments properly #3263
• Closes fix: make space work for BarPlot #4435

Type of change

• Internal cleanup with potential for breaking changes in attribute passthrough

Checklist

• Added an entry in CHANGELOG.md (for new features and breaking changes)
• Added or changed relevant sections in the documentation
• Added unit tests for new algorithms, conversion methods, etc.
• Added reference image tests for new plotting functions, recipes, visual options, etc.

[MakieBot]

Compile Times benchmark

Note, that these numbers may fluctuate on the CI servers, so take them with a grain of salt. All benchmark results are based on the mean time and negative percent mean faster than the base branch. Note, that GLMakie + WGLMakie run on an emulated GPU, so the runtime benchmark is much slower. Results are from running:

using_time = @ctime using Backend
# Compile time
create_time = @ctime fig = scatter(1:4; color=1:4, colormap=:turbo, markersize=20, visible=true)
display_time = @ctime Makie.colorbuffer(display(fig))
# Runtime
create_time = @benchmark fig = scatter(1:4; color=1:4, colormap=:turbo, markersize=20, visible=true)
display_time = @benchmark Makie.colorbuffer(fig)

	using	create	display	create	display
GLMakie	5.31s (5.25, 5.37) 0.05+-	116.65ms (111.20, 125.43) 4.57+-	508.21ms (422.98, 709.08) 130.32+-	10.09ms (9.70, 10.49) 0.26+-	26.67ms (26.17, 26.93) 0.27+-
master	5.32s (5.18, 5.37) 0.07+-	117.55ms (111.25, 122.07) 3.46+-	433.16ms (422.33, 444.48) 8.40+-	9.72ms (9.56, 9.89) 0.10+-	26.75ms (26.69, 26.90) 0.07+-
evaluation	1.00x invariant, -0.01s (-0.11d, 0.84p, 0.06std)	1.01x invariant, -0.9ms (-0.22d, 0.69p, 4.02std)	0.85x noisy🤷‍♀️, 75.06ms (0.81d, 0.18p, 69.36std)	0.96x slower X, 0.38ms (1.91d, 0.01p, 0.18std)	1.00x invariant, -0.08ms (-0.42d, 0.46p, 0.17std)
CairoMakie	4.95s (4.89, 5.01) 0.04+-	114.72ms (110.07, 120.72) 3.80+-	170.19ms (166.15, 176.28) 3.82+-	9.83ms (9.49, 10.40) 0.33+-	1.20ms (1.14, 1.24) 0.04+-
master	4.98s (4.93, 5.07) 0.05+-	112.64ms (110.30, 116.68) 2.10+-	170.48ms (166.44, 173.84) 2.29+-	9.47ms (9.34, 9.67) 0.13+-	1.18ms (1.15, 1.24) 0.04+-
evaluation	1.01x invariant, -0.04s (-0.74d, 0.19p, 0.05std)	0.98x invariant, 2.08ms (0.68d, 0.24p, 2.95std)	1.00x invariant, -0.29ms (-0.09d, 0.87p, 3.06std)	0.96x slower X, 0.36ms (1.42d, 0.03p, 0.23std)	0.99x invariant, 0.01ms (0.34d, 0.54p, 0.04std)
WGLMakie	5.40s (5.30, 5.55) 0.08+-	113.67ms (109.93, 129.03) 6.88+-	5.22s (5.12, 5.50) 0.13+-	13.94ms (13.14, 15.43) 0.72+-	128.39ms (119.74, 140.94) 6.92+-
master	5.47s (5.34, 5.65) 0.11+-	112.71ms (109.20, 121.16) 4.21+-	5.28s (4.94, 5.93) 0.36+-	12.42ms (11.99, 13.53) 0.52+-	134.16ms (125.23, 144.23) 6.80+-
evaluation	1.01x invariant, -0.07s (-0.73d, 0.20p, 0.10std)	0.99x invariant, 0.97ms (0.17d, 0.76p, 5.54std)	1.01x invariant, -0.06s (-0.22d, 0.69p, 0.24std)	0.89x slower❌, 1.52ms (2.41d, 0.00p, 0.62std)	1.04x invariant, -5.77ms (-0.84d, 0.14p, 6.86std)

[ffreyer]

This is more dangerous than I first thought. There are many recipes that use mixin_generic_plot_attributes() which defines fxaa = true which then wasn't passed along. With shared_attributes it gets picked up and passed. Some recipes define something used in multiple plot types, like strokewidth, but only pass it to one child plot. These then need to be explicitly excluded or replaced. Most recipes only define a subset of possible attributes. Before the undefined ones would be ignored, now they would get passed automatically, possibly in bad ways like fxaa. Also we don't actually have tests for all plot types in basic_plots. E.g. waterfall and rainclouds have none.

I also noticed some issues with my shared_attributes approach. Because Attributes are set up to update Observables instead of replacing elements, doing attr[:fxaa] = false will affect the fxaa Observable (if it exists), which is the same (===) as parent.fxaa and the same as the fxaa observable passed to another plot. You have to use attr[:fxaa] = Observable(false) instead if you want to replace it. That seems like a source of errors to me.
The warn_on_missing that I added as a test is also pretty useless when replacements happen after the call.

[ffreyer]

Some things I'm (re)considering;

• Should we base passthrough in shared_attributes on attribute_names(parent) rather than attributes(parent), i.e. exclude everything that isn't defined in @recipe? This would make it less likely for attributes to get passed incorrectly, but increase the burden to include everything on the recipe writer
• Should replacements be part of shared_attributes to avoid having to wrap with an Observable and to make warn_on_missing more useful?

[ffreyer]

I'm marking this as breaking since it changes how attributes are passed along, i.e. it will make some attributes work that didn't work before. I hope that this is an improvement in most cases, but there will be some cases where an attribute is wrongfully included. I hope that I can catch most of these through CI, but I doubt it will be all of them.

[ffreyer]

After some discussions with Simon we came to the conclusion that it's not a good idea to work on this or merge this before we work on the Plot update/Observable refactor #4360. Depending on how we do that refactor, the thing shared_attributes can do will change. E.g. if update!(plot, attributes...) becomes the recommended way to update attributes (and plot args), then shared_attributes shouldn't accept replacements, e.g. shared_attributes(..., linewidth = calculated_linewidth). We may also drop this function entirely and make it part of plot!(parent_plot, ....) instead.

Notes from Discussion

Current Attribute priority is: (high to low)

1. user supplied kwargs
2. theme (scene.theme.<PlotType>.<attribute_name>)
3. plot attribute defaults (i.e. what @recipe generates) (this may refer back to theme with @inherit

In the future we could have parent_plot.attributes as a source between user supplied and theme, which would replace the automatic pass-through shared_attributes (already) provides.

To take care of renames, e.g. parent.strokewidth -> lines.linewidth, we could add a renames= [:strokewidth => :linewidth, ...] arg or kwarg. As an arg this probably needs a new struct to be extracted correctly. That would also allow you to do Inherited(linewidth = :strokewidth, ...) which I find more intuitive.

Replacements work the same as on master in this framework, i.e. you just pass them as a plot kwarg. plot!(..., linewidth = calculated_linewidth).

---

To manage direct inheritance and renaming, we could add

struct InheritedAttribute
    name_in_parent::Symbol
end

as an option for Attributes. This would effectively replace @inherit and be usable at runtime. shared_attributes could generate these for attributes inherited from a parent, and for renamed attributes. These could be chained

mesh.color = InheritedAttribute(:color)
# checks parent:
poly.color = InheritedAttribute(:patchcolor)
# checks parent:
scene.theme.patchcolor = some_color

and if this chain does not complete we error.

---

General Questions & Notes:

• Should we communicate that an attribute value comes from inheritance, or simply treat it as a value?
• I don't think we talked about it, but I still like the idea of having some tool to check which attributes as recipe covered.
• It seems likely that Observables will not be the preferred way to handle calculations in the future, and that the plot needs to exist for updates.