Skip to content

[MRG] Fix plot_svm_scale_c.py and plot_discretization_classification.py use deprecated plt api#11586

Merged
rth merged 4 commits intoscikit-learn:masterfrom
dsleo:matplotlib-fix
Jul 17, 2018
Merged

[MRG] Fix plot_svm_scale_c.py and plot_discretization_classification.py use deprecated plt api#11586
rth merged 4 commits intoscikit-learn:masterfrom
dsleo:matplotlib-fix

Conversation

@dsleo
Copy link
Copy Markdown
Contributor

@dsleo dsleo commented Jul 17, 2018

Reference Issues/PRs

fixes #11564

What does this implement/fix? Explain your changes.

Remove the iterative use of plt.subplot that will soon be deprecated.

Any other comments?

Not sure there is something to do for plot_discretization_classification.py, no warning were raised when I tried to reproduce. Am I missing something ?

rth
rth reviewed Jul 17, 2018
View reviewed changes
Copy link
Copy Markdown
Member

@rth rth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This LGTM, but plot_discretization_classification.py would also need to be fixed to solve the parent PR.

The the issue is generally the use of plt.subplot (see https://stackoverflow.com/a/46934052/1791279), in particular the loop at the very end will overwrite some axis.

The solution could be to create an array of axis with fix, ax = plt.subplots(..) and use ax[i] directly.

@agramfort
Copy link
Copy Markdown
Member

agramfort commented Jul 17, 2018

plt.subplot is used in many example...

alex@:examples(matplotlib-fix)$ git grep "plt.subplot(" | wc
     130     583    9215

@dsleo you want to complete this in this PR?

@rth
Copy link
Copy Markdown
Member

rth commented Jul 17, 2018 •
edited
Loading

The immediate concern for the release is to fix those that call plt.subplot(i, j, k) twice on the same axis, as that will overwrite it in the future. These are raising warnings and I think that's why they were mentionned by @amueller in the parent issue.

Fixing all the other examples to use the latest matplotlib API would definitely be useful, but I think it's less urgent and could be done is a separate PR.

@agramfort agramfort changed the title [WIP] Fix plot_svm_scale_c.py and plot_discretization_classification.py use deprecated plt api [MRG] Fix plot_svm_scale_c.py and plot_discretization_classification.py use deprecated plt api Jul 17, 2018
@agramfort
Copy link
Copy Markdown
Member

agramfort commented Jul 17, 2018

ok I pushed here the fix for plot_discretization_classification.py

rth
rth approved these changes Jul 17, 2018
View reviewed changes
Copy link
Copy Markdown
Member

@rth rth left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! (Waiting for Circle CI to render..)

@agramfort
Copy link
Copy Markdown
Member

agramfort commented Jul 17, 2018

here are the 2 rendered examples:

https://28680-843222-gh.circle-artifacts.com/0/doc/auto_examples/svm/plot_svm_scale_c.html#sphx-glr-auto-examples-svm-plot-svm-scale-c-py
https://28680-843222-gh.circle-artifacts.com/0/doc/auto_examples/preprocessing/plot_discretization_classification.html#sphx-glr-auto-examples-preprocessing-plot-discretization-classification-py

LGTM

@rth
Copy link
Copy Markdown
Member

rth commented Jul 17, 2018

Merging, thanks!

(Circle CI : deploy doesn't do anything in a PR, and this fix should not have any Windows specifics parts, so not waiting for Appveyor).

@rth rth merged commit fb0e0fc into scikit-learn:master Jul 17, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@rth rth rth approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

plot_svm_scale_c.py and plot_discretization_classification.py use deprecated plt api

3 participants

@dsleo @agramfort @rth