MRG, BUG: Refactor beamformer code #7656

larsoner · 2020-04-22T14:20:54Z

codecov · 2020-04-22T16:49:59Z

Codecov Report

Merging #7656 into master will increase coverage by 0.03%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #7656      +/-   ##
==========================================
+ Coverage   90.12%   90.15%   +0.03%     
==========================================
  Files         455      455              
  Lines       83836    84149     +313     
  Branches    13262    13368     +106     
==========================================
+ Hits        75557    75867     +310     
+ Misses       5413     5412       -1     
- Partials     2866     2870       +4

britta-wstnr · 2020-04-22T18:43:07Z

@larsoner The test looks fine to me, thanks for putting it together! The vector case will be a whole other story I fear but let's discuss this later - let's try and get the scalar version green for this one first!

britta-wstnr · 2020-04-27T12:25:53Z

So the test fails (at least) for the two filters ('unit-noise-gain' and 'unit-gain' filters) not relying on the same lead field if the orientation is picked.
We are not storing the updated lead field in the filters object - maybe we should? Furthermore, it would also make sense to store the selected orienation in that case (max_power_ori in our code).

larsoner · 2020-04-27T13:24:53Z

Can the orientation picking be done last, after all other computations? If so, then maybe we shouldn't even bother doing it during filter creation but rather during apply_lcmv?

britta-wstnr · 2020-04-27T13:27:16Z

No, it should ideally be done before the filter computation and then we pick the lead field and recompute the filter.

larsoner · 2020-04-27T13:29:06Z

Why even bother storing the leadfield in the filters object?

britta-wstnr · 2020-04-27T13:32:39Z

Well in this case it would come in handy for the test. I do not know how many use-cases there are, one argument for it is that we change the lead field if we do orientation selection. The plotting of the orientation itself is sometimes done, to get an overview of which orientations the filter uses, so that would in any case be useful (and would give the actually used lead field as well, if needed).

larsoner · 2020-04-27T13:55:56Z

+1 for storing the selected orientations (surprised we didn't already do this?). Whether you store the entire leadfield or the selected one is up to you. The selected one is just the np.dot in the direction of the orientation selection for each vertex, right? I guess you have more flexibility if you store the entire leadfield and let people do the dot when they want it ... ?

BTW would it be possible at some point to provide the "variance explained" like we do for minimum norm and mixed norm solvers? Probably not for this PR but a follow-up one, it would be nice to have.

mne/beamformer/_compute_beamformer.py

* upstream/master: DOC: Order added reference FIX: Working working version ENH: More efficient actually working csd, needs review MAINT: Update dataset and add constant test (mne-tools#7866) DOC: update link to glasser supplementary info; convert to footbib (mne-tools#7864) FIX: Fix subtract_evoked with decim (mne-tools#7855) FIX: Fix reading of old TFRs (mne-tools#7851) DOC: Add evoked movecomp to example (mne-tools#7852) Deprecate meg=True in pick_types (mne-tools#7823) ENH: Allow reading broken file (mne-tools#7846) MRG, ENH: Add mixed source estimate support to compute_source_morph (mne-tools#7734) FIX: Fix bug with metadata and event_repeated (mne-tools#7733) MRG, ENH: Add axes to plot_evoked_white (mne-tools#7831) MRG, ENH: Add example of projection to source space (mne-tools#7705)

* upstream/master: (24 commits) WIP: Fix Travis (mne-tools#7906) WIP: Prototype of notebook viz (screencast) (mne-tools#7758) MRG, FIX: Speed up I/O tests, mark some slow (mne-tools#7904) Proper attribution for Blender tutorial (mne-tools#7900) MAINT: Check usage [ci skip] (mne-tools#7902) Allow find_bad_channels_maxwell() to return scores (mne-tools#7845) Warn if NIRx directory structure has been modified from original format (mne-tools#7898) Pin pvyista to 0.24.3 (mne-tools#7899) MRG: Add support for reading and writing sufaces to .obj (mne-tools#7824) Fix _auto_topomap_coords docstring. (mne-tools#7895) MRG, FIX: Ensure Info H5-writeable (mne-tools#7887) Website contents (mne-tools#7889) MRG, ENH: Add mri_resolution="sparse" (mne-tools#7888) MRG, ENH: Allow disabling FXAA (mne-tools#7877) remove "and and" [ci skip] (mne-tools#7882) fix evoked nave → inverse guidance (mne-tools#7881) ENH: Better error messages (mne-tools#7879) FIX : EDF+ Annotation Timestamps missing sub-second accuracy (mne-tools#7875) FIX: Fix get_channel_types (mne-tools#7878) MRG, BUG: Fix combine evokeds (mne-tools#7869) ...

* upstream/master: MRG, ENH: Add method to project onto max power ori (mne-tools#7883) WIP: Warn if untested NIRX device (mne-tools#7905) MRG, BUG: Fix bug with volume morph and subject_to!="fsaverage" (mne-tools#7896) MRG, MAINT: Clean up use of bool, float, int (mne-tools#7917) ENH: Better error message for incompatible Evoked objects (mne-tools#7910) try to fix nullcontext (mne-tools#7908)

britta-wstnr

@larsoner I updated the FieldTrip data according to FieldTrip's newest release (and fixed some resulting deprecation warnings in our script while I was already on it, see here: mne-tools/mne-scripts#24).
For me, test_external passes with this data.

I still get 3 errors for the test_localization_bias_free, but I feel you have the better overview there right now.

I in principle fixed the W @ G.T test for the Sekihara unit-noise-gain vector beamformer, just did not know how to implement it proper (see review).

Close to merging, maybe? 😄

britta-wstnr · 2020-07-02T16:40:07Z

mne/beamformer/tests/test_lcmv.py

+        # got = w @ use_G.swapaxes(-2, -1)
+        # s = np.linalg.svd(got, compute_uv=False)
+        # theta = np.repeat(s[..., :1], s.shape[-1], axis=-1)
+        # assert_allclose(s, theta, rtol=1e-1, err_msg='G.T @ w = θI')


@larsoner This is not right I think - it is easier than you tested for.
w @ G.T should not have a constant on the diagonal, it just should be a diagonal matrix. This holds for me in the test above:
np.allclose(np.diag(np.diag(got[0])), got[0])

I just did not know how to do this on our 3-dimensional matrix ....

Last I checked it, it wasn't diagonal I don't think. But I can look early next week. If you want to try something, feel free to just make it a simple loop and I can vectorize the check later

* upstream/master: (30 commits) MRG: Add remove_labels to _Brain (mne-tools#7964) Add get_picked_points (mne-tools#7963) ENH: Add OpenGL info to mne sys_info (mne-tools#7976) [MRG] Fix reject_tmin and reject_tmax for reject_by_annotation in mne.Epochs (mne-tools#7967) mrg: Add scalar mult and div operators for AverageTFR (mne-tools#7957) MRG, MAINT: Cleaner workaround for Sphinx linking issue (mne-tools#7970) MRG, ENH: Speed up epochs.copy (mne-tools#7968) MRG, BUG: Allow ref mags to have a comp grade (mne-tools#7965) do not forget to pass adjacency (mne-tools#7961) [MRG] fix Issue with stc.project after restricting to a label (mne-tools#7950) Only process nirx event file if present (mne-tools#7951) MRG+1: BUG: info['bads'] order shouldn't matter in write_evokeds() (mne-tools#7954) Fix some small glitches introduced via mne-tools#7845 (mne-tools#7952) Add time player (mne-tools#7940) MAINT: Clean up VTK9 offset array [circle front] (mne-tools#7953) MAINT: Skip a few more on macOS (mne-tools#7948) fix links [skip travis] (mne-tools#7949) MRG, MAINT: Tweak CIs (mne-tools#7943) MRG, BUG: Fix vector scaling (mne-tools#7934) MRG, VIZ, BUG: handle CSD channel type when topo plotting (mne-tools#7935) ...

larsoner · 2020-07-08T16:44:48Z

@agramfort do you want to see if the changes make sense to you based on the doc updates, etc.?

agramfort

I tried this branch on some phantom data and I don't see any regression on the LCMV side.

@britta-wstnr it's some data from beamformer NI paper

wmvanvliet

Merge when green!

* upstream/master: (21 commits) MRG: Add SSP projectors to Report (mne-tools#7991) BUG: Fix warning (mne-tools#8006) WIP: TFR Doc/test changes (mne-tools#7998) MAINT: Remove numpydoc test on 3.6 (mne-tools#8005) MAINT: Better error message for mismatch (mne-tools#8007) MRG: Allow removal of active projectors if channels they applied to have meanwhile been dropped (mne-tools#8003) MRG Freesurfer coordinate frame tutorial (mne-tools#7578) FIX: Fix stockwell checks (mne-tools#7996) MRG, ENH: Add array-spacing plugin and reorganize deps (mne-tools#7997) MRG, ENH: Reduce memory usage of Welch PSD (mne-tools#7994) STY: One more [ci skip] STY: Docstyle [ci skip] Report parsing (mne-tools#7990) MRG, ENH: BrainVision impedance parsing (mne-tools#7974) BUG: Fix missing source space points (mne-tools#7988) [MRG] Strip base directory name from Report captions when using parse_folder (mne-tools#7986) DOC: Update estimates [skip travis] (mne-tools#7987) DOC: Try to improve find_bad_channels_maxwell doc (mne-tools#7982) VIZ, BUG: fix tickmarks in evoked topomap colorbar (mne-tools#7980) Add low-pass filter to find_bad_channels_maxwell() (mne-tools#7983) ...

larsoner · 2020-07-14T18:48:11Z

Thanks @britta-wstnr @wmvanvliet !

agramfort · 2020-07-14T19:05:10Z

Congrats 🎉 🍾🍻

wmvanvliet · 2020-07-14T19:35:59Z

Whee!

larsoner added 3 commits April 22, 2020 10:20

TST: Add failing test

9393b91

FIX: Tweak

4535112

FIX: Flake

5ed2f69

larsoner added 3 commits April 30, 2020 17:27

MAINT: Refactor into steps

e843bbd

FIX: Get things green

a5f958b

FIX: Just max power is bad

b947dfa

larsoner force-pushed the refactor branch from 1123f14 to b947dfa Compare May 1, 2020 01:30

FIX: Cleaner

cc5a737

larsoner force-pushed the refactor branch from 658a826 to cc5a737 Compare May 1, 2020 01:42

larsoner changed the title ~~TST: Add failing test~~ WIP: Refactor beamformer code May 1, 2020

larsoner and others added 9 commits April 30, 2020 21:55

FIX: Cleaner logic

a2790d2

FIX: xfail

8a6191a

FIX: Tol

fdcce12

WIP: Not nec pos semi

21d11f8

FIX: More

1efab06

FIX: No reg fixed

d75a2c9

return max-power-ori

efde559

return max-power-ori

8902fa6

checking for filter equivalence

4805d82

britta-wstnr reviewed May 8, 2020

View reviewed changes

mne/beamformer/_compute_beamformer.py Outdated Show resolved Hide resolved

britta-wstnr reviewed May 8, 2020

View reviewed changes

mne/beamformer/_compute_beamformer.py Show resolved Hide resolved

first pass on refactoring _compute_beamformer

dcf5f0c

Alex and others added 3 commits June 5, 2020 14:13

added reference

0a391bf

DOC: Order

a6ea90c

agramfort force-pushed the master branch from 4bfeb85 to 031875d Compare June 8, 2020 06:47

larsoner added 3 commits June 17, 2020 13:33

WIP: Sekihara

23a4b2e

britta-wstnr mentioned this pull request Jul 2, 2020

Update beamformer testing data after changes in FieldTrip mne-tools/mne-testing-data#66

Merged

britta-wstnr reviewed Jul 2, 2020

View reviewed changes

larsoner added 3 commits July 8, 2020 11:21

ENH: Add diagonal matrix test

dd483de

MAINT: Better tests

6f52db6

larsoner changed the title ~~WIP, BUG: Refactor beamformer code~~ MRG, BUG: Refactor beamformer code Jul 8, 2020

larsoner added this to the 0.21 milestone Jul 8, 2020

larsoner and others added 2 commits July 8, 2020 13:04

DOC: Cleaner docs [ci skip]

faf27ca

Improve tests for inversion='single' case

17b6544

agramfort approved these changes Jul 9, 2020

View reviewed changes

STY: Flake

d20319f

larsoner mentioned this pull request Jul 14, 2020

API: Confirm best defaults for LCMV/DICS before 0.21 #8008

Closed

STY: Rename

bbd89ce

wmvanvliet approved these changes Jul 14, 2020

View reviewed changes

larsoner added 2 commits July 14, 2020 13:13

FIX: Tol

4a122ec

larsoner merged commit 1391ad2 into mne-tools:master Jul 14, 2020

larsoner deleted the refactor branch July 14, 2020 18:48

larsoner mentioned this pull request Oct 1, 2023

Beamformer: Selection of orientation that optimizes output power. #7225

Closed

Uh oh!

MRG, BUG: Refactor beamformer code #7656

MRG, BUG: Refactor beamformer code #7656

Uh oh!

Conversation

larsoner commented Apr 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Apr 22, 2020

Codecov Report

Uh oh!

britta-wstnr commented Apr 22, 2020

Uh oh!

britta-wstnr commented Apr 27, 2020

Uh oh!

larsoner commented Apr 27, 2020

Uh oh!

britta-wstnr commented Apr 27, 2020

Uh oh!

larsoner commented Apr 27, 2020

Uh oh!

britta-wstnr commented Apr 27, 2020

Uh oh!

larsoner commented Apr 27, 2020

Uh oh!

Uh oh!

Uh oh!

britta-wstnr left a comment

Choose a reason for hiding this comment

Uh oh!

britta-wstnr Jul 2, 2020

Choose a reason for hiding this comment

Uh oh!

larsoner Jul 2, 2020

Choose a reason for hiding this comment

Uh oh!

larsoner commented Jul 8, 2020

Uh oh!

agramfort left a comment

Choose a reason for hiding this comment

Uh oh!

wmvanvliet left a comment

Choose a reason for hiding this comment

Uh oh!

larsoner commented Jul 14, 2020

Uh oh!

agramfort commented Jul 14, 2020 via email

Uh oh!

wmvanvliet commented Jul 14, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

larsoner commented Apr 22, 2020 •

edited

Loading