-
-
Notifications
You must be signed in to change notification settings - Fork 8.2k
DOC: colormaps docstring update #889
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
Conversation
("overwrite" = original file no longer exists. "override" = file still exists, but we're ignoring what it says)
some typo fixes and small changes to the matplotlibrc stuff
… as best I can tell
lib/matplotlib/pyplot.py
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As an identifier with a _ prefix is supposed to be private, by Python convention, we probably shouldn't recommend using it in the docs. I think we should add a public alias for this and then recommend using the that in the docs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at this again, I don't see why this very function colormap couldn't be used to return a list of colormap names. This function currently only exists for documentation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Making colormaps() return a list of colormaps seems pretty logical to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would probably get all the colormaps with:
import matplotlib.cm as cm
print cm.cmap_d.keys()
But having a pyplot function which returns a list of cmap names sounds sensible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Additionally, it would be nice to use full sphinx cross linking here (even if the thing you are linking to is not documented). e.g.
... which can be found in :class:`~matplotlib.cm.cmap_d`.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, cm.cmap_d.keys() and cm.datad.keys() both include all the reversed names like gray_r, which is probably not desirable for printing? _cmapnames is created from datad before it gets the reversed names added to it, so I used that. Didn't realize it shouldn't be exposed to the user like that, though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the other hand, if you use register_cmap(), the new colormap shows up in cmap_d.keys(), but not in datad.keys() or _cmapnames, so cmap_d.keys() seems like the right answer. I guess showing the _r colormaps too is legitimate, since ones you create with register_cmap don't have any _r version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds like you are turning over some stones and finding some interesting "features". It might be worth putting some comments/attribute docstrings in matplotlib.cm at the datad, _cmapnames and cmap_d variables. Additionally, some of these inconsistencies might be improved by making the default colormaps go through the register_cmap function rather than have them modify the data structures directly (but that is a bigger change, and I can't guarantee that it is entirely sensible).
|
This looks great. The rst formatting seems to be fine. Hop on the mailing list if you're having trouble building the docs -- I or others can help. It would be awesome to also include the |
|
This is excellent work. Thank you for putting the time in for doing this! |
lib/matplotlib/pyplot.py
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally we would like to get line widths <= 80 characters for smaller screens, would you mind putting newlines through this and other long lines.
|
@endolith great stuff! The more people contributing to the documentation the better we can get it. @ mpl devs: As far as I can see it, this would be suitable to go into 1.1.x. Maybe we should spark off another issue which implements the syntax discussed for setting a default palette instead of the |
|
See #896. Directly relevant to the discussion here. |
lib/matplotlib/pyplot.py
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You might replace the parenthetical explanation by saying, "...meant to be used for cyclic variables such wind direction or time of day",
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, that makes sense.
"meant for plotting cyclic values that wrap around at the endpoints, such as wind direction or time of day"?
that could actually be a 4th category, couldn't it? I got the 3 categories from colorbrewer documentation, but they don't have any cyclic ones. any idea what the flag and prism maps are meant for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Flag and prism might be used as alternatives for ordinary line contours, primarily giving a sense of shapes and gradients; but I suspect they are rarely used at all. Matlab has them, so we do too, for better or worse.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I added a description of cyclic as a separate scheme
…chemes, discourage use of qualitative and redundant schemes, change some details based on luminance plots
`gist_yarg`, `gist_gray`, and `binary` are identical to `gray`, so they are "on a deprecation path", and `spectral` was renamed to `nipy_spectral` to avoid conflicts with `Spectral` matplotlib#889
This is an attempt at updating the colormaps docstring to include all the new colormaps and more information about where they came from and what they're supposed to do, some of which was in comments in _cm.py, but not otherwise visible to users. (https://gist.github.com/2719900)
Warning: I don't know if I did the rst formatting correctly, if the descriptions of colormaps are totally correct, if we should also mention that the "base" colormaps are copied from Matlab, or how to properly do forks and pull requests in GitHub. :)