Skip to content

NeuralynxIO: Error from no closing time, turned into a warning #1731

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

eduardosand
Copy link

I have some recordings that were spliced from a larger recording. AS such, there is no timeclosed parameter or anything. I don't need to do downstream tasks, so it'd be nice to change the raise error for not having that parameter in the header, to something more like a warning.

@zm711
Copy link
Contributor

zm711 commented Jun 16, 2025

@samuelgarcia are you okay with this behavior?

This looks fine to me as long as Sam is okay with it.

@zm711 zm711 assigned zm711 and samuelgarcia and unassigned zm711 Jun 16, 2025
@zm711 zm711 changed the title Error from no closing time, turned into a warning NeuralynxIO: Error from no closing time, turned into a warning Jun 16, 2025
@zm711
Copy link
Contributor

zm711 commented Jun 30, 2025

@samuelgarcia wanted to ping you again to review this :)

@zm711
Copy link
Contributor

zm711 commented Jul 2, 2025

@samuelgarcia and @PeterNSteinmetz since you are the neuralynx folks would it be okay to have a file without a closing time or does that indicate some sort of corruption we aren't willing to support.

@eduardosand we had a big tweak that @PeterNSteinmetz had done for us. At this point your PR has conflicts once we get approval from Sam or Peter you want to resolve conflicts?

@PeterNSteinmetz
Copy link
Contributor

PeterNSteinmetz commented Jul 2, 2025

Yes, it is fine to not have a closing time. Indeed, most of the early versions of the neuralynx files did not have this field in the header at all.

I suspect the newer header time parsing should tolerate this lack better and may just resolve @eduardosand 's issue.

@zm711
Copy link
Contributor

zm711 commented Jul 2, 2025

Okay @eduardosand want to give this a tweak?

@samuelgarcia
Copy link
Contributor

Hi I am OK to not raise an error for closing but I think the new refactoring of @PeterNSteinmetz already has this behavior.
See https://github.com/NeuralEnsemble/python-neo/blob/master/neo/rawio/neuralynxrawio/nlxheader.py#L335
In short I think this PR is not relevant anymore.

@eduardosand can you test it ? And so we can close this PR.

@zm711 zm711 added this to the future milestone Jul 7, 2025
@zm711
Copy link
Contributor

zm711 commented Jul 7, 2025

We think this is fine (ie other PRs have fixed the issue) so let us know if you're able to test main so we can decide to close this or whether an additional tweak needs to be made. Thanks @eduardosand.

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.

4 participants