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

MRG: Add reader functions for FieldTrip data #5141

Merged
merged 268 commits into from Oct 18, 2018

Conversation

Projects
None yet
6 participants
@thht
Copy link
Contributor

commented Apr 18, 2018

This pull request adds reader functions for FieldTrip data as outlined in issue #4833.
It has WIP status because test data needs to merged into mne-testing-data before it can be merged. I already made that PR.
Documentation should follow MNE standards.
tests pass except for test_read_ctf, test_module_nesting and test_plot_evoked_cov. the latter is a locale issue on my machine. no idea about the first two but i doubt it relates to this PR.

@@ -70,6 +70,7 @@ Reading raw data
read_raw_egi
read_raw_fif
read_raw_eximia
read_raw_ft

This comment has been minimized.

Copy link
@agramfort

agramfort Apr 18, 2018

Member

ft -> fieldtrip

it's better to be explicit.

from ..externals import pymatreader


def read_raw_ft(ft_structure_path, data_name='data'):

This comment has been minimized.

Copy link
@agramfort

agramfort Apr 18, 2018

Member

you can use https://pypi.org/project/pep257/ on this file to check the docstring formatting.

you should also use make docstring from the Makefile in the root folder to check them.

"""

ft_struct = pymatreader.read_mat(ft_structure_path, ignore_fields=['previous'], variable_names=[data_name])

This comment has been minimized.

Copy link
@agramfort

agramfort Apr 18, 2018

Member

use flake8 and make flake

This comment has been minimized.

Copy link
@thht

thht Apr 18, 2018

Author Contributor

ok, done

@codecov

This comment has been minimized.

Copy link

commented Apr 18, 2018

Codecov Report

Merging #5141 into master will increase coverage by 0.03%.
The diff coverage is 93.62%.

@@            Coverage Diff             @@
##           master    #5141      +/-   ##
==========================================
+ Coverage   88.44%   88.48%   +0.03%     
==========================================
  Files         363      368       +5     
  Lines       68236    68737     +501     
  Branches    11528    11606      +78     
==========================================
+ Hits        60352    60822     +470     
- Misses       5043     5064      +21     
- Partials     2841     2851      +10
@@ -235,7 +235,7 @@ def _data_path(path=None, force_update=False, update_path=True, download=True,
path = _get_path(path, key, name)
# To update the testing or misc dataset, push commits, then make a new
# release on GitHub. Then update the "releases" variable:
releases = dict(testing='0.46', misc='0.3')
releases = dict(testing='0.47', misc='0.3')

This comment has been minimized.

Copy link
@larsoner

larsoner Apr 18, 2018

Member

The release you need is 0.48 with hash 819c0094f03d494b340d5778cd383a8f

This comment has been minimized.

Copy link
@thht

thht Apr 18, 2018

Author Contributor

thanks!

@@ -4,3 +4,4 @@
from . import jdcal
from . import six
from . import tempita
from . import pymatreader

This comment has been minimized.

Copy link
@larsoner

larsoner Apr 18, 2018

Member

Don't do this import, it will force a hard h5py dependency. Instead later you should be able to do from ..externals.pymatreader import read_mat and it should still work


import mne
import numpy as np
from ..externals import pymatreader

This comment has been minimized.

Copy link
@larsoner

larsoner Apr 18, 2018

Member

then you'll need to nest this import wherever you want to use pymatreader to avoid always importing it (which again will create a hard h5py dependency that we don't want)

@@ -43,6 +43,8 @@
from .eeglab import read_raw_eeglab, read_epochs_eeglab, read_events_eeglab
from .eeglab import read_annotations_eeglab
from .eximia import read_raw_eximia
from .fieldtrip import (read_raw_fieldtrip,read_epochs_fieldtrip,

This comment has been minimized.

Copy link
@larsoner

larsoner Apr 18, 2018

Member

PEP8 space between comma and next value (Travis will also tell you this)


import mne
import numpy as np

This comment has been minimized.

Copy link
@larsoner

larsoner Apr 18, 2018

Member

two newlines before function def

def read_epochs_fieldtrip(ft_structure_path, data_name='data',
trialinfo_map=None,
omit_trialinfo_index=True,
omit_non_unique_trialinfo_index=True):

This comment has been minimized.

Copy link
@larsoner

larsoner Apr 18, 2018

Member

More PEP8 problems -- it's probably worth getting the style checking running locally (and I recommend using an editor with automated PEP8 checking enabled)

.. warning:: Only epochs with the same amount of channels and samples are
supported!
The data is read as it is. Events however are represented entirely

This comment has been minimized.

Copy link
@larsoner

larsoner Apr 18, 2018

Member

This is getting too long. This should be moved to a Notes section.

return custom_epochs


def read_evoked_fieldtrip(ft_structure_path, comment='', data_name='data'):

This comment has been minimized.

Copy link
@larsoner

larsoner Apr 18, 2018

Member

I would make the default comment=None, meaning "use the op.basename of the path" (assuming there is no better default already in the FT .mat file)

@thht

This comment has been minimized.

Copy link
Contributor Author

commented Apr 18, 2018

ok, thanks for all the comments!
i think, i fixed all the issue you brought up. the only comment, github does not mark as "outdated" is the one about the docstring to the test. and that one is added.

  • unittests are green on my side (except for the test_read_ctf failing at some assertion, but that is probably not my issue?)
  • make flake is happy
  • pep257 is happy
  • make docstring is happy
  • sphinx builds and the result looks good

what's next?

@agramfort

This comment has been minimized.

Copy link
Member

commented Apr 18, 2018

@larsoner

This comment has been minimized.

Copy link
Member

commented Apr 18, 2018

You'll also need to add an entry to doc/whats_new.rst

@thht

This comment has been minimized.

Copy link
Contributor Author

commented Apr 18, 2018

@agramfort yes, we are. we are using the in order to build the montage. no idea whether this is the right way or what you are referring to. if not, please give me a hint.

@larsoner yes, i know. but the docs said to do that last, so right before the actual merge. so i will wait until you guys are satisfied and then do it.

@agramfort

This comment has been minimized.

Copy link
Member

commented Apr 18, 2018

@larsoner

This comment has been minimized.

Copy link
Member

commented Apr 18, 2018

The measurement info isn't going to be complete using the DigMontage method. For MEG sensors there is the coil kind and type, which has to do with the kind of channel (MEG, EEG, etc.) and if MEG, which coil it is (e.g., Elekta grad, KIT, etc.). Is this information in the FieldTrip data somewhere? For us it lives in info['chs'][idx]['kind'] and 'coil_type'.

@thht

This comment has been minimized.

Copy link
Contributor Author

commented Apr 19, 2018

alright:

  1. i think i will update io.rst when we are done with the code. now that it looks like we are going to extend it in order to translate sensor types and locations correctly, the outcome of this might have an impact on what to write there.
  2. a general question: can i rebase this branch regularly? or would that break things because of the break in the timeline (dont know how to explain it better...)?
  3. travis is complaining that my test fails when h5py is not present. the way it fails is actually intended behavior (i.e. throw an ImportError). how can i deal with that?
  4. i will do some digging in FieldTrip and MNE to see how channel types, kinds and positions are handled and how we can approach importing that information.

@mne-tools mne-tools deleted a comment from larsoner Apr 19, 2018

from mne.datasets import testing


@testing.requires_testing_data

This comment has been minimized.

Copy link
@agramfort

agramfort Apr 19, 2018

Member

you need to add a decorator saying that this test requires hdf5. See how we do it in other test files for different libraries which are soft dependencies.

@agramfort

This comment has been minimized.

Copy link
Member

commented Apr 19, 2018

@thht yes it's fine to rebase regularly. It's better than merge commits with master.

@thht thht force-pushed the thht:th/add_fieldtrip2mne branch from eea209f to d1a0846 Apr 19, 2018

@thht

This comment has been minimized.

Copy link
Contributor Author

commented Apr 19, 2018

as i said, i am going to improve how channels are handled in the next days. the required unittests should also improve coverage.

one quick question: do i need to take care that only those channels are present in info['chs'] that are actually in the data? i ask this because FieldTrip always maintains the names and positions of all gradiometers and electrodes even if sensors are dropped from the data structure. the channels that are actually present in the data are kept in a list. it looks to equivalent in MNE (info['ch_names'] being the ones actually present, info['chs'] being all there ever have been). is this correct?

@larsoner

This comment has been minimized.

Copy link
Member

commented Apr 19, 2018

Yes we only keep information for the channels that remain

@thht

This comment has been minimized.

Copy link
Contributor Author

commented Apr 19, 2018

@larsoner just to clarify: that means that both 'ch_names' and 'chs' only contain channels present in the data. correct?

@larsoner

This comment has been minimized.

Copy link
Member

commented Apr 19, 2018

Yes. And you shouldn't need to set ch_names yourself but instead populate info['chs'] and call info._update_redundant()

@cbrnr

This comment has been minimized.

Copy link
Contributor

commented Apr 19, 2018

MNE source files don't have copyright clauses AFAIK, I think you should remove these everywhere.

@larsoner

This comment has been minimized.

Copy link
Member

commented Apr 19, 2018

... except the ones in mne/externals, those should stay verbatim copies of the upstream files (if possible)

@thht

This comment has been minimized.

Copy link
Contributor Author

commented Apr 24, 2018

@cbrn yes, they do. see for example mne/io/base.py, mne/io/diff.py these files (and many more) have short copyright notices at the top in the same style i used for the files not in mne/externals.

@cbrnr

This comment has been minimized.

Copy link
Contributor

commented Apr 24, 2018

Not quite, they don't have a Copyright line, only Authors and License lines.

@thht

This comment has been minimized.

Copy link
Contributor Author

commented Apr 24, 2018

@cbrnr oh. yes you are right. sorry i missed that!

@thht

This comment has been minimized.

Copy link
Contributor Author

commented Apr 24, 2018

so, i did some digging concerning channel locations, kinds and coil type.

  1. FieldTrip stores the position of both EEG and MEG sensors in head space by default. Transformation matrices are not present in structures produced by using the normal FieldTrip pipeline. neither is the reference position needed for storing the EEG sensor location in MNE.
  2. the kind of the channel can be translated quite easily.
  3. the type of coil seems to be a bit harder. FieldTrip stores whether it is a magnetometer or planar/axial gradiometer and it has a field that says what system the data originates from (in my case "neuromag306", which seems quite generic as i am quite sure that this does not differentiate between at least the vectorview and the triux system).

to be clear: all the "missing" information can be possibly found in a FieldTrip data structure by digging into the previous configuration options. but they are not always there (i delete them sometimes because they take up lots of space). so this would be very unreliable.

so it basically seems to boil down to this:

  1. is there any way that MNE would be happy with locations in headspace only? without a transformation matrix?
  2. i honestly do not think that i would be able to implement any possible coil-type translation, especially for systems that i do not know. in any case, this translation is never going to be perfect because of the different ways, these information is stored in FieldTrip and MNE.

we could try to implement a "best-effort" solution, translating coil-types as best as possible for Elekta systems. this would accompanied by proper warnings for non- or not-so-well supported systems. this solution could also be extendible so if someone else wants to add his/her system, it would be easy to do so.

what do you think?

@thht thht force-pushed the thht:th/add_fieldtrip2mne branch from 915c125 to 8bb1039 Apr 24, 2018

@larsoner

This comment has been minimized.

Copy link
Member

commented Apr 24, 2018

is there any way that MNE would be happy with locations in headspace only? without a transformation matrix?

There are places we assume that the dev_head_t exists and is correct. I see two potential options:

  1. Try to work backward to estimate the transform using the MEG positions in head coordinates, plus some canonical sensor positions. This would require all MEG systems to be identical, which I doubt is the case for all systems (e.g., KIT).
  2. We set info['dev_head_t'] to be the identity matrix. The only place I can think of that this will be problematic is in maxwell_filter if you specify something in device coordinates. But I think we can check for identity dev_head_t and warn in this case.

i honestly do not think that i would be able to implement any possible coil-type translation, especially for systems that i do not know. in any case, this translation is never going to be perfect because of the different ways, these information is stored in FieldTrip and MNE.

If FieldTrip and MNE can both compute correct forward matrices for the same sensor types, there must be a suitable translation between FieldTrip's notation for coil types and MNE's. Knowing the contents of mne.io.fiff.constants, I expect we could do it.

However, I wouldn't want to saddle you with having to do all this translation. So how about you make it work for Neuromag systems, and for any other system, emit a warning that the coil type is not currently translated properly (and set it to FIFF_COIL_NONE or whatever the correct "unknown" constant is)?

i am quite sure that this does not differentiate between at least the vectorview and the triux system

This seems fine. The MNE sample data (VectorView) has FIFFV_COIL_VV_MAG_T1 and FIFFV_COIL_VV_PLANAR_T1 for the mag and grad channels, as does our TRIUX data in mne-testing-data.

@thht thht force-pushed the thht:th/add_fieldtrip2mne branch from 18d2eb8 to e8967f6 Oct 18, 2018

@thht

This comment has been minimized.

Copy link
Contributor Author

commented Oct 18, 2018

ok, the bug is tested for and fixed. i rebased the branch once more.

@larsoner
Copy link
Member

left a comment

Also please retitle to MRG if you are (as I suspect) done and I'll merge once CIs come back happy

Show resolved Hide resolved mne/io/fieldtrip/tests/test_fieldtrip.py

@thht thht changed the title WIP: Add reader functions for FieldTrip data MRG: Add reader functions for FieldTrip data Oct 18, 2018

@larsoner

This comment has been minimized.

Copy link
Member

commented Oct 18, 2018

Thanks for this great feature @thht! It should help cross-package integration quite a bit

@larsoner larsoner merged commit fea9566 into mne-tools:master Oct 18, 2018

4 checks passed

ci/circleci Your tests passed on CircleCI!
Details
codecov/project 88.48% (+0.03%) compared to 6ec7728
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@agramfort

This comment has been minimized.

Copy link
Member

commented Oct 19, 2018

big thanks @thht !

@thht thht deleted the thht:th/add_fieldtrip2mne branch Oct 19, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.