CINPLA / expipe-cinpla-legacy

All that was expipe and more
GNU General Public License v3.0
2 stars 2 forks source link

Rectified MUA signal calculation etc. #8

Closed espenhgn closed 5 years ago

espenhgn commented 6 years ago

Not ready to merge!

Working on incorporating Kilosort as spike sorter with our processing step. Requirements:

Adds a new expipe openephys process <action_id> --kilosort option (default is False) that exists side by side with older options e.g., for klusta. Saves band-pass filtered data on int16 flat binary format, scaled by a factor 1/0.195 as in raw data (preserves unit of raw data). The output can be investigated in phy using the template-gui:

cd <~>/expipe_temp_storage/<project_name>/<action_id>/main.exdir/acquisition/<folder_name>
phy template-gui params.py

Issues:

espenhgn commented 6 years ago

Hi. I didn't wan't to rewrite the klusta parts and remnants of spyking circus parts yet, so these args are just placeholders for now. A single argument --spikesorter with choices between klusta, kilosort etc., would be better, with --no-spikes renamed --convert-spikes (less ambiguous).

As for the output, for kilosort it doesn't appear to make sense to associate a unit to a channel group as has been done for klusta output. What was the logic behind that originally? Precedence in e.g., NWB? With kilosort, you do get a file with all templates after the initial sorting, but the manual step in Phy (merging/splitting) does not update these files, hence the templates must be recomputed and associated with a particular group in a latter step. I think for my use case, I would look at the correlation between a unit and every other LFP channel anyway and am not convinced that it makes sense to a priori set a location for the unit then. I think it is more logical to put UnitTimes in parallel with the channel_group entries. Thoughts?

espenhgn commented 6 years ago

Just a few comments on the last commit(s):

This should add more flexibility by disentangling preprocessing from chosen spike sorter. Usage examples:

--filter-method klusta can be used with klusta, but do we need that option?

Klusta-specific arguments have klusta in them, but I would prefer a syntax like expipe openephys process <action_id> .... --spikesorter-args arg0 value0 arg1 value1 ... but could not find an example on how to do this with click.

espenhgn commented 5 years ago

@lepmik @alejoe91 I will merge this ASAP as the process cli stuff is now in a separate repo @ github.com/CINPLA/COBRA_visual_ephys

lepmik commented 5 years ago

would be better if you made a new PR since most of the history is now only relevant for visual-ephys? Not entirely clear to me what this PR is about anymore..

espenhgn commented 5 years ago

I retitled the PR. By now, it is mostly a function to compute rectified MUA signals and some minor fixes needed for my stuff.

lepmik commented 5 years ago

It's not the title that matters, it's all the commits which does not belong in expipe-cinpla but in visual-ephys. Moreover, some (most) of the commits have a HUGE history of filedeletion etc. so it's really hard for me to get a overview of what will change when merging this PR!?

Regarding changes in pyopenephys, they should be committed in that repository since it is standalone and we only pull updates from that repo (until it is stable and then we will use pypi to install it)

I suggest you make a new branch from expipe-cinpla and add the changes to expipe-io-neuro, and other changes if they are relevant and omit those to pyopenephys which should be PR'd there.

lepmik commented 5 years ago

Hey! why do you merge without agreement? Bad behavior!!!