Skip to content

Conversation

@larsoner
Copy link
Member

@larsoner larsoner commented Sep 6, 2024

Closes #12748

  1. Un-nest sklearn stuff in mne/decoding except places that we use private attributes. Someday maybe we shouldn't use them at all, but I'll save that for later. In the meantime I'd rather not break all of mne.decoding if any of those imports fail in the future.
  2. Remove now-unnecessary custom mixin classes.
  3. Use check_array now that we require sklearn >= 1.2 (it's available as of 1.1)
  4. The cov stuff didn't end up being too terrible -- just had to write a little helper for get_params / set_params

@larsoner larsoner added this to the 1.9 milestone Sep 6, 2024
Copy link
Member

@drammock drammock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for tackling this! changes look reasonable / straightforward, so I'm inclined to trust our test suite here. Just one question.

Comment on lines 333 to 336
@property
def picks_(self):
"""Backward compatible wrapper for picks."""
return self.picks
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we want to deprecate .picks_ or just keep it around forever?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's pretty trivial to keep it I think so probably not worth deprecating

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see comment above

@hoechenberger
Copy link
Member

thanks for working on this, Eric!

Copy link
Member

@agramfort agramfort left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just some discussion on picks_ vs picks
if sklearn becomes just required in mne.decoding and not any mne import I am ok with that. We do have a CI run without sklearn?

Comment on lines 333 to 336
@property
def picks_(self):
"""Backward compatible wrapper for picks."""
return self.picks
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see comment above

@larsoner
Copy link
Member Author

larsoner commented Sep 9, 2024

if sklearn becomes just required in mne.decoding and not any mne import I am ok with that. We do have a CI run without sklearn?

Yep the minimal CI should already make sure everything still works without sklearn

@larsoner
Copy link
Member Author

Got approval and latest commits were just getting things green so in it goes!

@larsoner larsoner merged commit 5425ef4 into mne-tools:main Sep 13, 2024
@larsoner larsoner deleted the est branch September 13, 2024 13:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Type incompatibility between sklearn's API and SlidingEstimator

4 participants