MNT: Remove cmap_d colormap access #22298
Conversation
ecb5c08 to
fce605e
Compare
lib/matplotlib/cm.py
Outdated
| if not force and name in self._builtin_cmaps: | ||
| msg = f"Trying to re-register the builtin cmap {name!r}." | ||
| raise ValueError(msg) | ||
| elif not force: | ||
| raise ValueError( | ||
| f'A colormap named "{name}" is already registered.') | ||
| else: | ||
| _api.warn_external(f"Trying to register the cmap {name!r} " | ||
| "which already exists.") |
There was a problem hiding this comment.
Would reorder to
| if not force and name in self._builtin_cmaps: | |
| msg = f"Trying to re-register the builtin cmap {name!r}." | |
| raise ValueError(msg) | |
| elif not force: | |
| raise ValueError( | |
| f'A colormap named "{name}" is already registered.') | |
| else: | |
| _api.warn_external(f"Trying to register the cmap {name!r} " | |
| "which already exists.") | |
| if force: | |
| _api.warn_external(f"Trying to register the cmap {name!r} " | |
| "which already exists.") | |
| else: | |
| raise ValueError( | |
| f"Trying to re-register the builtin cmap {name!r}." | |
| if name in self._builtin_cmaps else | |
| f'A colormap named "{name}" is already registered.') |
There was a problem hiding this comment.
Thanks! This feels a bit awkward to me in general... I'm actually wondering if want to outright refuse to override a builtin? Right now we could use force=True and replace the builtin, but in unregister we don't allow that removal.
There was a problem hiding this comment.
Good point. I was just cleaning up the structure, not considering something else. But I agree: We should not allow overriding builtins. Please change.
There was a problem hiding this comment.
Oops, I forgot to look at our current function. Unfortunately cm.register_cmap() doesn't use force, it uses override_builtin, even more explicitly allowing this behavior. So, I'm not sure how we want to handle this. I really don't think people should be changing a colormap in their underlying library and then re-registering it as the same name, but apparently there are a few (~2) cases that do that :(
https://github.com/search?q=override_builtin&type=issues
There was a problem hiding this comment.
We have override_builtin for compatibility (#18503 (comment)) and at least for now cannot get rid of it. However, I don't want to have that in the new ColormapRegistry API. Since we're here switching to having the registry as the underlying structure, we have to feed that parameter in privately. Either using a private parameter
def register_cmap(...):
...
_colormaps.register(cmap, name=name, force=True, _override_builtin=override_builtin)
or using a private flag (with uglier code, but slight peference since it does not show up in the docs):
def register_cmap(...):
...
_colormaps._allow_override_builtin = override_builtin
_colormaps.register(cmap, name=name, force=True)
_colormaps._allow_override_builtin = False
👍 |
d009a4b to
432c355
Compare
| cmap._global = True | ||
| _cmap_registry[name] = cmap | ||
| return | ||
| # override_builtin is allowed here for backward compatbility |
There was a problem hiding this comment.
IMHO we should deprecate overwriting builtin colormaps. But that can be a separate PR.
There was a problem hiding this comment.
I agree with deprecating that functionality and saving it for a separate PR :)
|
Ping for a second review on this deprecation removal. |
There was a problem hiding this comment.
I pushed an additional commit with a new test. @greglucas or anyone can merge on CI green, but would like if at least one more person looked at the test I added.
The existing test bounced on an earlier code path. This restores missing coverage.
|
Hi ! This breaks yt since it is using edit: Nevermind, I just found that we had internal knowledge of this deprecation, just a somewhat improper workaround. Carry on ! |
|
Thanks for the report. For the deprecation period |
|
For reference this is the current state of yt I think this was written as a quick workaround when cmap_d was deprecated, but it doesn't follow the recommended alternative, so this is definitely on us. |
|
Ah, you have gated against the new version, but are trying to get the registry via a private attribute (which we have changed here). You should instead use Note also, that you use |
|
Thanks @timhoffm, that's gold ! you're welcome to review my fix for this if you want to yt-project/yt#3912 |
|
Thanks for testing the upcoming changes so fast, @neutrinoceros! Also an FYI since it looks like you're building from MPL and numpy source, we do upload main-branch wheels to: https://anaconda.org/scipy-wheels-nightly if it would be helpful to install from the binary instead. |
This was put into effect in matplotlib#22298, but was not given an API note.
This was put into effect in matplotlib#22298, but was not given an API note.
PR Summary
This is the removal of the deprecated
mpl.cm.cmap_ddictionary. This is related to #20853, but does not close that as none of thempl.cmglobals are deprecated here.Note that rather than adding pop() to the Registry, I went with the
unregister()that was already oncm.unregister_cmap(). I'm not sure if we'd rather it behave like a dictionary with item access/removal that way, or with the register/unregister methods. I don't have a major preference one way or the other.PR Checklist
Tests and Styling
pytestpasses).flake8-docstringsand runflake8 --docstring-convention=all).Documentation
doc/users/next_whats_new/(follow instructions in README.rst there).doc/api/next_api_changes/(follow instructions in README.rst there).