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

Respect scale and nonlinear values in PlotUtils cgrads #1979

Merged
merged 9 commits into from
May 30, 2022

Conversation

asinghvi17
Copy link
Member

@asinghvi17 asinghvi17 commented May 25, 2022

Description

Fixes #1977

Essentially, reverts a change from #1723.

I'd like to release a v0.16.7 with this change as well. since it is somewhat serious.

Before

iTerm2 pNXQmN
(note how they are all the same)

After

iTerm2 NH1H6Q

Type of change

Delete options that do not apply:

  • Bug fix (non-breaking change which fixes an issue)

Checklist

  • Added an entry in NEWS.md (for new features and breaking changes)
  • Added reference image tests for new plotting functions, recipes, visual options, etc.

@MakieBot
Copy link
Collaborator

MakieBot commented May 25, 2022

Compile Times benchmark

Note, that these numbers may fluctuate on the CI servers, so take them with a grain of salt.

using time

master:  9.20 < 9.26 > 9.41, 0.07+-
pr:      9.16 < 9.27 > 9.38, 0.08+-
speedup: 0.99 < 1.00 > 1.01, 0.01+-
median:  +0.17% => invariant

This PR does not change the using time.

ttfp time

master   25.01 < 25.19 > 25.40, 0.12+-
pr       24.90 < 25.16 > 25.39, 0.15+-
speedup: 1.00 < 1.00 > 1.00, 0.00+-
median:  -0.10% => invariant

This PR does not change the ttfp time.

@asinghvi17
Copy link
Member Author

CI errors are because of a missing reference image for the new test.

@SimonDanisch
Copy link
Member

Alright, I took a deep dive to make sure this is correct now... I guess I misunderstood cgrad a bit when I made that change.
I added some more tests to make sure we do the right thing now

@SimonDanisch SimonDanisch merged commit 0562d9f into master May 30, 2022
@SimonDanisch SimonDanisch deleted the as/colormap branch May 30, 2022 21:29
@asinghvi17
Copy link
Member Author

Thanks for taking this forward!

On a side note - I wonder if returning a Sampler might be a more efficient solution? Colormaps with intentionally tiny variations, like those with e.g. scale = x -> log(100, x), might then be somewhat misrepresented.

I'm not sure whether [W]GLMakie will accept Sampler (from colorsampler.jl) as a native type but I think something like that might be a somewhat more elegant solution to the problem.

@SimonDanisch
Copy link
Member

Yeah we need a refactor of the whole colormap pipeline and then make sure that wherever colormaps are used, they work exactly the same without hacks like we have here

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

Successfully merging this pull request may close these issues.

Unexpected behavior of colormaps (cgrad)
4 participants