Skip to content

Fix ring_index

Timothy Davies requested to merge tdavies__fix_ring_index into spiir-O4-EW-development

Issue

Closes #37
(We've had trouble with these macros, I'm going to try it in a code block)

I've added some "Test Cases" for ring_index. Prior to the changes, the following cases failed:

  • ind < 0 resulted in arbitrary behaviour, as ind was cast to an unsigned type
  • ind = -n * len resulted in ring_index(ind, len) = len), as ind < 0 evaluated to true, which used to add len to the result
    • This is likely the source of nondeterminism

Tests

I've run the following tests (As well as the tests in #37 (closed)):

  • 2 ~2h runs while only adding len before modulo, as it was before !28 (merged). (For future reference, I could probably have just changed the times at which marginalized stats files are generated and done 5 minute runs)
  • 2 ~2h runs in the current state (9243cc3d).
  • 1 run with assert(false) in place of the current assert.
    • Each thread reports the error, and the program exits immediately. All results can be found here: /fred/oz016/tdavies/projects/testing/run_fix_sampling_fix_bg_short

Marginalized stats were deterministic in the runs above (except assert(false) ofc), and identical to the backgrounds prior to !28 (merged) /fred/oz016/tdavies/projects/testing/run_rerun_yapf_bg_short/

I also ran the "unit tests" added in this commit, all pass.

Arguably weird design decisions

  • The test file can be viewed more as documentation and a proof-of-concept rather than serious testing. It's a set of test cases, to be run manually on a copy of the code, lacking any links for developers to follow if ring_index were edited.
    I think that lack of links is for the best since the tests and code are so uncoupled.
    i.e. Better to leave test maintainence totally unenforced than to do it half baked and suggest in a comment that a piece of code can be relied on.
    I'd argue it's a good start. It gets the benefits of TDD while we're waiting on discussions and developer time to choose and setup testing frameworks. They're simple enough it shouldn't be any real technical debt down the line. If it gets maintained with changes to ring_index, that's a bonus, but not a requirement.
  • I've put it in a directory testing/tests. Somewhat arbitrary, I figure 1 layer will be insufficient as soon as we add a test helper, regardless of what framework we use.
  • I'm using an assert to describe the function contract. This is something of an old c standard, I put the discussion in the test comments (maybe it should be exclusively in gitlab).

Related: #38

Edited by Timothy Davies

Merge request reports