-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
[NF] Add 'truncate' and 'join' methods to colormaps. #7716
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
c70c1b5
to
615ca22
Compare
lib/matplotlib/colors.py
Outdated
The fraction of the new colormap that should be occupied | ||
by self. By default, this is ``self.N / (self.N + | ||
other.N)``. | ||
|
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.
What does the N
kwarg do?
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've updated the docstring.
Current coverage is 62.10% (diff: 100%)@@ master #7716 diff @@
==========================================
Files 109 174 +65
Lines 46648 56062 +9414
Methods 0 0
Messages 0 0
Branches 0 0
==========================================
+ Hits 31061 34815 +3754
- Misses 15587 21247 +5660
Partials 0 0
|
lib/matplotlib/colors.py
Outdated
cmap_trunc = cmap.truncate(0.2, 0.7) | ||
|
||
""" | ||
assert minval < maxval, "minval must be less than maxval" |
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.
Can you use if not cond: raise
instead of asserts? A while ago we went through and removed all of the run-time uses of asserts for input validation as (in principle) the interpreter can decide to ignore the assert statements.
lib/matplotlib/colors.py
Outdated
@@ -742,6 +742,107 @@ def func_r(x): | |||
|
|||
return LinearSegmentedColormap(name, data_r, self.N, self._gamma) | |||
|
|||
def join(self, other, name=None, frac_self=None, N=None): |
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.
It might be a good idea to also add add a __add__
method so you can do cm1 + cm2
and get a 50/50 map? Could be too cute for it's own good.
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 had this idea as well, but thought reviewers would find it 'too cute'! It's included now.
lib/matplotlib/colors.py
Outdated
@@ -742,6 +742,107 @@ def func_r(x): | |||
|
|||
return LinearSegmentedColormap(name, data_r, self.N, self._gamma) | |||
|
|||
def join(self, other, name=None, frac_self=None, N=None): |
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.
Maybe put a generic version of this (using ListedColormap
in all case on Colormap
? Might also make sense to have a generic join_color_map(*cmaps, fractions=None)
as a top-level function (rather than as methods)?
👍 , left some comments about the API This will definitely need a |
Most of the suggested changes have been made. A few things:
|
After adding the |
@tacaswell I'm sure you're busy. I just wanted to make sure this wasn't forgotten. Any thoughts on my questions above? |
That idea is interesting. I would ditch the args-style indexing (e.g., `new_cm
= cmap[0.2, 0.7, 64]`). But the slicing approach really makes sense, and to
also allow the numpy-style complex slicing is quite clever.
…On Mon, Jan 30, 2017 at 6:03 PM, Levi Kilcher ***@***.***> wrote:
@tacaswell <https://github.com/tacaswell> I'm sure you're busy. I just
wanted to make sure this wasn't forgotten. Any thoughts on my questions
above?
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#7716 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AARy-EBi8f1Uy45wvMfPOhWmn-l9l7_hks5rXmxCgaJpZM4LYmOg>
.
|
688ff01
to
a0ee437
Compare
Can anyone help me understand what is breaking here? |
It looks like it's just PEP8 violations, for lines that are longer than 79 characters. You should be able to see which lines are causing the error here: https://travis-ci.org/matplotlib/matplotlib/jobs/197464743#L1767 |
Not sure if there is bandwidth to include this in 2.1, but I'm available to work on it. I think I've addressed all existing comments, except for the Also, I've addressed @WeatherGod 's comments on the proposed |
7680655
to
5ac5bb0
Compare
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.
This looks interesting and helpful. Needs to pass the tests. Not sure if I think we need an image test.
plt.pcolormesh(data, cmap=cmap) | ||
plt.colorbar(orientation='vertical') | ||
|
||
|
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.
Not sure this requires image comparisons (which are repo-heavy). Can you just test on the returned colormaps?
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.
Hi Jody! I hadn't thought of this, but it makes sense. Are the colormaps sitting somewhere that I can compare to, or... ?
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.
Hi Levi, I'd just evaluate the colormap at 5 or 6 locations and assert that the values returned are what you say they should be. That seems trivial but if someone mucks with your code they will have to also change the test and thats a good warning to double check what was done.
This is based largely on https://gist.github.com/denis-bz/8052855 Which was based on http://stackoverflow.com/a/18926541/2121597
Note that these methods now always return `ListedColormap`. Also: switch from `assert` to `raise ValueError`.
reorder `join` inputs (for consistency) +doc fixes/changes
OK. I think this is ready to go. Let me know if there is anything else you need. |
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.
This looks really useful to me. Not 100% sure about the format of the getitem string, and I'm not even sure where this would render in the docs. I also wish this had an actual example somewhere so folks will know about it, otherwise, I think the only documentation is in the whats-new. OTOH that could be done for a second PR...
|
||
# ### float indexing | ||
# for float-style indexing, the values must be in [0.0, 1.0] | ||
# Truncate the colormap between 20 and 80%. |
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.
Typo? 80->60?
At the weekly call this week this got substantial discussion. The upshot was that this seems a lot of maintenence, support, and API overhead for something that the user should be able to do themselves, perhaps with better documentation. Basically truncating and subsetting colormaps boils down to making the appropriate matrix to feed to The Todo here is to add a section to https://matplotlib.org/tutorials/colors/colormaps.html#sphx-glr-tutorials-colors-colormaps-py that explains colormap manipulation better, and perhaps adding an example or two to the example directory. @lkilcher thanks so much for the contribution, and sorry it languished so long. If you want to give input to the examples when those PRs come in, I'll ping you, or contributions along those lines would be most welcome. |
This is disappointing, and it's too bad this conversation didn't happen before I put time into the tests etc.. I can understand that Making a clearly documented interface to the arrays makes sense, and certainly replaces the need for I apologize for coming across as a disgruntled contributor. I just know quite a few people who find MPL colormaps very cumbersome, and I put time into this because it got positive feedback from some senior MPL devs. I also know there are those out there who think creating/modifying new colormaps is a bad idea, but I find that reasoning arrogant. Hopefully someone will actually work on MEP21 (especially killing LSCs), and perhaps a simple I'll track of your progress on #11905 and see if I have anything to add. |
FWIW I share your concern about the opacity of colormaps and how to create them, hence #11905, because info about WRT being able to create your own colormaps, I hope I didn't make it sound like people shouldn't be allowed to do that, or that anyone on the dev team thinks that. In general, the devs are concerned about the balance between "what we can do" and "what we should do". Some features got out of control in the past, and are now maintenance drags on the library. I think the current philosophy is to try to keep things as lightweight as possible and to have fewer black boxes. |
Closing in lieu of documentation of how to do the same thing w/ ListedColormaps. @lkilcher again apologies for the long review process that eventually didn't get merged. We really want people to contribute, so we try to avoid this, but sometimes ideas fall between the cracks. |
This is functionality that I've been wanting built-in for a while. The inspiration largely comes from here and here. I know @tacaswell had asked that this functionality be included way back in 2014.
I think there is still an 'issue' here if you have/want a discontinuous
LinearSegmentedColormap
because this implementation just smooths those over. Still, ifN
is high, that shouldn't be too much of a problem. I've developed a version of theLinearSegmentedColormap.truncate
method that actually crops thecdict
appropriately, but since MEP21 seems to indicate that MPL may be moving away from LSC's, I didn't bother including it. Plus, it's a bit tricky to create ajoin
method that correctly joins LSC's to listed cmaps.Suggestions welcome!