Skip to content

Conversation

@KristijanArmeni
Copy link
Contributor

Reference issue

Fixes #12404.

What does this implement/fix?

So far, read_raw_neuralynx() did not propagate online filter information (highpass/lowpass) or acquisition date from the .ncs file headers to relevant RawNeuralynx().info fields. The challenge is that filter params can be different across .ncs files/channels in the dataset.

For each .ncs file, it reads in the lowpass/highpass freqs and then stores, conservatively, the highest highpass freq and the lowest lowpass frequency as the dataset frequencies. If settings are non-uniform across channels and there's a channel with no online filters, then it assumes nyquist as the highpass value and 0 as the lowpass value.

A small test asserts are added in test_neuralynx() to test that info fields are correctly populated on the testing dataset (has online filter applied).

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 reasonable, one idea in there, and don't forget doc/changes/devel/12463.newfeature.rst!

Comment on lines 116 to 120
if not write_meas:
logger.warning(
"Not all .ncs files have the same recording_opened date. "
+ "Not writing meas_date to info."
)
Copy link
Member

Choose a reason for hiding this comment

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

Maybe instead of this you could just use the meas date from the first file if that's well-defined?

Copy link
Contributor Author

@KristijanArmeni KristijanArmeni Feb 26, 2024

Choose a reason for hiding this comment

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

Yeah, I wasn't sure if a strict check to require all .ncs meas_dates to be exactly the same is needed. For example, I see that a real dataset with cca 80 chans happens to have a meas date for one channel off by a second (see snippet below); so it invalidates the strict check.

Not sure if this can happen with acquisition systems and datetime objects (like a numeric precision thing when writing to disk) and it's not an issue or is it unusual? (don't know myself much hands-on about Neuralynx acq systems)

meas_dates # shape = (n_channels,)
array([datetime.datetime(2022, 4, 2, 11, 42, 12),
       datetime.datetime(2022, 4, 2, 11, 42, 12),
       datetime.datetime(2022, 4, 2, 11, 42, 12),
       ...,
       datetime.datetime(2022, 4, 2, 11, 42, 12),
       datetime.datetime(2022, 4, 2, 11, 42, 13),  # <--- a different value, off by a sec
       datetime.datetime(2022, 4, 2, 11, 42, 12),
       ...,
       datetime.datetime(2022, 4, 2, 11, 42, 12)]

Copy link
Member

Choose a reason for hiding this comment

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

I suppose it could be a bit off (?). One option would be use the median time of all files, and warn if that's off by more than say a couple of seconds. Or just always use the first one. Or something else. I think as long as we pick something reasonable and document what it is in the read_raw_neuralynx docstring it's okay

# test that we picked the right info from headers
assert raw.info["highpass"] == expected_hp_freq, "highpass freq not set correctly"
assert raw.info["lowpass"] == expected_lp_freq, "lowpass freq not set correctly"
assert raw.info["sfreq"] == expected_sfreq, "sampling freq not set correctly"
Copy link
Member

Choose a reason for hiding this comment

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

and something about the meas_date?

nlx_reader = NeuralynxIO(dirname=fname)
print(nlx_reader.header)
print(nlx_reader.file_headers.items())

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@larsoner mind having a look at this docstring in read_raw_neuralynx()? I'm formatting something wrong, but can't figure out what. Ruff doesn't detect anything locally.

With this commit, it's failing like so:

FAILED mne/tests/test_docstring_parameters.py::test_docstring_parameters - AssertionError: 2 errors found:
mne.io : mne.io.neuralynx.neuralynx.read_raw_neuralynx : GL02 : Closing quotes should be placed in the line after the last text in the docstring (do not close the quotes in the same line as the text, or leave a blank line between the last text and the quotes)
mne.io : mne.io.neuralynx.neuralynx.read_raw_neuralynx : GL03 : Double line break found; please use only one blank line to separate sections or paragraphs, and do not leave blank lines at the end of docstrings

But if I delete the blank line (l. 68), then I think sphinx will break when building docs and I see this log (full CircleCI link):

/home/circleci/project/mne/io/neuralynx/neuralynx.py:docstring of mne.io.neuralynx.neuralynx.read_raw_neuralynx:64: WARNING: Explicit markup ends without a blank line; unexpected unindent.

Copy link
Contributor Author

@KristijanArmeni KristijanArmeni Feb 27, 2024

Choose a reason for hiding this comment

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

Ah, wait, I think I see it. I guess I need to indent the code chunk after .. code-block:: python

Co-authored-by: Eric Larson <larson.eric.d@gmail.com>
KristijanArmeni and others added 2 commits February 27, 2024 14:55
Co-authored-by: Eric Larson <larson.eric.d@gmail.com>
@KristijanArmeni
Copy link
Contributor Author

@larsoner docs now passing, thanks! I merged main here, but the pip-pre tests keep failing, seems like these are unrelated to this PR, but can't determine for sure.

@larsoner
Copy link
Member

but the pip-pre tests keep failing, seems like these are unrelated to this PR, but can't determine for sure.

I'll push a little commit to this PR to save some CI cycles

@larsoner larsoner enabled auto-merge (squash) February 28, 2024 14:29
@larsoner
Copy link
Member

Pushed a tiny commit to use rST link rather than raw http link and marked for merge when green, thanks in advance @KristijanArmeni !

@KristijanArmeni
Copy link
Contributor Author

KristijanArmeni commented Feb 28, 2024

Pushed a tiny commit to use rST link rather than raw http link and marked for merge when green, thanks in advance @KristijanArmeni !

Great, thanks for helping @larsoner !

@larsoner larsoner merged commit f2fa901 into mne-tools:main Feb 28, 2024
@KristijanArmeni KristijanArmeni deleted the nlx_info branch March 1, 2024 19:40
snwnde pushed a commit to snwnde/mne-python that referenced this pull request Mar 20, 2024
…euralynx.info` (mne-tools#12463)

Co-authored-by: Eric Larson <larson.eric.d@gmail.com>
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.

preserve (select) header information from .ncs files in RawNeuralynx

2 participants