-
Notifications
You must be signed in to change notification settings - Fork 69
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
BUG: Fix bug with multichannel classification #853
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice
This is failing on N2pc but really it's because the epochs after ICA all get just DC values. See current output on Here are the components: @hoechenberger have you ever seen this? |
Wow. No, never seen this before. Crazy |
how many ICA components are you keeping here? |
All of them:
After "cleaning" -- i.e., applying ICA with nothing excluded -- the signals are just at DC. If you look at the component plots above they look very weird to me, all dominated by some posterior lateral electrodes on the left and right |
28 See the report here, it's weird: There is one source floating all over the place |
... and I don't understand why
There is this oddity:
So there is one row of the mixing matrix with is almost 30 orders of magnitude greater than the others. |
... and to reproduce if you run |
Okay I think it might be a rank problem:
That last component explaining 1e-32 part of the variance is probably just noise. So the data are rank deficient but somehow we're not capturing that. I'll look into the |
@@ -1343,7 +1343,7 @@ | |||
limit may be too low to achieve convergence. | |||
""" | |||
|
|||
ica_n_components: Optional[Union[float, int]] = 0.8 | |||
ica_n_components: Optional[Union[float, int]] = None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hoechenberger to me I'm not sure why we set this to 0.8. It seems like a bugfix to me to change it to match the MNE-Python default
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good to me
@@ -25,6 +25,8 @@ | |||
- Fix bug where `--no-cache` had no effect (#839 by @larsoner) | |||
- Fix bug where the Maxwell filtering step would fail if [`find_noisy_channels_meg = False`][mne_bids_pipeline._config.find_noisy_channels_meg]` was used (#847 by @larsoner) | |||
- Fix bug where raw, empty-room, and custom noise covariances were errantly calculated on data without ICA or SSP applied (#840 by @larsoner) | |||
- Fix bug where multiple channel types (e.g., MEG and EEG) were not handled correctly in decoding (#853 by @larsoner) | |||
- Fix bug where the previous default for [`ica_n_components`][mne_bids_pipeline._config.ica_n_components] of `0.8` was too conservative, changed the default to `None` to match MNE-Python (#853 by @larsoner) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's the argument in whats-new form 🙂
Okay I think this fixes things so I'll mark for merge-when-green |
Before merging …
docs/source/changes.md
)When MEG+EEG was used, only EEG was (effectively) being used for classification. This also unifies how rank reduction is done on the data across classifiers as much as possible.
Closes #849