fix clustermap test: colors parameter

[bnavigator]

the tree_kws parameter for clustermap is forwarded to matplotlib.collections.LineCollection. But that constructor does not have a color kwarg. The unit test fails on openSUSE Tumbleweed with MPL 3.4.1:

[  333s] _________________________ TestClustermap.test_tree_kws _________________________
[  333s] 
[  333s] self = <seaborn.tests.test_matrix.TestClustermap object at 0x7f3043a83100>
[  333s] 
[  333s]     def test_tree_kws(self):
[  333s]     
[  333s]         rgb = (1, .5, .2)
[  333s]         g = mat.clustermap(self.df_norm, tree_kws=dict(color=rgb))
[  333s]         for ax in [g.ax_col_dendrogram, g.ax_row_dendrogram]:
[  333s]             tree, = ax.collections
[  333s] >           assert tuple(tree.get_color().squeeze())[:3] == rgb
[  333s] E           assert (0.2, 0.2, 0.2) == (1, 0.5, 0.2)
[  333s] E             At index 0 diff: 0.2 != 1
[  333s] E             Full diff:
[  333s] E             - (1, 0.5, 0.2)
[  333s] E             + (0.2, 0.2, 0.2)
[  333s] 
[  333s] seaborn/tests/test_matrix.py:1320: AssertionError

[mwaskom]

I can't reproduce a failure on my system, nor has the automated CI failed on this. I think the change is correct, but I'm curious to understand what's happening here.

[codecov]

Codecov Report

Merging #2562 (99f1a79) into master (5ce48af) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #2562   +/-   ##
=======================================
  Coverage   97.45%   97.45%           
=======================================
  Files          17       17           
  Lines        6326     6326           
=======================================
  Hits         6165     6165           
  Misses        161      161           

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5ce48af...99f1a79. Read the comment docs.

[bnavigator]

I am not sure either. Here is a full buildlog:

seaborn-fail-colors_log.txt

The patch fixes the failure. Maybe your system and the CI have a different default of colors than the openSUSE build system, which happens to be the desired color? Is there some preset which has the same value and it is not imported by the openSUSE build system? What happens if you set rgb to a different tuple, is the return still as expected?

[mwaskom]

When I try it locally, also with mpl 3.4.1, the dendrogram is a greenish color as expected when setting this kwarg and gray otherwise. Very strange.

[bnavigator]

On my system:

color is ignored (no effect), colors changes the tree color as expected and colornoexist raises AttributeError: 'LineCollection' object has no property 'colornoexist'. Strange!

[bnavigator]

Ok I traced it down a little:

matplotlib

color and colors are both "valid" parameters, although only colors is documented. https://github.com/matplotlib/matplotlib/blob/v3.4.1/lib/matplotlib/collections.py#L1541

IPython pdb debug:

IPdb [7]: up
> /usr/lib/python3.8/site-packages/seaborn/matrix.py(780)dendrogram()
    778         ax = plt.gca()
    779 
--> 780     return plotter.plot(ax=ax, tree_kws=tree_kws)
    781 
    782 


IPdb [8]: pp tree_kws
{'color': (1, 0.5, 0.2)}

IPdb [9]: down
> /usr/lib/python3.8/site-packages/seaborn/matrix.py(691)plot()
    689         else:
    690             coords = zip(self.independent_coord, self.dependent_coord)
--> 691         lines = LineCollection([list(zip(x, y)) for x, y in coords],
    692                                **tree_kws)
    693 


IPdb [10]: pp tree_kws
{'color': (1, 0.5, 0.2), 'colors': '.2', 'linewidths': 0.5}

seaborn 0.11.1

The additional kw is added here:

seaborn/seaborn/matrix.py

Line 685 in 96a5350

tree_kws.setdefault("colors", ".2")

On my system this first sets the edge_color to (1, 0.5, 0.2) and immediately after to "0.2".

Maybe the order on your system is reversed. Or did you check with current master, which has a different setdefault?

master

seaborn/seaborn/matrix.py

Line 693 in 5ce48af

tree_kws.setdefault("colors", tree_kws.pop("color", (.2, .2, .2)))

[bnavigator]

Duh! #2490

[mwaskom]

Cool ... so do I understand correctly that this has already been "fixed"?

[bnavigator]

You tell me, it's your PR! At least it shows that you already acknowledged the problem months ago.

I made the mistake to not explicitly state, that my problem is with the released version, not the current development branch.

A minor problem I still see: If for whatever stupid reason both color and colors exist in tree_kws, the actually color set will be dependent on the order within the dictionary.

Regardless, IMHO test_tree_kws should not call the undocumented parameter. Or at least not alone. If you want to test against backwards compatibility regressions, ok. But at least also test with the "correct" parameter too.

[mwaskom]

Tough to remember what code I wrote last week is supposed to do, much less two months ago :)

That said, now that you've helpfully dug this up, I do remember it as something that popped up when I was testing the matplotlib 3.4 release candidate.

A minor problem I still see: If for whatever stupid reason both color and colors exist in tree_kws, the actually color set will be dependent on the order within the dictionary.

This seems like a matplotlib implementation detail and something seaborn should probably avoid intervening on.

Regardless, IMHO test_tree_kws should not call the undocumented parameter. Or at least not alone. If you want to test against backwards compatibility regressions, ok. But at least also test with the "correct" parameter too.

The goal of the test is to check that tree_kws is passed through correctly, so it doesn't really matter what kwarg is supplied. And actually, testing against color is somewhat useful because the mpl devs occasionally try to remove support for the singular color from various functions and I need to protest that having color= always work is useful for writing generic higher-level interfaces (i.e. FacetGrid).