Skip to content

Api deprecate cmap functions #23668

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

Merged
merged 10 commits into from
Aug 20, 2022

Conversation

tacaswell
Copy link
Member

PR Summary

This is my attempt to turn the crank on the depractions on the cm functions.

The changes were more extensive than I anticipated

PR Checklist

Tests and Styling

  • Has pytest style unit tests (and pytest passes).
  • Is Flake 8 compliant (install flake8-docstrings and run flake8 --docstring-convention=all).

Documentation

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

@tacaswell tacaswell added this to the v3.6.0 milestone Aug 18, 2022
Copy link
Contributor

@greglucas greglucas left a comment

Choose a reason for hiding this comment

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

I will say, this looks like it will be noisy, but I think that it all makes everything much more explicit and readable IMO. So, overall I think it is a good direction.

@QuLogic QuLogic requested a review from timhoffm August 19, 2022 00:57
@tacaswell tacaswell force-pushed the api_deprecate_cmap_functions branch from 07d80fd to 396c88c Compare August 19, 2022 01:01
@tacaswell tacaswell marked this pull request as ready for review August 19, 2022 01:08
@tacaswell tacaswell force-pushed the api_deprecate_cmap_functions branch 2 times, most recently from 418ee64 to 8f0c814 Compare August 19, 2022 01:15
@tacaswell
Copy link
Member Author

OK, really done force-pushing now...

Copy link
Contributor

@greglucas greglucas left a comment

Choose a reason for hiding this comment

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

👍 Good choice with the name of the private function, I think that is better.

@@ -2250,7 +2250,7 @@ def test_contour_hatching():
x, y, z = contour_dat()
fig, ax = plt.subplots()
ax.contourf(x, y, z, 7, hatches=['/', '\\', '//', '-'],
cmap=plt.get_cmap('gray'),
cmap=mpl.colormaps['gray'],
Copy link
Member

Choose a reason for hiding this comment

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

Can we just use strings if we aren't using resample or using the Colormap for anything?

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 can, but I was just mechanically changing (a lot of this was done with search-and-relpace)

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've decided to leave this. It makes the test a bit more complicated, but I do not know if we are definitely exercising the ability to pass Colormap objects through this top level API else where.

@tacaswell
Copy link
Member Author

I'll try to get these changes in first thing tomorow morning, will not be offended if you want to push change @QuLogic .

@tacaswell tacaswell force-pushed the api_deprecate_cmap_functions branch from 8f0c814 to 9321499 Compare August 19, 2022 16:35
@tacaswell
Copy link
Member Author

@timhoffm I think this is ready to review again.

@tacaswell tacaswell force-pushed the api_deprecate_cmap_functions branch from 9321499 to ae90168 Compare August 19, 2022 17:43
@tacaswell tacaswell force-pushed the api_deprecate_cmap_functions branch from e830ea7 to 093ee4a Compare August 19, 2022 20:11
tacaswell and others added 5 commits August 19, 2022 17:14
- matplotlib.cm.get_cmap
- matplotlib.cm.register_cmap
- matplotlib.cm.unregister_cmap
- matplotlib.pyplot.register_cmap

in preference for working with the ColormapRegistry on the top level module.

Co-authored-by: Greg Lucas <greg.m.lucas@gmail.com>
Co-authored-by: Tim Hoffmann <2836374+timhoffm@users.noreply.github.com>
Co-authored-by: Tim Hoffmann <2836374+timhoffm@users.noreply.github.com>
Co-authored-by: Tim Hoffmann <2836374+timhoffm@users.noreply.github.com>
@tacaswell tacaswell force-pushed the api_deprecate_cmap_functions branch from f51e784 to a987e31 Compare August 19, 2022 21:20
@tacaswell
Copy link
Member Author

@timhoffm I think I got everything

@timhoffm
Copy link
Member

Anybody can merge when green.

@QuLogic QuLogic merged commit 9b1fcf6 into matplotlib:main Aug 20, 2022
@tacaswell tacaswell deleted the api_deprecate_cmap_functions branch August 20, 2022 14:36
trexfeathers added a commit to trexfeathers/iris that referenced this pull request Sep 28, 2022
pp-mo pushed a commit to SciTools/iris that referenced this pull request Sep 28, 2022
* Change deprecated MPL colormap registration - matplotlib/matplotlib#23668.

* Adapt benchmark for importing palette to cope with MPL 3.6.

* What's New entry.

* What's New typo.

* Reformat import_iris imports.
DimitriPapadopoulos added a commit to DimitriPapadopoulos/niworkflows that referenced this pull request Apr 17, 2024
	matplotlib.cm.get_cmap → matplotlib.colormaps

This is new in matplotlib 3.9:
	matplotlib/matplotlib#23668
DimitriPapadopoulos added a commit to DimitriPapadopoulos/niworkflows that referenced this pull request Apr 17, 2024
	matplotlib.cm.get_cmap → matplotlib.colormaps

matplotlib/matplotlib#23668
effigies pushed a commit to nipreps/niworkflows that referenced this pull request Apr 17, 2024
matplotlib.cm.get_cmap → matplotlib.colormaps

matplotlib/matplotlib#23668

Fixes #865.
effigies pushed a commit to nipreps/niworkflows that referenced this pull request Jun 10, 2024
matplotlib.cm.get_cmap → matplotlib.colormaps

matplotlib/matplotlib#23668

Fixes #865.
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.

4 participants