Skip to content

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 3 commits into from
Closed

Rename cm.py to colormaps.py. #11413

wants to merge 3 commits into from

Conversation

efiring
Copy link
Member

@efiring efiring commented Jun 11, 2018

Closes #1476.

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

@efiring
Copy link
Member Author

efiring commented Jun 11, 2018

This is the minimal version; actually changing all of the imports from cm to colormaps 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
timhoffm reviewed Jun 11, 2018
View reviewed changes
Copy link
Member

@timhoffm 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 the renamed colormaps module.
Copy link
Member

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

@ImportanceOfBeingErnest
Copy link
Member

Currently the cm module contains the public function register_cmap, get_cmap and the ScalarMappable class.
The colors module contains a lot of color conversion stuff but also hosts the Colormap class and its descendents, as well as all the normalizations.
So the confusing part would be that a new colormaps.py would neither contain the Colormap, nor the normalizations.

In total I'm a bit sceptical about replacing one confusion ("What does cm stand for?") with another one ("Why can't I import Colormap from colormaps?").

If truely rearanging stuff for the better, one would rather move the colormaps and normalizations out of colors and into cm - then renaming that module to colormaps would make more sense, I'd say.

@ImportanceOfBeingErnest
Copy link
Member

I just saw that MEP21 says about cm

cm
rename the module to something more descriptive - mappables?

jklymak
jklymak approved these changes Jun 12, 2018
View reviewed changes
Copy link
Member

@jklymak 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.

@timhoffm
Copy link
Member

timhoffm commented Jun 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 to colormaps in 3.0 and then to mappables 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.

@jklymak
Copy link
Member

jklymak commented Jun 12, 2018

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

dstansby
dstansby approved these changes Jun 25, 2018
View reviewed changes
Copy link
Member

@dstansby 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 tacaswell modified the milestones: v3.1, v3.0 Jun 25, 2018
@tacaswell tacaswell added the Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. label Jun 25, 2018
tacaswell
tacaswell requested changes Jun 25, 2018
View reviewed changes
Copy link
Member

@tacaswell 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 into colormaps.py as well.

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

@tacaswell tacaswell modified the milestones: v3.0, v3.1 Jul 7, 2018
@tacaswell
Copy link
Member

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 NelleV removed the Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. label Jul 12, 2018
@NelleV
Copy link
Member

NelleV commented Jul 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.

@jklymak jklymak modified the milestones: v3.1.0, v3.2.0 Feb 5, 2019
@jklymak
Copy link
Member

jklymak commented Feb 5, 2019

Re milestoning, but if we want to do this, we should try and actually do it 😉

@timhoffm timhoffm modified the milestones: v3.2.0, v3.3.0 Aug 16, 2019
@QuLogic QuLogic added the status: needs comment/discussion needs consensus on next step label Apr 1, 2020
@greglucas greglucas mentioned this pull request Apr 26, 2020
Merged
13 tasks
@efiring
Copy link
Member Author

efiring commented Apr 27, 2020

Support has evaporated...

@efiring efiring closed this Apr 27, 2020
@tacaswell
Copy link
Member

We discussed this on the call and agreed that this will probably cause more trouble that it was worth.

@QuLogic QuLogic removed this from the v3.3.0 milestone Apr 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@timhoffm timhoffm timhoffm left review comments

@tacaswell tacaswell tacaswell requested changes

@jklymak jklymak jklymak approved these changes

@dstansby dstansby dstansby approved these changes

Assignees
No one assigned
Labels
status: needs comment/discussion needs consensus on next step
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

should cm be renamed to colormaps?
8 participants
@efiring @ImportanceOfBeingErnest @timhoffm @jklymak @tacaswell @NelleV @dstansby @QuLogic