Skip to content

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

Merged
merged 7 commits into from
Sep 2, 2022

Conversation

stefmolin
Copy link
Contributor

Reference Issues/PRs

Towards #23462

What does this implement/fix? Explain your changes.

Add parameter validation to PatchExtractor.

Any other comments?

@glemaitre glemaitre changed the title MAINT Parameter validation for PatchExtractor. MAINT Parameter validation for PatchExtractor Aug 22, 2022
@glemaitre glemaitre self-requested a review August 22, 2022 14:01
@@ -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()
Copy link
Member

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.

Copy link
Member

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.

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.

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.

@glemaitre glemaitre added the Validation related to input validation label Aug 23, 2022
@jeremiedbb
Copy link
Member

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

@jeremiedbb jeremiedbb merged commit 5e9fa42 into scikit-learn:main Sep 2, 2022
@stefmolin stefmolin deleted the maint/patch-extractor branch September 2, 2022 12:27
glemaitre pushed a commit to glemaitre/scikit-learn that referenced this pull request Sep 12, 2022
Co-authored-by: jeremie du boisberranger <jeremiedbb@yahoo.fr>
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