-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
FIX: Handle diagonal covariances in plot_cov #13582
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
base: main
Are you sure you want to change the base?
Conversation
0051bbf to
9d9f2d2
Compare
larsoner
left a comment
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.
Sorry for the slow response, just a few tweaks!
Was this PR generated or assisted by an LLM?
| @@ -0,0 +1 @@ | |||
| Fix bug where plotting covariance matrices created by :func:`mne.cov.make_ad_hoc_cov` would raise an IndexError, by :newcontrib:`Arnav Kumar` (:gh:`13582`). | |||
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.
| Fix bug where plotting covariance matrices created by :func:`mne.cov.make_ad_hoc_cov` would raise an IndexError, by :newcontrib:`Arnav Kumar` (:gh:`13582`). | |
| Fix bug where plotting covariance matrices created by :func:`mne.cov.make_ad_hoc_cov` would raise an IndexError, by :newcontrib:`Arnav Kumar`. |
| cov = make_ad_hoc_cov(info, std={"eeg": 1}) | ||
| # This should not raise an IndexError | ||
| fig1, fig2 = cov.plot(info) | ||
| plt.close("all") |
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.
Not needed, we have a pytest fixture that does this part
| ) | ||
| cov = make_ad_hoc_cov(info, std={"eeg": 1}) | ||
| # This should not raise an IndexError | ||
| fig1, fig2 = cov.plot(info) |
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.
Outputs are unused so no need to assign
| fig1, fig2 = cov.plot(info) | |
| cov.plot(info) |
| info = create_info( | ||
| [f"EEG{i:03d}" for i in range(n_channels)], sfreq, ch_types="eeg" | ||
| ) |
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.
This will do the same thing more compactly!
| info = create_info( | |
| [f"EEG{i:03d}" for i in range(n_channels)], sfreq, ch_types="eeg" | |
| ) | |
| info = create_info(n_channels, sfreq, ch_types="eeg") |
Fixes #13142.
What does this implement/fix?
This PR fixes a bug where plotting covariance matrices created by
make_ad_hoc_cov()would raise anIndexError: too many indices for array: array is 1-dimensional, but 2 were indexed.Root cause:
make_ad_hoc_cov()creates diagonal covariance matrices stored as 1D arrays (the diagonal values)plot_cov()function's internal_index_info_cov()helper was directly accessingcov.data[ch_idx][:, ch_idx], which fails whencov.datais 1DSolution:
_index_info_cov()to usecov._get_square()instead of directly accessingcov.data_get_square()method already exists in theCovarianceclass and properly handles both diagonal (1D) and full (2D) covariance matrices by converting diagonal arrays to full 2D matrices usingnp.diag()when neededChanges:
mne/viz/misc.py: Updated_index_info_cov()to use_get_square()methodmne/viz/tests/test_misc.py:test_plot_cov_diagonal()to verify diagonal covariance plotting works correctlyAdditional information
_get_square()method)make_ad_hoc_cov()to create a diagonal covariance and verifies plotting succeeds without errors