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

BUG: handle temporal discontinuities in Neuralynx .ncs files #12279

Merged
merged 59 commits into from
Dec 20, 2023

Conversation

KristijanArmeni
Copy link
Contributor

Reference issue

Addresses #12247

What does this implement/fix?

Tries do deal with temporal gaps in .ncs files. Conceptually, read_raw_neuralynx() now checks for any temporal differences between consecutive neo.Block[0].segments start/stop times and if any of those are larger than the sampling period, call it a gap (i.e. missing > 1 sample). Then, infer the number of missing samples for each gap based on gap times, do the bookkeeping, generate arrays of 0's for each gap data segment and insert them between respective valid segments.

I can read-in a large dataset locally (and test_neuralyx.py passes), but now raw.plot() will choke (see more below).

Example behavior on a local dataset where largest discontinuity seems to be 30 msecs (sampling rate = 4kHz).

>>> raw = read_raw_neuralynx(dataset, exclude_fname_patterns=file_patterns, preload=False)
Checking files in /local/neuralynx/dataset/
Ignoring .ncs files:
[...]
N = 418 discontinuous Neo segments detected with delta > 0.00025 sec
(max = 0.030219078063964844 sec, min = 0.00031304359436035156)

Note, that this assumes that all segments returned by Neo are valid data. I tried asking about his in neo github, but haven't heard back.

Additional information: raw.plot() breaks

Essentially, calling .plot with some selection raw.plot(start=10, duration=2) will choke with the output below.

My hunch is the reloading of chunks for plotting somehow interacts with my implementation, where all_data array constructed internally from neo segments + times of inferred gaps, won't match the number of samples of the array generated as block = np.zeros(n_channels, stop-start).

Not sure how to best troubleshoot this (I can't share the data I use locally to reproduce and the testing dataset can't be used for this since it's continuous), but hopefully there's something obvious I'm missing?

Traceback (most recent call last):
  File "/home/kriarm/miniconda3/envs/mnedev/lib/python3.11/site-packages/mne_qt_browser/_pg_figure.py", line 2958, in run
    data_chunk, times_chunk = browser._load_data(start, stop)
                              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/kriarm/git/mne-python/mne/viz/_figure.py", line 321, in _load_data
    return self.mne.inst[:, start : stop + 2]
           ~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^
  File "/home/kriarm/git/mne-python/mne/io/base.py", line 853, in __getitem__
    return self._getitem(item)
           ^^^^^^^^^^^^^^^^^^^
  File "/home/kriarm/git/mne-python/mne/io/base.py", line 860, in _getitem
    data = self._read_segment(start=start, stop=stop, sel=sel)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "<decorator-gen-165>", line 12, in _read_segment
  File "/home/kriarm/git/mne-python/mne/io/base.py", line 478, in _read_segment
    _ReadSegmentFileProtector(self)._read_segment_file(
  File "/home/kriarm/git/mne-python/mne/io/base.py", line 2525, in _read_segment_file
    return self.__raw.__class__._read_segment_file(
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/kriarm/git/mne-python/mne/io/neuralynx/neuralynx.py", line 349, in _read_segment_file
    block[idx] = all_data  # shape = (n_channels, n_samples)
    ~~~~~^^^^^
ValueError: could not broadcast input array from shape (64,990915) into shape (64,990957)

@larsoner
Copy link
Member

larsoner commented Dec 8, 2023

Not sure how to best troubleshoot this (I can't share the data I use locally to reproduce and the testing dataset can't be used for this since it's continuous), but hopefully there's something obvious I'm missing?

First, to cut raw.plot out of the equation, doing raw[:, 2 * int(round(raw.info["sfreq"]))] or so should also replicate the problem. Or maybe even just a raw.load_data() would do it.

Not sure how to best troubleshoot this (I can't share the data I use locally to reproduce and the testing dataset can't be used for this since it's continuous), but hopefully there's something obvious I'm missing?

I would make sure first that raw.load_data() works. This will force you to have a model for the complete set of data, and when something is wrong, it will be clear because you can think of things from sample 0 to sample N-1 of the entire recording. Once that works, you need to map from start, stop into that segment properly. The problem could be with the initial construction of the whole signal, or with the mapping of start, stop into a particular contiguous segment of the whole signal.

@KristijanArmeni
Copy link
Contributor Author

Thanks! Indeed, raw.load_data() gives the same error (I guess I was testing with raw.get_data(), but could be I mixed up something). Anyway, it's more tractable to debug now. Will report back once there's some progress.

@KristijanArmeni
Copy link
Contributor Author

@larsoner Fixed the bug on my end and added commits to detect gaps in Neo segments, infer missing samples, add them as 0's, and annotate as BAD_ACQ_SKIP.

Example gaps

For context, this is a ~40-min recording of story listening.

I cannot comment on whether these look like a typical Neuralynx ring buffer error or not since I have never eye-balled these before. But it seems they can be burst-like, interspersed with a very short (1 or 2 sample) valid samples (this is a 4Khz dataset, so timescale is stretched out to microsecond precision in plots). And these are very short, mostly microsecond-level gaps in the case of my datasets.

sub-006_gaps
sub-006_gaps2

Creating annotations

Right now, I add the all information for annotations in _raw_extras and annotations can be created like so:

gap_annot = raw._raw_extras[0]["gap_annotations"]
annot = mne.Annotations(
    onset=gap_annot["onset"],
    duration=gap_annot["duration"],
    description=gap_annot["descriptions"],
)
raw.set_annotations(annot)

This requires the user to set annotations. Not sure, if it is preferred that annotations should be added under the hood.

@larsoner
Copy link
Member

Yes I think the gaps should be automatically annotated. Then the user can decide what to do with them (including removing them if they want)

@KristijanArmeni
Copy link
Contributor Author

@larsoner read_raw_neuralynx now automatically sets annotations if gaps are detected. See in a32ff97 how its done and if in good order.

Example call and feedback on a local dataset with gaps detected:

raw = read_raw_neuralynx("/path/to/local/dataset",
   ...:                  exclude_fname_patterns=glob_patterns,
   ...:                  preload=False)
Checking files in /path/to/local/dataset
Ignoring .ncs files:
CSC2.ncs
CSC3.ncs
... # prints all files
xBLT1.ncs
Checking for temporal discontinuities in Neo data segments.
N = 418 discontinuous Neo segments detected with delta > 0.00025 sec.
(max = 0.030219078063964844 sec, min = 0.00031304359436035156)
Annotating gaps as BAD_ACQ_SKIP.

In [2]: raw.annotations
Out[2]: <Annotations | 418 segments: BAD_ACQ_SKIP (418)>

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 to me so far! One idea for a potential refactoring and a couple small cleanup ideas

mne/io/neuralynx/neuralynx.py Outdated Show resolved Hide resolved
mne/io/neuralynx/neuralynx.py Outdated Show resolved Hide resolved
mne/io/neuralynx/neuralynx.py Outdated Show resolved Hide resolved
mne/io/neuralynx/neuralynx.py Show resolved Hide resolved
mne/io/neuralynx/neuralynx.py Outdated Show resolved Hide resolved
@KristijanArmeni
Copy link
Contributor Author

@larsoner updated datasets/config.py and added test in e3d4d.

It seems CIs are failing due to some neo import issues (via _soft_import or so?). Looking at the error messages, can't really figure why -- is it anything related to the test script/read_raw_neuralynx? (it passes locally)

Otherwise, a recap: Created 3 gaps (130 invalid samples) in 2 .ncs files, then read them in with read_raw_neuralynx, check that it inferred the correct number of missing samples and test the loaded data against the "ground truth" files (.mat loaded in via scipy).

Also added a line to devel.rst.

@requires_testing_data
def test_neuralynx_gaps():
"""Test gap detection."""
# ignore files with no gaps
Copy link
Member

Choose a reason for hiding this comment

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

This is one problem

Suggested change
# ignore files with no gaps
pytest.importorskip("neo")
# ignore files with no gaps

but a worse problem is that it seems like neo is not being installed on any of those CIs. I'll push a commit

@larsoner
Copy link
Member

Feel free to take out of draft mode if you're done then ping me @KristijanArmeni and I'll merge assuming CIs are green (other than the Windows pip-pre one, which is having unrelated issues)

@KristijanArmeni KristijanArmeni marked this pull request as ready for review December 19, 2023 16:21
@KristijanArmeni
Copy link
Contributor Author

KristijanArmeni commented Dec 19, 2023

Feel free to take out of draft mode if you're done then ping me @KristijanArmeni

Thanks and done! @larsoner

and I'll merge assuming CIs are green (other than the Windows pip-pre one, which is having unrelated issues)

I pulled this latest branch locally and test_neuralynx.py passes fine. This is good for me as far as testing and implementation is concerned -- as far as I can understand the failing CIs are unrelated directly to this PR (but have sth to do with neo install failing?).

@larsoner
Copy link
Member

Some are, for example:

  └─ neo   does not exist (perhaps a typo or a missing channel).

is because I changed environment.yml in this PR to add neo, but used the wrong name. After I push a commit to fix this, all CIs should come back green other than windows pip-pre.

@larsoner larsoner changed the title [WIP] FIX, handle temporal discontinuities in Neuralynx .ncs files BUG: handle temporal discontinuities in Neuralynx .ncs files Dec 19, 2023
@KristijanArmeni
Copy link
Contributor Author

is because I changed environment.yml in this PR to add neo, but used the wrong name. After I push a commit to fix this, all CIs should come back green other than windows pip-pre.

Yeah, looks like the CIs are green, now @larsoner. Thanks! (This branch still needs a merge from main, but other than that it seems good)

@larsoner larsoner enabled auto-merge (squash) December 20, 2023 13:21
@larsoner
Copy link
Member

Updated to latest main and marking for merge when green, thanks in advance @KristijanArmeni !

@larsoner larsoner merged commit 97512a1 into mne-tools:main Dec 20, 2023
27 checks passed
larsoner added a commit to larsoner/mne-python that referenced this pull request Dec 20, 2023
* upstream/main:
  BUG: handle temporal discontinuities in Neuralynx `.ncs` files (mne-tools#12279)
  MAINT: Work around bad SciPy nightly wheels (mne-tools#12317)
  fix 404 link on devel landing page (mne-tools#12316)
  Switch from `epoch_data` to `data` for TFR array functions (mne-tools#12308)
  [pre-commit.ci] pre-commit autoupdate (mne-tools#12307)
  fix icon link colors (mne-tools#12301)
  Bump actions/download-artifact from 3 to 4 (mne-tools#12304)
  Bump github/codeql-action from 2 to 3 (mne-tools#12303)
  Bump actions/upload-artifact from 3 to 4 (mne-tools#12302)
larsoner added a commit to pmolfese/mne-python that referenced this pull request Dec 20, 2023
* upstream/main:
  MAINT: Use towncrier for release notes (mne-tools#12299)
  MAINT: More [ci skip]
  MAINT: Add bot entry [ci skip]
  BUG: handle temporal discontinuities in Neuralynx `.ncs` files (mne-tools#12279)
  MAINT: Work around bad SciPy nightly wheels (mne-tools#12317)
@KristijanArmeni KristijanArmeni deleted the nlx-gaps branch January 2, 2024 20:02
snwnde pushed a commit to snwnde/mne-python that referenced this pull request Mar 20, 2024
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.

2 participants