-
-
Notifications
You must be signed in to change notification settings - Fork 25.8k
DOC Ensures that AgglomerativeClustering passes numpydoc validation #20544
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 AgglomerativeClustering passes numpydoc validation #20544
Conversation
I had a couple of questions:
return_distance : bool, default=None
If True, return the distance between the clusters.
return_distance : bool, default=False
whether or not to return the distances between the clusters. For line 180: If it is of type |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just marking as "Request changes" to see that we review the PR.
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>
…omerativeClustering
…/scikit-learn into AgglomerativeClustering
I resolved the conflict but I think that there is still a couple of failures associated with the validation. |
This is the error, and I'm not sure where to resolve it: E # Errors
E
E - GL08: The object does not have a docstring |
The full error message is:
So the problem is related to @glemaitre any idea? |
The |
@@ -984,8 +1017,7 @@ def fit(self, X, y=None): | |||
return self | |||
|
|||
def fit_predict(self, X, y=None): | |||
"""Fit the hierarchical clustering from features or distance matrix, | |||
and return cluster labels. | |||
"""Fit the hierarchical clustering from features, or distance matrix. | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a short paragraph to make the difference method between the fit_predict
and the fit
methods explicit, for instance: "In addition to fitting, this method also return the result of the clustering assignment for each sample in the training set."
…omerativeClustering
…omerativeClustering
This is odd, right? Instead of (not that I think that is causing the error.)
|
Nop. |
Ok, it's been added back. Still not sure what is causing the error. ? |
|
||
Returns | ||
------- | ||
self | ||
self : object | ||
Returns the transformer. | ||
""" | ||
X = self._validate_data( | ||
X, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error comes from the undocumented property in line 1230:
@property
def fit_predict(self):
"""Raise `AttributeError`."""
raise AttributeError
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. I pushed a couple of small changes. Let see if the CIs are happy.
I see that we should have a subsequent PR because FeatureAgglomeration
should not accept a **params
parameter at fit
.
The CI for the numpydoc validation passed. So I will merge it now. |
Thanks @reshamas |
…cikit-learn#20544) Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>
Reference Issues/PRs
Addresses #20308
What does this implement/fix? Explain your changes.
Fixes numpydoc errors on AgglomerativeClustering functions
Any other comments?
#DataUmbrella LATAM sprint