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

[NeuralynxRawIO] Clean time parsing #1568

Open
wants to merge 16 commits into
base: master
Choose a base branch
from

Conversation

PeterNSteinmetz
Copy link
Contributor

At long last, replaced the complicated header dictionaries with more general patterns and checks. There are a few more tweaks on this that will be nice but this is a good chunk. As before, depends on the prior chain with NeuraviewTestAndRefactor.

@PeterNSteinmetz
Copy link
Contributor Author

PeterNSteinmetz commented Oct 17, 2024

Sadly the delay in review has now caused there to be conflicts between my others use of the latest master python-neo and these changes. I guess no conflicts with python-neo per se yet.

I think this is the sort of thing that causes a lot of potential contributors to give up.

@PeterNSteinmetz
Copy link
Contributor Author

I should also mention that the change to the BaseRawIO._signal_channel_dtype broke a lot of derived classes that I have. I know that is not part of the public interface but thought I should mention it.

@zm711
Copy link
Contributor

zm711 commented Nov 5, 2024

Hey Peter yeah, sorry the buffer io was something we had planned for quite a spell and needed to get done. Sam was putting all of his effort into that lately. We have one last big buffer based set of changes, but reviewing these PRs is on Sam's todo list. (I remind him to do it :) )

@PeterNSteinmetz
Copy link
Contributor Author

I guess I don't understand the need for buffer_id, even having read the enhancement proposal, but I assume it is important at some level.

I ended up merging the changes in this PR into my master branch forked from the main master. There weren't many conflicts so hopefully that should not be a problem in the future when these changes here are merged into the main master.

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