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

FIX: CTF Projector comp #5125

Merged
merged 6 commits into from Apr 22, 2018
Merged

FIX: CTF Projector comp #5125

merged 6 commits into from Apr 22, 2018

Conversation

bloyl
Copy link
Contributor

@bloyl bloyl commented Apr 12, 2018

Closes #5124

When computing ssp projection operators, include the meg_ref channels for data objects that include compensation channels (i.e CTF data), otherwise creating epochs raises an exception.

@bloyl
Copy link
Contributor Author

bloyl commented Apr 12, 2018

A more general approach would be to modify pick_types to take a comp keyword that would include channels listed in info['comps']. That would obviously have wider ramifications.

exclude='bads')
else:
picks = pick_types(my_info, meg=True, eeg=True, eog=True, ref_meg=False,
exclude='bads')
Copy link
Member

Choose a reason for hiding this comment

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

you can just do ref_meg = len(my_info['comps']) > 0; picks = pick_types(..., ref_meg=ref_meg).

Then we'll need a test that 1) proj can actually be calculated for CTF data (this should fail on master), and 2) that the proj do not contain ref channels.

@bloyl
Copy link
Contributor Author

bloyl commented Apr 12, 2018 via email

@larsoner
Copy link
Member

I don't know if there are ECG or EOG, but you can use any MEG/EEG channel as a surrogate if necessary (should be an option in the proj computation function). It doesn't matter if the projectors are correct here, just that they can be computed.

@codecov
Copy link

codecov bot commented Apr 12, 2018

Codecov Report

Merging #5125 into master will increase coverage by 0.08%.
The diff coverage is 93.33%.

@@            Coverage Diff             @@
##           master    #5125      +/-   ##
==========================================
+ Coverage   88.06%   88.15%   +0.08%     
==========================================
  Files         356      357       +1     
  Lines       65303    65805     +502     
  Branches    11128    11198      +70     
==========================================
+ Hits        57511    58008     +497     
+ Misses       4963     4959       -4     
- Partials     2829     2838       +9

@bloyl
Copy link
Contributor Author

bloyl commented Apr 12, 2018

In writing the test I've bumped into something I think is strange.

on master I only get this exception when I use average=True. This means that calls like

epochs = Epochs(raw, events, None, tmin, tmax, baseline=None, preload=True,
                             picks=picks, reject=reject, flat=flat, proj=True)

Don't actually call pick_channels or _pick_drop_channels until the actual data array is read, even if preload=True. Is this the expected behaviour?

@larsoner
Copy link
Member

That sounds like a bug. Perhaps the comps check needs to be replicated in this other code path, too. It's likely that there are some paths like Epochs(..., picks=picks) that don't actually use the pick_channels / _pick_drop_channels path for efficiency reasons.

@bloyl
Copy link
Contributor Author

bloyl commented Apr 12, 2018

What is your advice for this PR?
I say I add the test and then we can merge this and I'll open another issue for the epochs stuff.
Hows that sounds?

@larsoner
Copy link
Member

Sure, fixing one bug at a time can simplify things a bit

@@ -122,4 +125,31 @@ def test_compute_proj_parallel():
bads=['MEG 2443'])
assert_array_almost_equal(projs, projs_2, 10)


def test_compute_proj_ctf():
Copy link
Member

Choose a reason for hiding this comment

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

don't forget the @testing.requires_testing_data decorator here

l_freq=None, h_freq=None,
reject=None, tmax=dur_use,
filter_length=6000)
assert_true(len(projs) == (5 + n_projs_init))
Copy link
Member

Choose a reason for hiding this comment

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

With pytest it's better to do now assert len(projs) == ... directly, it gives nicer error messages than assert_true

@@ -122,4 +125,31 @@ def test_compute_proj_parallel():
bads=['MEG 2443'])
assert_array_almost_equal(projs, projs_2, 10)


def test_compute_proj_ctf():
"""Trivial Test to show that projector code completes on CTF data"""
Copy link
Member

Choose a reason for hiding this comment

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

For consistency "Test to show that ..."

@larsoner larsoner added this to the 0.16 milestone Apr 15, 2018
l_freq=None, h_freq=None,
reject=None, tmax=dur_use,
filter_length=6000)
assert len(projs) == (4 + n_projs_init)
Copy link
Member

Choose a reason for hiding this comment

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

Can you check that none of the compensation channels are in the projs?

@bloyl
Copy link
Contributor Author

bloyl commented Apr 16, 2018

I added a check, although I think the best way would be would be something like

for p in projs:
    if _bad_chans_comp(raw.info, pick_ch_names)[0]:
         raise RuntimeError('something')

with _bad_chans_comp from #5133

Once this is merged I'll add an additional check to that PR.

@larsoner larsoner merged commit 21d35c8 into mne-tools:master Apr 22, 2018
@larsoner
Copy link
Member

Thanks @bloyl

@bloyl bloyl deleted the CTF_comp_projs branch May 3, 2018 13:19
britta-wstnr pushed a commit to britta-wstnr/mne-python that referenced this pull request Jul 5, 2018
* FIX: keep meg_ref channels if comp channels exist

* STYLE: more compact Code

* ENH: added test for ssp computation on CTF data.

* resp to review

* FIX: PEP8

* FIX: check ctf projs are the correct channels
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

2 participants