Fix a cross-band interpolation bug, and allow time_vector in interpolate_motion

[cwindolf]

Hi all, this PR addresses two issues related to motion interpolation, specifically estimating motion in the LFP band and applying the correction to the AP band.

• First, @DaohanZhang's IndexErrors, discussed at:
  • https://github.com/evarol/dredge/issues/1
  • fixes https://github.com/SpikeInterface/spikeinterface/issues/3328
  • I was able to reproduce this issue in real data and the changes here and at https://github.com/evarol/dredge/commit/5cfe179f5896de859fc4b658bd5330f499dd57c8 resolve this issue. A test case has been added.
• Second, in fixing that issue I found it easiest just to handle time vectors with a searchsorted. Hopefully this is not controversial anymore?
  • As discussed in https://github.com/SpikeInterface/spikeinterface/issues/3029, this is nowhere near a performance issue. Your computer will spend 1e6 amount of time in the matrix multiply.
  • The implementation here is commented and tested and allows for unevenly spaced motion and recording time bins
  • I would consider that this closes https://github.com/SpikeInterface/spikeinterface/issues/3029 -- @JoeZiminski and others, would you agree?

I also added a small test for cross-band registration. The idea was to generate an "LFP" and an "AP" recording, which are zeros except for a channel of 1s which jumps after 3s. Motion is estimated from the LFP recording and then used to interpolate the AP recording, and we assert that the result has no drift at all.

Interestingly, since the LFP recording's time bins are treated as bin centers by interpolate_motion but as left bin edges by time_to_sample_index(), there is a half-bin offset between the motion and the recording which I had to account for in this test. But, I actually think this is the right way to go? 2 reasons:

• interpolate_motion: It makes sense to treat motion time bins as centers even when they correspond to LFP recording samples; this artificial case is not realistic and in real life we believe that behavior leads to less error.
• time_to_sample_index(): obviously, would not want to change the behavior here, everybody expects the time in seconds to be "floored" to the previous sample.
  • If I'm wrong about that, then the searchsorted() line needs to be changed in time_to_sample_index() to map to the nearest bin, and logic similar to what I've written in interpolate_motion_on_traces() can be used. But, that's a separate issue from this PR -- just wanted to mention that in case my understanding is off.

(And, to @DaohanZhang: this PR together with a recent dredge repo commit (https://github.com/evarol/dredge/commit/5cfe179f5896de859fc4b658bd5330f499dd57c8) should fix your issue, if you'd like to test them. I've updated the demo notebook there to reflect the latest updates -- dredge's understanding of spikeinterface's time handling needed to be updated.)

[cwindolf]

Hi @samuelgarcia -- updated this PR with the stuff we talked about. To recap: we need the interpolation time bin edges to interpolate traces, which we were recomputing over and over for each interpolation chunk. Now they are cached. They are cached in the motion object, but also in the interpolated recording object if for example a user specifies their own interpolation time bin size.

I also added a small thing to vectorize time_to_sample_index, flagged in a comment above. If that feels outside the scope here I'm happy to put it in another PR!

[cwindolf]

Thanks a ton for the review @JoeZiminski ! I think I've made changes corresponding to all of your comments. No clue why some of my comments got duplicated... that's what I get for trying out this github code spaces thing!

[samuelgarcia]

This is OK for me.