-
-
Notifications
You must be signed in to change notification settings - Fork 313
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
improve colorbar errors for plots that don't use a colormap #3090
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))
|
src/makielayout/blocks/colorbar.jl
Outdated
error(""" | ||
Plot $(func) doesn't have a color attribute that uses a colormap to map to colors, so it cannot be used to create a colorbar. | ||
Please create the colorbar manually e.g. via `Colorbar(f[1, 2], colorrange=the_range, colormap=the_colormap)`. | ||
This could also mean, that you're using a recipe, which itself doesn't calculate the colorrange, and leaves it to its children. |
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.
Did this not work before (the barplot example)? Would seem to be a big regression if so.
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.
It was indeed able to create one, but it created the wrong Colorbar before, since it didn't connect the colorrange, which only happens for the meshplot inside.
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.
hm one way or another I think it should work, it's very weird having to access the nested plots, no? maybe an internal function specifying the child plot with the color that gets called by the Colorbar
constructor that can be overloaded?
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.
I mean we could search for subplots that do have a complete colormap... Now with calculated_colors
, it should be pretty clear, that such a plot does have numbers that are mapped to colors... The question is, how do we deal with multiple subplots?
We could check if they use the same calculated_colors
and if not error...and if it's just one it should quite clearly be the plot to be used for Colorbar
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.
Maybe a combination, like with data_limits? Some plots need to overload data_limits
but there is a fallback for all recipes, that usually does the correct thing by calculating data_limits
of the children
…rg/Makie.jl into sd/improve-colorbar-error
…rg/Makie.jl into sd/improve-colorbar-error
I'm going to merge this since I need to update the reference images in other PRs and need to base at least the datashader PR against this to make the colorbar integration easier. |
Nevermind. |
@SimonDanisch I've ran several examples, and I couldn't identify a regression related to this PR, so we can probably remove the commented test. |
ColorMap
(TODO, rename toColorMapping
), internally in Colorbar, to have a more consistent interfacecgrad
correctly to Colorbar now, which wasn't the case anymore after Clean up colormap handling and implement colorscale + alpha kw #2900extract_colormap
(TODO, also rename toextract_colormapping
), which Colorbar now uses to extract the colormapping from any plot type. Has a fallback for any recipe, which recursively searches the child plots. One can also directly overload it for a recipe, where this isn't a good default. Will error if multiple colormappings are found in a recipe.colormap=Categorical(colormap)
, which does a true categorical colormap:With the new
extract_colormapping
we also get colormaps for recipes that didn't have colormap support before:Closes #3188, #3119, #2835, #2655, #1778