Skip to content

BUG: Fix bug with CSP rank#12476

Merged
larsoner merged 8 commits intomne-tools:mainfrom
larsoner:rank
Apr 16, 2024
Merged

BUG: Fix bug with CSP rank#12476
larsoner merged 8 commits intomne-tools:mainfrom
larsoner:rank

Conversation

@larsoner
Copy link
Copy Markdown
Member

@larsoner larsoner commented Mar 1, 2024 •
edited
Loading

Closes #9094

WIP because I haven't thoroughly tested or anything. But at least modifying the example things work out of the box. The "trick" is to use _smart_eigh to figure out the correct subspace, do the eigh(a, b) in that subspace, and project back the same way we do with covariances. (Now you shouldn't even have to supply rank because it'll compute it from the cov for you if you don't!)

This really only works with a single channel type at the moment -- to properly support multiple channel types people should pass an info I think. So the TODO list is roughly:

  • Make tests more thorough
  • Make sure everything is green
  • Add a test using MEG+EEG to make sure it works
  • Add info support to make multiple channel types support robust
  • Add an example using MEG+EEG

@larsoner larsoner mentioned this pull request Mar 1, 2024
Closed
@larsoner larsoner added this to the 1.7 milestone Mar 1, 2024
@larsoner larsoner modified the milestones: 1.7, 1.8 Apr 9, 2024
larsoner added 2 commits April 15, 2024 11:28
@larsoner
Merge remote-tracking branch 'upstream/main' into rank
455a3dc 
* upstream/main: (50 commits)
  ENH: Improve OPM auditory dataset and example (mne-tools#12539)
  MAINT: Bump to latest pydata-sphinx-theme (mne-tools#12228)
  MRG: Simplify manual installation instructions a little by dropping explicit mention of (lib)mamba (mne-tools#12362)
  fix PSD weights handling when bad annotations present (mne-tools#12538)
  Fix phase loading (mne-tools#12537)
  align FFT windows to good data spans in psd_array_welch (mne-tools#12536)
  explicitly disallow multitaper in presence of bad annotations (mne-tools#12535)
  MAINT: Clean up PyVista contexts (mne-tools#12533)
  MAINT: Complete API change of ordered (mne-tools#12534)
  MAINT: Reinstall statsmodels and improve logging (mne-tools#12532)
  MAINT: Remove scipy.signal.morlet2 (mne-tools#12531)
  Update README badge links (mne-tools#12529)
  BUG: Fix bug with reading his_id from snirf (mne-tools#12526)
  [pre-commit.ci] pre-commit autoupdate (mne-tools#12524)
  Fix file format check in _check_eeglab_fname function (mne-tools#12523)
  MAINT: Reenable picard in pre testing (mne-tools#12525)
  MAINT: Bump to large resource class (mne-tools#12522)
  MAINT: Restore 2 jobs on Windows (mne-tools#12520)
  Add exclude_after_unique option to mne.io.read_raw_edf/read_raw_edf to search for exclude channels after making channel names unique (mne-tools#12518)
  Improve consistency of sensor types in code and documentation (mne-tools#12509)
  ...
@larsoner
FIX: Better
a940687
@larsoner larsoner marked this pull request as ready for review April 15, 2024 15:59
@larsoner
Copy link
Copy Markdown
Member Author

larsoner commented Apr 15, 2024

@drammock realized we should get this in to 1.7 as well. Ready for review/merge from my end!

@larsoner
Copy link
Copy Markdown
Member Author

larsoner commented Apr 15, 2024

Argh https://physionet.org/ is down but example renders okay locally

main PR
image image
image image

@larsoner
Copy link
Copy Markdown
Member Author

larsoner commented Apr 15, 2024

Related MIT-LCP/physionet#154

@larsoner larsoner modified the milestones: 1.8, 1.7 Apr 15, 2024
cbrnr
cbrnr approved these changes Apr 16, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@cbrnr cbrnr left a comment

Choose a reason for hiding this comment

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

LGTM!

@ivopascal
Copy link
Copy Markdown
Contributor

ivopascal commented Apr 16, 2024 •
edited
Loading

LGTM, just one question: does this also resolve the issue with channel types? I remember someone reported that computing the rank on EEG-only data suddenly created a rank for MEG channels, but unfortunately I cannot find that issue (or forum question).

I think this refers to my problem, though it created a rank for MAG instead of MEG. It's discussed here #9094 (comment)

A MWE is here: https://colab.research.google.com/drive/1LBq8Sg-30Cnlj9sYLPtEcsPIyBtsvxF8?usp=sharing#scrollTo=_hS__cmSGVIU

The only problem is that the logger still indicates MAG channels which don't exist, but all other problems are solved.
From the logger: Setting small MAG eigenvalues to zero (without PCA)

larsoner added 2 commits April 16, 2024 09:25
@larsoner
Merge remote-tracking branch 'upstream/main' into rank
68415ae 
* upstream/main:
  fix prefilter management for EDF/BDF (mne-tools#12441)
  [pre-commit.ci] pre-commit autoupdate (mne-tools#12541)
  ENH: Allow removal of (up to two) bad marker coils in read_raw_kit() (mne-tools#12394)
  Implement `picks` argument to `Raw.plot()` (mne-tools#12467)
  Add Meggie under Related Software documentation (mne-tools#12540)
@larsoner
FIX: Logging
0ae2b18
@larsoner
Copy link
Copy Markdown
Member Author

larsoner commented Apr 16, 2024

Okay pushed a commit to fix that "mag" business as well, the logging in the changed example (if you enable it) looks like:

Computing rank from data with rank=None
    Using tolerance 7e-05 (2.2e-16 eps * 64 dim * 4.9e+09  max singular value)
    Estimated rank (data): 63
    data: rank 63 computed from 64 data channels with 0 projectors
    Setting small data eigenvalues to zero (without PCA)
Reducing data rank from 64 -> 63
Estimating class=0 covariance using EMPIRICAL
Done.
Estimating class=1 covariance using EMPIRICAL
Done.

@larsoner larsoner enabled auto-merge (squash) April 16, 2024 14:47
@larsoner larsoner merged commit e23e9e1 into mne-tools:main Apr 16, 2024
@larsoner larsoner deleted the rank branch April 16, 2024 16:52
@cbrnr
Copy link
Copy Markdown
Contributor

cbrnr commented Apr 16, 2024

Thanks @larsoner!

@mscheltienne
Copy link
Copy Markdown
Member

mscheltienne commented Jul 1, 2024

Seems like this PR introduces a bug, reported here: https://mne.discourse.group/t/the-api-reference-of-csp-needs-updata/9018/2

@mscheltienne mscheltienne mentioned this pull request Jul 1, 2024
Closed
@cbrnr cbrnr mentioned this pull request Aug 13, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cbrnr cbrnr cbrnr approved these changes

@drammock drammock Awaiting requested review from drammock drammock is a code owner

@agramfort agramfort Awaiting requested review from agramfort agramfort is a code owner

@dengemann dengemann Awaiting requested review from dengemann

@jasmainak jasmainak Awaiting requested review from jasmainak

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

1.7

Development

Successfully merging this pull request may close these issues.

CSP error: LinAlgError : The leading minor of order 64 of B is not positive definite.

4 participants

@larsoner @ivopascal @cbrnr @mscheltienne