Skip to content
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.

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

DOC Ensures that SVC passes numpydoc validation #20457

Merged
merged 13 commits into from
Jul 13, 2021

Conversation

jmloyola
Copy link
Member

@jmloyola jmloyola commented Jul 3, 2021

Reference Issues/PRs

Addresses #20308

What does this implement/fix? Explain your changes.

This PR ensures SVC is compatible with numpydoc.

  • Remove SVC from DOCSTRING_IGNORE_LIST.
  • Verify all tests passing.

Any other comments?

#DataUmbrella Sprint

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.

Just a couple of changes in the property docstring. Could you merge main into your branch?

sklearn/svm/_base.py Outdated Show resolved Hide resolved
sklearn/svm/_base.py Outdated Show resolved Hide resolved
sklearn/svm/_base.py Outdated Show resolved Hide resolved
sklearn/svm/_base.py Outdated Show resolved Hide resolved
jmloyola and others added 5 commits July 6, 2021 09:27
Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>
Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>
Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>
Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>
@jmloyola
Copy link
Member Author

jmloyola commented Jul 6, 2021

Thanks for the comments @glemaitre.
I merged upstream/main into this branch.
Let me know if there is something else I should do.

@jmloyola
Copy link
Member Author

jmloyola commented Jul 6, 2021

@glemaitre, how should we document the properties?
After the suggestions, the numpydoc validation throws this error for all docstrings of the properties:
- SS02: Summary does not start with a capital letter

sklearn/svm/_base.py Outdated Show resolved Hide resolved
@thomasjpfan
Copy link
Member

To fix the circleci issues, you need to merge upstream/main into this PR.

@jmloyola
Copy link
Member Author

I think this PR is ready. But, obviously, any suggestions are more than welcome 🤓.

Let me know what you think @thomasjpfan, @amueller, @glemaitre

@jmloyola jmloyola requested a review from glemaitre July 12, 2021 12:56
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.

A couple of more changes

sklearn/svm/_base.py Outdated Show resolved Hide resolved
sklearn/svm/_base.py Outdated Show resolved Hide resolved
sklearn/svm/_base.py Outdated Show resolved Hide resolved
sklearn/svm/_base.py Outdated Show resolved Hide resolved
jmloyola and others added 4 commits July 13, 2021 08:59
Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>
Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>
Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>
Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>
@jmloyola
Copy link
Member Author

Thank you @glemaitre for the comments, let me know if there is something else that I need to do.

@glemaitre glemaitre merged commit 9c2b3b6 into scikit-learn:main Jul 13, 2021
@glemaitre
Copy link
Member

All good. Thanks @jmloyola we can merge.

@jmloyola jmloyola deleted the fix_numpydoc_svc branch July 13, 2021 16:22
samronsin pushed a commit to samronsin/scikit-learn that referenced this pull request Nov 30, 2021
Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants