BUG: addresses #14855 by fixing color kwarg conflict #14871

alexandercbooth · 2016-12-13T00:34:57Z

closes scatter_matrix color vs c #14855
tests passed
passes git diff upstream/master | flake8 --diff

codecov-io · 2016-12-13T01:36:03Z

Codecov Report

Merging #14871 into master will decrease coverage by 0.02%.
The diff coverage is 50%.

@@            Coverage Diff             @@
##           master   #14871      +/-   ##
==========================================
- Coverage   91.01%   90.99%   -0.03%     
==========================================
  Files         143      143              
  Lines       49400    49399       -1     
==========================================
- Hits        44963    44952      -11     
- Misses       4437     4447      +10

Impacted Files	Coverage Δ
pandas/tools/plotting.py	`71.75% <50%> (-0.02%)`	⬇️
pandas/io/gbq.py	`25% <0%> (-58.34%)`	⬇️
pandas/core/common.py	`90.96% <0%> (-0.34%)`	⬇️
pandas/core/frame.py	`97.86% <0%> (-0.1%)`	⬇️

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 9d3554c...3245f09. Read the comment docs.

jreback · 2016-12-13T11:14:14Z

pandas/tools/plotting.py

-    kwds.setdefault('c', plt.rcParams['patch.facecolor'])
+    if 'color' in kwds:
+        pass
+    else:


kwds.setdefault('c', kwdgs.pop('color', plt.rcParams['patch.facecolor'])) is a big more idiomatic

nvm, I see that these kwards are slightly different. Can you update / add to the comment (and reference the issue).

jreback · 2016-12-13T11:14:32Z

pls add a whatsnew note (for 0.19.2 is fine)

jreback · 2016-12-13T11:15:54Z

pls add a test

sinhrks · 2016-12-14T09:55:52Z

pandas/tools/plotting.py

@@ -359,7 +359,10 @@ def scatter_matrix(frame, alpha=0.5, figsize=None, ax=None, grid=False,
    density_kwds = density_kwds or {}

    # workaround because `c='b'` is hardcoded in matplotlibs scatter method
-    kwds.setdefault('c', plt.rcParams['patch.facecolor'])
+    if 'color' in kwds:


shouldn't overwrite if either ‘c’, ‘color’ or ‘facecolor’ is specified.

I'm sorry, I don't quite understand. Could you explain a little bit more?

I meant, plt.scatter(..., facecolor='red') and plt.scatter(..., c='red') both draws points in red. So scatter_matrix(..., facecolor='red') and scatter_matrix(..., c='red') should be the same.

Ahh gotcha. Thanks!
So one of the problems turned out that difference between c and color was an edge border on the markers. We can take care of that by setting the default to 'none'. After making the changes here is the current behavior:

All are now the same.

alexandercbooth · 2016-12-16T01:15:12Z

Sorry for the slow response, long week at work. Thank you both for the feedback, let me know if there's something else I can change.

alexandercbooth · 2016-12-16T17:50:33Z

I also notice that this exact same bug is present in scatter_plot. It can be fixed in exactly the same way as I have done above:

I would be happy to fix it as well. I could just tack it on here or open another PR, whichever is best.

tacaswell · 2016-12-16T18:04:56Z

pandas/tools/plotting.py

+    if 'facecolor' in kwds:
+        kwds.setdefault('facecolor', plt.rcParams['patch.facecolor'])
+    if 'c' in kwds:
+        kwds.setdefault('c', plt.rcParams['patch.facecolor'])


I do not understand this, if 'c' is in kwds, then setdefault is a no-op.

You probably also want to version guard this, mpl 2.0 scatter respects the color cycle
http://matplotlib.org/2.0.0rc1/users/dflt_style_changes.html#scatter

Good thinking, @tacaswell! I investigated it a bit further and it turns out we can resolve this issue and the same issue for scatter_plot by removing the old bug fix, removing the new changes and adding in the edgecolor='none' as default. With this, we still get the identical behavior with c, color or facecolor set and no error.

tacaswell · 2016-12-17T21:52:16Z

The change to the default value of c went in via matplotlib/matplotlib#4079 which is in mpl >=1.5.0. pandas will need to do some version checking to correctly support older versions of mpl. I thought back to 1.3.1 is currently supported?

jreback · 2016-12-30T21:39:13Z

status of this?

jorisvandenbossche

This seems to work (I don't fully understand how passing facecolor solves the color cs c conflict).
The only thing is that this slightly changes the plot appearance. As previously c only changed the facecolor, while it now also seems to change the edgecolor.

jorisvandenbossche · 2017-01-02T10:48:23Z

doc/source/whatsnew/v0.19.2.txt

@@ -80,3 +80,6 @@ Bug Fixes
 - Explicit check in ``to_stata`` and ``StataWriter`` for out-of-range values when writing doubles (:issue:`14618`)

 - Bug in ``unstack()`` if called with a list of column(s) as an argument, regardless of the dtypes of all columns, they get coerced to ``object`` (:issue:`11847`)
+
+- Bug in ``pd.scatter_matrix()`` could accept either ``color`` or ``c``, but
+  not both (:issue:`14855`)


Can you move this to 0.20.0.txt?

jorisvandenbossche · 2017-01-02T11:16:30Z

pandas/tests/plotting/test_misc.py

@@ -77,6 +77,12 @@ def scat(**kwds):
            _check_plot_works(scat, diagonal='hist')
        with tm.assert_produces_warning(UserWarning):
            _check_plot_works(scat, range_padding=.1)
+        with tm.assert_produces_warning(UserWarning):
+            _check_plot_works(scat, color='rgb')


Is there a reason you pass here 'rgb' and not just 'r'? As passing multiple colors seems a bit odd for a single scatter plot (I think now the points just get the [r, g, b, r, g, b, r, g, b, ...] colors sequentially?)

Purely for aesthetic purposes. I have trouble seeing some colors and wanted to visually confirm these changes rendered the same objects.

jreback · 2017-01-21T23:09:09Z

status on this? @sinhrks @jorisvandenbossche @TomAugspurger

alexandercbooth · 2017-01-22T01:38:24Z

Thanks Jeff and everyone else for your patience on this; it's been a really hectic month at work. I appreciate all the feedback from everyone. I will try to address all concerns I can tomorrow and report back.
Thanks!

alexandercbooth · 2017-01-24T19:19:15Z

I squashed and rebased since the PR was stale, but it's my first time doing that so I apologize if I made a mistake.

jreback · 2017-03-23T13:19:53Z

@alexandercbooth can you rebase

@TomAugspurger ok on this?

DOC: add whats new entry for addressing pandas-dev#14855 TEST: add test for adding color kwarg to scatter_matrix BUG: add comment that addresses the issue BUG: set default edgecolor to none and fix facecolor TST: add facecolor test BUG: remove new changes and old bug fix

jreback · 2017-03-23T22:55:31Z

lgtm. @TomAugspurger ok? (don't merge yet)

jreback · 2017-03-25T16:05:22Z

@TomAugspurger ?

TomAugspurger · 2017-04-05T21:16:38Z

Sorry for the delay, +1 on this.

@jreback, when you merge PRs that need a rebase (conflicts in whatsnew), you use the merge-pr.py script, right? I can do it, just want to double check first. Don't want to make @alexandercbooth rebase again.

jreback · 2017-04-05T21:19:26Z

@TomAugspurger yes you can use the script. it will stop when it can't auto rebase (it will squash), you then fix the conflicts, git add, then continue the script. Then I stop again (its now rebased), just to make sure the commits message and everything is ok. Then have it push upstream.

TomAugspurger · 2017-04-05T21:48:14Z

@alexandercbooth thanks!

alexandercbooth · 2017-04-05T21:57:36Z

My pleasure!

jreback reviewed Dec 13, 2016

View reviewed changes

jreback added the Visualization plotting label Dec 13, 2016

sinhrks suggested changes Dec 14, 2016

View reviewed changes

tacaswell suggested changes Dec 16, 2016

View reviewed changes

jorisvandenbossche reviewed Jan 2, 2017

View reviewed changes

sinhrks approved these changes Jan 3, 2017

View reviewed changes

jreback added this to the 0.20.0 milestone Jan 21, 2017

alexandercbooth force-pushed the fix-color-scatterm-bug branch from da23aab to aa07157 Compare January 24, 2017 19:16

alexandercbooth added 2 commits March 23, 2017 16:44

DOC: moving whatsnew entry to 0.20.0

3245f09

alexandercbooth force-pushed the fix-color-scatterm-bug branch from aa07157 to 3245f09 Compare March 23, 2017 20:50

TomAugspurger closed this in ba30e3a Apr 5, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BUG: addresses #14855 by fixing color kwarg conflict #14871

BUG: addresses #14855 by fixing color kwarg conflict #14871

alexandercbooth commented Dec 13, 2016

codecov-io commented Dec 13, 2016 •

edited by codecov bot

Loading

jreback Dec 13, 2016

jreback Dec 13, 2016

jreback commented Dec 13, 2016

jreback commented Dec 13, 2016

sinhrks Dec 14, 2016 •

edited

Loading

alexandercbooth Dec 16, 2016

sinhrks Dec 16, 2016

alexandercbooth Dec 16, 2016

alexandercbooth commented Dec 16, 2016

alexandercbooth commented Dec 16, 2016

tacaswell Dec 16, 2016

alexandercbooth Dec 16, 2016

tacaswell commented Dec 17, 2016

jreback commented Dec 30, 2016

jorisvandenbossche left a comment

jorisvandenbossche Jan 2, 2017

jorisvandenbossche Jan 2, 2017

alexandercbooth Jan 24, 2017

jreback commented Jan 21, 2017

alexandercbooth commented Jan 22, 2017

alexandercbooth commented Jan 24, 2017

jreback commented Mar 23, 2017

jreback commented Mar 23, 2017

jreback commented Mar 25, 2017

TomAugspurger commented Apr 5, 2017

jreback commented Apr 5, 2017

TomAugspurger commented Apr 5, 2017

alexandercbooth commented Apr 5, 2017

BUG: addresses #14855 by fixing color kwarg conflict #14871

BUG: addresses #14855 by fixing color kwarg conflict #14871

Conversation

alexandercbooth commented Dec 13, 2016

codecov-io commented Dec 13, 2016 • edited by codecov bot Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jreback commented Dec 13, 2016

jreback commented Dec 13, 2016

sinhrks Dec 14, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexandercbooth commented Dec 16, 2016

alexandercbooth commented Dec 16, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tacaswell commented Dec 17, 2016

jreback commented Dec 30, 2016

jorisvandenbossche left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jreback commented Jan 21, 2017

alexandercbooth commented Jan 22, 2017

alexandercbooth commented Jan 24, 2017

jreback commented Mar 23, 2017

jreback commented Mar 23, 2017

jreback commented Mar 25, 2017

TomAugspurger commented Apr 5, 2017

jreback commented Apr 5, 2017

TomAugspurger commented Apr 5, 2017

alexandercbooth commented Apr 5, 2017

codecov-io commented Dec 13, 2016 •

edited by codecov bot

Loading

sinhrks Dec 14, 2016 •

edited

Loading