mne-python icon indicating copy to clipboard operation
mne-python copied to clipboard

Published 20 hours ago • verified publisher icon mne-tools

[FIX] Remove _check_tfr_params from Transformer constructor

Open dod12 opened this issue 3 years ago • 1 comments

Reference issue

Fixes #10971.

What does this implement/fix?

The mne.decoding.time_frequency.TimeFrequency transformer called the mne.time_frequency.tfr._check_tfr_params function 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_tfr method anyway. The _check_option call can remain, since it doesn't modify parameters.

dod12 avatar Aug 03 '22 15:08 dod12

Hello! 👋 Thanks for opening your first pull request here! ❤️ We will try to get back to you soon. 🚴🏽‍♂️

welcome[bot] avatar Aug 03 '22 15:08 welcome[bot]

It seems like some of the tests failed due to CI issues (#11023 and #11012). I have merged the latest main, so hopefully that should resolve the failing tests.

dod12 avatar Aug 13 '22 10:08 dod12

Failing test is unrelated and being tackled in #11034. Thanks for looking into this @Dod12 , I'll go ahead and merge!

If you're feeling motivated, you could add similar tests for all of our decoding classes, they probably need it...

larsoner avatar Aug 15 '22 14:08 larsoner

🎉 Congrats on merging your first pull request! 🥳 Looking forward to seeing more from you in the future! 💪

welcome[bot] avatar Aug 15 '22 14:08 welcome[bot]