Skip to content
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

MRG: Fix DICS rank handling #8594

Merged
merged 8 commits into from Dec 17, 2020
Merged

MRG: Fix DICS rank handling #8594

merged 8 commits into from Dec 17, 2020

Conversation

wmvanvliet
Copy link
Contributor

DICS handing of the rank parameter was completely broken. It was changed from a single number to something more complicated, but the code was not updated appropriately.

Still needs unit test to cover this case, but here at least is a fix for the code.

@larsoner
Copy link
Member

larsoner commented Dec 2, 2020

I wonder if fixing this would allow us to remove the reduce_rank=True from here:

https://mne.tools/dev/auto_examples/inverse/plot_dics_source_power.html?highlight=dics#compute-source-power-using-dics-beamformer

@larsoner
Copy link
Member

larsoner commented Dec 2, 2020

Spoiler alert: it doesn't. I guess this makes sense since the reduce_rank has to do with the 3-orientations rank as opposed to the n_ch rank / whitening.

@larsoner larsoner added this to the 0.22 milestone Dec 2, 2020
@wmvanvliet
Copy link
Contributor Author

The computations have not changed. It's just that setting the rank parameter to anything other than None was broken.

* upstream/master: (38 commits)
  MRG, ENH: Add DICS bias tests (mne-tools#8610)
  MRG, BUG, ENH: Add window option (mne-tools#8662)
  BUG: Fix alpha for volumes (mne-tools#8663)
  MRG, BUG: Fix bugs with envcorr (mne-tools#8658)
  MRG, ENH: Progressbar for csd_morlet (mne-tools#8608)
  Render is necessary now (mne-tools#8657)
  VIZ: Fix head size (mne-tools#8651)
  MRG, MAINT: bump sphinxcontrib-bitex version (mne-tools#8653)
  MRG, MAINT: Improve server env (mne-tools#8656)
  BUG: Mayavi center (mne-tools#8644)
  VIZ, ENH: allow show/hide annotations by label (mne-tools#8624)
  Add regression test for EEGLAB data with a chanlocs struct (mne-tools#8647)
  FIX: scalar_bar (mne-tools#8643)
  MRG: Small fix to tutorial; rename plot_events ordinate label to "Event id"; improve some SSP docstrings (mne-tools#8612)
  MRG, ENH: make plot alignment use defaults for colors (mne-tools#8553)
  BUG: Fix passing of channel type (mne-tools#8638)
  FIX: fixed loop over norm PSF/CTF options (mne-tools#8636)
  MRG, BUG: Pass kwargs (mne-tools#8630)
  DOC: Clearer error message (mne-tools#8631)
  BUG: Fix number of labels (mne-tools#8629)
  ...
@larsoner larsoner changed the title Fix DICS rank handling MRG: Fix DICS rank handling Dec 16, 2020
Copy link
Member

@larsoner larsoner left a comment

Choose a reason for hiding this comment

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

I just pushed a test and latest.inc update, LGTM +1 for merge

@larsoner larsoner merged commit c23df98 into mne-tools:master Dec 17, 2020
2 checks passed
@wmvanvliet
Copy link
Contributor Author

Thanks @larsoner! I really appreciate you picking this up.

@wmvanvliet wmvanvliet deleted the dics_rank branch October 18, 2021 06:06
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.

None yet

3 participants