Attempt to make faster MultiRingBuffer

[spxiwh]

We are having difficulties in getting pycbc_live to keep up with data in real-time in its current configuration. There may be a few patches from me aimed at this (even if in some cases, we're only speeding up by 1/2%).

It does seem that the main process is becoming a problem in pycbc_live's inability to keep up. One place that was non-negligible in a profile I ran was the RingBuffer used to store singles.

This does look like it is quite inefficient as the buffers are replaced (or resized if that is possible??) every time triggers are added (using numpy append). By using more memory, and tracking the slice of the buffer that is relevant, it is possible to greatly reduce the times we resize the buffer arrays.

At the moment this is causing failures, so I need to fix that, but @ahnitz do you have any thoughts about this change?

[spxiwh]

... There did also seem to be a very-edge-case bug where expired triggers would not be removed if all triggers are expired. I'm not sure if it is ever possible to actually get to that condition, but this does resolve that.

[spxiwh]

This seems to be working now, so removing WIP flag. Still haven't properly assessed the impact of this in terms of the analysis keeping up.

[ahnitz]

@spxiwh @titodalcanton Maybe it's worth adding a unittest for this class and the coinc one at this time? Especially with these functions, I worry about small bugs creeping in.

[ahnitz]

@spxiwh I think the changes you've made though are OK in general. I've read through and I think I understand that you just preallocate some buffer size and then only do large copies / resets when 30% of the buffer is now expired.

I'd be curious what cases causes this class to be a non-negligible cost. In the past, I think this wasn't a huge impact, so I left the class with a purposefully simple implementation.

[spxiwh]

@ahnitz Do you have a suggestion for what would be a good unittest here? Maybe a case that runs this through O(50) iterations with a quick expiry time, and then somehow checks both the single and coinc buffers?

I have been struggling to generate a representative callgraph of the main process (and probably I've since realised that what I have below is not quite right, as other processes become important once the coinc buffers get full), but I attach the callgraph that motivated me to look at this (and logsignalrate). The code works as you describe, and I think is similar to how the coinc buffer works.

[spxiwh]

[spxiwh]

The requested unit test is suggesting there is some issue with this change, so back on hold until I fix that.

[spxiwh]

@titodalcanton I've now added the relevant unittest, and assuming this passes tests, I would like to merge this now.

The unittest is a modification of the script you provided to run through the pycbc_live coincidence code. I've adjusted the parameters so that in 15 iterations it will have to expire many coincident triggers and resize the various array buffers. I have then extracted and saved old copies of the coinc.py and stat.py files, and run the code against those. We then compare outputs in terms of:

• Must have the same number of coincident triggers
• Coincident trigger contents (e.g. stat values etc.) must identically agree
• Number of single triggers must be identical for every template.
• Properties of all single-detector triggers must agree.

I'm not sure if it's going to be worth having this unittest around long-term, if the stat.py and coinc.py libraries change significantly. However, while we are looking to make optimization changes, I would advocate having this present as a check for any PR looking to optimize these two libraries.

Note that I also included the bugfix that this unittest uncovered.

[spxiwh]

@titodalcanton @ahnitz I would like to get this merged as it contains the unittest that will be used to validate other open PRs. Can we get this approved? (I will shortly rebase this to pick up the FAR fix).

[titodalcanton]

Are old_coinc.py and old_stat.py just verbatim copies of the old coinc and stat modules? Perhaps it would be good to add a comment in their toplevel docstrings to state this, and why they are now here. I also do not particularly like the idea of introducing those long files, most of which I guess will be unused by the test, but maybe this can be addressed in a later PR.

[spxiwh]

Are old_coinc.py and old_stat.py just verbatim copies of the old coinc and stat modules? Perhaps it would be good to add a comment in their toplevel docstrings to state this, and why they are now here. I also do not particularly like the idea of introducing those long files, most of which I guess will be unused by the test, but maybe this can be addressed in a later PR.

These are just verbatim copies (with the FAR bug fixed). I do think that once development on pycbc_live stops for O4 this can be removed and [perhaps] replaced with a data file storing the "stock" output to compare against. I think while there's a few optimization patches queued up, having this will be very useful, including in terms of the formal LVK review.