Skip to content
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

ENH: Add newly proposed colormaps #4707

Merged
merged 2 commits into from
Jul 23, 2015
Merged

Conversation

stefanv
Copy link
Contributor

@stefanv stefanv commented Jul 15, 2015

This PR adds four new color maps to matplotlib, including the newly proposed default viridis.

The colormaps are currently specified by 256 data values.

@tacaswell tacaswell added this to the next point release milestone Jul 15, 2015
@tacaswell tacaswell added the Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. label Jul 15, 2015
@tacaswell
Copy link
Member

hurray!

@efiring
Copy link
Member

efiring commented Jul 15, 2015

Thank you!
At some point it would be nice to condense the increasingly monstrous _cm.py; it has a lot of useless precision. In many cases the LinearSegmentedColormap machinery is not needed. In the end everything is being done with a lookup table, usually with 256 entries. For something like these new colormaps, the only benefit of the LinearSegmentedColormap route is that it allows the generation of a lookup table with a larger number of entries.

'pink',
'prism',
'spring',
'summer',
'sunset',
Copy link
Member

Choose a reason for hiding this comment

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

The advantage of "sunrise" over "sunset" is that the map goes from dark to light; but maybe one of the other naming schemes will override all this anyway. (Also, "sunrise" has a more upbeat, optimistic feel...)

@tacaswell
Copy link
Member

I am in favor of making one of the 'rocket' 🚀 per @mwaskom

@@ -1971,12 +1971,16 @@ def colormaps():
by changing the hue component in the HSV color space
jet a spectral map with dark endpoints, blue-cyan-yellow-red;
based on a fluid-jet simulation by NCSA [#]_
neoncity perceptually uniform shares of blue-red-yellow
Copy link
Member

Choose a reason for hiding this comment

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

"shares" --> "shades"

@tacaswell
Copy link
Member

@efiring Can you be in charge of the naming issues on our side? Hopefully this thread stays calmer than the skimage thread.

@efiring
Copy link
Member

efiring commented Jul 17, 2015

OK; but just as there was no agreement among those polled about the virtues of A, B, and C, I expect virtually no agreement about how to name A, B, and C. We'll work it out, though.

@WeatherGod
Copy link
Member

I am fine with the names as-is.
On Jul 17, 2015 3:10 AM, "Eric Firing" notifications@github.com wrote:

OK; but just as there was no agreement among those polled about the
virtues of A, B, and C, I expect virtually no agreement about how to name
A, B, and C. We'll work it out, though.


Reply to this email directly or view it on GitHub
#4707 (comment)
.

@stefanv
Copy link
Contributor Author

stefanv commented Jul 17, 2015

The only vague agreement I could sense in the other thread was for 'rocket'. Shall I replace viola with rocket?

@efiring
Copy link
Member

efiring commented Jul 19, 2015

@stefanv, I'm going to make a strong recommendation, with my rationale. If you or Nathaniel are strongly opposed, then I will defer to you; otherwise, I hope my recommendation will prove in the long run to be a satisfactory way to get the paint on the bikeshed and move on. I expect my recommendation will disappoint and/or annoy most of the people who have weighed in on this topic, but so be it.

General criteria for most non-default colormaps:

  1. Names should have some visual connection to the color sequence in the map, and the closer, the better.
  2. Names should be short and simple.
  3. In case of subtle variations on a theme, ideally the naming should clearly indicate the grouping.

Starting with (1), the viable candidates are related to fire (I will put "rocket" in this category), magma, and sunrise/sunset. All of these can have some near-black, all can have some very light yellows, and some reds and orange tones. The problem with the first two of these is that I don't think they have magenta shades, which are prominent in A, B, and C. To my eye, the sunrise/sunset names are by far the closest to the actual shades in the maps. Sunrise has the additional advantage of going in the right direction, dark to light.

(2) eliminates suggestions like "softsunrise".

(3) with (2) favors the use of numbers to distinguish subtle variations.

My recommendation:

  • B is "sunrise"
  • A is "sunrise1"
  • C is "sunrise2"

I gave B the priority of having no number because I think it is (by a whisker) the best default of the group: C is too blue, A lacks contrast with a white background at the high end.

@njsmith
Copy link

njsmith commented Jul 19, 2015

My personal favorite is still this: scikit-image/scikit-image#1599 (comment)
but I'll leave it at that :-)

@efiring
Copy link
Member

efiring commented Jul 22, 2015

@stefanv What do you think? Please either pick my suggestion, or any of @njsmith's variations, or whatever you negotiate with Nathaniel, and make the change to the PR. I will merge it with pleasure. No problem, no regrets, no more discussion needed.

@njsmith
Copy link

njsmith commented Jul 22, 2015

@stefanv: just on the off chance you've been waiting to catch me at the
office or something: I'm out of town all week, but not terribly busy so if
you do want my input on anything feel free to call or whatever. Or not :-).
On Jul 22, 2015 12:19 PM, "Eric Firing" notifications@github.com wrote:

@stefanv https://github.com/stefanv What do you think? Please either
pick my suggestion, or any of @njsmith https://github.com/njsmith's
variations, or whatever you negotiate with Nathaniel, and make the change
to the PR. I will merge it with pleasure. No problem, no regrets, no more
discussion needed.


Reply to this email directly or view it on GitHub
#4707 (comment)
.

@stefanv
Copy link
Contributor Author

stefanv commented Jul 22, 2015

@efiring Is this the most optimal way of representing the colormap values? I put a very large table in there, but I can also do it differently (I just followed what was in the file already).

I'll chat to Nathaniel and figure out what to do with the naming

@efiring
Copy link
Member

efiring commented Jul 22, 2015

Short answer: please use your list of rgb values to initialize a ListedColormap; and ideally, reduce the number of digits printed out in the list.

@stefanv, in fact I don't like the way that _cm.py has ballooned even before this PR. I think the LinearSegmentedColormap initialization via dictionaries for r, g, and b is highly inefficient and awkward for all of the maps that are being specified with a large number of points. The specification can be cut down to 33% simply by feeding the rgb values into the from_list() method that will generate the same dictionary that you have now. To make it even more efficient, use the list of rgb tuples to initialize a ListedColormap instead of a LinearSegmentedColormap. As far as I can see, there will be no difference in the end result, but the ListedColormap goes straight to the LUT instead of taking a roundabout route to the same LUT. File size and parsing time can be cut down even more by reducing the number of significant figures in the rgb values to something reasonable. LinearSegmentedColormap was designed to allow the creation of both smooth and discontinuous maps from dictionaries with only a few entries; it's purpose is to do the interpolation, and it does so in rgb space. There is no point in using it when a dense table of rgb values has been created by other means.

@njsmith
Copy link

njsmith commented Jul 23, 2015

So looking at the code to make sure I understand what you mean -- it looks like LinearSegmentedColormap ends up just discretizing to 256 points (by default, but everyone uses the default), and if I'm counting right the PR is already generating 256 points, so we might as well just do ListedColormap(array with shape (256, 3), name=name)? And write down that array with, say, 4 digits of precision, because it's going to an 8-bit-per-channel monitor anyway? (I guess there do exist workflows that can handle a bit more color precision than that, but even 5 or 6 digits is enough for a full 16-bit workflow if I'm counting right.)

@efiring
Copy link
Member

efiring commented Jul 23, 2015

@njsmith, yes, exactly.

@efiring
Copy link
Member

efiring commented Jul 23, 2015

The travis failure is in the doc build, and is unrelated.

@stefanv
Copy link
Contributor Author

stefanv commented Jul 23, 2015

Eric, I gave it a shot--let me know what you think. Happy to rework as necessary.

@@ -1971,12 +1971,16 @@ def colormaps():
by changing the hue component in the HSV color space
jet a spectral map with dark endpoints, blue-cyan-yellow-red;
based on a fluid-jet simulation by NCSA [#]_
neoncity perceptually uniform shades of blue-red-yellow
Copy link
Member

Choose a reason for hiding this comment

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

I think that updating the names in this docstring is the last thing remaining.

@stefanv
Copy link
Contributor Author

stefanv commented Jul 23, 2015

Good catch!

@efiring
Copy link
Member

efiring commented Jul 23, 2015

PEP8 is being pesky as usual. It is unhappy about the space after each '[' and before each ']'.

@stefanv
Copy link
Contributor Author

stefanv commented Jul 23, 2015

Updated to fix spacing

@efiring
Copy link
Member

efiring commented Jul 23, 2015

Github's presentation of what has passed and what has failed has been changed recently, and the new version is self-contradictory, with two inconsistent statements, both of which are wrong. In any case, looking at Travis, there are two failures. One failure is the typical false alarm (random glitch associated with LaTeX), but pep8 still has one complaint: in _cm_listed.py, it wants all imports to be at the top of the file. Sorry I missed that the last time.

@stefanv
Copy link
Contributor Author

stefanv commented Jul 23, 2015

grumble I passed this through flake8 beforehand--which checker do you use?

@stefanv
Copy link
Contributor Author

stefanv commented Jul 23, 2015 via email

@efiring
Copy link
Member

efiring commented Jul 23, 2015

pep8 testing is done with the pep8 module, invoked in lib/matplotlib/tests/test_coding_standards.py. That, in turn, can be invoked from the tests.py script in the base directory, or via

python setup.py test --pep8-only

efiring added a commit that referenced this pull request Jul 23, 2015
ENH: Add newly proposed colormaps
@efiring efiring merged commit f7a5786 into matplotlib:master Jul 23, 2015
@efiring
Copy link
Member

efiring commented Jul 23, 2015

I don't know why github says a check failed; I looked at the travis page, and everything was green. In any case, the 4 new maps are now in place. Thanks very much for all the work you and @njsmith have done leading up to this.

@stefanv
Copy link
Contributor Author

stefanv commented Jul 23, 2015

Hah, interesting--this update was made to the PEP8 checker since my last install (a few months ago). Thanks for the explanation and the reviews, Eric!


# This function was autogenerated by boilerplate.py. Do not edit as
# changes will be lost
def magma():
Copy link
Contributor

Choose a reason for hiding this comment

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

I thought these functions were not supposed to be created any longer? The originals were only included for Matlab compatibility, and all other colormaps should be set with set_cmap('magma')?

See #889 (comment)

Copy link
Member

Choose a reason for hiding this comment

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

There are auto generated so all that is done here is to regenerate pyplot with boilerplate.py

If you want to change that I suggest raising a new issue.

@endolith
Copy link
Contributor

@efiring

in fact I don't like the way that _cm.py has ballooned even before this PR. I think the LinearSegmentedColormap initialization via dictionaries for r, g, and b is highly inefficient and awkward for all of the maps that are being specified with a large number of points.

Should all the ColorBrewer maps be converted to ListedColormap based on the original values from the spreadsheet, then? (Defined using 12 points per colormap at most.)

LinearSegmentedColormap was designed to allow the creation of both smooth and discontinuous maps from dictionaries with only a few entries; it's purpose is to do the interpolation, and it does so in rgb space.

Should it interpolate in a perceptual space instead?

@OrkoHunter
Copy link

Is it true that the plot used in the new paper of the gravitational field used the color map added in this PR ? It looks so, doesn't it?

@pelson
Copy link
Member

pelson commented Feb 12, 2016

Is it true that the plot used in the new paper of the gravitational field used the color map added in this PR ? It looks so, does it?

Yes. The tutorial demonstrates it here: https://losc.ligo.org/s/events/GW150914/GW150914_tutorial.html.

@duncanmmacleod
Copy link
Contributor

Yes, the LSC has been pretty good early adopters of the new defaults for
matplotlib 2.0, including this now famous figure.

Independent of colour maps, a significant fraction of the figures in this
paper and in the so-called 'companion' papers released on the arXiv today
use matplotlib for figure generation, with a scattering of matlab and
mathemetica in there as well. So, thanks go to the matplotlib community for
a great resource with which to present our work.

Duncan

On 12 February 2016 at 01:39, Phil Elson notifications@github.com wrote:

Is it true that the plot used in the new paper of the gravitational field
used the color map added in this PR ? It looks so, does it?

Yes. The tutorial demonstrates it here:
https://losc.ligo.org/s/events/GW150914/GW150914_tutorial.html.


Reply to this email directly or view it on GitHub
#4707 (comment)
.

Duncan Macleod
duncan.macleod@ligo.org

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants