Repository navigation
Conversation
…dd a test, and add a numba version matching the count matrix impl because this would otherwise require a hot python loop
cwindolf
added
bug
| lab_st1[ind_st1] = "TP" | ||
| lab_st2[ind_st2] = "TP" | ||
| mapped_st = sts2[u2][seg_index] | ||
| assert np.all(np.diff(sts) >= 0) and np.all(np.diff(mapped_st) >= 0) |
There was a problem hiding this comment.
I don't think this is a danger. Is there a good reason to have it?
| assert len(np.unique(ind_st2)) == len(ind_st2) | ||
| lab_st1[ind_st1] = "TP" | ||
| lab_st2[ind_st2] = "TP" | ||
| mapped_st = sts2[u2][seg_index] |
There was a problem hiding this comment.
| mapped_st = sts2[u2][seg_index] | |
| mapped_st = sorting2.get_unit_spike_train(u2, seg_index) |
We now have a good caching system for spike trains, so we don't need to do the copying trick on lines 677->678, and can directly use this for better readability.
Also mapped_st is a very confusing name.
| from numba import jit | ||
|
|
||
| @jit(nopython=True, nogil=True) | ||
| def compute_pair_match_indices(spike_frames1, spike_frames2, delta_frames): |
There was a problem hiding this comment.
I think we should generally change the nomenclature from frames to samples to match core spikeinterface language.
chrishalcrow
left a comment
chrishalcrow
left a comment
There was a problem hiding this comment.
The actual math/algorithm looks great, and I approve of that.
But this bit of code is very old, and is using language that is out of date with the rest of SpikeInterface (or at least the newer bits of it...). Would be great if we could bring it up to date.
|
Thank you Chris! Will fix that up and let you know. Appreciate your review! |
Hi!
So, I gather that many have moved on to just using the count matrix for all comparisons, but I am still a devoted user of
do_score_labels(). For a long long time, I have had the asserts which check that spikes are only matched once across sortings commented out, because they did not hold with the previous sorting-based implementation. I have added a test which fails on the old version that illustrates the issue. (Note: the same issue, I think, is present in the count matrix, but I am not addressing that here. There is a note about that in the test I added, the count matrix would fail one of the cases if tested.)I eventually decided to just write a numba loop that makes sure to only match each spike once. It's pretty similar to the one in the count matrix function, with minor changes to ensure unique single matches. I followed the style of that function's numba caching.
I would note: numba is not listed as a dependency in the
"comparison"group in pyproject.toml, but I think it is one in practice, since the count matrix uses it too with no Python fallback? I can add that here if helpful.Wondering if this change feels okay? Is this score labeling used by many? I believe the result should not change except in good ways / edge cases as a result of this implementation.
Thanks!!