Skip to content

MRG: Make y a required parameter in CSP.fit_transform() #10084

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.

Sign up for GitHub

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

Jump to bottom
Merged
larsoner merged 2 commits into from
Dec 3, 2021
Merged

MRG: Make y a required parameter in CSP.fit_transform() #10084

larsoner merged 2 commits into from
Dec 3, 2021

Conversation

@hoechenberger
Copy link
Member

@hoechenberger hoechenberger commented Dec 3, 2021

Fixes #10079

@hoechenberger hoechenberger marked this pull request as ready for review December 3, 2021 12:32
@hoechenberger hoechenberger added the backport-candidate on-merge: backport to maint/1.11 label Dec 3, 2021
larsoner
larsoner reviewed Dec 3, 2021
View reviewed changes
@hoechenberger
Copy link
Member Author

hoechenberger commented Dec 3, 2021 •
edited
Loading

@larsoner Any idea how to fix the docstring test?

/usr/bin/bash --noprofile --norc /home/vsts/work/_temp/27adf643-783b-47e9-a1c0-cac31926ffad.sh
Running pydocstyle
mne/decoding/csp.py:235 in public method `fit_transform`:
        D102: Missing docstring in public method
make: *** [Makefile:115: pydocstyle] Error 1
##[error]Bash exited with code '2'.

@larsoner
Copy link
Member

larsoner commented Dec 3, 2021

I would add a docstring manually. Maybe you can deduplicate and make use of fill_doc?

@hoechenberger
Copy link
Member Author

hoechenberger commented Dec 3, 2021

I would add a docstring manually.

The thing is, I'm using copy_doc, I thought this should take care of such things 🤔

larsoner
larsoner reviewed Dec 3, 2021
View reviewed changes
@hoechenberger
Copy link
Member Author

hoechenberger commented Dec 3, 2021

Thanks, Eric! This seems to do the trick

@hoechenberger hoechenberger changed the title Make y a required parameter in CSP.fit_transform() MRG: Make y a required parameter in CSP.fit_transform() Dec 3, 2021
@larsoner
Copy link
Member

larsoner commented Dec 3, 2021

Worth a latest.inc or no?

@hoechenberger
Copy link
Member Author

hoechenberger commented Dec 3, 2021

It doesn't really change the behavior – code that doesn't pass y now simply fails earlier. Hence, I don't think we'll need a changelog entry here

@agramfort
Copy link
Member

agramfort commented Dec 3, 2021 via email

@larsoner larsoner merged commit c1cbc78 into mne-tools:main Dec 3, 2021
@larsoner
Copy link
Member

larsoner commented Dec 3, 2021

Thanks @hoechenberger !

@hoechenberger hoechenberger deleted the hoechenberger/issue10079 branch December 3, 2021 18:56
larsoner added a commit to GuillaumeFavelier/mne-python that referenced this pull request Dec 6, 2021
@larsoner
Merge remote-tracking branch 'upstream/main' into enh/coreg_feats
28773f4 
* upstream/main:
  Use fixes._compare_version for version checks everywhere (mne-tools#10091)
  Fast annotation from mask (mne-tools#10089)
  fix trace offsets in butterfly mode (mne-tools#10087)
  fix plot_compare_evokeds topo legend axes placement (mne-tools#9927)
  doc: clarify ica.apply include and exclude params (mne-tools#10086)
  MRG: Make y a required parameter in CSP.fit_transform() (mne-tools#10084)
  Add scrollbar to report tag dropdown menu (mne-tools#10082)
@larsoner larsoner removed the backport-candidate on-merge: backport to maint/1.11 label Apr 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@larsoner larsoner larsoner left review comments

@agramfort agramfort agramfort approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

CSP.fit() requires y to work, however the parameter is optional in fit_transform()

3 participants

@hoechenberger @larsoner @agramfort