[MRG+1] Fix title, overlapping plots and axis labels in plot_ols_ridge_variance.py by xhluca · Pull Request #12296 · scikit-learn/scikit-learn

xhluca · 2018-10-05T02:11:02Z

Reference Issues/PRs

What does this implement/fix? Explain your changes.

I removed plt.axes, and replaced ax with plt. The resulting plot does not have the odd overlap between the title and the plot. It also fixes the axis range.

Any other comments?

Below are what they look like with the new code:

xhluca · 2018-10-05T02:11:39Z

@qinhanmin2014 @ab-10 Please review this PR

qinhanmin2014

LGTM

qinhanmin2014 · 2018-10-05T07:23:11Z

examples/linear_model/plot_ols_ridge_variance.py

@@ -47,25 +47,22 @@
    fig = plt.figure(fignum, figsize=(4, 3))


fig is not used.

That's true!

qinhanmin2014 · 2018-10-05T07:23:26Z

examples/linear_model/plot_ols_ridge_variance.py

@@ -47,25 +47,22 @@
    fig = plt.figure(fignum, figsize=(4, 3))
    plt.clf()


Do we need it?

don't we need to clear the figure after we draw the figure? I'll experiment with both

Ok it doesn't really make a difference whether plt.clf() is used or not (tested both on notebook and python 3.6). I presume it might make a difference when using older versions of pyplot, but that be hard to test since I don't know which version the original tutorial was written in.

Hmm, I guess it's useless because it does not occur very often, but let's keep it here.

qinhanmin2014

LGTM, thanks @xhlulu

qinhanmin2014

Apologies, the label on x-axis is overlapping with the code. You might refer to other similar examples to fix it.
See https://35569-843222-gh.circle-artifacts.com/0/doc/auto_examples/linear_model/plot_ols_ridge_variance.html

xhluca · 2018-10-06T02:19:13Z

examples/linear_model/plot_ols_ridge_variance.py

+    plt.xlim(0, 2)
    fignum += 1

+plt.tight_layout()


Ok figured out the error. Basically by default the x axis label is too low, so it is partially cropped out of the picture generated. However, when you set tight_layout() it rescales the plot layout to correctly show the labels.

I think that problems happens in other examples too, including this one: http://scikit-learn.org/dev/auto_examples/applications/plot_out_of_core_classification.html#plot-results

Should I start a separate PR addressing the issue, or should I address it in this one?

Thank you so much for guiding me through this PR! It's my second time contribution, first time outside of documentation so I'm still new to this :)

Please fix this example here and start a new PR/issue for other examples. The first plot still has the same problem.

qinhanmin2014

LGTM, thanks @xhlulu

xhluca · 2018-10-06T17:44:14Z

Thank you for the good catches @qinhanmin2014

rth

This switches this example back to the legacy matplotlib API. Defining axis and calling ax.plot instead of plt.plot is now the recommended way. See for instance #11564

There should be a way to fix the overlap of the title while keeping axis. I would start with,

fig, ax = plt.subplots(1, 2, figsize=figsize)

then if necessary increase the spacing between the title and the plot as suggested in this SO answer. It shouldn't be necessary though since you don't do it here, and it renders fine. The key point may be to remove plt.axes([.12, .12, .8, .8])...

xhluca · 2018-10-07T13:49:59Z

@rth The examples described in #11564 create images using subplots, and warns about plt.subplot old behavior being deprecated. This examples generates two different plots (without subplots) and show them to the user.

rth · 2018-10-07T18:11:12Z

This examples generates two different plots (without subplots) and show them to the user.

The issue more generally that as soon as you have multiple figures or subplots, plt.plot will plot implicitly in one of them depending on the context (or the current figure state), that is bad and for this reason the explicit ax.plot is preferred even if you have 2 figures with 1 subplot each -- that way you can actually track where each plot should be made. See matplotlib docs,

In general, try to use the object-oriented interface over the pyplot interface.

My point that this is orthogonal to the things you are aiming to fix. It is possible to fix title and axis label overlap without reverting to the matplotlib pyplot API inherited from Matlab...

xhluca · 2018-10-07T21:10:14Z

This examples generates two different plots (without subplots) and show them to the user.

The issue more generally that as soon as you have multiple figures or subplots, plt.plot will plot implicitly in one of them depending on the context (or the current figure state), that is bad and for this reason the explicit ax.plot is preferred even if you have 2 figures with 1 subplot each -- that way you can actually track where each plot should be made. See matplotlib docs,

In general, try to use the object-oriented interface over the pyplot interface.

My point that this is orthogonal to the things you are aiming to fix. It is possible to fix title and axis label overlap without reverting to the matplotlib pyplot API inherited from Matlab...

Thank you for the links and detailed explanation. It makes a lot of sense now. I will revert back to the old OOP method, but swap out plt.plot with ax.plot

xhluca · 2018-10-07T21:16:34Z

examples/linear_model/plot_ols_ridge_variance.py

-    ax.set_xlim(0, 2)
-    fignum += 1
+
+    fig.tight_layout()


Notice here I use fig.tight_layout() to avoid labels being cropped. Is this method preferred or would subplots_adjust be better?

qinhanmin2014 · 2018-10-09T13:44:28Z

I think we can merge this one. It's not perfect but at least it works.

qinhanmin2014

LGTM, thanks @xhlulu

rth · 2018-10-09T13:50:55Z

Yes, it's much better, thanks @xhlulu !

xhluca · 2018-10-09T15:20:56Z

@rth @qinhanmin2014 Thank you for the advice and guidance!

…riance.py (scikit-learn#12296)

Fix title, overlapping plots and axis labels

dfe31b2

qinhanmin2014 approved these changes Oct 5, 2018

View reviewed changes

Remove unused variable assignment

f8395ce

qinhanmin2014 approved these changes Oct 6, 2018

View reviewed changes

qinhanmin2014 reviewed Oct 6, 2018

View reviewed changes

Fix x-axis cropping

7141d62

xhluca commented Oct 6, 2018

View reviewed changes

Fix tight_layout()

385fd3b

qinhanmin2014 approved these changes Oct 6, 2018

View reviewed changes

qinhanmin2014 changed the title ~~Fix title, overlapping plots and axis labels in plot_ols_ridge_variance.py~~ [MRG+1] Fix title, overlapping plots and axis labels in plot_ols_ridge_variance.py Oct 6, 2018

rth suggested changes Oct 7, 2018

View reviewed changes

Revert back to OOP method, fix labelling and overlap

5a34e2d

xhluca commented Oct 7, 2018

View reviewed changes

flake8 fix

e6ed594

qinhanmin2014 approved these changes Oct 9, 2018

View reviewed changes

qinhanmin2014 merged commit 5df8cd3 into scikit-learn:master Oct 9, 2018

jnothman pushed a commit to jnothman/scikit-learn that referenced this pull request Oct 15, 2018

EXA Fix title, overlapping plots and axis labels in plot_ols_ridge_va…

6dbfc85

…riance.py (scikit-learn#12296)

		@@ -47,25 +47,22 @@
		fig = plt.figure(fignum, figsize=(4, 3))
		plt.clf()

Uh oh!

Conversation

xhluca commented Oct 5, 2018

Reference Issues/PRs

What does this implement/fix? Explain your changes.

Any other comments?

Uh oh!

xhluca commented Oct 5, 2018

Uh oh!

qinhanmin2014 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qinhanmin2014 left a comment

Choose a reason for hiding this comment

Uh oh!

qinhanmin2014 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qinhanmin2014 left a comment

Choose a reason for hiding this comment

Uh oh!

xhluca commented Oct 6, 2018

Uh oh!

rth left a comment

Choose a reason for hiding this comment

Uh oh!

xhluca commented Oct 7, 2018

Uh oh!

rth commented Oct 7, 2018

Uh oh!

xhluca commented Oct 7, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qinhanmin2014 commented Oct 9, 2018

Uh oh!

qinhanmin2014 left a comment

Choose a reason for hiding this comment

Uh oh!

rth commented Oct 9, 2018

Uh oh!

xhluca commented Oct 9, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants