Remove peak_sign in favour of main_channel_indices

[chrishalcrow]

Sequel to #4374

I didn't have permission to push to Sam's branch, so I made my own...

Idea is that when you make your analyzer, it must have a peak_sign, peak_mode and hence a main_channel_index. We then use these throughout the codebase instead of propagating the other stuff.

Some design things:

• If you have a SortingAnalyzer, you set peak_sign, peak_mode and hence a main_channel_index immediately (or ASAP). Then none of the get_main_template type functions are allowed to receive these args. BUT if you have a Template, you must explicitly pass these args. So we're trying to make it simple for the analyzer (for users) but explicit for templates (for developers)

Cool things:

• peak_sign no longer appears in metrics.

@samuelgarcia did the tedious work, but now I'm refining and getting the tests to pass...

[chrishalcrow]

Hey @ecobost , I went through your comments on #4374 and mostly resolved them (sorry it's on a new PR, I didn't have permission to push to Sam's branch). Here's a list of "controversial" things to discuss.

• I think I've been too hrash getting rid of peak_sign and peak_mode in estimate_sparsity. This was actually used for Templates sensibly... I should re-insert... I'll do this soon!!
• @ecoboost pointed this out: at the moment, the main_channel_indices are a sorting property, and (ideally) the analyzer gets it from there. But in many cases the analyzer computes the main_channel_indices, and it's then set as a sorting property. In this case, the sorting inherits from the analyzer, which sounds bad. How to resolve?
• estimate_sparisty default is now "both", matching create_sorting_analyzer. It's only used in create_sorting_analyzer
• @ecobost suggests removing sparsity_kwargs from estimate sparsity. I'm against this, but mostly because I use them, which isn't a great reason.
• It's not clear if we should allow a user to pass their own "peak_sign" to e.g. get_template_amplitudes, because this is usually a bad idea. I suggest adding a override_peak_error flag, so that expert users have to turn this on to mess with it. I think @alejoe91 prefers to just allow the user to pass it.
• I've added peak_sign_for_signal and peak_mode_for_signal in compute_snrs. I think these have use cases: I can imagine people wanting to compute the peak_to_peak value on main_channel.
• At the moment, we're deprecating get_template_extremum_amplitude, get_template_extremum_channel_peak_shift, get_template_extremum_channel in favour of similarly named functions (but with main_channel). I'm a bit worried about this - our lab uses get_template_extremum_channel in various places and I'd bet it's used by other developers. Maybe we should just keep the old naming convention and use extremum_channel_indices. Or just keep get_template_extremum_channel for a while??

@samuelgarcia @alejoe91 could you take a look at this - would be great to get merged next week!!

[chrishalcrow]

@samuelgarcia this feels a bit weird. Should we really be computing sparsity here?

[samuelgarcia]

alessio wanted main_channel_ids here no ?
lets have the two ?

[alejoe91]

No, this is ok since you are also passing the recording. So it's not ambiguous. My main issue was in how we store the property I'm the sorting object, which is now correctly using main_chamnel_ids