Harmonization of the way we are computing sliding_rp_violations

[yger]

After optimizing all metrics, I realized a slight discrepancies in sliding_rp_violations compared with our way of computing cross-correlograms. This has to do with several things, I think it would be worth harmonizing everything

• window_size_s in sliding_rp_violations is half the entire duration of the cross-correlograms, while in compute_correlograms, we assumed that window_size_s is the entire duration. There is thus here a factor 2 that could be misleading
• in compute_correlograms, we have a built-in mecanism to ensure the numbers of bins is odd, to avoid having an ambiguity at 0 with a bin -t, t that would not be properly symmetric. This is also not the case in sliding_rp_violations, that computes the number of bins in its own way, I guess less strict and precise.
  This PR uses compute_correlograms instead to solve the problem, but this would slightly change the results of the metrics. Who implemented that? Is it a feature, or should it be harmonized?

[yger]

Ideally, this function should even make use of auto_correlograms, since they will be added in #4307

[zm711]

What's the status of this after #4704 and #4682 @alejoe91?

[alejoe91]

@yger what's the status of this?

[yger]

The problem remains. As said, there are inconsitencies in this function when compared to compute_correlogram. We should harmonize: window_size_s and the problem of the bin size. I don't think this should change too much the results

[alejoe91]

As the commented text says, in order to maintain the behavior we should set 2 * window_size_s * 1000, no?

We can make it clear in the params that this is "half" the window_size

[alejoe91]

@yger what do you thiink?

[yger]

Yes, we should revert to 2window_size1000, and make clear that param is half the window size as opposed to the other way of computing correlogram. My point was that since the computation mostly use the information near 0, I think twice the window size is an overkill here. But happy to keep it as it is, since I do not have time to properly test the numerical difference between the two. I'll edit the PR

[alejoe91]

Tested on a real dataset with 142 units and indeed there are some differences:

Here's a summary:

same values: 24/142
different values: 49/142

# nans main: 66/142
# nans this PR: 69/142

It seems that this implementation slightly overestimates the sliding RP values.

@oliche @yger thoughts?

[alejoe91]

The mismatch is due to different rounding:

Old code: bin_size = int(0.25/1000 * 30000) = 7 samples → 0.233 ms/bin
New code: bin_size = int(round(7.5)) = 8 samples → 0.267 ms/bin

In both cases though, the rp_centres/rp_edges are computed before the CCG computation, which effectively does the rounding and modifies the actual bins.

I pushed a fix in the last commit: the rp_centers/rp_bins are now computed after the ccg computation, so it is correct! @yger @oliche what do you think?

[alejoe91]

Reminder for myself: drop new scatter for reference

[alejoe91]

Ok after the implementation fix here are the new results:

# same values: 32/142
# different values: 41/142
# nans main: 66
# nans this PR: 69

And scatter:

There are more "same" values (32 versus 24) and the new values are more similar to the main implementation. Merging!