Skip to content

DOC Ensures that VotingRegressor passes numpydoc validation #20450

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

Merged
merged 1 commit into from
Jul 10, 2021

Conversation

Anavelyz
Copy link
Contributor

@Anavelyz Anavelyz commented Jul 3, 2021

Reference Issues/PRs

References Addresses #20308

What does this implement/fix? Explain your changes.

Updated parameters and returned values descriptions.

Any other comments?

#DataUmbrella Sprint
cc: (pair programming partner) @marielaraj

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.

You need to remove VotingRegressor from the list of estimator located in maint_tools/test_docstring.py

@Anavelyz
Copy link
Contributor Author

Anavelyz commented Jul 6, 2021

Hi, thanks for reviewing. At first I removed it but since I did not know how to solve 2 failed I returned the change.

One of the errors is:

ValueError: 
       
None
VotingRegressor.n_features_in_
 Parsing of the method signature failed, possibly because this is a property.

# Errors
 - GL08: The object does not have a docstring

@glemaitre
Copy link
Member

Around line 121, you need to add a docstring for the property:

@property
def n_features_in_(self):
    """int : Number of features seen during `fit`."""
    ...

@Anavelyz
Copy link
Contributor Author

Anavelyz commented Jul 6, 2021

A thousand thanks.

I added this text in 'named_estimators': """Object allows access any fitted sub-estimators by name.""""
I'm not sure if it's the right one, I also thought of something like "Create an object that allows access any fitted sub-estimators by name"

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.

Could you merge main into your branch. It will solve the failure in the CI.
I added a suggestion to add the returned type for one of the property.

@Anavelyz
Copy link
Contributor Author

Thanks. Merged!

@thomasjpfan
Copy link
Member

@Anavelyz To fix the issue with circleci, you need to merge this PR with main.

@Anavelyz
Copy link
Contributor Author

About this comment @thomasjpfan, where do I find a guide to do it correctly?
I'm afraid of breaking something hahaha

@thomasjpfan
Copy link
Member

For your case:

git checkout numpydoc
git fetch upstream main
git merge upstream/main

@Anavelyz
Copy link
Contributor Author

Ready!

@thomasjpfan
Copy link
Member

git push to push your change to your branch on github.

@thomasjpfan
Copy link
Member

Something went wrong with the merge, would it be okay if I try to fix it for you?

@Anavelyz
Copy link
Contributor Author

Yes, it's okay. Thank you!

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

@thomasjpfan thomasjpfan merged commit 1fd45fb into scikit-learn:main Jul 10, 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.

3 participants