NotificationsYou must be signed in to change notification settings
Fork7.9k
Star21.4k

Rename cm.py to colormaps.py.#11413

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

Jump to bottom

Closed

efiring wants to merge3 commits intomatplotlib:masterfromefiring:rename_cm

Closed

Rename cm.py to colormaps.py.#11413

efiring wants to merge3 commits intomatplotlib:masterfromefiring:rename_cm

Conversation

Copy link

Member

PR Summary

PR Checklist

Has Pytest style unit tests
Code is PEP 8 compliant
New features are documented, with examples if plot related
Documentation is sphinx and numpydoc compliant
Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

Rename cm.py to colormaps.py.

bb11c83

Closesmatplotlib#1476.

Copy link

MemberAuthor

This is the minimal version; actually changing all of the imports fromcm tocolormaps will touch a large number of files, and isn't urgent. First, let's see whether there is support for doing this at all.

timhoffm reviewed

Jun 11, 2018

View reviewed changes

Copy link

Member

timhoffm left a comment

Choose a reason for hiding this comment

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

The colormaps module has to be added to the docs.

lib/matplotlib/cm.py Outdated

		@@ -1,389 +1,5 @@
		"""
		Builtin colormaps, colormap handling utilities, and the`ScalarMappable` mixin.
		This is a compatibility shim for therenamed colormaps module.

Copy link

Member

timhoffmJun 11, 2018

Choose a reason for hiding this comment

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

Link to the colormaps module

efiring added2 commits

June 10, 2018 19:36

Shorten some lines in list_colormaps (moved from pyplot)

468953d

Use Sphinx link in cm.py docstring.

963d7f6

Copy link

Member

ImportanceOfBeingErnest commentedJun 11, 2018

Currently thecm module contains the public functionregister_cmap,get_cmap and theScalarMappable class.
Thecolors module contains a lot of color conversion stuff but also hosts theColormap class and its descendents, as well as all the normalizations.
So the confusing part would be that a newcolormaps.py would neither contain theColormap, nor the normalizations.

In total I'm a bit sceptical about replacing one confusion ("What doescm stand for?") with another one ("Why can't I importColormap fromcolormaps?").

If truely rearanging stuff for the better, one would rather move the colormaps and normalizations out ofcolors and intocm - then renaming that module tocolormaps would make more sense, I'd say.

Copy link

Member

ImportanceOfBeingErnest commentedJun 12, 2018

I just saw thatMEP21 says aboutcm

cm
rename the module to something more descriptive - mappables?

jklymak approved these changes

Jun 12, 2018

View reviewed changes

Copy link

Member

jklymak left a comment

Choose a reason for hiding this comment

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

This re-org makes sense to me, and I think the new name is sensible.

Moving other classes around may be desirable as well, but thats a longer process.

Copy link

Member

timhoffm commentedJun 12, 2018•
edited
Loading

I propose to hold this up until after the 3.0 release. If we are not absolutely sure this is the the right name, or if we want to do additional changes, that should be within one release. It would be really unfortunate to move stuff tocolormaps in 3.0 and then tomappables in 3.1.

Also saying, we‘ve cleaned up our package structure: Here is what it looks like. Is easier to sell than having a bunch of releases saying class X is now in package Y.

Copy link

Member

jklymak commentedJun 12, 2018

Big 👎 on "mappables" 😉. That'd be far worse than "cm" in my opinion!

dstansby approved these changes

Jun 25, 2018

View reviewed changes

Copy link

Member

dstansby left a comment

Choose a reason for hiding this comment

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

👍 with a rebase

tacaswell modified the milestones:v3.1,v3.0

Jun 25, 2018

tacaswell added the Release criticalFor bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. label

Jun 25, 2018

tacaswell requested changes

Jun 25, 2018

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.

Marking as 'request changes' as I want to have a careful think about this before it gets merged.

May make sense to (if we can to it with out circular imports) move color map and norm classes from colors intocolormaps.py as well.

We should either do the whole big move for 3.0 or push the whole thing off to 3.1.

tacaswell modified the milestones:v3.0,v3.1

Jul 7, 2018

Copy link

Member

tacaswell commentedJul 7, 2018

After a bit more thought I think pushing this to 3.1 so we can do the bigger re-organization, change all the examples, etc is the right thing to do.

NelleV removed the Release criticalFor bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. label

Jul 12, 2018

Copy link

Member

NelleV commentedJul 12, 2018

Because of the change of the milestone proposed by@tacaswell, I've removed the release critical tag from this PR. If this is not correct and if the PR should be still considered as release critical, please add the tag back.