[MRG+1] Fix weight normalization for LCMV beamformer #5511
Conversation
|
Here is an overview of whether the old and new LCMV code produce the same output. #5511 is the current PR
|
Codecov Report
@@ Coverage Diff @@
## master #5511 +/- ##
==========================================
+ Coverage 88.27% 88.35% +0.07%
==========================================
Files 360 363 +3
Lines 66898 67665 +767
Branches 11320 11463 +143
==========================================
+ Hits 59056 59784 +728
- Misses 5020 5042 +22
- Partials 2822 2839 +17 |
wmvanvliet
changed the title
massich
left a comment
massich
left a comment
There was a problem hiding this comment.
this PR changes a condition, where there's no test for both branches. It would be good to have it. Rather than that LGTM
|
Super, thanks! Makes it definitely easier to check, I am on it. |
massich
changed the title
|
@britta-wstnr are you ok with this? |
|
Sorry guys, I am packed these days. Not forgotten though and on top of list for next week! |
| rank_Cm = estimate_rank(Cm, tol='auto', norm=False, | ||
| return_singular=False) | ||
| noise = noise[len(noise) - rank_Cm] | ||
| noise = noise[rank_Cm] |
There was a problem hiding this comment.
I do not think this is correct. Note that we use linalg.eigh which returns the eigenvalues in ascending order. The old version gives the same results as Fieldtrip.
There was a problem hiding this comment.
All tests are passing which suggests we need a test that would have caught this case
There was a problem hiding this comment.
aah, ascending order... how weird!
|
@britta-wstnr I think I've fixed it now. The table with results has been updated. The NAI beamformer's output is now the same as in master. |
|
@wmvanvliet yep, NAI beamformer is fine again. |
|
@britta-wstnr feel free to click the green button (we usually squash and merge nowadays) if you're happy |
|
thx @wmvanvliet @britta-wstnr for the team effort ! |
|
Ok, now we are in good shape to move ahead with #5447 |
5 participants
This fixes a bug in the LCMV beamformer code:
Fix weight normalization when
pick_ori='normal', weight_norm='unit_noise_gain'. The existing implementation has a bug where the weights were first normalized to have unit length before picking the normal direction. Instead, the weights should be normalized to unit length after picking the orientation.