Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
Don't fail on equal-but-differently-named cmaps in qt figureoptions.#29148
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Merged
Uh oh!
There was an error while loading.Please reload this page.
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
Currently, opening the Qt figureoptions UI for an image whose cmap isnot registered in the colormap registry, has a name not matching anyregistry entry, but is actually equal (`==`, i.e. has the same LUT andcolorbar-extension attributes) to a registry entry, leads to an error.A typical example would be```import cmap, pylab as p # third-partyp.imshow([[0, 1]], cmap=cmap.Colormap("bids:magma").to_mpl())```and opening the qt figure options; this leads to the error "index'bids:magma' is invalid ...".Note that if the cmap is different from any registeredcmap then we already add it to the UI combobox (try e.g.`cmap=cmap.Colormap("imagej:fire")`); the only problem was if it wasequal to a registered cmap (this arises because when the code wasoriginally written, cmap instance equality was by identity, not bycomparing LUTs, so the `cmap not in cm._colormaps.values()` checkbehaved differently).Fix that by checking whether the colormap *name* is registered. Thebehavior is still ill-defined in the opposite (theoretical) case of anunregistered cmap different from any registered cmap but with a matchingname, but I'd argue that case is more pathological.Test by running the above code and opening the qt figureoptions.
greglucas approved these changesNov 22, 2024
QuLogic approved these changesNov 23, 2024
b1d2ecd
intomatplotlib:main 44 of 46 checks passed
Uh oh!
There was an error while loading.Please reload this page.
meeseeksmachine pushed a commit to meeseeksmachine/matplotlib that referenced this pull requestNov 23, 2024
…ed cmaps in qt figureoptions.
meeseeksmachine pushed a commit to meeseeksmachine/matplotlib that referenced this pull requestNov 23, 2024
…ed cmaps in qt figureoptions.
timhoffm added a commit that referenced this pull requestNov 24, 2024
…148-on-v3.9.xBackport PR#29148 on branch v3.9.x (Don't fail on equal-but-differently-named cmaps in qt figureoptions.)
ksunden added a commit that referenced this pull requestDec 4, 2024
…148-on-v3.10.xBackport PR#29148 on branch v3.10.x (Don't fail on equal-but-differently-named cmaps in qt figureoptions.)
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading.Please reload this page.
Currently, opening the Qt figureoptions UI for an image whose cmap is not registered in the colormap registry, has a name not matching any registry entry, but is actually equal (
==
, i.e. has the same LUT and colorbar-extension attributes) to a registry entry, leads to an error. A typical example would beand opening the qt figure options; this leads to the error "index 'bids:magma' is invalid ...".
Note that if the cmap is different from any registered cmap then we already add it to the UI combobox (try e.g.
cmap=cmap.Colormap("imagej:fire")
); the only problem was if it was equal to a registered cmap (this arises because when the code was originally written, cmap instance equality was by identity, not by comparing LUTs, so thecmap not in cm._colormaps.values()
check behaved differently).Fix that by checking whether the colormapname is registered. The behavior is still ill-defined in the opposite (theoretical) case of an unregistered cmap different from any registered cmap but with a matching name, but I'd argue that case is more pathological.
Test by running the above code and opening the qt figureoptions.
(aka how some code I wrote 9 years ago (#5469, a bit scary it's been that long) got broken by some code written 5.5 years later (#20227), and how to fix that with a one-line patch with 20 lines of explanation. I guess that's technically a regression :-))
PR summary
PR checklist