Implement cache spike vector for phy sorting reader

[samuelgarcia]

Try to implement the spike vector caching for phy reader in a faster way than #4502

@alejoe91 @grahamfindlay : the implementaion of the caching was slow mainly dur the final lexsort which is normaly not need.

Perfs for the file provide by Graham: (400Mspikes 342 units)

read_phy() + sorting.to_spike_vector() + sorting.to_reordered_spike_vector()

• main : 19s + 260s + 125s
• PR 4502 : 16s + 212s (39 + lexsort 200) + 125s
• this PR : 12s + 12s + 125s

I will work in a sperarte PR to improve the select_unit() and caching.

Important note for us: we should reread all the sorting reader and make the same trick when it is necessary.

[alejoe91]

This LGTM! @grahamfindlay can you take a look?

[grahamfindlay]

Eh, sorry I haven't had time to pick #4502 back up.

As I understand it, this makes 2 changes:

1. __init__() doesn't waste time trying to remove spikes and unit ids from "bad clusters" from the full flat (npy) arrays on load (read_phy()), if there aren't any bad clusters to begin with. This accounts for the small (~4s) improvement in read_phy() time.
2. Overrides the parent class _compute_and_cache_spike_vector(). The new one just never lexsorts. This accounts for the large improvement in to_spike_vector(). The result is 12s to build the spike vector. Nice!

My only real concern would be this, in the new _compute_and_cache_spike_vector():

# the order for 2 units with the same sample_index is not garanty here but should be OK

If it's okay to not sort the co-temporal spikes, why were we doing it in the first place? Should we also remove this guarantee from the parent class's implementation?

In my last comment on #4502 I mentioned that I had found a way to do the lexsorting much more quickly than np.lexsort(). That also built the spike vector in ~12s, but also maintained the sortedness guarantee, and also didn't require an override of the parent method (i.e., should work for all sorters). Then I... never committed it 🫣. But I have it stored on a local branch somewhere, I can dig it up, if you want to see it.

In any case, I don't see any reason that should block this. This looks good and is a big immediate win (assuming it really is okay to break the spike vector sortedness promise)!

[samuelgarcia]

1. __init__() doesn't waste time trying to remove spikes and unit ids from "bad clusters" from the full flat (npy) arrays on load (read_phy()), if there aren't any bad clusters to begin with. This accounts for the small (~4s) improvement in read_phy() time.

Yep.

2. Overrides the parent class _compute_and_cache_spike_vector(). The new one just never lexsorts. This accounts for the large improvement in to_spike_vector(). The result is 12s to build the spike vector. Nice!

My only real concern would be this, in the new _compute_and_cache_spike_vector():

# the order for 2 units with the same sample_index is not garanty here but should be OK

This is the main point we need to discuss. Normally it should be OK that the spike vector do not garanty the order aas soon as the order is always the same.
The spike amplitudes, amplitudes scaling, spike location will be relative to thios vector!

If it's okay to not sort the co-temporal spikes, why were we doing it in the first place? Should we also remove this guarantee from the parent class's implementation?

Maybe we haven't discuss it enough.

In my last comment on #4502 I mentioned that I had found a way to do the lexsorting much more quickly than np.lexsort(). That also built the spike vector in ~12s, but also maintained the sortedness guarantee, and also didn't require an override of the parent method (i.e., should work for all sorters). Then I... never committed it 🫣. But I have it stored on a local branch somewhere, I can dig it up, if you want to see it.

I don't see this piece of code in the PR and I would be happy to see it.