Skip to content
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

Merged
merged 30 commits into from
Sep 12, 2023

Conversation

SimonDanisch
Copy link
Member

@SimonDanisch SimonDanisch commented Jul 24, 2023

  • refactors Colorbar to use image + heatmap instead of the heavier poly + image. If it wasn't for CairoMakie, we could just use heatmap alone. Once CairoMakie supports irregular gridded heatmaps with interpolation, we can get rid of the image call
  • Uses now ColorMap (TODO, rename to ColorMapping), internally in Colorbar, to have a more consistent interface
  • Forwards cgrad correctly to Colorbar now, which wasn't the case anymore after Clean up colormap handling and implement colorscale + alpha kw #2900
  • Introduces extract_colormap (TODO, also rename to extract_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.
  • throws better errors
  • introduces experimental colormap=Categorical(colormap), which does a true categorical colormap:
f, ax, pl = barplot(1:3, color=1:3, colormap=Categorical(:viridis))
Colorbar(f[1, 2], pl)
f
image We may remove this, since its not very elegant to pass `categorical=true` to Colorbar and cgrad, but `cgrad` doesn't define a true categorical colormap, and we don't have any alternative to define a truly categorical colormap. But, since I already implemented the support for it, I wanted a way to expose it. I think the most elegant way would be to do `barplot(...; color=Categorical(1:3))` since that's where the categories actually are, and where we also need to know that things are categorical. To not balloon this PR even more, I refrained from implementing this for now.

With the new extract_colormapping we also get colormaps for recipes that didn't have colormap support before:
image

Closes #3188, #3119, #2835, #2655, #1778

@MakieBot
Copy link
Collaborator

MakieBot commented Jul 24, 2023

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(display(fig))
using create display create display
GLMakie 12.05s (11.90, 12.17) 0.10+- 1.13s (1.12, 1.14) 0.01+- 790.47ms (775.59, 836.01) 20.77+- 10.75ms (10.58, 10.88) 0.11+- 140.25ms (139.05, 142.00) 1.11+-
master 12.03s (11.94, 12.19) 0.10+- 1.24s (1.22, 1.25) 0.01+- 807.78ms (799.72, 828.59) 10.46+- 10.80ms (10.69, 10.99) 0.11+- 140.54ms (139.12, 142.18) 1.17+-
evaluation +0.10%, 0.01s invariant (0.11d, 0.83p, 0.10std) -9.25%, -0.1s faster✅ (-9.53d, 0.00p, 0.01std) -2.19%, -17.32ms invariant (-1.05d, 0.08p, 15.61std) -0.51%, -0.06ms invariant (-0.49d, 0.37p, 0.11std) -0.21%, -0.3ms invariant (-0.26d, 0.64p, 1.14std)
CairoMakie 9.92s (9.83, 10.00) 0.05+- 1.08s (1.07, 1.10) 0.01+- 210.70ms (208.03, 212.31) 1.52+- 9.61ms (9.54, 9.74) 0.08+- 5.64ms (5.61, 5.71) 0.03+-
master 9.92s (9.84, 9.99) 0.06+- 1.19s (1.18, 1.21) 0.01+- 224.12ms (220.99, 228.09) 2.52+- 9.71ms (9.63, 9.76) 0.05+- 5.73ms (5.65, 5.81) 0.06+-
evaluation -0.01%, -0.0s invariant (-0.02d, 0.97p, 0.05std) -9.79%, -0.11s faster✅ (-10.99d, 0.00p, 0.01std) -6.37%, -13.42ms faster✅ (-6.46d, 0.00p, 2.02std) -1.02%, -0.1ms faster ✓ (-1.40d, 0.03p, 0.07std) -1.64%, -0.09ms faster ✓ (-1.95d, 0.00p, 0.05std)
WGLMakie 15.23s (14.85, 15.63) 0.28+- 1.50s (1.48, 1.54) 0.03+- 13.61s (13.18, 13.96) 0.26+- 17.83ms (15.94, 24.00) 2.77+- 1.32s (1.30, 1.36) 0.03+-
master 15.37s (15.13, 15.84) 0.28+- 1.60s (1.54, 1.68) 0.04+- 13.61s (13.25, 13.79) 0.19+- 17.07ms (16.14, 18.12) 0.78+- 1.33s (1.29, 1.42) 0.05+-
evaluation -0.86%, -0.13s invariant (-0.47d, 0.40p, 0.28std) -6.86%, -0.1s faster✅ (-2.84d, 0.00p, 0.04std) +0.02%, 0.0s invariant (0.01d, 0.98p, 0.23std) +4.28%, 0.76ms invariant (0.37d, 0.51p, 1.78std) -1.08%, -0.01s invariant (-0.36d, 0.51p, 0.04std)

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.
Copy link
Member

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.

Copy link
Member Author

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.

Copy link
Member

@jkrumbiegel jkrumbiegel Jul 27, 2023

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?

Copy link
Member Author

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

Copy link
Member Author

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

@SimonDanisch
Copy link
Member Author

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.
@t-bltg, would be still nice to review & test this - we can just fix whatever bugs come up in a new PR!

@SimonDanisch SimonDanisch merged commit 0fb9fe5 into master Sep 12, 2023
24 of 26 checks passed
@SimonDanisch SimonDanisch deleted the sd/improve-colorbar-error branch September 12, 2023 20:13
@t-bltg
Copy link
Collaborator

t-bltg commented Sep 12, 2023

Maybe we can add the example of #2493 to the test suite (comparison against Python so we know it's right), if we didn't have it already in the ref imgs (on my phone ;)).

Nevermind.

@t-bltg
Copy link
Collaborator

t-bltg commented Sep 13, 2023

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

KeyError: key :highclip not found with Colormap on contour!
4 participants