issue_comments

Thanks @rabernat .

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

@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.

@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.

@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 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.

@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?

This is turning into a real rabbit hole...

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.

I think this latest commit solves everything.

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

Seaborn should not be a dependency.

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:

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

@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.)

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

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

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.

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.

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: https://github.com/pydata/pandas/pull/9826#issuecomment-132637777

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.

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

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)

@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.

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

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

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