[MRG+1] Matplotlib v2update #8514

alisonspencer · 2017-03-04T17:00:05Z

Reference Issue

Fixing plot appearance for matplotlib v2 compatibility in #8364

What does this implement/fix? Explain your changes.

Added 'edgecolor' option to scatterplots to make dots easier to see

Any other comments?

jmschrei · 2017-03-04T17:19:16Z

examples/tree/plot_tree_regression.py

-plt.plot(X_test, y_1, color="cornflowerblue", label="max_depth=2", linewidth=2)
-plt.plot(X_test, y_2, color="yellowgreen", label="max_depth=5", linewidth=2)
+plt.scatter(X, y, c="darkorange", edgecolor='k', label="data")
+plt.plot(X_test, y_1, color="cornflowerblue", edgecolor='k',


Does plt.plot have an argument edgecolor?

jmschrei · 2017-03-04T17:19:27Z

examples/tree/plot_tree_regression.py

+plt.scatter(X, y, c="darkorange", edgecolor='k', label="data")
+plt.plot(X_test, y_1, color="cornflowerblue", edgecolor='k',
+         label="max_depth=2", linewidth=2)
+plt.plot(X_test, y_2, color="yellowgreen", edgecolor='k',


jmschrei · 2017-03-04T17:20:19Z

Thanks for the contributions! Please see my comment

codecov · 2017-03-04T17:51:17Z

Codecov Report

Merging #8514 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #8514   +/-   ##
=======================================
  Coverage   95.48%   95.48%           
=======================================
  Files         342      342           
  Lines       60913    60913           
=======================================
  Hits        58160    58160           
  Misses       2753     2753

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 cdd693b...c67045d. Read the comment docs.

codecov · 2017-03-04T17:51:18Z

Codecov Report

Merging #8514 into master will increase coverage by <.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #8514      +/-   ##
==========================================
+ Coverage   95.48%   95.48%   +<.01%     
==========================================
  Files         342      342              
  Lines       60913    61013     +100     
==========================================
+ Hits        58160    58259      +99     
- Misses       2753     2754       +1

Impacted Files	Coverage Δ
sklearn/model_selection/_split.py	`98.6% <0%> (-0.17%)`	❌
sklearn/ensemble/gradient_boosting.py	`95.79% <0%> (ø)`	✅
sklearn/model_selection/init.py	`100% <0%> (ø)`	✅
sklearn/feature_selection/univariate_selection.py	`99.46% <0%> (ø)`	✅
sklearn/calibration.py	`98.87% <0%> (ø)`	✅
sklearn/gaussian_process/tests/test_kernels.py	`98.86% <0%> (+0.01%)`	✅
sklearn/metrics/tests/test_common.py	`99.52% <0%> (+0.01%)`	✅
sklearn/utils/estimator_checks.py	`93.36% <0%> (+0.08%)`	✅
sklearn/model_selection/tests/test_split.py	`95.96% <0%> (+0.25%)`	✅

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 cdd693b...e64f8f6. Read the comment docs.

amueller · 2017-03-04T17:54:32Z

You can post a link to the generated HTML by linking to the output of circle. I keep forgetting how to easily get the link though :-/

amueller · 2017-03-04T17:54:53Z

http://scikit-learn.org/circle?{BUILD_NUMBER}

alisonspencer · 2017-03-04T18:39:12Z

@jmschrei Thanks! I think that should fix it
@amueller The goal being to check that you made the fix you thought you did? :)

https://9273-843222-gh.circle-artifacts.com/0/home/ubuntu/scikit-learn/doc/_build/html/stable/auto_examples/tree/plot_tree_regression.html#sphx-glr-auto-examples-tree-plot-tree-regression-py

https://9273-843222-gh.circle-artifacts.com/0/home/ubuntu/scikit-learn/doc/_build/html/stable/auto_examples/tree/plot_tree_regression_multioutput.html

https://9273-843222-gh.circle-artifacts.com/0/home/ubuntu/scikit-learn/doc/_build/html/stable/auto_examples/decomposition/plot_pca_iris.html#sphx-glr-auto-examples-decomposition-plot-pca-iris-py

amueller · 2017-03-04T19:07:35Z

@katelie indeed, and so that the reviewers can check it without building the documentation themselves.
Travis is still giving an error.

jmschrei · 2017-03-04T19:44:15Z

Thanks for the links @katelie. I'm not seeing too much of a difference in the first two figures, and just darker grid lines in the last example. Is that intentional?

amueller · 2017-03-04T20:01:02Z

Hm I'm not we need the edges in these ones, they look fine to me without them. Sorry if the explanation of the issue was unclear @katelie

alisonspencer · 2017-03-04T20:05:09Z

@jmschrei @amueller Okay, not a problem. It is subtle. I thought part of the issue was to make the examples look consistent with the past behavior with matplotlib v1.

amueller · 2017-03-04T20:11:49Z

@katelie it just needs to look decent. Many of the examples were completely meaningless because the points were the same color as the background. So we mostly want the example to show the same point and look decent. It doesn't need to look the same way.

jmschrei · 2017-03-04T21:38:23Z

This PR is functional and adds in a minor improvement in the grid lines even if the other figures seem mostly unchanged. @amueller should we merge or close this?

GaelVaroquaux · 2017-03-06T06:43:19Z

I am +0 for merge.

lesteve · 2017-03-06T13:17:04Z

This PR is functional and adds in a minor improvement in the grid lines even if the other figures seem mostly unchanged.

😕 are you comparing against the dev doc?

master:

this PR:

Without edgecolor it is rather hard to see the light grey points.

Personally I prefer having edgecolors for scatter plots with overlapping data points so I am +1 on this.

jmschrei · 2017-03-06T17:13:27Z

I think there is a major difference there. If I google the example, I see the following:

stable (http://scikit-learn.org/stable/auto_examples/decomposition/plot_pca_iris.html):

new:

NelleV · 2017-03-06T19:39:09Z

I don't think the change helps for the decision trees example nor the multi-output decision tree output.
It does greatly improve the PCA one.

lesteve · 2017-03-06T23:25:10Z

@jmschrei you should compare the PR rendering against the dev doc not the stable one. The stable doc does not use matplotlib 2. In the case you mention you should be looking at http://scikit-learn.org/dev/auto_examples/decomposition/plot_pca_iris.html.

lesteve · 2017-03-06T23:35:06Z

@NelleV maybe it is a matter of habit but as I said above, scatter plots with overlapping data points without edgecolor looks a bit weird (maybe smudgy if I had to pick another word) to me. I just put the other images here for easier comparison.

plot_tree_regression_multioutput

dev doc:

this PR:

plot_tree_regression

dev doc:

this PR:

lesteve · 2017-03-06T23:38:15Z

@NelleV maybe it is a matter of habit but as I said above, scatter plots with overlapping data points without edgecolor looks a bit weird (maybe smudgy if I had to pick another word) to me.

I don't think we should spent too much time on this though and I am more than fine merging only the change to plot-pca-iris.

lesteve · 2017-03-09T17:53:55Z

OK let's not let this one die. I just left the changes for plot_pca_iris and I am going to merge this one.

lesteve · 2017-03-09T17:55:51Z

Thanks @katelie !

for better rendering with matplotlib 2

alisonspencer added 3 commits March 4, 2017 11:08

Added edgecolor to scatterplot

4b290ce

Added edgecolor to scatterplot

6d95067

Added edgecolor to scatters in tree module

34ec89f

jmschrei reviewed Mar 4, 2017

View reviewed changes

Removed edgecolor from plt.plot

c67045d

lesteve changed the title ~~Matplotlib v2update~~ [MRG+1] Matplotlib v2update Mar 6, 2017

Revert more controversial changes

e64f8f6

lesteve merged commit 34a3c99 into scikit-learn:master Mar 9, 2017

Przemo10 mentioned this pull request Mar 17, 2017

update fork (#1) #8606

Closed

herilalaina pushed a commit to herilalaina/scikit-learn that referenced this pull request Mar 26, 2017

[MRG+1] add edgecolor to plot_pca_iris.py (scikit-learn#8514)

6fdd612

for better rendering with matplotlib 2

massich pushed a commit to massich/scikit-learn that referenced this pull request Apr 26, 2017

[MRG+1] add edgecolor to plot_pca_iris.py (scikit-learn#8514)

a908778

for better rendering with matplotlib 2

Sundrique pushed a commit to Sundrique/scikit-learn that referenced this pull request Jun 14, 2017

[MRG+1] add edgecolor to plot_pca_iris.py (scikit-learn#8514)

cb359ee

for better rendering with matplotlib 2

NelleV pushed a commit to NelleV/scikit-learn that referenced this pull request Aug 11, 2017

[MRG+1] add edgecolor to plot_pca_iris.py (scikit-learn#8514)

1a7cfba

for better rendering with matplotlib 2

paulha pushed a commit to paulha/scikit-learn that referenced this pull request Aug 19, 2017

[MRG+1] add edgecolor to plot_pca_iris.py (scikit-learn#8514)

3cd5e13

for better rendering with matplotlib 2

maskani-moh pushed a commit to maskani-moh/scikit-learn that referenced this pull request Nov 15, 2017

[MRG+1] add edgecolor to plot_pca_iris.py (scikit-learn#8514)

e3611f1

for better rendering with matplotlib 2

jwjohnson314 pushed a commit to jwjohnson314/scikit-learn that referenced this pull request Dec 18, 2017

[MRG+1] add edgecolor to plot_pca_iris.py (scikit-learn#8514)

7c4cc79

for better rendering with matplotlib 2

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

[MRG+1] Matplotlib v2update #8514

[MRG+1] Matplotlib v2update #8514

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

[MRG+1] Matplotlib v2update #8514

[MRG+1] Matplotlib v2update #8514

Uh oh!

Conversation

Reference Issue

What does this implement/fix? Explain your changes.

Any other comments?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

plot_tree_regression_multioutput

plot_tree_regression

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!