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

NeuralynxIO: Factor out header handling. #1404

Merged

Conversation

PeterNSteinmetz
Copy link
Contributor

This factors parsing the NlxHeader into parts and provides an option to construct without testing for time and date.

@PeterNSteinmetz
Copy link
Contributor Author

This is failing because the test data are not in ephy_testing_data yet. For that I am waiting on clarification of how to check the files in. See the google groups thread "New workflow with datalad?"

@zm711
Copy link
Contributor

zm711 commented Feb 16, 2024

Regarding your google group message about incorporating some of your local code. Could you describe a bit more what it is doing? I think making things more robust/flexible would be great, but it also needs to fit within the Neo framework. So you can either open an issue to discuss what you're proposing if you have a hunch that your code might be bigger changes to neuralynx. Or you could open a draft PR and see what happens in the test suite with the rest of the code if you incorporate some of your code.

@zm711 zm711 changed the title Factor out header handling. NeuralynxIO: Factor out header handling. Feb 16, 2024
@zm711 zm711 added the IO label Feb 16, 2024
@apdavison apdavison added this to the 0.13.1 milestone Feb 23, 2024
@PeterNSteinmetz
Copy link
Contributor Author

PeterNSteinmetz commented Feb 23, 2024

I added pull request #116 for ephy_testing_data for the dataset which will allow this to pass checks. That pull request on the dataset should be ready to go.

@zm711
Copy link
Contributor

zm711 commented Feb 23, 2024

@PeterNSteinmetz , @samuelgarcia is going to look over everything very soon for your recent PRs so you'll have feedback on everything asap.

@samuelgarcia
Copy link
Contributor

Hi @PeterNSteinmetz.
I remove the new tests for now and I will merge this into main.
So I can start also a PR on top of it.
I let you open a new PR when files are ready.

@samuelgarcia samuelgarcia merged commit 0fcc37f into NeuralEnsemble:master Feb 27, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants