Error on unknown plot attributes #2966
Conversation
Compile Times benchmarkNote, 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(display(fig))
|
| plot_attributes = merged_get!( | ||
| ()-> default_theme(scene, FinalType), | ||
| plotsym(FinalType), scene, attributes | ||
| plotsym(FinalType), scene, attributes; | ||
| allowlist = Set([:text, :xautolimits, :yautolimits, :zautolimits, :interpolate_in_fragment_shader, :label, :fxaa, :rasterize, :model]) |
There was a problem hiding this comment.
inspectable, material, texture, transformation, are also things which should be added, plus all the GLMakie lighting stuff (specular, diffuse, etc)
It would also be nice to have a signature which does not perform this check, for easy programmatic passing of attributes inherited from the parent plot!
There was a problem hiding this comment.
transformation is already removed elsewhere prior to this check
Wouldn't it be better to group all backend related keywords under one umbrella rather than just letting them potentially collide with the rest?
I think I would like it better if an RPRMakie plot had meshscatter(data; RPRMakie = (; material = ...)
There was a problem hiding this comment.
and rasterize in CairoMakie
There was a problem hiding this comment.
If we want both name checking and the ability to add arbitrary attributes to a plot we need to separate them somehow. I was thinking we could have one whitelisted attribute Dict of core attributes, which may become fields of specific plot object (struct Scatter ... end etc) if we want that, and one Dict with everything else.
Having RPRMakie = (; ...) etc is similar to that I guess. Though I would say attributes that influence other objects (inspectable, label, ...) and overwrites (model, view, ...) should also move out of the directly defined attributes. I guess the latter could also be part of the backend groups. The former is kind of the "random stuff that you want to attach to a plot" group, which I think is useful to have in a dynamic way.
There was a problem hiding this comment.
random stuff that you want to attach to a plot
You could still do that by doing plotobject.attributes[:something] = xyz no? For me it's currently most important to make our user interface more robust against user error
There was a problem hiding this comment.
we could also add a misc allowed key or something which should only ever be set to an Attributes instance with further keys. Then we could disallow such a key for any other recipe so we know it will never conflict, and people can use it to carry arbitrary stuff forward. I want to avoid a situation where people make recipes and we break them by introducing some new add-on keyword arg.
In my case I pass kwargs to the wrong function. Eg I pass something to |
|
This has not moved forward in a while, I think the main blocker was that we do like to add certain special attributes to plots here and there, and there was no way to detect them. Now I had the idea, what if we solve this by convention and say, any keyword starting with Edit: The other thing that's difficult is backend-specific keywords. But we could also solve this by convention and prefix those with |
Description
It's quite annoying that plot functions silently accept misspelled keyword arguments. The reason is that unlike
Blocks likeAxisetc., plot objects likeScatterdo not have a completely well-defined list of attributes that they accept. Instead, they have dynamically generated default themes that might merge in themes of other plot types etc.Also, there are a couple of extra keyword arguments in use that are not meant for the plot objects themselves but serve as metadata for other functionality, for example if
xautolimits = falseis encountered by anAxis, it will ignore the x-dimension of that plot for the limit calculation. Because this system has grown in a rather ad-hoc way where keywords were added by different people for different reasons, it's a bit messy and unclear what's allowed and what's not.Still I believe an allowlist that is too broad is better than having none at all, especially because misspelled keyword arguments are the main thing we want to filter out.
So here I've implemented a very simple method, I just crosscheck the input plot attributes against the keys in the dynamically generated default theme, and against a (for now static and general) allowlist. If it's not in there it's likely to be wrong. Helpfully, a list of all allowed keywords is printed out for the user to quickly find the correct spelling, for example.
It looks like this:
I made some simple plots in the REPL which worked after making a short allowlist, however I'm sure CI will uncover quite a few more that need to be added.