8000 DOC: use notebook-style for plot_ols_3d by SamAdamDay · Pull Request #22547 · scikit-learn/scikit-learn · GitHub
[go: up one dir, main page]
Skip to content

DOC: use notebook-style for plot_ols_3d #22547

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 6 commits into from
Feb 23, 2022
Merged

DOC: use notebook-style for plot_ols_3d #22547

merged 6 commits into from
Feb 23, 2022

Conversation

SamAdamDay
Copy link
Contributor

Reference Issues/PRs

Addresses #22406.

What does this implement/fix? Explain your changes.

Fix notebook-style issues in examples/linear_model/plot_ols_3d.py.

  • Added text blocks before loading and training code.
  • Changed # ####### pattern to # %%.

Any other comments?

On my machine I get the following depreciation warning in the built HTML:

examples/linear_model/plot_ols_3d.py:47: MatplotlibDeprecationWarning: Axes3D(fig) adding itself to the figure is deprecated since 3.4. Pass the keyword argument auto_add_to_figure=False and use fig.add_axes(ax) to suppress this warning. The default value of auto_add_to_figure will change to False in mpl3.5 and True values will no longer work in 3.6.  This is consistent with other Axes classes.

I'm happy to remedy this in a separate PR (or in this one if you prefer).

@SamAdamDay
Copy link
Contributor Author

Ah, so for my previous PR I forgot to create a separate branch, so that commit and the merge is included here. Is that a problem? I'll do a hard git reset on my main branch now so this doesn't happen again for future PRs.

@lesteve lesteve mentioned this pull request Feb 21, 2022
Closed
47 tasks
@lesteve lesteve added the Quick Review For PRs that are quick to review label Feb 22, 2022
@lesteve
Copy link
Member
lesteve commented Feb 22, 2022 •
edited
Loading

Thanks for your PR!

Ah, so for my previous PR I forgot to create a separate branch, so that commit and the merge is included here. Is that a problem?

I think it's fine since we use squash + merge on the PR, so as long as the diff makes sense we don't care about the commit history.

On my machine I get the following depreciation warning in the built HTML:

Indeed, it seems like the needed change is the following one, it is small so I think it is fine to do it in this PR:

ax = Axes3D(fig, elev=elev, azim=azim, auto_add_to_figure=False)
fig.add_axes(ax)

Edit: having said we run our examples with matplotlib 2.2.3 to make sure they run with an old matplotlib version and maybe this parameter does not exist, this needs to be double-checked ...

lesteve
lesteve reviewed Feb 22, 2022
View reviewed changes
examples/linear_model/plot_ols_3d.py Outdated
@@ -29,12 +31,15 @@
y_train = y[:-20]
y_test = y[-20:]

# %%
# Next we fit a linear regression model.

ols = linear_model.LinearRegression()
ols.fit(X_train, y_train)
Copy link
Member
@lesteve lesteve Feb 22, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

This looks good, I think the only thing I am not sure about is the output of .fit, I feel the output is not really interesting:

image

so I would probably use the following code even if it is a bit more complicated:

Suggested change
ols.fit(X_train, y_train)
_ = ols.fit(X_train, y_train)

Copy link
Member

Choose a reason for hiding this comment

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

Something which would be nice as well, would be to move the imports in the cell where there are first used (e.g. the matplotlib import to the last cell where we do the plot). This would look closer to what you would write if you were writing the code live.

@SamAdamDay
Copy link
Contributor Author

Indeed, it seems like the needed change is the following one, it is small so I think it is fine to do it in this PR:

Edit: having said we run our examples with matplotlib 2.2.3 to make sure they run with an old matplotlib version and maybe this parameter does not exist, this needs to be double-checked ...

In matplotlib 2.2.3 auto_add_to_figure is not a recognised parameter in the constructor of Axes3D, and circleci gives an error. We might potentially do the following.

ax = Axes3D(fig, elev=elev, azim=azim)
fig.add_axes(ax)

Briefly looking at the matplotlib code it seems that if an axis is already added to a figure, there's no harm in adding it again. But I guess the code is kind of confusing until matplotlib 3.5 when auto_add_to_figure defaults to False.

This looks good, I think the only thing I am not sure about is the output of .fit, I feel the output is not really interesting

Something which would be nice as well, would be to move the imports in the cell where there are first used

I've made these changes as you recommend.

@lesteve
Copy link
Member
lesteve commented Feb 23, 2022

Thanks a lot for checking the old matplotlib, I think you are right but I also think I found a way with fig.add_suplot(..., projection='3d') to get rid of the warnings and support matplotlib 2.2.3. I pushed a commit in your branch, let's see what the CI has to say about it.

@SamAdamDay
Copy link
Contributor Author

Looks great, thanks!

@lesteve
Copy link
Member
lesteve commented Feb 23, 2022

The plots don't look exactly the same but let's say this is good enough, merging thanks a lot!

@lesteve lesteve changed the title DOC use notebook-style for plot_ols_3d DOC: use notebook-style for plot_ols_3d Feb 23, 2022
@lesteve lesteve merged commit 391baef into scikit-learn:main Feb 23, 2022
@lesteve lesteve mentioned this pull request Feb 23, 2022
Closed
8 tasks
@lesteve
Copy link
Member
lesteve commented Feb 23, 2022

By the way, thanks for noticing the warning! I opened #22586 to get rid of similar warnings in other examples.

@nathansquan nathansquan mentioned this pull request Feb 23, 2022
Merged
thomasjpfan pushed a commit to thomasjpfan/scikit-learn that referenced this pull request Mar 1, 2022
@SamAdamDay @lesteve @thomasjpfan
DOC: use notebook-style for plot_ols_3d (scikit-learn#22547)
77ffef2 
Co-authored-by: Loïc Estève <loic.esteve@ymail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lesteve lesteve lesteve left review comments

Assignees
No one assigned
Labels
Documentation Quick Review For PRs that are quick to review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@SamAdamDay @lesteve
0