/matplotlib

DOC: Add missing cmaps to perception doc (fix for #8073) #8156

Merged
merged 6 commits into from Apr 2, 2017
+121 −140

Conversation

Reviewers

@QuLogic QuLogic

@NelleV NelleV

@phobson phobson

@dstansby dstansby

Assignees
No one assigned
Labels
Documentation
Projects
None yet
Milestone
  v2.0.1
5 participants
@afvincent @NelleV @dstansby @QuLogic @phobson
@afvincent
Contributor

afvincent commented Feb 26, 2017 • edited

Fixes #8073 .

Add the missing tab10 and tab20* colormaps to the relevant web page, that are currently missing as noticed by #8073 . I also added the Wistia colormap that also appears to be missing from this documentation.

I did a minor clean up of the grayscale.py script (removing some outdated comments for example). Furthermore, the last commit is an overhaul of the lightness.py script, aiming at making it a bit more resilient to adding or deleting one colormap map without having to tweak a lot of "magical" constant values determined by hand. I manly tried to factorize some parts when it seemed possible to do some.

The documentation is still building well :). Here is a self-contained archive version of the colormaps.html web page, with the updated plots: Choosing_Colormaps_doc.zip. For example, the figures with the Vega-related and Wistia colormaps:
grayscale_figure_3_sequential_2_colormaps
grayscale_figure_5_qualitative_colormaps
lightness_figure_3_sequential_2_colormaps
lightness_figure_5_qualitative_colormaps

If this was to be merged, please backport it to 2.0.0-doc (See @tacaswell's comment in #8073). I am still not totally friend with git and did not manage to directly target the 2.0.0-doc branch 🐑 ...

Edit: added a keyword to automatically close the related issue.

@afvincent afvincent added Documentation needs_review labels Feb 26, 2017

afvincent added this to the 2.0.1 (next bug fix release) milestone Feb 26, 2017

@afvincent
Contributor

afvincent commented Feb 27, 2017

I am not sure that I read the codecov logs correctly, but as far as I understand them, the failure is related to changes done by commits that are different from the ones of this PR.
doc/users/plotting/colormaps/colormaps.py
- 'pink', 'spring', 'summer', 'winter']),
+ ('Sequential (2)', ['afmhot', 'autumn', 'bone', 'cool', 'copper',
+ 'gist_heat', 'gray', 'hot', 'pink', 'spring',
+ 'Wistia', 'summer', 'winter']),
@phobson

phobson Mar 1, 2017

Member

Taking a step back, I think we have three subgroups of sequential colormaps:

  1. single color
  2. blended colors
  3. temperature/seasons

I guess I'd really like to see plots for each of these groups.

Opinion: And within each group, we should sort/group as logically as possible, as opposed to alphabetically

@afvincent

afvincent Mar 25, 2017

Contributor

See 3c29961: I tried to group the colormap in a more logical or hue-base fashion, rather than per alphabetical order.

doc/users/plotting/colormaps/lightness.py
mpl.rcParams.update({'font.size': 12})
-# indices to step through colormap
+# Number of colormap par subplot for particular cmap categories
@phobson

phobson Mar 1, 2017

Member

"par" -> "per" ?

@afvincent

afvincent Mar 1, 2017

Contributor

Yep (French went through the cracks of my mind…).

@afvincent

afvincent Mar 25, 2017

Contributor

Fixed!

doc/users/plotting/colormaps/lightness.py
mpl.rcParams.update({'font.size': 12})
-# indices to step through colormap
+# Number of colormap par subplot for particular cmap categories
+_DSUBS = {'Perceptually Uniform Sequential': 4, 'Sequential': 6,
@phobson

phobson Mar 1, 2017

Member

It'd be nice if we could compute the values in someway. I that would require the colors module to be reorganzed in some way. And that's definitely out of the scope of this PR.

@afvincent

afvincent Mar 25, 2017

Contributor

The new order (and the addition of some previously missing colormaps) in 3c29961 allow a more uniform set of values for _DSUBS and _DC. Basically only two different values are sufficient to clean reference plots.

I have kept an explicit dictionary with all values inside, in case one would want to further tweak these values. One could also use these dictionary to just describe the non-standard cases and rely on .get(key, default) for the others.

@afvincent
Contributor

afvincent commented Mar 1, 2017

@phobson Thank you for the feedback :). I will try to see what is feasible this weekend.
@NelleV
Contributor

NelleV commented Mar 8, 2017

@afvincent don't hesitate to ping us when it is done. This PR disappeared from the frontpage.
@afvincent
Contributor

afvincent commented Mar 8, 2017

@NelleV Ok and don't worry, it is still in my to-do list. I was just (and am still) trying to finish the other stuff I am working on in parallel, like the eventplot stuff (with your comments waiting to be addressed for example ;) ).
@afvincent
Contributor

afvincent commented Mar 12, 2017

Removing the need_review flag for the moment because I still have to see what can be done about @phobson's suggestions, and some other on-going PRs that I have are likely to require more time than I expected in the next few days. I will ping when this PR will be ready again for review.

Note to self: I think Wistia is missing from the colormaps that I demonstrated in the gallery example.

afvincent removed the needs_review label Mar 12, 2017

afvincent changed the title from DOC: Add missing cmaps to perception doc (fix for #8073) to [WiP] DOC: Add missing cmaps to perception doc (fix for #8073) Mar 12, 2017

afvincent added some commits Feb 20, 2017

@afvincent afvincent Add missing colormaps to the relevant doc pages c847be2
@afvincent afvincent tweak cmap list order for a better fit of "Wistia" 38526b4
@afvincent afvincent Cleaning up 'grayscale.py' script a769e5b
@afvincent afvincent Overhaul and refactoring of the 'lightness.py' script 0c9a280
@afvincent afvincent Reordering the colormaps and add a few missing ones

All checks have passed

5 successful checks
codecov/patch — Coverage not affected when comparing 199cea7...3c29961
Details
codecov/project/library — 58.05% (+0.05%) compared to 199cea7
Details
codecov/project/tests — 98.57% (target 97.9%)
Details
continuous-integration/appveyor/pr — AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr — The Travis CI build passed
Details
3c29961
@afvincent
Contributor

afvincent commented Mar 25, 2017

Rebased and added some missing colormaps (binary, gist_gray and gist_yarg). Also tried to group the colormaps in a more logical fashion.

I do not really understand why, but I modified doc/examples/color/colormaps_reference.rst (which more or less looks like a copy of doc/mpl_examples/color/colormaps_reference.py) and it appears to not be tracked by git. Is it expected?

Building the docs results in:
Choosing Colormaps — Matplotlib 2.0.0.post3789.dev0+g0c9a2805f documentation.zip.
The plots for people allergic to zip:
lightness_00.pdf
lightness_01.pdf
lightness_02.pdf
lightness_03.pdf
lightness_04.pdf
lightness_05.pdf
and
grayscale_01_00.pdf
grayscale_01_01.pdf
grayscale_01_02.pdf
grayscale_01_03.pdf
grayscale_01_04.pdf
grayscale_01_05.pdf

Ping to @NelleV and @phobson
@NelleV
Contributor

NelleV commented Mar 25, 2017

the doc/examples folder is generated from the examples. The fact that it is not tracked is to be expected.
@NelleV
Contributor

NelleV commented Mar 25, 2017

Once the sphinx-gallery PR is merged, those python script should really be moved to our examples folder.
I'll do a full review after coffee :D
@NelleV

NelleV approved these changes Mar 25, 2017
View changes

LGTM

NelleV changed the title from [WiP] DOC: Add missing cmaps to perception doc (fix for #8073) to [MRG+1] DOC: Add missing cmaps to perception doc (fix for #8073) Mar 25, 2017

doc/users/plotting/colormaps/lightness.py
+ 'Sequential (2)': 6, 'Diverging': 6, 'Qualitative': 4,
+ 'Miscellaneous': 6}
+
+# Spacing between the colormaps of a subplot
@QuLogic

QuLogic Mar 25, 2017 • edited

Member

I think it makes more sense to change the figure size like it used to be done. Now you get a different sized Axes and different number of ticks for each figure.

@afvincent

afvincent Mar 26, 2017

Contributor

I do not understand to which situation you are refering to: looking at blame on GH, the dc and dsub strategy is used since at least 3 years. I tried to not modify the spirit of the plots, but rather to refactor what could be, and to make the magic values a bit more uniform across all the different cases.

@QuLogic

QuLogic Mar 26, 2017

Member

Oh, this comment probably shouldn't be here but on a subplots line. In any case, I missed the fact that you kept the changing figure size, so it should be the same as before.

But if the figure size changes based on the number of plots, then I don't understand why 00 and 05 have 3 y-ticks, but the others have 5.

@afvincent

afvincent Mar 26, 2017

Contributor

I did not even notice the different amount of ticks ^^... If I had to guess, I would say that it may be an artifact from plt.tight_layout() that gives more or less place to the subplots depending on length of the x-tick labels.

@QuLogic

QuLogic Mar 26, 2017

Member

That might be it. It would be nice if we didn't have to use tight_layout, but I guess then we'd need to know the size of the colormap labels first.

@afvincent
Contributor

afvincent commented Mar 26, 2017 • edited

Please do not merge yet, I saw a minor typo (l.37 on lightness.py). I'll try to fix it as soon as I get back to my laptop.

Edit: the typo has been fixed. I the unconsistent number of y-ticks that @QuLogic noticed is an issue, what about to simply use an ad-hoc formatter? Maybe even something as simple as ax.set_yticks([0, 50, 100]) or ax.set_yticks([0, 25, 50, 75, 100]) if one want to keep the script length as short as possible.
@afvincent afvincent Fix minor typo

All checks have passed

5 successful checks
codecov/patch — Coverage not affected when comparing 199cea7...e442647
Details
codecov/project/library — 58.46% (+0.45%) compared to 199cea7
Details
codecov/project/tests — 98.57% (target 97.9%)
Details
continuous-integration/appveyor/pr — AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr — The Travis CI build passed
Details
e442647

@dstansby dstansby merged commit 50bb88a into matplotlib:master Apr 2, 2017
5 checks passed

5 checks passed

codecov/patch Coverage not affected when comparing 199cea7...e442647
Details
codecov/project/library 58.46% (+0.45%) compared to 199cea7
Details
codecov/project/tests 98.57% (target 97.9%)
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

dstansby changed the title from [MRG+1] DOC: Add missing cmaps to perception doc (fix for #8073) to DOC: Add missing cmaps to perception doc (fix for #8073) Apr 2, 2017

@dstansby dstansby added a commit that referenced this pull request Apr 2, 2017

@dstansby dstansby Merge pull request #8156 from afvincent/add_missing_cmaps_to_percepti… 
…on_doc

DOC: Add missing cmaps to perception doc (fix for #8073)
 8be411f
@dstansby
Contributor

dstansby commented Apr 2, 2017

Backported to 2.0.x via. 8be411f
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment