MRG+1: ENH: refactor beamformer channel picks with utils #5872

britta-wstnr · 2019-01-25T12:18:26Z

Moved some of the channel picking facility to utils:
_check_info_inv (formerly _compute_beamformer._setup_picks) picks channels based on the data / noise cov matrix and forward model, taking into account channels marked as bad in info and reference channels.
Next step for this PR is to write tests for this function.

Two additional functionalities have been added: check for compensation and dropping ref channels during computation of spatial filter (will comment in code to mark those).

ping @agramfort

To Do:

add tests

britta-wstnr · 2019-01-25T12:19:22Z

mne/beamformer/_compute_beamformer.py

+    if not get_current_comp(info) == get_current_comp(forward['info']):
+        raise ValueError('Data and forward model do not have same '
+                         'compensation applied.')
+


@agramfort : this is new, checking whether compensation is identical between data and forward model

britta-wstnr · 2019-01-25T12:20:02Z

mne/beamformer/tests/test_lcmv.py

+    # test whether different compensations throw error
+    info_comp = evoked.info.copy()
+    set_current_comp(info_comp, 1)
+    pytest.raises(ValueError, make_lcmv, info_comp, fwd, data_cov)


@agramfort : this is the test for the compensation comparison

modern style is:

with pytest.raises(ValueError, match='does not match'): make_lcmv(...)

It's clearer what's being caught, and more precise in catching it

britta-wstnr · 2019-01-25T12:21:42Z

mne/utils/check.py

+    ref_chs = pick_types(info, meg=False, ref_meg=True)
+    ref_chs = [info['ch_names'][ch] for ch in ref_chs]
+    ch_names = [ch for ch in ch_names if ch not in ref_chs]
+


@agramfort : this code block is new as well, throwing out any reference channels that might still be in info for the beamformer. Now that we have this function in utils we probably need to reconsider whether this behavior is wanted by any inverse model?

IIRC the forward does not contain the reference channels (only implicitly via compensation if it has been applied when computing the forward) so I don't think it should matter.

Yes, that is true, I acutally checked that. Talking to @agramfort, we thought about getting rid of them to ensure further computation doesn't go wrong in any case for the beamformer.

agramfort · 2019-01-25T14:45:37Z

+1 on this. For others, the idea is have common check functions that will be used across inverse methods that have data, a forward or filters or an inv and eventually some covs.

all inverse methods end up doing similar checks. Let's unify this.

sounds good?

larsoner · 2019-01-25T14:52:16Z

Yes! +1 for as much unification as possible across minimum_norm, beamformer, inverse_sparse, and cov.py for how this stuff is handled.

britta-wstnr · 2019-01-26T18:50:26Z

For test writing: test_check.py does not load any data (yet). _check_info_inv operates on info, forward, noise_cov, and data_cov. Is it worth loading real data for that, or should I rather build some fake dictionaries?

For the use of the new function anywhere but in the beamformer module I would be very glad about some help, as I don't know my way within those functions good enough yet to catch where it would be needed.

agramfort · 2019-01-27T16:36:31Z

no strong feeling about loading real data or not. We tend to use real data everywhere but maybe it would be interesting to consider having a way to mock more objects to speed up the tests by avoiding IO of complex fif files.

larsoner · 2019-01-27T18:27:21Z

I/O is generally not our testing bottleneck, it's computation time.

It's usually easiest and fast enough to load the necessary files from the testing dataset

britta-wstnr · 2019-01-30T17:54:15Z

Based on test_lcmv.py, the data covariance matrix would need to be estimated, but I guess that is no bottleneck either? If okay, I would add all this to test_check

agramfort · 2019-01-30T20:09:59Z

+1

…

codecov · 2019-02-04T13:20:55Z

Codecov Report

Merging #5872 into master will decrease coverage by 0.06%.
The diff coverage is 95.19%.

@@            Coverage Diff             @@
##           master    #5872      +/-   ##
==========================================
- Coverage   88.76%   88.69%   -0.07%     
==========================================
  Files         401      401              
  Lines       72425    72631     +206     
  Branches    12122    12146      +24     
==========================================
+ Hits        64288    64423     +135     
- Misses       5212     5284      +72     
+ Partials     2925     2924       -1

britta-wstnr · 2019-02-05T15:26:26Z

ping @larsoner @agramfort
I implemented some tests now, I think the failing CI is unrelated to my commits?

larsoner · 2019-02-05T15:27:46Z

If you rebase it will probably fix CircleCI

larsoner · 2019-02-05T15:29:00Z

The Travis flake errors are legitimate. They suggest you have some un-covered lines in utils/check.py:

https://travis-ci.org/mne-tools/mne-python/jobs/488493098#L3176

britta-wstnr · 2019-02-05T15:34:30Z

Thanks @larsoner, misread the Travis output. Will fix and rebase!

britta-wstnr · 2019-02-06T16:42:04Z

@larsoner I think Travis timed out ...

larsoner · 2019-02-06T17:29:10Z

All better, ready for review/merge from your end?

mne/utils/tests/test_check.py

larsoner

Minor nitpicks here

mne/beamformer/_compute_beamformer.py

larsoner · 2019-02-06T20:47:12Z

mne/beamformer/tests/test_lcmv.py

+    # test whether different compensations throw error
+    info_comp = evoked.info.copy()
+    set_current_comp(info_comp, 1)
+    pytest.raises(ValueError, make_lcmv, info_comp, fwd, data_cov)


modern style is:

with pytest.raises(ValueError, match='does not match'): make_lcmv(...)

It's clearer what's being caught, and more precise in catching it

mne/utils/check.py

britta-wstnr · 2019-02-07T12:25:47Z

Addressed @agramfort 's comments, will get to @larsoner 's a bit later. Thanks guys!

britta-wstnr · 2019-02-13T18:56:46Z

ping @larsoner : I need help with CIs again ... I don't think it is related to what I did? Should I rebase?

larsoner · 2019-02-13T18:59:52Z

Ignore errors related to physionet download. This will require checking Travis logs. Or wait an hour until #5932 is in, and I can restart Travis (which builds a merged version of the PR, so will get the fixes automatically on build restart).

massich · 2019-02-13T19:14:11Z

@larsoner the failing download is actually this one https://travis-ci.org/mne-tools/mne-python/jobs/490227429#L2490 it does it sometimes.

but yes, right now the CIs are broken. My fault.

britta-wstnr · 2019-02-13T19:25:49Z

Okay, thanks -- then let's see what Travis says later!

larsoner

LGTM +1 for merge

larsoner · 2019-02-13T20:46:28Z

Travis isn't building a merged version (I forgot it still uses the old .travis.yml, and merges the rest of the code), you can rebase, or just ignore the 3.7 build for now.

mne/utils/__init__.py

agramfort · 2019-02-14T08:34:04Z

we could merge this but I would suggest to see if the new _check_info_inv can be shared among all inverse solvers. @larsoner can you have a look?

larsoner · 2019-02-14T15:16:34Z

@agramfort @britta-wstnr I started looking into it. There is a lot of overlap between:

mne.minimum_norm.inverse._prepare_forward plus the depth-prior-computing parts of make_inverse_operator, and
mne.beamformer._compute_beamformer._prepare_beamformer_input plus _check_info_inv (which could actually be refactored to live in _prepare_beamformer_input).

These two code paths seem to do in principle the same things:

check for channel naming consistency
do channel picking of the gain matrix and cov matrix
check comps
compute a depth prior

The best thing to do is probably to merge these things, probably by modifying _prepare_forward as necessary. Let me know if you think it makes sense to do here, which could delay merge for some days or weeks, or if you'd rather see a separate PR.

agramfort · 2019-02-14T15:22:09Z

that's what I feared... no strong feeling. Do you think it would take time to fix?

…

larsoner · 2019-02-14T15:33:38Z

I didn't think rank would take very long but it did. Hopefully only an hour or so, but who knows. Regardless, we should do it.

I'll try to add it here (won't push until it's close) and if it takes too long we can just merge.

larsoner · 2019-02-15T16:14:27Z

I think we should merge this as-is, and I'll tackle unification separately. It's going to need to be a multi-step process, and there's no need to hold this up in the meantime.

massich · 2019-02-15T16:43:49Z

ok to merge as is, and refactor in #5947

britta-wstnr commented Jan 25, 2019

View reviewed changes

agramfort added the mne sprint label Jan 27, 2019

britta-wstnr force-pushed the check_comps_lcmv branch from 953927f to 4f506e7 Compare February 6, 2019 09:19

agramfort reviewed Feb 6, 2019

View reviewed changes

mne/utils/tests/test_check.py Outdated Show resolved Hide resolved

agramfort reviewed Feb 6, 2019

View reviewed changes

mne/utils/tests/test_check.py Outdated Show resolved Hide resolved

agramfort reviewed Feb 6, 2019

View reviewed changes

mne/utils/tests/test_check.py Outdated Show resolved Hide resolved

larsoner reviewed Feb 6, 2019

View reviewed changes

larsoner approved these changes Feb 13, 2019

View reviewed changes

larsoner changed the title ~~Beamformer: refactor channel picks with utils~~ MRG+1: ENH: refactor beamformer channel picks with utils Feb 13, 2019

britta-wstnr added 12 commits February 14, 2019 08:39

add check for ref chans and check comps

439e5da

test compensations

69c9f89

move and rename _setup_picks to _check_info_inv

0d1c4f1

move and rename _pick_channels_spatial_filter

aff00a1

add _get_data function to test file

073515e

fixes for _get_data

97f3a01

add test for channel picking for inverse methods

c2c932a

typo

062322d

fix logging

02c1fde

replace deepcopy

618dfe7

addressing eric's comments

fe4b5be

fix comment

e4ba0d3

britta-wstnr force-pushed the check_comps_lcmv branch from 19cdb47 to e4ba0d3 Compare February 14, 2019 08:16

britta-wstnr commented Feb 14, 2019

View reviewed changes

mne/utils/__init__.py Show resolved Hide resolved

larsoner mentioned this pull request Feb 15, 2019

MRG: Refactor forward and cov prep #5947

Merged

6 tasks

larsoner merged commit 44c2ba4 into mne-tools:master Feb 15, 2019

Uh oh!

MRG+1: ENH: refactor beamformer channel picks with utils #5872

MRG+1: ENH: refactor beamformer channel picks with utils #5872

Uh oh!

Conversation

britta-wstnr commented Jan 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

britta-wstnr Jan 25, 2019

Choose a reason for hiding this comment

Uh oh!

britta-wstnr Jan 25, 2019

Choose a reason for hiding this comment

Uh oh!

larsoner Feb 6, 2019

Choose a reason for hiding this comment

Uh oh!

britta-wstnr Jan 25, 2019

Choose a reason for hiding this comment

Uh oh!

larsoner Jan 25, 2019

Choose a reason for hiding this comment

Uh oh!

britta-wstnr Jan 25, 2019

Choose a reason for hiding this comment

Uh oh!

agramfort commented Jan 25, 2019

Uh oh!

larsoner commented Jan 25, 2019

Uh oh!

britta-wstnr commented Jan 26, 2019

Uh oh!

agramfort commented Jan 27, 2019

Uh oh!

larsoner commented Jan 27, 2019

Uh oh!

britta-wstnr commented Jan 30, 2019

Uh oh!

agramfort commented Jan 30, 2019 via email

Uh oh!

codecov bot commented Feb 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

britta-wstnr commented Feb 5, 2019

Uh oh!

larsoner commented Feb 5, 2019

Uh oh!

larsoner commented Feb 5, 2019

Uh oh!

britta-wstnr commented Feb 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

britta-wstnr commented Feb 6, 2019

Uh oh!

larsoner commented Feb 6, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

larsoner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

larsoner Feb 6, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

britta-wstnr commented Feb 7, 2019

Uh oh!

britta-wstnr commented Feb 13, 2019

Uh oh!

larsoner commented Feb 13, 2019

Uh oh!

massich commented Feb 13, 2019

Uh oh!

britta-wstnr commented Feb 13, 2019

Uh oh!

larsoner left a comment

britta-wstnr commented Jan 25, 2019 •

edited

Loading

codecov bot commented Feb 4, 2019 •

edited

Loading

britta-wstnr commented Feb 5, 2019 •

edited

Loading