[h-mayorquin]

I am not present in the library as much as I would like, and every time I come back I face the same problem of remembering what is the spike vector and what is the lexsorted one. I propose the following renames. I hope they improve clarity and help me remember what the two representations are for without having to look it up:

• _cached_spike_vector -> _cached_time_ordered dict, with spikes|segment_slices fields
• _cached_lexsorted_spike_vector -> _cached_unit_grouped dict, with spikes|unit_segment_slices fields
• _cached_spike_vector_to_indices -> _cached_time_ordered_indices_by_unit
• _cached_spike_vector_segment_slices -> removed, now _cached_time_ordered["segment_slices"]

These are private attributes and none of them is serialized (pickling goes through to_dict()). Saved objects are not affected.

[samuelgarcia]

Good idea and more

• _cached_unit_grouped_spike_vectors as a dict could also disapear because we use only one case.
  And we could have without dict
• _cached_spike_vector_segment_slices could be group with _cached_time_ordered_spike_vector it is what we do in the dict lexsort

[h-mayorquin]

I did the changes requested by Sam.

[grahamfindlay]

I think _cached_time_ordered_spike_vector and _cache_unit_grouped_spike_vector are good names.

I would also propose _cached_time_ordered_view and _cached_unit_grouped_view for these.
Since each cache is a dict holding the array ("spikes") and its slices, the attribute isn't actually a spike vector anymore, right? Then time_ordered_spikes() would return _cached_time_ordered_view["spikes"], which I think reads nicely. And it also reads nicely at view["spikes"] and view["slices"], whereas _cached_unit_grouped_spike_vector["spikes"] says "spikes" twice.

The only name I have an issue with is _cached_time_ordered_indices_by_unit. To me, it reads as "indices that are time-ordered, by unit", which is NOT what it is. Something like _cached_time_ordered_vector_indices_by_unit would be more accurate. Or, if you go with the views thing, _cached_time_ordered_view_indices_by_unit.

But more importantly, I am not sure whether this cache needs to exist as a separate thing at all. The unit-grouped cache already stores "order", from to_reordered_spike_vector(return_order=True), which is the permutation from unit-grouped position back to time-ordered position. order[start:stop] for a given (unit, segment) slice is the same as get_spike_vector_to_indices()[semgent][unit]. So get-spike_vector_to_indices could return views into order, and _cached_time_ordered_indices_by_unit could go. The only catch is that a caller that wants the indices but hasn't called to_reordered_spike_vector yet would have to do that, and the caller might want just the indices and not the rest of the reordered spike vector (I think this is already an issue with the cache as-is, but now we could fix it). I think that would be easy to solve by letting reorder_spike_vector_by_unit_and_segment return only order (and counts) without materializing ordered_spikes. This would also have the advantage that we could save a lot of memory for long recordings. This is trivial for the numpy path and just a flag for the numba kernel. But in any case, I could do that as a separate follow-up. We could keep the cache and just pick a better name for it, for now.

[h-mayorquin]

Thanks @grahamfindlay.

a) Agree, they are not spike vectors anymore. I would avoid "view" though, as the unit-grouped one holds a full reordered copy (ordered_spikes) and I think "view" promises no copy. I went with just the order: _cached_time_ordered and _cached_unit_grouped. Then it reads self._cached_time_ordered["spikes"] and self._cached_unit_grouped["order"] without saying "spikes" twice. Done in the last commit.

b) and c) Agree that _cached_time_ordered_indices_by_unit duplicates order. As c removes it, I left its name as it is here instead of renaming it only to delete it later. I would be happy if you do it as a follow-up. Returning only order without materializing ordered_spikes is a nice memory win. It also goes in the direction of the proposal I opened for the public API:
#4851

[alejoe91]

@samuelgarcia @chrishalcrow this is ok to merge on my end!

[JoeZiminski]

Thanks @h-mayorquin for this I am not very familiar with the internals of the spike vector so was a useful read. To confirm, the current state / plan is:

We previously had (after renaming):
_cached_time_ordered_spike_vector
_cached_unit_grouped_spike_vectors (plural as it is a dict keyed by lexsort)
_cached_time_ordered_indices_by_unit
_cached_time_ordered_segment_slices

where _cached_time_ordered_spike_vector was a time-ordered spikes vector and _cached_time_ordered_segment_slices was a list of indices to slice the segments out of the time-orded spikes vector. These are now combined into:

_cached_time_ordered = {
    "spikes": <time-ordered spike vector>,
    "segment_slices": <start/stop boundaries for each segment>,
}

previously _cached_unit_grouped_spike_vectors was a dictionary keyed by lexsort (now removed as its always unit-ordered) with the unit-ordered spike vector, an 'order' which are indices from time-ordered to unit-ordered vector, and slices which can slice the segments similar to above. It turned out _cached_time_ordered_indices_by_unit and order are redundant and so _cached_time_ordered_indices_by_unit is pegged for removal later. Now the _cached_unit_grouped_spike_vectors is:

_cached_unit_grouped = {
    "ordered_spikes": <unit-grouped spike vector>,
    "order": <indices that rearrange time-ordered spikes into unit-grouped order>,
    "slices": <start/stop boundaries for each unit/segment pair>,
}

This PR is done so feel free to ignore, I find the last change slightly confusing as its not clear what these things are (e.g. time_ordered what?). I would suggested _cached_time_ordered_spikes["vector"] to get the spike vector, similarly _cached_unit_grouped_spikes["vector"]. I feel ordered_spikes could just be spikes (or vector under the suggested renaming) because its implied this is reordered by group in the dictionary name. Finally slices on the _cached_unit_grouped could be clearer similar to segment_slices e.g. unit_by_segment_slices. Anyways just suggestions in case they are useful, this PR is a big improvement so LGTM!

[h-mayorquin]

Thanks @JoeZiminski, your summary is correct.

I find the last change slightly confusing as its not clear what these things are (e.g. time_ordered what?). I would suggested _cached_time_ordered_spikes["vector"] to get the spike vector, similarly _cached_unit_grouped_spikes["vector"].

I would be OK with adding "spikes" to the names but see the discussion with Graham above. Once the caches became dicts the names said "spikes" twice, as in _cached_unit_grouped_spike_vector["spikes"]. Your "vector" key avoids that, but I think the key already answers time_ordered what? at every place the cache is used: self._cached_time_ordered["spikes"] reads as the time-ordered spikes and self._cached_time_ordered["segment_slices"] as their segment slices.

I feel ordered_spikes could just be spikes (or vector under the suggested renaming) because its implied this is reordered by group in the dictionary name. Finally slices on the _cached_unit_grouped could be clearer similar to segment_slices e.g. unit_by_segment_slices.

I agree with both. I renamed ordered_spikes to spikes and slices to unit_segment_slices (the array is indexed by unit and then segment). Now self._cached_unit_grouped["spikes"] reads the same way as self._cached_time_ordered["spikes"], so the key answers the "what?" for both caches.

[alejoe91]

@h-mayorquin could you update the original PR message with the final renaming?

[alejoe91]

• unit_segment_slices

Updated the PR message. Merging :)