Extend docstrings for amplitude scaling and collisions

[JoeZiminski]

In investigating #2882 I had some trouble understanding what the code around amplitude scalings and fit collisions. The code itself is easy to follow, but there is a lack of context and supporting information on data structures which can make it tricky to understand what is going on without breaking into the code. I think I'll add the fix for #2882 in a different PR so the scope does not become too large.

This PR:

1. does some minor variable renamings for clarity
2. extends docstrings for find_collisions, fit_collision and AmplitudeScaling classes and adds a few extra explanatory comments.
3. Uncomments the plot-debugging code, makes functions private and add short docstring.

Some questions:

1. A lot of the computations depend on the first entry into the collisions structure being the spike of interest. If the code is refactored in a way that changes this by accident, it will be difficult to track down but have major negative affect (all amplitude scalings will be wrong). Make the structure of collisions can be changed, so this has two entries (spike) and (colliding_spikes). This imo would make this more robust in future.
2. The spikes structure is an array with entries An array of spikes (sample_index, channel_index, amplitude, segment_index, unit_index, in_margin). I had added this explicitly to docstrings (initially, I just assumed it was array of indexes). But, there is now some duplication in the docstring, personally I don't think this too bad a thing, but might be a point for discussion.

Update before merge

• tests were not adressed here but can be later, based on this comment
• the implicit spike-of-interest required to be in the first position of the collisions (1 above) dict is quite fiddly to address, and it would be easy to design a test to catch this case, this can be open in a new PR.

[zm711]

Just a few initial comments.

[alejoe91]

Thanks Joe! I'd be happy to discuss about this during the hackahton.

I brief comment about the _plot_* tests. These are designed as debug plotting routines, so I don't think they should be tested here. If we want to make a proper plotting routine for collisions (which might be cool for testing sorters and debugging), it's a better idea to port them to the widgets module

[h-mayorquin]

Thanks a lot for doing this. I learn from these efforts as I have not used the methods myself.

[JoeZiminski]

Thanks @h-mayorquin! I've addressed the points and used a bullet-point approach for the definitions I agree much clearer now, let me know what you think!

Cheers @alejoe91 I agree if these are not really in-use functions it does not make as much sense to add to the maintainability burden by adding tests for them. Do you think these would be useful enough to add as general widgets? Happy to do this, otherwise, do you think they could be moved to a personal repo? I'm conflicted, as I can see the use of having quick debugging functions in the main codebase, but if they are in the main codebase they should have some tests to check they don't become stale / unusable. But then, this adds maintainability burden 🤔

[h-mayorquin]

To me this is clearly an improvement. Some final comments but this is good to go for me.

[JoeZiminski]

Thanks everyone for their feedback! @alejoe91 I think this is good to go, currently the _plot_collisions function is broken, it wants a sac._extension_data attribute which I think was deprecated, and sac.collissions attribute is currently not used. I left a note on this. Let me know if it needs anything further, cheers!

[alejoe91]

Thanks for the clarifications @JoeZiminski! I agree that this function was a bit obscure..it's very nice to see additional eyes on it!

We can keep the TODOs for a follow up, ok?

[JoeZiminski]

Sure sounds good @alejoe91, thanks for this!