The Wayback Machine - https://web.archive.org/web/20250620194025/https://github.com/scikit-learn/scikit-learn/pull/20705
Skip to content

DOC Use numpydoc_use_plots to embed figures for plotting APIs in the documentation #20705

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 15 commits into from
Aug 30, 2021
Merged

DOC Use numpydoc_use_plots to embed figures for plotting APIs in the documentation #20705

merged 15 commits into from
Aug 30, 2021

Conversation

harupy
Copy link
Contributor

@harupy harupy commented Aug 8, 2021 •
edited
Loading

@harupy
Copy link
Contributor Author

harupy commented Aug 8, 2021 •
edited
Loading

As commented here: #18331 (comment), this PR requires updating the mininum matplotlib version to 3.1.0 (released on May 19, 2019) from 2.2.2 (released on Mar 18, 2018):

scikit-learn/sklearn/_min_dependencies.py

Line 28 in 4b0d291

"matplotlib": ("2.2.2", "benchmark, docs, examples, tests"),

@harupy
Copy link
Contributor Author

harupy commented Aug 8, 2021

It looks like something is wrong with the det-curve example:

image

https://147423-843222-gh.circle-artifacts.com/0/doc/modules/generated/sklearn.metrics.DetCurveDisplay.html#sklearn.metrics.DetCurveDisplay

@glemaitre
Copy link
Member

We need to use a real dataset then :) Build a curve with 3 samples will not really work :)

@glemaitre
Copy link
Member

You will need also to update the README regarding the matplotlib version for the test to pass.
I don't think that bumping matplotlib version will be a big issue.

@harupy
Copy link
Contributor Author

harupy commented Aug 8, 2021 •
edited
Loading

We need to use a real dataset then :) Build a curve with 3 samples will not really work :)

Agree, I filed #20709.

You will need also to update the README regarding the matplotlib version for the test to pass. I don't think that bumping matplotlib version will be a big issue.

Thanks, updated README.rst!

@harupy harupy mentioned this pull request Aug 8, 2021
Closed
harupy
harupy commented Aug 8, 2021
View reviewed changes
doc/conf.py Outdated

# Options for the `::plot` directive:
# https://matplotlib.org/stable/api/sphinxext_plot_directive_api.html
plot_formats = ["png"]
Copy link
Contributor Author

@harupy harupy Aug 8, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

plot_formats defaults to ['png', 'hires.png', 'pdf'], but just creating a png should be enough:

https://github.com/matplotlib/matplotlib/blob/a8977fde9ae67fc736b29c009eb40eb985924301/lib/matplotlib/sphinxext/plot_directive.py#L108

@harupy harupy force-pushed the use-numpydoc_use_plots branch from a8983a7 to 3477bb3 Compare August 8, 2021 11:31
@harupy
Copy link
Contributor Author

harupy commented Aug 9, 2021 •
edited
Loading

@glemaitre Addressed your comments. Could you take another look?

@glemaitre
Copy link
Member

I think that we could make the change for the DetCurve in this PR as well to be sure that once merge, all the plotting will look okay. Could you trigger a full documentation build by including [doc build] in your commit message?

It will allow to check that everything is fine.

@harupy harupy force-pushed the use-numpydoc_use_plots branch from d6aae2a to 2499fbb Compare August 9, 2021 11:39
@harupy
Copy link
Contributor Author

harupy commented Aug 9, 2021 •
edited
Loading

@glemaitre Got it, pushed a few commits to address your comments.

@harupy
Copy link
Contributor Author

harupy commented Aug 9, 2021 •
edited
Loading

Now the plot for the DetCurveDisplay example looks like this:

image

https://147631-843222-gh.circle-artifacts.com/0/doc/modules/generated/sklearn.metrics.DetCurveDisplay.html?highlight=detcurve#sklearn.metrics.DetCurveDisplay.from_estimator

I think increasing n_samples for make_classification makes the line look like e a curve.

@glemaitre
Copy link
Member

Yes, a bit more samples could be good then :)
You can have a look at the example where we have a better line: https://scikit-learn.org/stable/auto_examples/model_selection/plot_det.html#sphx-glr-auto-examples-model-selection-plot-det-py

@harupy
Copy link
Contributor Author

harupy commented Aug 9, 2021

Now it looks like this:

image

https://147675-843222-gh.circle-artifacts.com/0/doc/modules/generated/sklearn.metrics.DetCurveDisplay.html?highlight=detcurve#sklearn.metrics.DetCurveDisplay

@glemaitre
Copy link
Member

Nice :). It is enough for me.

@harupy
Copy link
Contributor Author

harupy commented Aug 9, 2021 •
edited
Loading

yeah, it's enough for an example.

thomasjpfan
thomasjpfan reviewed Aug 9, 2021
View reviewed changes
doc/conf.py Outdated
]

# this is needed for some reason...
# see https://github.com/numpy/numpydoc/issues/69
numpydoc_class_members_toctree = False

# Produce `plot::` directives for examples that contain `import matplotlib` or
Copy link
Member

Choose a reason for hiding this comment

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

Bumping the matplotlib version for nicer plotting in docstrings does not look like a strong enough reason. A compromise would be to only generate the plots for newer version of matplotlib:

# support for `plot::` directives in sphinx 3.2 require matplotlib 3.1.0
if parse_version(mpl.__version__) > parse_version("3.1.0"):
    extensions.append("matplotlib.sphinxext.plot_directive")

This will still generate the plots for the hosted docs.

Copy link
Member

Choose a reason for hiding this comment

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

It is fine with me.

This said, I am not sure that I would encourage anymore to not have the latest major revision of a Python package -> pandas 1.x, matplotlib 3.x. But for sure this is not the debate here :)

Copy link
Contributor Author

@harupy harupy Aug 11, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Looks good!

@glemaitre
Copy link
Member

I still think that we should generate the full documentation with a message commit containing [doc build]. I would like the output of all the displays classes.

@harupy harupy force-pushed the use-numpydoc_use_plots branch from 28b48d2 to 27f9f74 Compare August 11, 2021 12:32
@harupy harupy force-pushed the use-numpydoc_use_plots branch from 08d146f to 4a57119 Compare August 11, 2021 12:39
@harupy
Copy link
Contributor Author

harupy commented Aug 12, 2021

@glemaitre @thomasjpfan Could you take another look?

thomasjpfan
thomasjpfan approved these changes Aug 14, 2021
View reviewed changes
Copy link
Member

@thomasjpfan thomasjpfan left a comment

Choose a reason for hiding this comment

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

LGTM

doc/conf.py
@@ -49,6 +50,21 @@
"sphinxext.opengraph",
]

# Support for `plot::` directives in sphinx 3.2 requires matplotlib 3.1.0 or newer
if parse(mpl.__version__) >= parse("3.1.0"):
Copy link
Member

Choose a reason for hiding this comment

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

When we finally update our matplotlib min version, we can remove this version parsing.

@thomasjpfan thomasjpfan changed the title Use numpydoc_use_plots to embed figures for plotting APIs in the documentation DOC Use numpydoc_use_plots to embed figures for plotting APIs in the documentation Aug 14, 2021
@glemaitre
Copy link
Member

We need an additional plot.show in plot_partial_dependence. It also missing an import.
Could you also update the plot_det_curve in the same you did it for the DetCurveDisplay.

Everything else seems fine.

@harupy
Copy link
Contributor Author

harupy commented Aug 16, 2021

@glemaitre Addressed your comments :)

glemaitre
glemaitre reviewed Aug 16, 2021
View reviewed changes
sklearn/metrics/_plot/det_curve.py Outdated
>>> X_train, X_test, y_train, y_test = train_test_split(
... X, y, test_size=0.4, random_state=0)
>>> clf = SVC(random_state=0).fit(X_train, y_train)
>>> plot_det_curve(clf, X_test, y_test)
Copy link
Member

Choose a reason for hiding this comment

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

Here we will raise a deprecation warning. It might be best to keep the skip then.

@harupy harupy requested a review from glemaitre August 16, 2021 15:47
glemaitre
glemaitre reviewed Aug 16, 2021
View reviewed changes
@harupy harupy requested a review from glemaitre August 17, 2021 02:45
@harupy
Copy link
Contributor Author

harupy commented Aug 18, 2021

@glemaitre Could you take another look?

@harupy harupy requested a review from thomasjpfan August 21, 2021 09:58
@harupy
Copy link
Contributor Author

harupy commented Aug 24, 2021

@glemaitre @thomasjpfan gentle ping.

@thomasjpfan
Copy link
Member

I have already approved this PR. I think this is a big enough change to require another review to approve before merging.

glemaitre
glemaitre approved these changes Aug 30, 2021
View reviewed changes
Copy link
Member

@glemaitre glemaitre left a comment

Choose a reason for hiding this comment

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

I checked another time the output and LGTM

@glemaitre glemaitre merged commit c3db2cd into scikit-learn:main Aug 30, 2021
@glemaitre
Copy link
Member

Thanks @harupy

samronsin pushed a commit to samronsin/scikit-learn that referenced this pull request Nov 30, 2021
@harupy @glemaitre @samronsin
DOC Use numpydoc_use_plots to embed figures for plotting APIs in th…
e797804 
…e documentation (scikit-learn#20705)

Co-authored-by: Guillaume Lemaitre <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@glemaitre glemaitre glemaitre approved these changes

@thomasjpfan thomasjpfan Awaiting requested review from thomasjpfan

Assignees
No one assigned
Labels
Documentation module:metrics
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Example code sklearn.metrics.DetCurveDisplay.__init__ plots nothing
3 participants
@harupy @glemaitre @thomasjpfan