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

Add extractors for SiNAPS Research Platform #2952

Merged
merged 20 commits into from
Jun 24, 2024
Merged

Conversation

NinelK
Copy link
Contributor

@NinelK NinelK commented May 31, 2024

Contains extractors from binary format (raw + filtered + aux channels) as well as an additional extractor from *.h5 format (only filtered channels)

@zm711 zm711 added the extractors Related to extractors module label May 31, 2024
@alejoe91 alejoe91 added the hackathon-24 Contributions during the SpikeInterface Hackathon May 24 label Jun 1, 2024
@alejoe91
Copy link
Member

alejoe91 commented Jun 1, 2024

@NinelK can you allow edits from maintainers? The pre-commit bot failed to refactor the code according to black

@NinelK
Copy link
Contributor Author

NinelK commented Jun 3, 2024

@NinelK can you allow edits from maintainers? The pre-commit bot failed to refactor the code according to black

it was allowed in the settings on the side, not sure why pre-comit.ci can not push.
I ran black locally, and it seems like this resolves the issue.

from ..preprocessing import UnsignedToSignedRecording


class SinapsResearchPlatformH5RecordingExtractor_Unsigned(BaseRecording):
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we can just apply the unsigned_to_signed in the get_traces, which is simpler to read and will get rid of the complex class interplay. I can implement that

Copy link
Member

Choose a reason for hiding this comment

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

done

@samuelgarcia samuelgarcia added this to the 0.101.0 milestone Jun 12, 2024
@alejoe91
Copy link
Member

@NinelK after discussing with @samuelgarcia, we decided that in this case it's worth adding the unsigned to signed scaling directly to the SinapsH5Extractors (avoiding that class trick, which makes it less readable).

Also moved everything to a single file and cleaned docs and helper functions. Ready to merge on my end

@alejoe91
Copy link
Member

@NinelK can you test it one more time on your side? I don't have h5 test data

@NinelK
Copy link
Contributor Author

NinelK commented Jun 24, 2024

@NinelK can you test it one more time on your side? I don't have h5 test data

I'm on it now

@NinelK
Copy link
Contributor Author

NinelK commented Jun 24, 2024

All works now, just fixed 1 issue with a missing argument

@alejoe91 alejoe91 merged commit fa39967 into SpikeInterface:main Jun 24, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
extractors Related to extractors module hackathon-24 Contributions during the SpikeInterface Hackathon May 24
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants