search

flatironinstitute / spikeforest2

SpikeForest -- spike sorting analysis for website -- version 2
Apache License 2.0
34 stars 12 forks source link

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

Open mhhennig opened 4 years ago

mhhennig commented 4 years ago

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

mhhennig commented 4 years ago

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

colehurwitz commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

@mhhennig @colehurwitz should be fixed now.

colehurwitz commented 4 years ago

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 commented 4 years ago

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