This is the removal of the deprecatedmpl.cm.cmap_d dictionary. This is related to#20853, but does not close that as none of thempl.cm globals are deprecated here.

Note that rather than adding pop() to the Registry, I went with theunregister() 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

Has pytest style unit tests (andpytest passes).
IsFlake 8 compliant (installflake8-docstrings and runflake8 --docstring-convention=all).

Documentation

New features are documented, with examples if plot related.
New features have an entry indoc/users/next_whats_new/ (follow instructions in README.rst there).
API changes documented indoc/api/next_api_changes/ (follow instructions in README.rst there).
Documentation is sphinx and numpydoc compliant (the docs shouldbuild without error).

greglucas force-pushed therm-cmapd branch 3 times, most recently fromecb5c08 tofce605eCompare

January 24, 2022 15:08

tacaswell added this to thev3.6.0 milestone

Jan 24, 2022

timhoffm reviewed

Jan 26, 2022

View reviewed changes

lib/matplotlib/cm.py OutdatedShow resolvedHide resolved

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.")

Copy link

Member

timhoffmJan 26, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Would reorder to

Suggested change

	ifnotforceandnameinself._builtin_cmaps:
	msg=f"Trying to re-register the builtin cmap{name!r}."
	raiseValueError(msg)
	elifnotforce:
	raiseValueError(
	f'A colormap named "{name}" is already registered.')
	else:
	_api.warn_external(f"Trying to register the cmap{name!r} "
	"which already exists.")
	ifforce:
	_api.warn_external(f"Trying to register the cmap{name!r} "
	"which already exists.")
	else:
	raiseValueError(
	f"Trying to re-register the builtin cmap{name!r}."
	ifnameinself._builtin_cmapselse
	f'A colormap named "{name}" is already registered.')

Copy link

ContributorAuthor

greglucasJan 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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 useforce=True and replace the builtin, but in unregister we don't allow that removal.

Copy link

Member

timhoffmJan 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Good point. I was just cleaning up the structure, not considering something else. But I agree: We should not allow overriding builtins. Please change.

Copy link

ContributorAuthor

greglucasJan 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Oops, I forgot to look at our current function. Unfortunatelycm.register_cmap() doesn't useforce, it usesoverride_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

Copy link

Member

timhoffmJan 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

We haveoverride_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

lib/matplotlib/cm.py OutdatedShow resolvedHide resolved

Copy link

Member

timhoffm commentedJan 26, 2022•
edited
Loading

Note that rather than adding pop() to the Registry, I went with the unregister() that was already on cm.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.

👍unregister() is the better approach.Colormaps does intentionally not take on the fullMutableMapping API. Register/unregister are similar to insertion/deletion but follow special rules, so we don't want to extend the mapping analogy to the modification API.

greglucas force-pushed therm-cmapd branch 4 times, most recently fromd009a4b to432c355Compare

January 28, 2022 14:36

timhoffm approved these changes

Jan 29, 2022

View reviewed changes

doc/api/next_api_changes/deprecations/22298-GL.rstShow resolvedHide resolved

lib/matplotlib/cm.py

		cmap._global = True
		_cmap_registry[name] = cmap
		return
		# override_builtin is allowed here for backward compatbility

Copy link

Member

timhoffmJan 29, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

IMHO we should deprecate overwriting builtin colormaps. But that can be a separate PR.

Copy link

ContributorAuthor

greglucasJan 30, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I agree with deprecating that functionality and saving it for a separate PR :)

greglucas force-pushed therm-cmapd branch from432c355 to53aa1abCompare

January 30, 2022 00:08

greglucas mentioned this pull request

Feb 3, 2022

[Doc]: Deprecation note inget_cmap is non-actionable#22362

Closed

QuLogic added the status: needs rebase label

Feb 11, 2022

greglucas force-pushed therm-cmapd branch from53aa1ab to578d67cCompare

February 12, 2022 03:00

greglucas removed the status: needs rebase label

Feb 12, 2022

greglucas force-pushed therm-cmapd branch from578d67c to1b8866eCompare

March 4, 2022 15:02

Copy link

ContributorAuthor

greglucas commentedApr 10, 2022

Ping for a second review on this deprecation removal.

timhoffm added the status: needs review label

Apr 15, 2022

tacaswell approved these changes

May 5, 2022

View reviewed changes

Copy link

Member

tacaswell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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.

greglucasand others added2 commits

May 5, 2022 18:44

MNT: Remove cmap_d colormap access

fb902f7

TST: test re-registering builtin colormap

f184145

The existing test bounced on an earlier code path.  This restores missingcoverage.

greglucas force-pushed therm-cmapd branch from98117b0 tof184145Compare

May 6, 2022 00:44

greglucas merged commitcb28f3a intomatplotlib:main

May 6, 2022

greglucas deleted the rm-cmapd branch

May 6, 2022 02:22

Copy link

Contributor

neutrinoceros commentedMay 6, 2022•
edited
Loading

Hi ! This breaks yt since it is usingcmap_d internally. I see that it was considered deprecated but we never caught that despite trying to treat warnings as errors in CI. I'm not asking for it to be reverted, I just wanted to raise awareness that maybe the deprecation warning wasn't working as intended.

edit: Nevermind, I just found that we had internal knowledge of this deprecation, just a somewhat improper workaround. Carry on !

neutrinoceros mentioned this pull request

May 6, 2022

Upcoming incompatibility with matplotlib 3.6yt-project/yt#3911

Closed

Copy link

Member

timhoffm commentedMay 6, 2022

Thanks for the report. For the deprecation periodcmap_d has been a proxy to the original dict, which should warn upon access. Maybe we forgot to issue a warning for one case? How are you usingcmap_d?

Copy link

Contributor

neutrinoceros commentedMay 6, 2022

For reference this is the current state of yt
https://github.com/yt-project/yt/blob/6270f345039f39d25eff8db3cbda7739dbb99f3a/yt/visualization/color_maps.py#L90-L93

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.

Copy link

Member

timhoffm commentedMay 6, 2022•
edited
Loading

Ah, you have gated against the new version, but are trying to get the registry via a private attribute (which we have changed here).

https://github.com/yt-project/yt/blob/6270f345039f39d25eff8db3cbda7739dbb99f3a/yt/visualization/color_maps.py#L91

You should instead usematplotlib.colormaps orpyplot.colormaps.

Note also, that you useplt.colormaps() in that module twice, which returns a list of colormaps. This is continued to be supported, but the recommended way is to simply useplt.colormaps here as well, sinceplt.colormaps is now the registry/container.

Copy link

Contributor

neutrinoceros commentedMay 6, 2022

Thanks@timhoffm, that's gold ! you're welcome to review my fix for this if you want toyt-project/yt#3912
and I suggest we move the discussion there if needed. Apologies for the noise everyone !

Copy link

ContributorAuthor

greglucas commentedMay 6, 2022

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 referencedMay 6, 2022

DEV: Add codespell to pre-commit hooks#22777

Merged

STY: Fix typos in colormap#22993

Merged

QuLogic added a commit to QuLogic/matplotlib that referenced this pull request

Aug 23, 2022

Document that get_cmap returns a copy now

f5843d5

This was put into effect inmatplotlib#22298, but was not given an API note.

QuLogic added a commit to QuLogic/matplotlib that referenced this pull request

Aug 23, 2022

Document that get_cmap returns a copy now

2a1da87

This was put into effect inmatplotlib#22298, but was not given an API note.

zxdawn mentioned this pull request

Nov 8, 2022

Proplot not compatible with matplotlib 3.6proplot-dev/proplot#390

Open

arildj78 mentioned this pull request

Feb 17, 2023

Exporting causes fatal errorSINTEF/Geomaker#62

Open

QuLogic removed the status: needs review label

May 12, 2023

Labels

None yet

5 participants

Movatterモバイル変換

Uh oh!

MNT: Remove cmap_d colormap access#22298

MNT: Remove cmap_d colormap access#22298

Uh oh!

Conversation

greglucas commentedJan 24, 2022

PR Summary

PR Checklist

Uh oh!

Uh oh!

timhoffmJan 26, 2022

Choose a reason for hiding this comment

Uh oh!

greglucasJan 27, 2022

Choose a reason for hiding this comment

Uh oh!

timhoffmJan 27, 2022

Choose a reason for hiding this comment

Uh oh!

greglucasJan 27, 2022

Choose a reason for hiding this comment

Uh oh!

timhoffmJan 27, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

timhoffm commentedJan 26, 2022• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

Uh oh!

timhoffmJan 29, 2022

Choose a reason for hiding this comment

Uh oh!

greglucasJan 30, 2022

Choose a reason for hiding this comment

Uh oh!

greglucas commentedApr 10, 2022

Uh oh!

tacaswell left a comment

Choose a reason for hiding this comment

Uh oh!

neutrinoceros commentedMay 6, 2022• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

timhoffm commentedMay 6, 2022

Uh oh!

neutrinoceros commentedMay 6, 2022

Uh oh!

timhoffm commentedMay 6, 2022• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

neutrinoceros commentedMay 6, 2022

Uh oh!

greglucas commentedMay 6, 2022

Uh oh!

Uh oh!

timhoffm commentedJan 26, 2022•
edited
Loading

neutrinoceros commentedMay 6, 2022•
edited
Loading

timhoffm commentedMay 6, 2022•
edited
Loading