Skip to content
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

Update decoding classes for compliance with Scikit-Learn best practice #11217

Closed
Dod12 opened this issue Oct 1, 2022 · 1 comment · Fixed by #13065
Closed

Update decoding classes for compliance with Scikit-Learn best practice #11217

Dod12 opened this issue Oct 1, 2022 · 1 comment · Fixed by #13065
Labels

Comments

@Dod12
Copy link
Contributor

Dod12 commented Oct 1, 2022

Describe the new feature or enhancement

Scikit-learn best practice states that there should be no input validation or modification of parameters in the constructor. Violation of this policy has previously led to bug #10971.

Modifying a parameter in the constructor can lead to undefined behavior when cloning the estimator. Performing input validation in the constructor leads to an issue where sklearn.model_selection.GridSearchCV and other similar estimators may pass invalid options to the class, since they set parameter values using the set_params method instead of the constructor. The recommendation is to validate inputs at the first place they are used in the estimator.

Describe your proposed implementation

My suggestion is to move all modification of parameters to the fit method and the creation of a separate _check_params method similar to _check_Xy called from fit. This solves the above problems since set_params is called after the constructor and before fit.

Describe possible alternatives

In some of the estimators input validation is sufficiently complex to warrant the creation of a separate method, whilst in others it is reasonable to add it directly to the fit method. Another alternative would be to perform all the logic currently in the constructor in a separate method, including parameter modification as needed.

Additional context

I'm currently working on a PR implementing this suggestion.

@Dod12 Dod12 added the ENH label Oct 1, 2022
@agramfort
Copy link
Member

agramfort commented Oct 1, 2022 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants