Trap music #11679
Trap music #11679
Conversation
This reverts commit 7c42fe4.
|
I should not have suppressed the verbose parameter in the function decorated by @verbose, which I do not understand well enough. Reverted. |
larsoner
left a comment
larsoner
left a comment
There was a problem hiding this comment.
Looks good so far, just a couple of minor comments!
mne/beamformer/_rap_music.py
Outdated
| @verbose | ||
| def rap_music( | ||
| evoked, forward, noise_cov, n_dipoles=5, return_residual=False, verbose=None | ||
| evoked, forward, noise_cov, n_dipoles=5, return_residual=False, verbose=None, use_trap=False |
There was a problem hiding this comment.
By convention verbose is always last, and we should add * to prevent errors at the user end
| evoked, forward, noise_cov, n_dipoles=5, return_residual=False, verbose=None, use_trap=False | |
| evoked, forward, noise_cov, n_dipoles=5, return_residual=False, *, use_trap=False, verbose=None |
mne/beamformer/_rap_music.py
Outdated
| %(verbose)s | ||
| use_trap : boolean | ||
| Use the TRAP-MUSIC variant if True. The default value is False. |
There was a problem hiding this comment.
| %(verbose)s | |
| use_trap : boolean | |
| Use the TRAP-MUSIC variant if True. The default value is False. | |
| use_trap : boolean | |
| Use the TRAP-MUSIC variant if True (default False). | |
| %(verbose)s |
|
|
||
|
|
||
| @testing.requires_testing_data | ||
| def test_rap_music_trap(): |
There was a problem hiding this comment.
How long does this take to run? Ideally tests will take < 1 sec
There was a problem hiding this comment.
It should not be longer than the previous tests. Not sure whether they are under the second. The difference between RAP and TRAP is just one array manipulation where we remove one column in an array at each iteration.
mne/beamformer/_rap_music.py
Outdated
| n_dipoles=5, | ||
| return_residual=False, | ||
| *, | ||
| use_trap=False, |
There was a problem hiding this comment.
I would rather have a public function called trap_music next to rap_music to make it easier to discover. It means rap_music with use_trap param should be made a private function and then trap_music public function would call _rap_music(..., use_trap=True)
There was a problem hiding this comment.
Hopefully done. I hope I made the right decision w.r.t the decorators and documentation. Also corrected a minor doc error.
|
I took the chance to remove some dead code. The rank cannot obviously be 0. Note that the comment on the line after the one removed (that forces rank to be at least 2) after is misleading. When fixed orientation is used, the rank is (and should be) 1. Fortunately, the Ug[:,:2] on a matrix with one column just returns the Ug vector.... |
|
Well the dead code was not that dead. I do not know why the test passes a leadfield with a null vector in test_rap_music_sphere ??? Reverted for the time being. |
|
I think I made all the suggested changes and added the entry in latest.inc. |
|
Thanks @papadop ! |
* 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
Reference issue
Example: Fixes #7679 .
What does this implement/fix?
It implements TRAP-MUSIC, which is a very simple variant of RAP-MUSIC.
Basically at each step the signal space is reduced by one dimension instead of being kept of constant diemnsion.
I also noticed some unused parameters and functions which I removed.
Additional information
On a simple example I tested the approach, it did not make a lot of difference. But technically, this is more sound than the original RAP. Yet, I kept the default to RAP instead of TRAP to avoid changing a behaviour.
The changes were tested outside of mne (but calling mne routines for everything outside the _rap_music_implementation.py file. I also did not test the new test in test_rap_music.py, but the change is very trivial. I leave it to any checker to catch eventual copy-paste errors, I may have made when I transferred the outside-of-mne implementation to mne.