[FIX] Remove _check_tfr_params from Transformer constructor#11004
Merged
larsoner merged 12 commits intomne-tools:mainfrom Aug 15, 2022
Merged
[FIX] Remove _check_tfr_params from Transformer constructor#11004larsoner merged 12 commits intomne-tools:mainfrom
larsoner merged 12 commits intomne-tools:mainfrom
Conversation
Since the _check_tfr_param function changes some of the values it checks, scikit-learn estimators must perform this check at runtime, not during estimator instantiation. Failing to do so leads to an error.
If the constructor modifies non-default parameters, scikit-learn will throw an error during cloning.
Checking and transforming in the constructor violates sklearn best-practice. Since this check is already performed in the `_compute_tfr` method, we can simply remove the check from the transformer class.
Since options checking doesn't modify the value it makes sense to keep it in the constructor.
…n into time_frequency
|
Hello! 👋 Thanks for opening your first pull request here! ❤️ We will try to get back to you soon. 🚴🏽♂️ |
Contributor
Author
Member
|
🎉 Congrats on merging your first pull request! 🥳 Looking forward to seeing more from you in the future! 💪 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Reference issue
Fixes #10971.
What does this implement/fix?
The
mne.decoding.time_frequency.TimeFrequencytransformer called themne.time_frequency.tfr._check_tfr_paramsfunction in the constructor. This function violates scikit-learn best practices on estimators, since it modifies some of the input parameters.This PR removes the call to
_check_tfr_params, since it is called later in the_compute_tfrmethod anyway. The_check_optioncall can remain, since it doesn't modify parameters.