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

autoextractor for sorting does not accept positional argument for get_unit_spike_train #11

Open
mhhennig opened this issue Jan 15, 2020 · 7 comments
Assignees

Comments

@mhhennig
Copy link
Contributor

This breaks compatibility with several functions in spikeinterface, where the unit_id is given as a positional argument. Could it be changed?

@mhhennig
Copy link
Contributor Author

PS: The same is the case for get_traces() for the recording extractor.

@colehurwitz
Copy link

I agree with Matthias on this one that it should probably take in positional arguments since it would be more consistent with spikeextractors. Also, do you think we should make the machinery of the autorecording extractor a part of spikeextractors? @magland

@magland
Copy link
Collaborator

magland commented Jan 17, 2020

Yes I agree. We definitely want to conform with the requirements of SpikeInterface.

I think AutoRecordingExtractor could eventually go into SpikeExtractors, but we need to think about it some more... right now, this AutoRecordingExtractor has functionality that is very specific to spikeforest, so one might call it SFRecordingExtractor. But the idea is a powerful one -- just point it to some data and the extractor figures out how to read it.

@colehurwitz
Copy link

Yes, I agree with both those points. Maybe you can fix the positional argument thing @magland and I can look at the AutoRecordingExtractor to see what can be translated over!

@magland magland self-assigned this Jan 17, 2020
@magland
Copy link
Collaborator

magland commented Jan 17, 2020

@mhhennig @colehurwitz
should be fixed now.

@colehurwitz
Copy link

Awesome! I think this issue is obsolete then. We can maybe leave it up so we remember about transferring the autoextractors over at some point.

@magland
Copy link
Collaborator

magland commented Jan 17, 2020

I'll close it in a few days - once we are sure it is fixed and that there are no unexpected consequences.

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

No branches or pull requests

3 participants