[MRG] Apply numpydoc validation to VotingRegressor methods#15500
[MRG] Apply numpydoc validation to VotingRegressor methods#15500Tiffany8 wants to merge 2 commits intoscikit-learn:masterfrom
Conversation
…otingregressor-docstrings # Conflicts: # maint_tools/test_docstrings.py
| "RidgeClassifier.fit", | ||
| "RidgeClassifierCV.decision_function", | ||
| "SGDClassifier.decision_function", | ||
| "VotingRegressor$", |
There was a problem hiding this comment.
I am unfamiliar with this $ notation. I see it above for LogisticRegression$ and not below for KernelDensity. What does it mean and what made you decide to put $?
There was a problem hiding this comment.
It means end of the string with regular expression. LogisticRegression without $ would match any of its methods, not just the main docstring
|
|
||
| def score(self, X, y, sample_weight=None): | ||
| """Returns the coefficient of determination R^2 of the prediction. | ||
| """Return the coefficient of determination R^2 of the prediction. |
There was a problem hiding this comment.
I think the convention has been to write "Returns"
There was a problem hiding this comment.
This change is okay since we follow https://www.python.org/dev/peps/pep-0257/
The docstring is a phrase ending in a period. It prescribes the function or method's effect as a command ("Do this", "Return that"), not as a description; e.g. don't write "Returns the pathname ...".
| y : numpy array of shape [n_samples] | ||
| Target values. | ||
|
|
||
| **fit_params : Any number of parameters |
There was a problem hiding this comment.
Maybe say "dict of keyword arguments"
There was a problem hiding this comment.
| **fit_params : Any number of parameters | |
| **fit_params : dict |
|
Apart from the comments looks good to me pending CI |
| Returns | ||
| ------- | ||
| self : object | ||
| Returns self. |
There was a problem hiding this comment.
| Returns self. | |
| Fitted estimator. |
although I agree that it's probably not very useful in any case.
| ------- | ||
| predictions | ||
| array-like of shape (n_samples, n_classifiers), being | ||
| Array-like of shape (n_samples, n_classifiers), being |
There was a problem hiding this comment.
no actually this should be,
predictions : array-like of shape (n_samples, n_classifiers)
Values predicted by each regressor.
(with the right indentation)
There was a problem hiding this comment.
out of curiosity, why should this be an array-like instead of an array. Can it output anything other than ndarrays? (My only guess would be that if a dataframe was input it could output one and that dfs fall under the array-like definition, but haven't followed in detail the inclusion of dfs as possible inputs)
There was a problem hiding this comment.
You are right, it can only return an array. We use array-like for input dtypes, and it was probably copy-pasted there...
|
@Tiffany8, do you think you could find some time to synchronize to upstream and address the comments? Thanks! |
|
@rth is it possible to remove the "Needs work" label from this merged PR? thanks. |
|
Sure, done. It wasn't merged but closed BTW. |
Reference Issues/PRs
Addresses ##15440.
What does this implement/fix? Explain your changes.
Applied numpydoc validation to the estimator VotingRegressor and a some of the methods and added them to the docstring test whitelist
Specifically, changes included:
Any other comments?