Skip to content

Fix background stats refresh

spiir-O4-review-v5 ran into a critical bug in science mode on ER-15: It wasn't uploading any triggers.

Reading the new livetime_1w field, @manoj.kovalam found that we weren't refreshing backgrounds, so we never exceeded our required 1 million data points for upload.

The issue was that I'm stoopid. In !182 (merged) I asked Luke to subtract two uint64s !182 (comment 700212). Thinking of the case of an underflow, I said we need a MIN(0, unsigned_a - unsigned_b). That's obviously crazy, even if it doesn't underflow, it will always be 0.

Instead we should subtract and cast to a signed value of the same size. The GstClockTime type already has types and #defines for this problem:
https://gstreamer.freedesktop.org/documentation/gstreamer/gstclock.html?gi-language=c#GstClockTimeDiff,
https://gstreamer.freedesktop.org/documentation/gstreamer/gstclock.html?gi-language=c#GST_CLOCK_DIFF

I've used that in this patch. For review, it's now included along with other fixes in spiir-O4-review-v5.3.

Tests

Our usual tests didn't catch the issue for two reasons:

  1. Running on the injection stream, our MDC test hit the 1 Million data points in its first background dump. (Our non-injection run on ER-15 hit about 700k.)
    1. @manoj.kovalam is going to launch a new run on ER-15, uploading to test.
    2. Plotting the IFAR of that MDC run may have indicated an issue.
  2. Our usual offline injection tests don't refresh backgrounds, so although I could see that the new columns were working, I didn't verify that they updated.
    1. I've now done a quick offline run with short intervals and printed the nevent_1w and far columns.
    2. This could be converted into an automated test with our new columns.
    3. We should always write specific test cases for new code changes.

Both v5 and v5.3 load the first set of backgrounds (backgrounds & load times vary slightly due to calcfap run time and atomic add):
v5:
image
v5.3:
image

v5.3 loads new backgrounds (based on nevent and livetime), but v5 never does:
v5.3 first refresh:
image
Last zerolags v5 vs v5.3:
image

Internal gitlab review should also have caught it. The issue was that it was a reviewer comment asking for changes, and that needs to be double checked too. I think in future a call to discuss when suggesting changes to an MR is the right way to go. We should consider test cases of any updated code in review.

Edited by Timothy Davies

Merge request reports