flatironinstitute / spikeforest_old

SpikeForest -- spike sorting analysis for website
Apache License 2.0
16 stars 7 forks source link

Changes in the default parameter of SpyKING CIRCUS 0.8.0 #58

Closed yger closed 5 years ago

yger commented 5 years ago

Here are some changes to get better results with SpyKING CIRCUS, at least on two GT files that I tested (MEA64c and Kampff). Note that I used version 0.8.0 of SpyKING CIRCUS (git master branch), that I am about to release as soon as I'm done struggling with conda packaging. Basically, I kept a large radius (as we did in our paper), made a mda file wrapper to bypass the export to numpy array, and such that SC can now natively read .mda file format, and slightly increase the automatic merging. I had a look to results on one of your synthetic data file (bionet), and clearly, the software is performing nicely but overclustering, thus leading to "bad" results with your metric. However, for the moment, I don't want to tweak parameters more, because I would like to wait to see performances with a greedy-merge metric, assuming you are going to implement such a thing soon. I also don't know why you had crashes with the software. If they are happening on specific files, maybe you could send me one, such that I can troubleshoot? This should not happen

alexmorley commented 5 years ago

Looks good to me (one minor comment is that I don't think you need to copy the mda file unless you are planning on mutating it, that way it's even better that SC can read the mda format :) )

yger commented 5 years ago

But spyking-circus is overwriting the file when filtering by default... Thus the needed copy. Otherwise, one other solution would be to use the parameters "output_dir" and/or "overwrite" of spyking-circus, to avoid overwriting, but this is equivalent to a copy.

Le ven. 14 juin 2019 à 17:34, Alexander Morley notifications@github.com a écrit :

@alexmorley commented on this pull request.

In spikeforest/spikesorters/spyking_circus/spyking_circus.py https://github.com/flatironinstitute/spikeforest/pull/58#discussion_r293859992 :

 # save binary file

if file_name is None: file_name = 'recording'

  • elif file_name.endswith('.npy'):
  • file_name = file_name[file_name.find('.npy')]
  • raw = recording.get_traces()
  • np.save(join(output_folder, file_name),
  • raw.astype(raw.dtype, order='F'))
  • elif not file_name.endswith('.mda'):
  • file_name = file_name[file_name.find('.mda')]
  • raw = recording.get_traces()

  • np.save(join(output_folder, file_name),

  • raw.astype(raw.dtype, order='F'))

  • shutil.copy(recording._timeseries_path, join(output_folder, file_name + '.mda'))

Unless spyking-circus writes to this file then you don't need to make a copy. Can save quite a lot of time for long recordings and/or when directories are on network-attached storage.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/flatironinstitute/spikeforest/pull/58?email_source=notifications&email_token=AAMYJ7563I3W4TK5R56TJO3P2O27XA5CNFSM4HYHCY7KYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOB3TE57I#pullrequestreview-249974525, or mute the thread https://github.com/notifications/unsubscribe-auth/AAMYJ756IRCVBHIDNIUXVMTP2O27XANCNFSM4HYHCY7A .

magland commented 5 years ago

The spikeforest processors never have the ability to overwrite files in the SF database. That gets taken care of in a layer above.

But with that said, the processor should not modify input files, because the processor may also be used in different contexts.

I will wait for the PyPI package to be available prior to merging.

samuelgarcia commented 5 years ago

@yger : please do also the same change in spiketoolkit/sorters/spykingcircus/

magland commented 5 years ago

I am merging this and am now going to create a new container with SC 0.8.0