Skip to content

Conversation

@tstenner
Copy link
Contributor

This PR adds a reader for the NeuroElectrics .nedf file format.

Compared to the NEPy package, it's about 70 times faster (due to vectorized reading with numpy.fromfile) and correctly handles (at least) the NEDF 1.3 and 1.4 files.

Once the sample data file (mne-tools/mne-testing-data#80) has been added, I'll add a unit test.

@tstenner tstenner force-pushed the nedf branch 2 times, most recently from ce7c4a4 to 4818496 Compare January 12, 2021 14:32
@larsoner
Copy link
Member

I'll went ahead and merged the testing dataset so that you can add tests -- if we need more data or documentation for the testing file we can always add it later

@larsoner
Copy link
Member

0.113 hash when I downloaded it was ce114ad6d5e3dbed06119386e6b1ce0c

@tstenner tstenner force-pushed the nedf branch 3 times, most recently from 269b555 to 0feb24f Compare January 12, 2021 19:35
Copy link
Member

@larsoner larsoner left a comment

Choose a reason for hiding this comment

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

Looks good! See comments about STIM channels and potential preload support

@tstenner tstenner force-pushed the nedf branch 2 times, most recently from 76f8901 to d64481d Compare January 14, 2021 11:58
@larsoner
Copy link
Member

Okay pushed commits to:

  1. Add _test_raw_reader
  2. Remove assert_equal in favor of plan pytest assert a == b
  3. Remove assert_raises from NumPy in favor of pytest.raises with more targeted matches
  4. Change handling of stim channels to create STI 014 out of the stim data
  5. Add non-preload support
  6. Handle last samples properly (previous version discarded the last 3 samples of data because they didn't fit evenly into the 5-sample standard)

Feel free to take a look and see if it makes sense @tstenner ! Happy to talk through / explain any changes, or fix things that you see are wrong with what I did

@larsoner
Copy link
Member

Also @tstenner can you add your name/URL to doc/changes/names.inc and update doc/changes/latest.inc ? Otherwise LGTM +1 for merge

@tstenner
Copy link
Contributor Author

Much appreciated, it's been a while since I've seen the insides MNE-Python.

Remove assert_equal in favor of plan pytest assert a == b

Ah, good to know. I saw a bunch of assert_equals in the BrainVision reader tests so I adopted this style.

Handle last samples properly (previous version discarded the last 3 samples of data because they didn't fit evenly into the 5-sample standard)

I'm not sure if it's needed. Most of our in-house readers ignore the last nsamples%5 samples, because it's at most 4 samples, i.e. 8 ms and there's always more data at the end than needed because the recording has to be stopped manually. I'd rather have a somewhat simpler implementation (without allocating space for and copying the whole data in 4/5 cases), but that's just my opinion.

@larsoner
Copy link
Member

I'm not sure if it's needed. Most of our in-house readers ignore the last nsamples%5 samples, because it's at most 4 samples, i.e. 8 ms and there's always more data at the end than needed because the recording has to be stopped manually. I'd rather have a somewhat simpler implementation (without allocating space for and copying the whole data in 4/5 cases), but that's just my opinion.

Indeed it's probably overkill in most situations, but if we can read the data correctly rather than omitting some samples 80% of the time I think we might as well do it. This code's one job is to pull/convert the right set of samples from disk, it's a shame to do it for all but the last few :) And ultimately the code to handle it did not end up being too complex.

If efficiency is a concern we can always make the code more efficient in the future, but I'd rather do that later if we find the copy/concatenate pattern to be a limiting factor in real use cases.

@larsoner larsoner merged commit 81051e6 into mne-tools:master Jan 19, 2021
@larsoner
Copy link
Member

Thanks @tstenner !

@tstenner tstenner deleted the nedf branch January 19, 2021 18:27
larsoner added a commit to vpeterson/mne-python that referenced this pull request Feb 25, 2021
* upstream/master: (66 commits)
  MRG, ENH: Add infant template downloader (mne-tools#8738)
  ENH: add reader for NeuroElectrics .nedf files (mne-tools#8734)
  DOC: improve glossary entry about fiducials (mne-tools#8763)
  MRG, ENH: Add Report.add_custom_css (mne-tools#8762)
  BUG, DOC: read_raw_egi didn't support pathlib.Path; update read_raw() docstring (mne-tools#8759)
  Add "dbs" as new channel type (mne-tools#8739)
  MRG, VIZ: Fix title position in plot_sensors (mne-tools#8752)
  MRG: Support for non-FIFF files in Report.parse_folder (mne-tools#8744)
  MRG, VIZ, FIX: sEEG picking in _prepare_topomap_plot() (mne-tools#8736)
  DOC: don't use single letter variable name in _compute_forward (mne-tools#8727)
  WIP: Fix search [skip github] [skip azp] (mne-tools#8742)
  WIP: Compare Beer-lambert to HOMER (mne-tools#8711)
  MRG: bump spyder version (mne-tools#8020)
  FIX anon with IO round trip (mne-tools#8731)
  fix set_bipolar_reference for Epochs (mne-tools#8728)
  WIP: Add width argument, reduce default (mne-tools#8725)
  ENH: Add toggle-all button to Report (mne-tools#8723)
  fix int/float conversion in nicolet header (mne-tools#8712)
  MRG, BUG: Fix Report.add_bem_to_section n_jobs != 1 (mne-tools#8713)
  MRG, DOC: Make "rank" options in docs more accessible (mne-tools#8707)
  ...
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.

3 participants