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 RandomForestClassifier passes numpydoc validation #20383

Conversation

alinealfa
Copy link
Contributor

Reference Issues/PRs

Addresses #20308

What does this implement/fix? Explain your changes.

Fixes numpydoc errors on RandomForestClassifier
Forces decorate property to update doc for deprecated method

Any other comments?

#DataUmbrella #sprint
cc. @marenwestermann

sklearn/ensemble/_forest.py Outdated Show resolved Hide resolved
sklearn/utils/deprecation.py Outdated Show resolved Hide resolved
sklearn/ensemble/_forest.py Outdated Show resolved Hide resolved
@glemaitre
Copy link
Member

#20385 has been merged. You can merge main into your branch to get the changes.

@glemaitre
Copy link
Member

Uhm black is not happy with the formatting of _forest.py. Once the suggestion applied could you execute black on the python file.

@marenwestermann
Copy link
Member

We ran black after implementing the changes and merging main into our branch but there was no reformatting.

@glemaitre
Copy link
Member

Yep black is not complaining anymore. However, my suggestion is too long. I propose to break the line and flake8 should be fine.

@glemaitre glemaitre self-requested a review July 6, 2021 09:10
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 took the liberty to merge main into your branch and change the docstring of feature_in_ to see if the tests are passing. If the CIs agree then we can merge it.

@glemaitre glemaitre self-requested a review July 7, 2021 10:12
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 merge main into the branch to solve the CI doc failure.

@glemaitre glemaitre merged commit f2a6e10 into scikit-learn:main Jul 7, 2021
@glemaitre
Copy link
Member

The CIs are green, let's merge. Thanks @alinealfa

thomasjpfan pushed a commit to thomasjpfan/scikit-learn that referenced this pull request Jul 7, 2021
samronsin pushed a commit to samronsin/scikit-learn that referenced this pull request Nov 30, 2021
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.

None yet

3 participants