BUG: Fix bug with MF LCMV rank #11664
Conversation
mne/beamformer/tests/test_lcmv.py
Outdated
| n = 102 if use_rank == 'full' else 71 | ||
| assert f"Making LCMV beamformer with rank {{'mag': {n}}}" in log |
There was a problem hiding this comment.
@britta-wstnr yes that old issue was meant to check/fix the beamformer behavior, but it didn't in the case of rank='info' (at least for make_lcmv) unfortunately. That's probably because we focused mostly on making estimate_rank work better, and if you pass rank=dict(...) where the dict comes from estimate_rank things will generally work okay. So the rank='info' case must have slipped through the cracks, as this test passes on this PR but fails on main for the rank='info' case.
|
@britta-wstnr any chance to look in the next day? We'd like to release possibly Friday and this should be in there. If not, @drammock could probably also look |
* upstream/main: ENH: Change known_config_types to dict (mne-tools#11166) MAINT: Improve README (mne-tools#11673)
|
Snuck in a fix for mne-kit-gui config var error: And overnight doc build failure due to PyVista release |
drammock
left a comment
drammock
left a comment
There was a problem hiding this comment.
Changes look good; I've left only a question but it seems like IIUC what the issue is, this is a good fix.
| - vtk>=9.2 | ||
| - traitlets | ||
| - pyvista>=0.32,!=0.35.2,!=0.38.0,!=0.38.1,!=0.38.2,!=0.38.3,!=0.38.4,!=0.38.5 | ||
| - pyvista>=0.32,!=0.35.2,!=0.38.0,!=0.38.1,!=0.38.2,!=0.38.3,!=0.38.4,!=0.38.5,!=0.38.6 |
|
|
||
|
|
||
| @testing.requires_testing_data | ||
| @pytest.mark.parametrize("use_rank", ("info", "computed", "full", None)) |
There was a problem hiding this comment.
so here, IIUC the point is that "info" and "computed" give the same answer (71); previously "info" gave the wrong answer post-maxfilter (right?). And the warning when using None is because... (?) computing the rank within make_lcmv gets a different answer than calling compute_rank() directly? I don't get why.
There was a problem hiding this comment.
And the warning when using None is because... (?) computing the rank within make_lcmv gets a different answer than calling compute_rank() directly? I don't get why.
In this case it's because it gets the rank estimate wrong. It's a known problem, though, and not what we're trying to fix here. Here we're just trying to fix that info case, and you're right that it's wrong on main but correct here.
* upstream/main: (32 commits) MAINT: Update download buttons [skip azp] [skip actions] [skip cirrus] Fix canvas.draw() in callback (mne-tools#11697) Remove recursion in plot_ica_components and use context manager for plt.ion/plt.ioff (mne-tools#11696) Update affiliation (mne-tools#11695) BUG: Fix bug with fwd restriction (mne-tools#11694) MRG: Suggest using "conda rename" in MNE updating instructions (mne-tools#11692) FIX: Regex [ci skip] MAINT: Apply deprecations [circle deploy] (mne-tools#11687) MAINT: Release 1.4.0 (mne-tools#11686) Trap music (mne-tools#11679) Fix call to plot_tfr_topomap from interactive AverageTFR.plot_topo function (mne-tools#11683) silence spectrum plot warning in examples/tutorials [circle full] (mne-tools#11682) Spectrum plot picks (mne-tools#11680) Update website conf (mne-tools#11675) BUG: Fix bug with MF LCMV rank (mne-tools#11664) ENH: Change known_config_types to dict (mne-tools#11166) MAINT: Improve README (mne-tools#11673) MAINT: Add to git-blame-ignore-revs [circle front] MAINT: Run black on codebase MAINT: Use black ...
3 participants
Closes #11403
@britta-wstnr can you look and mark for merge-when-green if you're happy?