DOC Ensures that plot_roc_curve passes numpydoc validation by MrinalTyagi · Pull Request #21547 · scikit-learn/scikit-learn

MrinalTyagi · 2021-11-04T04:49:46Z

Reference Issues/PRs

Addresses #21350
This PR ensures plot_roc_curve is compatible with numpydoc:

What does this implement/fix? Explain your changes.

Remove sklearn.metrics._plot.roc_curve.plot_roc_curve from DOCSTRING_IGNORE_LIST.
Verify that all tests are passing.

Any other comments?

Addresses #21350 This PR ensures precision_score is compatible with numpydoc: 1. Remove sklearn.metrics._plot.roc_curve.plot_roc_curve from DOCSTRING_IGNORE_LIST. 2. Verify that all tests are passing.

MrinalTyagi · 2021-11-04T05:11:55Z

@thomasjpfan @glemaitre Can u help me fix the conflicts?

glemaitre · 2021-11-04T09:25:07Z

sklearn/metrics/_plot/roc_curve.py

       1.2. Use one of the following class methods:
       :func:`~sklearn.metrics.RocCurveDisplay.from_predictions` or
       :func:`~sklearn.metrics.RocCurveDisplay.from_estimator`.
+


you need to move these lines as it was previously (see the diff).

did as you directed but tests still failing

sklearn/metrics/_plot/roc_curve.py

glemaitre

You will also need to merge the main branch into your branch when #21549 will be merged. The last release of setuptools broke our CIs and they will be working once the PR is merged.

ogrisel

This separating blank line is still needed but you should not add trailing whitespace:

sklearn/metrics/_plot/roc_curve.py

Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>

MrinalTyagi · 2021-11-05T04:06:55Z

If i move the deprecation warning to the original position, tests fail on my local machine. @glemaitre

ogrisel · 2021-11-05T15:09:22Z

If i move the deprecation warning to the original position, tests fail on my local machine. @glemaitre

Can you please share the error message or push the change so that we all see the error on the CI. I agree with @glemaitre that we want the deprecated directive after the main elements of the docstring.

cmarmo · 2021-11-05T20:59:59Z

Hi @MrinalTyagi

Can you please share the error message or push the change so that we all see the error on the CI. I agree with @glemaitre that we want the deprecated directive after the main elements of the docstring.

If you can find some time to fix the lint error

sklearn/metrics/_plot/roc_curve.py:421:1: W293 blank line contains whitespace

then tests will be run on the CI and it will be easier to find what is going wrong. Thanks!

MrinalTyagi · 2021-11-06T04:53:01Z

Hi @MrinalTyagi

Can you please share the error message or push the change so that we all see the error on the CI. I agree with @glemaitre that we want the deprecated directive after the main elements of the docstring.

If you can find some time to fix the lint error
sklearn/metrics/_plot/roc_curve.py:421:1: W293 blank line contains whitespace
then tests will be run on the CI and it will be easier to find what is going wrong. Thanks!

thank you for the help. :)

MrinalTyagi · 2021-11-06T04:53:35Z

Can someone merge this pr?

ogrisel

Can someone merge this pr?

No since you have not addressed this comment #21547 (comment) which is a rephrasing of the first comment of @glemaitre: #21547 (comment) (if I understand correctly).

Please move the deprecation directive lower in the docstring to keep the most important information first. If you get an error message you do not understand when you run the tests locally, please paste it here as a comment so that we can try to help you or just push the change to the PR so that we have an opportunity to read the message in the CI report.

ogrisel · 2021-11-06T08:27:49Z

Ok I understand the problem: the @deprecated decorator is already changing the content of the docstring and the numpydoc validation is working on this.

Since we already have the deprecation information in the altered docstring information from the @deprecated decorator, there is no need in putting it again in the docstring.

We could probably improve the @deprecated to improve the formatting and make it insert a .. deprecated directive after the top level oneliner but this is out of the scope of this PR.

For now, let's just remove the redundant ..deprecated directive

MrinalTyagi · 2021-11-06T08:31:25Z

Can someone merge this pr?

No since you have not addressed this comment #21547 (comment) which is a rephrasing of the first comment of @glemaitre: #21547 (comment) (if I understand correctly).

Please move the deprecation directive lower in the docstring to keep the most important information first. If you get an error message you do not understand when you run the tests locally, please paste it here as a comment so that we can try to help you or just push the change to the PR so that we have an opportunity to read the message in the CI report.

really sorry for not responding to the same as I thought all tests were clear so its all working fine.

MrinalTyagi · 2021-11-06T08:36:24Z

Ok I understand the problem: the @deprecated decorator is already changing the content of the docstring and the numpydoc validation is working on this.

Since we already have the deprecation information in the altered docstring information from the @deprecated decorator, there is no need in putting it again in the docstring.

We could probably improve the @deprecated to improve the formatting and make it insert a .. deprecated directive after the top level oneliner but this is out of the scope of this PR.

For now, let's just remove the redundant ..deprecated directive

By this you mean to completely remove this section of text, right?

.. deprecated:: 1.0
or the complete deprecation warning?
.. deprecated:: 1.0
plot_roc_curve is deprecated in 1.0 and will be removed in
1.2. Use one of the following class methods:
:func:~sklearn.metrics.RocCurveDisplay.from_predictions or
:func:~sklearn.metrics.RocCurveDisplay.from_estimator.

sklearn/metrics/_plot/roc_curve.py

This reverts commit dced759.

MrinalTyagi · 2021-11-09T16:08:36Z

@glemaitre can u update me if any changes is required as I by mistakenly pushed another issue work in this pr. Sorry for the inconvenience as I am new to this.

MrinalTyagi · 2021-11-14T11:21:55Z

@glemaitre @ogrisel any update on the pr? Let me know if I need to perform any modifications.

MrinalTyagi · 2021-11-20T12:18:04Z

@glemaitre @ogrisel Please tell me how to proceed with the following pr as I have already implemented all the changes requested

sklearn/metrics/_plot/roc_curve.py

MrinalTyagi · 2021-11-29T02:02:06Z

@ogrisel Please tell me how to proceed.

ogrisel

@MrinalTyagi +1 for completely removing the redundant section in the docstring and only keep the decorator.

Then LGTM (assuming CI is green).

glemaitre

The documentation looks much nicer.
LGTM. I will merge once all CI will be green.
Thanks @MrinalTyagi

MrinalTyagi · 2021-11-29T14:06:33Z

Thanks :)

ogrisel · 2021-11-29T14:15:29Z

Merged. Thanks again @MrinalTyagi and sorry for the slow reply.

…arn#21547) Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>

Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>

DOC Ensures that precision_score passes numpydoc validation

d00e156

Addresses #21350 This PR ensures precision_score is compatible with numpydoc: 1. Remove sklearn.metrics._plot.roc_curve.plot_roc_curve from DOCSTRING_IGNORE_LIST. 2. Verify that all tests are passing.

github-actions bot added module:metrics Documentation labels Nov 4, 2021

Update test_docstrings.py

7b67f0f

MrinalTyagi changed the title ~~DOC Ensures that precision_score passes numpydoc validation~~ DOC Ensures that plot_roc_curve passes numpydoc validation Nov 4, 2021

glemaitre reviewed Nov 4, 2021

View reviewed changes

Update roc_curve.py

a2a382f

glemaitre reviewed Nov 4, 2021

View reviewed changes

sklearn/metrics/_plot/roc_curve.py Outdated Show resolved Hide resolved

glemaitre reviewed Nov 4, 2021

View reviewed changes

sklearn/metrics/_plot/roc_curve.py Outdated Show resolved Hide resolved

glemaitre reviewed Nov 4, 2021

View reviewed changes

Update roc_curve.py

9779bd9

ogrisel reviewed Nov 4, 2021

View reviewed changes

sklearn/metrics/_plot/roc_curve.py Outdated Show resolved Hide resolved

sklearn/metrics/_plot/roc_curve.py Show resolved Hide resolved

MrinalTyagi and others added 3 commits November 5, 2021 09:26

Update sklearn/metrics/_plot/roc_curve.py

6028a0c

Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>

Update sklearn/metrics/_plot/roc_curve.py

7d27077

Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>

Update roc_curve.py

d69b2b9

Merge branch 'scikit-learn:main' into main

6b989b3

Update roc_curve.py

cbee54e

ogrisel requested changes Nov 6, 2021

View reviewed changes

MrinalTyagi requested a review from ogrisel November 6, 2021 16:26

Update roc_curve.py

cd33ec1

thomasjpfan mentioned this pull request Nov 7, 2021

Ensure that functions's docstrings pass numpydoc validation #21350

Closed

glemaitre reviewed Nov 8, 2021

View reviewed changes

sklearn/metrics/_plot/roc_curve.py Outdated Show resolved Hide resolved

Update roc_curve.py

ee02495

MrinalTyagi requested a review from glemaitre November 8, 2021 11:28

MrinalTyagi added 3 commits November 9, 2021 17:33

Merge branch 'scikit-learn:main' into main

b5f5757

Base version

dced759

Revert "Base version"

e32e5ee

This reverts commit dced759.

glemaitre reviewed Nov 22, 2021

View reviewed changes

sklearn/metrics/_plot/roc_curve.py Outdated Show resolved Hide resolved

Merge branch 'scikit-learn:main' into main

0606bee

ogrisel approved these changes Nov 29, 2021

View reviewed changes

Update roc_curve.py

35deb98

glemaitre approved these changes Nov 29, 2021

View reviewed changes

ogrisel merged commit 863bbc0 into scikit-learn:main Nov 29, 2021

samronsin pushed a commit to samronsin/scikit-learn that referenced this pull request Nov 30, 2021

DOC Ensures that plot_roc_curve passes numpydoc validation (scikit-le…

bf84cdf

…arn#21547) Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>

glemaitre pushed a commit to glemaitre/scikit-learn that referenced this pull request Dec 24, 2021

DOC Ensures that plot_roc_curve passes numpydoc validation (scikit-le…

d08ca04

…arn#21547) Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>

glemaitre pushed a commit that referenced this pull request Dec 25, 2021

DOC Ensures that plot_roc_curve passes numpydoc validation (#21547)

c020576

Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>

jmloyola mentioned this pull request Jan 21, 2022

DOC Ensures that load_boston passes numpydoc validation #22247

Merged

Kshitij68 mentioned this pull request Sep 9, 2022

DOC Ensures plot_precision_recall_curve passes the numpydoc validation #24403

Merged

Uh oh!

Conversation

MrinalTyagi commented Nov 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reference Issues/PRs

What does this implement/fix? Explain your changes.

Any other comments?

Uh oh!

MrinalTyagi commented Nov 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

glemaitre Nov 4, 2021

Choose a reason for hiding this comment

Uh oh!

MrinalTyagi Nov 4, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

glemaitre left a comment

Choose a reason for hiding this comment

Uh oh!

ogrisel left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

MrinalTyagi commented Nov 5, 2021

Uh oh!

ogrisel commented Nov 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cmarmo commented Nov 5, 2021

Uh oh!

MrinalTyagi commented Nov 6, 2021

Uh oh!

MrinalTyagi commented Nov 6, 2021

Uh oh!

ogrisel left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ogrisel commented Nov 6, 2021

Uh oh!

MrinalTyagi commented Nov 6, 2021

Uh oh!

MrinalTyagi commented Nov 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

MrinalTyagi commented Nov 9, 2021

Uh oh!

MrinalTyagi commented Nov 14, 2021

Uh oh!

MrinalTyagi commented Nov 20, 2021

Uh oh!

Uh oh!

MrinalTyagi commented Nov 29, 2021

Uh oh!

ogrisel left a comment

Choose a reason for hiding this comment

Uh oh!

glemaitre left a comment

Choose a reason for hiding this comment

Uh oh!

MrinalTyagi commented Nov 29, 2021

Uh oh!

ogrisel commented Nov 29, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

MrinalTyagi commented Nov 4, 2021 •

edited

Loading

MrinalTyagi commented Nov 4, 2021 •

edited

Loading

ogrisel commented Nov 5, 2021 •

edited

Loading

ogrisel left a comment •

edited

Loading

MrinalTyagi commented Nov 6, 2021 •

edited

Loading