Fix ring_index
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, asind
was cast to an unsigned type -
ind = -n * len
resulted inring_index(ind, len) = len)
, asind < 0
evaluated to true, which used to addlen
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
- Each thread reports the error, and the program exits immediately.
All results can be found here:
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 toring_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