Fix contour color #538

rabernat · 2015-08-18T18:24:36Z

This fixes #537 by adding a check for the presence of the colors kwarg.

shoyer · 2015-08-18T19:39:36Z

xray/plot/plot.py

@@ -452,6 +452,8 @@ def newplotfunc(darray, ax=None, xincrease=None, yincrease=None,
            # pcolormesh
            kwargs['extend'] = cmap_params['extend']
            kwargs['levels'] = cmap_params['levels']
+            if 'colors' in kwargs:


colors and cmap should probably be required to be mutually exclusive? I don't think it makes sense to supply both.

I agree. So you're saying it should raise an error if you supply both? That is what contour does.

I think so. On the other hand, it looks like we've already put the listed colors functionality of colors into cmap in #509.

@jhamman what do you think? I think we probably want to move the listed colors functionality shown here from cmap into colors. That would more naturally handle the single color case desired here.

Looking at the wrapped matplotlib functions only contour, contourf have the colors arg in the docstring. But all 4 take a cmap argument.

@shoyer are you suggesting that they all have a colors arg? The more uniform we can make everything here, the better IMHO.

Yes, I think that's probably a better way to provide explicit color(s) than overloading cmap. We can raise an error is colors is provided without the levels arguments.

I'm fine with this. It will just be important to distinguish between a list of colors and a ListedColorMap.

Is compatibility with matplotlib conventions considered important? If so, you probably don't want to raise an error if contour is called with colors but without levels. That is a pretty common usage pattern when e.g. you want to make a black and white contour plot with auto-determined contour levels. I would just do plt.contour(field, colors='k') and let matplotlib pick the levels.

@rabernat Good point- we should be able to do this.

Yep, agreed. The way we implemented this, the default levels=None means no levels for color mesh or imshow plots but 7 levels for contour and contourf.

On Thu, Aug 20, 2015 at 9:28 AM, Clark Fitzgerald
notifications@github.com wrote:

@@ -452,6 +452,8 @@ def newplotfunc(darray, ax=None, xincrease=None, yincrease=None,
# pcolormesh
kwargs['extend'] = cmap_params['extend']
kwargs['levels'] = cmap_params['levels']

if 'colors' in kwargs:

@rabernat Good point- we should be able to do this.
Reply to this email directly or view it on GitHub:
https://github.com/xray/xray/pull/538/files#r37550742

shoyer · 2015-08-18T19:40:34Z

It would be very useful to have a test here to verify that the colors argument does the right thing.

rabernat · 2015-08-24T14:14:48Z

So what is the conclusion about this PR? I am happy to keep tweaking it if we can converge on the desired behavior...

jhamman · 2015-08-24T15:50:48Z

I have no problems with this. @clarkfitzg - any more thoughts?

shoyer · 2015-08-24T16:07:15Z

We can do this as an interim fix, but I would prefer something more comprehensive:

add colors to the function/method signature for 2d plots
require colors when specifying a list of colors manually
colors is mutually exclusive with cmap
colors is only valid when levels is supplied or the plot is of type contour or contourf
deprecate supplying a list of colors with cmap (update the docs, issue a warning when this is done, eventually remove it entirely)

clarkfitzg · 2015-08-24T16:19:57Z

@shoyer has laid out a nice plan.

@rabernat do you feel like taking this on now? If not we can merge this and implement the rest later.

rabernat · 2015-08-25T00:54:33Z

I can probably get to this within a few days. I would enjoy learning more about the plotting code.

rabernat · 2015-08-26T14:59:29Z

This update mostly implements @shoyer's plan. However, it currently only works if seaborn is installed. This is because it completely bypasses matplotlibs colors keyword and directly sets the colors in the internal functions (e.g. _color_palette). It relies on seaborn's color_palette function to turn the color list into a palette. I can't see an obvious way around this. Maybe someone has a suggestion.

shoyer · 2015-08-26T19:01:31Z

Looks like you have commits from a few other changes (e.g., the 0.6 release) in this PR. Could you do a rebase -i to fix that? e.g., see the discussion here:
pandas-dev/pandas#9826 (comment)

rabernat · 2015-08-26T19:21:00Z

Sorry...I'm an idiot, and I always somehow manage to screw up these pull requests.

I did what you / that thread suggested

git fetch upstream master
git rebase -i upstream/master

When I try to push my PR again with git push origin fix_contour_color I get these errors

$ git push origin fix_contour_color 
To git@github.com:rabernat/xray.git
 ! [rejected]        fix_contour_color -> fix_contour_color (non-fast-forward)
error: failed to push some refs to 'git@github.com:rabernat/xray.git'
hint: Updates were rejected because the tip of your current branch is behind
hint: its remote counterpart. Merge the remote changes (e.g. 'git pull')
hint: before pushing again.
hint: See the 'Note about fast-forwards' in 'git push --help' for details.

rabernat · 2015-08-26T19:35:32Z

Ok, I think I fixed it. Just had to do a git push --force. Sorry for being such a git n00b. But honestly sometimes it is so confusing.

rabernat · 2015-08-26T19:53:19Z

The testing errors I think are due to seaborn not being present.

jhamman · 2015-08-26T22:31:47Z

Do we want to make seaborn a mandatory dependency for plotting?

rabernat · 2015-08-27T17:54:58Z

@jhamman is that question for me? I would say obviously not.

However, I think this dependency was already present even before this PR. There just wasn't a test that exposed it. If you passed a list of colors to cmap, I'm pretty sure you would get an error if seaborn was not installed. (Please correct me if I'm wrong.)

jhamman · 2015-08-27T18:26:16Z

You are correct. We should create a colormap if seaborn is not installed. The relevant line is here: https://github.com/rabernat/xray/blob/fix_contour_color/xray/plot/plot.py#L300

where we could add something like this sudo code:

if isinstance(cmap, list-like-thing):
    cmap = _cmap_from_list_of_colors(cmap)

clarkfitzg · 2015-08-27T23:59:33Z

@rabernat We were all Git noobs at some point- don't worry about it!

Seaborn should not be a dependency.

clarkfitzg · 2015-08-28T00:05:03Z

xray/plot/plot.py

@@ -414,13 +419,27 @@ def _plot2d(plotfunc):
    @functools.wraps(plotfunc)
    def newplotfunc(darray, ax=None, xincrease=None, yincrease=None,
                    add_colorbar=True, add_labels=True, vmin=None, vmax=None, cmap=None,
-                    center=None, robust=False, extend=None, levels=None,
+                    center=None, robust=False, extend=None, levels=None, colors=None,


You'll also want to add this to the def plotmethod lower in the function.

I think you still need to fix this. Also, I would put colors immediately following cmap in the function argument.

rabernat · 2015-08-31T00:08:36Z

xray/plot/plot.py

@@ -297,6 +298,9 @@ def _color_palette(cmap, n_colors):
        # Use homegrown solution if you don't have seaborn or are using viridis
        if isinstance(cmap, basestring):
            cmap = plt.get_cmap(cmap)
+        elif isinstance(cmap, (list, tuple)):
+            from matplotlib.colors import ListedColormap
+            cmap = ListedColormap(cmap, N=n_colors)


This is how you work around if seaborn is not installed.

rabernat · 2015-08-31T00:09:32Z

I think this latest commit solves everything.

shoyer · 2015-08-31T00:28:33Z

xray/plot/plot.py

@@ -386,8 +390,12 @@ def _plot2d(plotfunc):
        The mapping from data values to color space. If not provided, this
        will be either be ``viridis`` (if the function infers a sequential
        dataset) or ``RdBu_r`` (if the function infers a diverging dataset).
-        When ``levels`` is provided and when `Seaborn` is installed, ``cmap``
-        may also be a `seaborn` color palette or a list of colors.
+        When when `Seaborn` is installed, ``cmap`` may also be a `seaborn` 


Does this case really make sense for the cmap argument rather than colors?

I'm not entirely sure, but in any case it is still worth noting that color palettes are only valid arguments when plotting discrete colors.

Now the docs make it clear that you need to specify levels (or use contour/contourf) with a seaborn named palette in cmap. That is a slightly different case than specifying a list of colors. This seems consistent, because now you never give a list to cmap.

shoyer · 2015-08-31T00:34:04Z

I think this will be the first PR merged for v0.6.1. Could you add a brief note to the what's new documentation under a section for "API changes"? You'll need to create a new section for v0.6.1.

rabernat · 2015-08-31T04:31:04Z

This is turning into a real rabbit hole...

rabernat · 2015-08-31T04:35:45Z

xray/plot/plot.py

-    try:
-        from seaborn.apionly import color_palette
-        pal = color_palette(cmap, n_colors=n_colors)
-    except (TypeError, ImportError, ValueError):


The _color_palette function was a big source of confusion for me because it served two very different purposes:

It translated a list of colors into a palette using seaborn

It translated named colormaps (e.g. jet) into palettes, including possibly seaborn palettes

I have refactored the function a little to make these two roles more clear.
It turns out that seaborn is not needed for 1. It can be done with matplotlib.colors.ListedColormap. I have left the seaborn version of this here for now, but if it does the same thing as matplotlib, it can be removed.

shoyer · 2015-08-31T04:38:36Z

@rabernat indeed, your willingness to dive in here is highly appreciated! I think this is pretty close -- are there aspects that you're still unsure about?

rabernat · 2015-08-31T04:40:30Z

xray/plot/plot.py

-        may also be a `seaborn` color palette or a list of colors.
+        When when `Seaborn` is installed, ``cmap`` may also be a `seaborn` 
+        color palette. If ``cmap`` is seaborn color palette and the plot type
+        is not ``contour`` or ``contourf``, ``levels`` must also be specified.


Seaborn colormaps (e.g. husl) need to have the number of levels specified. This is the only way in which they behave differently from matplotlib colormaps. This is a little bit annoying, since you can do imshow(cmap='jet') but instead have to do imshow(cmap='husl', levels=20). I have added a test for this.

fixed seaborn dependency in cmap generation test for new contour color rules updated docs test for seaborn palettes forgot colors in plotmethod

rabernat · 2015-08-31T04:53:32Z

@shoyer There is still some redundancy between the colors and cmap keywords. I know what I would use these for, but I am still unsure how the original designer of the plotting api imagined these would work. My original problem was simply that I couldn't use colors the way I would in matplotplot.

I am also a little unsure about how seaborn fits into the plotting module. It is not a dependency, but certain parts of plotting are built around it. This leads to the need for lots of special cases and some ugly code.

shoyer · 2015-08-31T22:40:55Z

@jhamman is the original designer of this API so perhaps he can clarify :).

One possibly cleaner alternative is to have a separate palette argument for specifying Seaborn color palettes, rather than reusing cmap or trying to put it in colors. That would eliminate some ambiguity.

shoyer · 2015-08-31T22:44:47Z

xray/plot/plot.py

+            pal = color_palette(cmap, n_colors=n_colors)
+        except ImportError:
+            # if that fails, use matplotlib
+            # in this case, is there any difference between mpl and seaborn?


Seaborn is doing something extremely similar: https://github.com/mwaskom/seaborn/blob/v0.6/seaborn/palettes.py#L404-L408

jhamman · 2015-08-31T22:52:49Z

@rabernat and @shoyer -

Sorry I've been mostly absent in this conversation recently. I'm currently buried under a mound of C and Fortran code.

I included the optional seaborn call in the discrete colormap/colorbar PR to expose a few nice features in the seaborn color_palette api. Namely, I wanted access to all the seaborn named color palettes and to be able to pass a list of custom colors.

I would be okay with adding the palette argument but it seems a bit excessive since palette, cmap, and colors are all basically orthogonal. We don't have to stick to the matplotlib or seaborn api. I understand the hesitation to overload any one of these parameters.

rabernat · 2015-09-01T02:10:51Z

@jhamman One nice thing about this current PR is that it does allow people without seaborn to still use a custom list of colors. If we don't want to make seaborn a dependency, then surely that is a good thing. The only thing seaborn is now needed for is its special named palettes.

As for the named palettes, I don't think a new keyword is needed. cmap does what one would expect.

This has been a good learning experience for me. (Had never touched seaborn before.) But I probably won't work on this PR any more. Bottom line, it does fix the original issue without breaking anything. It also improves the testing of colors.

shoyer · 2015-09-01T17:20:52Z

OK, this seems like a clear improvement so I'm going to merge. Thanks again for all your work here!

Fix contour color

jhamman · 2015-09-01T17:48:12Z

Thanks @rabernat .

shoyer reviewed Aug 18, 2015
View reviewed changes

rabernat force-pushed the fix_contour_color branch from e5b66bc to 3887112 Compare August 26, 2015 19:33

clarkfitzg reviewed Aug 28, 2015
View reviewed changes

rabernat force-pushed the fix_contour_color branch from 3887112 to 61de94c Compare August 31, 2015 00:06

rabernat reviewed Aug 31, 2015
View reviewed changes

shoyer reviewed Aug 31, 2015
View reviewed changes

rabernat force-pushed the fix_contour_color branch from 61de94c to 74335ae Compare August 31, 2015 04:30

rabernat reviewed Aug 31, 2015
View reviewed changes

fixes pydata#537 by modifying color kwarg in plotting

ee4fab7

fixed seaborn dependency in cmap generation test for new contour color rules updated docs test for seaborn palettes forgot colors in plotmethod

rabernat force-pushed the fix_contour_color branch from 74335ae to ee4fab7 Compare August 31, 2015 04:41

shoyer reviewed Aug 31, 2015
View reviewed changes

shoyer added a commit that referenced this pull request Sep 1, 2015

Merge pull request #538 from rabernat/fix_contour_color

04f4e88

Fix contour color

shoyer merged commit 04f4e88 into pydata:master Sep 1, 2015

Uh oh!

Fix contour color #538

Fix contour color #538

Uh oh!

Conversation

rabernat commented Aug 18, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

@rabernat Good point- we should be able to do this.

Uh oh!

shoyer commented Aug 18, 2015

Uh oh!

rabernat commented Aug 24, 2015

Uh oh!

jhamman commented Aug 24, 2015

Uh oh!

shoyer commented Aug 24, 2015

Uh oh!

clarkfitzg commented Aug 24, 2015

Uh oh!

rabernat commented Aug 25, 2015

Uh oh!

rabernat commented Aug 26, 2015

Uh oh!

shoyer commented Aug 26, 2015

Uh oh!

rabernat commented Aug 26, 2015

Uh oh!

rabernat commented Aug 26, 2015

Uh oh!

rabernat commented Aug 26, 2015

Uh oh!

jhamman commented Aug 26, 2015

Uh oh!

rabernat commented Aug 27, 2015

Uh oh!

jhamman commented Aug 27, 2015

Uh oh!

clarkfitzg commented Aug 27, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rabernat commented Aug 31, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shoyer commented Aug 31, 2015

Uh oh!

rabernat commented Aug 31, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shoyer commented Aug 31, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rabernat commented Aug 31, 2015

Uh oh!