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

Support Intan "one-file-per-signal" and "one-file-per-channel" formats. #791

Merged
merged 12 commits into from
May 13, 2024

Conversation

h-mayorquin
Copy link
Collaborator

@h-mayorquin h-mayorquin commented Mar 27, 2024

As in the title. We need new releases of spikeinterface and neo for this.

Adding tests here to see if anything else is broken.

Related to issues:
#789

@h-mayorquin h-mayorquin force-pushed the support_intan_other_format branch from e3db627 to 96ec28a Compare March 30, 2024 18:02
@h-mayorquin h-mayorquin self-assigned this Mar 30, 2024
@h-mayorquin h-mayorquin marked this pull request as ready for review May 12, 2024 14:49
@h-mayorquin
Copy link
Collaborator Author

With the release of neo this is ready for review.

@CodyCBakerPhD
Copy link
Member

I thought this was a multi-file style of the format? So shouldn't there be a new interface for Intan that takes folder_path in that case?

@h-mayorquin
Copy link
Collaborator Author

I thought this was a multi-file style of the format? So shouldn't there be a new interface for Intan that takes folder_path in that case?

Good question. The file always has a header info.rhd that contains the available streams and channels with standarized names. So the format always point ot the this info file.

@CodyCBakerPhD
Copy link
Member

Good question. The file always has a header info.rhd that contains the available streams and channels with standarized names. So the format always point ot the this info file.

Oh, I see

Sometimes the .rhd is a header + binary content

Other times it's just .rhd header and binaries live separate?

@CodyCBakerPhD
Copy link
Member

Changes look fine and minimal to me

But what is your suggestion on release timing then? Since tests are passing, this all works with latest SI release, etc.? So we could cut a new release of NeuroConv without issue?

@h-mayorquin
Copy link
Collaborator Author

Are you planning on making a release soon? I think that @alejoe91 can help us to merge SpikeInterface/spikeinterface#2833 before the next release and then we can eliminate the ceiling.

@h-mayorquin
Copy link
Collaborator Author

Good question. The file always has a header info.rhd that contains the available streams and channels with standarized names. So the format always point ot the this info file.

Oh, I see

Sometimes the .rhd is a header + binary content

Other times it's just .rhd header and binaries live separate?

Yes.

@CodyCBakerPhD
Copy link
Member

Are you planning on making a release soon? I think that @alejoe91 can help us to merge SpikeInterface/spikeinterface#2833 before the next release and then we can eliminate the ceiling.

Is the timeline on that in the next 2-3 weeks? It's been a while on the NeuroConv side and the GUIDE also releases in early June so would be nice to keep things synchronized

@h-mayorquin
Copy link
Collaborator Author

I think the confusion is this.

  • We don't need the latest SI release for this. This will work with the current bounds that you have in main.
  • But the current bounds that you have in main will make the dev tests fail.

I guess I will just eliminate the ceiling here but then you will have the dev tests failing.

@CodyCBakerPhD
Copy link
Member

Thanks for clarifying; any ETA on the 'fix' that would otherwise happen on dev branch testing then?

@CodyCBakerPhD CodyCBakerPhD merged commit be17bfa into main May 13, 2024
32 of 36 checks passed
@CodyCBakerPhD CodyCBakerPhD deleted the support_intan_other_format branch May 13, 2024 02:15
@h-mayorquin
Copy link
Collaborator Author

I am not privy to spikeinterface latest plans. I will ask Alessio next time we meet.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants