-
-
Notifications
You must be signed in to change notification settings - Fork 25.8k
MAINT Parameter validation for PatchExtractor
#24215
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
Conversation
PatchExtractor
.PatchExtractor
sklearn/feature_extraction/image.py
Outdated
@@ -547,6 +559,7 @@ def transform(self, X): | |||
`n_patches` is either `n_samples * max_patches` or the total | |||
number of patches that can be extracted. | |||
""" | |||
self._validate_params() |
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.
Normally we don't validate the parameters in transform
yet.
This transformer is a bit special because it is stateless since it does not do anything at fit
time.
So I see a couple of trouble here with the current implementation:
First, we should inherit from TransformerMixin
:
class PatchExtractor(TransformerMixin, BaseEstimator)
Then, we should state programatically that the transformer is stateless by implementing the following method:
def __sklearn_is_fitted__(self):
"""Return True since PatchExtractor is stateless."""
return True
and add the associated tag:
def _more_tags(self):
return {"X_types": ["3darray"], "stateless": True}
I assume that we could implement these fixes in another PR. Then, it makes sense to validate the parameter in transform
since it could be called without calling fit
.
In short, the current PR seems fine to me as is.
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.
I opened #24230 to solve the issue.
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.
Putting my approval.
I would like to have a second review from @jeremiedbb to know what he thinks about parameter validation for the stateless estimators.
We've actually decided for previous stateless transformers that we don't want to validate at transform so I removed validation from transform. LGTM otherwise. Thanks @stefmolin |
Co-authored-by: jeremie du boisberranger <jeremiedbb@yahoo.fr>
Reference Issues/PRs
Towards #23462
What does this implement/fix? Explain your changes.
Add parameter validation to
PatchExtractor
.Any other comments?