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

Fix spike finding #21

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

corinneteeter
Copy link

@campagnola here is the current code; 53 failed, 519 passed. This pull request so that you can checkout the code (and my current data test set) and look at my current failures which I had difficulty making calls manually and want more eyes on to see if worth it. The current clamp is pretty good. Voltage clamp could have a bit more time.

@@ -95,45 +95,175 @@ def zero_crossing_events(data, min_length=3, min_peak=0.0, min_sum=0.0, noise_th

return events

def deal_unbalanced_initial_off(omit_ends, on_inds, off_inds):

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Start function names with _ if you don't intend then to be used externally.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do.

data = trace.data
data1 = data - baseline
# convert threshold array
if isinstance(threshold, float):
threshold = np.ones(len(data)) * abs(threshold)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why convert to array? this just slows down the comparisons that come later.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made it so that the input threshold could be an array so that it doesn't need to be constant (see line 130). For current clamp I alter the threshold across the duration of the stimulation pulse. (See spike_detection.py line 154)

masks = [(data1 > threshold).astype(np.byte), (data1 < -threshold).astype(np.byte)]
## find all positive and negative threshold crossings of baseline adjusted data

if debug:

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not so keen on merging debugging code unless there's a good reason to keep it here?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I figured, you would want it removed, but I didn't want to do it before you looked at the rest of the code incase I needed to look at stuff again. The debug plots are particularly insightful for your questions below about removing the +1 on line 191. Once you are happy it can be removed before merge along with the 'debug' argument into the functions.

# TODO: It might be a good idea to make offindexes one less so that region above threshold looks symmetrical
# find start and end inicies (above threshold) where waveform is above threshold
on_inds = list(np.argwhere(diff==1)[:,0] + 1) #where crosses from below to above threshold Note taking index after this happens
off_inds = list(np.argwhere(diff==-1)[:,0]) #where crosses from above to below threshold. Note taking index before this happens

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you remove the +1 here? The idea is you could take data[on_ind:off_ind] and get exactly the region above threshold.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The last index was actually below the threshold and I only wanted to use indexes above threshold. The +1 includes an area and a value below threshold in the sum and the area.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will also remove the #TODO on 188 as I did implement that here.




# sometimes an event happens at the beginning of the pulse window and the trace hasn't

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a generic function for detecting threshold crossings; not a good place for comments that require an understanding of your specific use case.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can make this statement more generic. But this if statement is here for the case when an event happens at the beginning.

if on_inds[i] == off_inds[i]:

# remove any point where an on off is seperated by less than 2 indicies
if (on_inds[i] + 1) == off_inds[i]:

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why? Again, this is a generic threshold-crossing function; we shouldn't make assumptions about the use case here.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was useful in the downstream stream code, I can pull it out and put in the other function.

Copy link
Author

@corinneteeter corinneteeter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@campagnola I tried to use the new event_detection code I checked out from master with the changes you made. I am not sure why but the changes you made to master are not showing up in this PR but you can see them on the master branch if opened outside not this PR.... In any case at https://github.com/aiephys/neuroanalysis/blob/master/neuroanalysis/event_detection.py#L118 causes an out of range index error. So I used my code again.

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

Successfully merging this pull request may close these issues.

2 participants