SpikeInterface / spikeinterface

A Python-based module for creating flexible and robust spike sorting pipelines.
https://spikeinterface.readthedocs.io
MIT License
533 stars 187 forks source link

Absolute path to recording isn't saved when using export_to_phy #2437

Closed ikharitonov closed 10 months ago

ikharitonov commented 10 months ago

In case of using _export_topy function and not copying the binary recording (copy_binary=False), "None" gets saved in params.py instead of the absolute path. Seems to happen because _waveformextractor.recording does not necessarily have to be a BinaryRecordingExtractor (e.g. in my case it is a CommonReferenceRecording) when checking for isinstance(waveform_extractor.recording, BinaryRecordingExtractor), hence the _recpath does not get extracted.

https://github.com/SpikeInterface/spikeinterface/blob/c2f18bd536437e9f500a4e24f7a0f3773bd79d3a/src/spikeinterface/exporters/to_phy.py#L144C55-L144C79

alejoe91 commented 10 months ago

That is the correct behavior. Sonce you recording is not binary, if you want the .dat file to be available to Phy you need copy_binary=True, and this will save the processed data in the Phy folder

ikharitonov commented 10 months ago

But I am using a binary recording in my case, just with some preprocessing applied on top (e.g. common reference)

zm711 commented 10 months ago

@ikharitonov,

The preprocessing steps are lazy. So your preprocessed recording object does not yet have have the preprocessing applied to it, but instead keeps an internal list of all the preprocessing which should be applied. The copy_binary=True will take all the preprocessing and apply it in parallel as it is writing the binary file (this can take a while because it is no longer lazy) which will give you a new binary file with the preprocessing applied. Your old binary file is still the same without any of the preprocessing applied to it yet. So in this case you want to copy_binary=True to make sure the binary file you are getting has all of the operations that you've performed on it included.

Does that distinction make sense?

ikharitonov commented 10 months ago

@zm711 that does make sense, thank you!