test: make the seeded RNG independent of the C++ standard library - #178
Merged
Merged
Conversation
The nightly MSan job links an instrumented libc++, and under it unit_test/nn failed two tolerance checks that pass under libstdc++. Not a defect: several suites initialize network weights from a seeded RNG and then assert on what training converges to, and std::uniform_real_distribution does not specify the mapping from engine output to value. libstdc++ and libc++ return different doubles from identical engine state, so the weights differ and trained results move. std::default_random_engine is likewise an implementation-chosen typedef. unit_test/include/portable_test_random.hpp freezes the sequence the tests were written against, in portable code: minstd_rand0 (x = 16807x mod 2^31-1) plus the generate_canonical<double, 53> mapping libstdc++ applies over it, which for this engine consumes exactly two draws. Verified bit-identical to libstdc++ over 20,000 draws -- 0 mismatches, max difference 0 -- so every tolerance in these suites keeps its current meaning and not one needed changing. std::mt19937, whose output the standard does specify, was the first choice and was rejected on evidence. It is statistically indistinguishable (mean ~0, mean|x| ~0.5 over 2000 draws) but produces a different sequence, and two tests do not survive that: lstm_weight_serialization at 0.605 against a 0.02 bound, and rmsprop_fixedpoint_xor at average error 9 against a bound of 4. Loosening those to accommodate a new generator would weaken two real gates to settle a question neither is asking about. test_case_portable_uniform_real_matches_frozen_sequence locks the first eight draws as an exact assertion. Every training test here is calibrated against those numbers, so a future edit to the generator would otherwise silently re-tune all of them; now it fails loudly. Verified to have teeth by perturbing an expected value and confirming the failure. Applied to unit_test/nn, unit_test/kan and unit_test/qlearn. All three pass under both g++ and clang++. qlearn's std::uniform_int_distribution use for maze state selection is left alone and documented: that mapping is implementation-defined too, but the suite passes under the instrumented libc++ today, and changing a passing suite's draw sequence risks its assertions for no observed benefit.
This was referenced Aug 31, 2026
danmcleran
added a commit
that referenced
this pull request
Aug 31, 2026
#178 converted unit_test/nn/nn_unit_test.cpp and unit_test/qlearn/qlearn_unit_test.cpp from CRLF to LF as a side effect of being edited through Python's text mode, which normalizes newlines on write. Both files have been CRLF for their whole history. The content change in that PR was 33 added and 8 removed lines in nn, and 7 added and 10 removed in qlearn. The recorded diff was 17011 and 1959 lines, because every line in both files counted as rewritten. That buries the real change, makes the PR unreviewable, and poisons git blame for two of the largest files in the tree. Converts both back. No content change: the suites build and pass unchanged. kan_unit_test.cpp was already LF and is untouched.
danmcleran
added a commit
that referenced
this pull request
Aug 31, 2026
Editing a CRLF file with a tool that normalizes newlines rewrites the whole file. In #178 a +33/-8 change to nn_unit_test.cpp was recorded as 17,011 lines, which buried the real change and poisoned git blame; #179 undid it by hand. Nothing in CI catches this -- line endings do not affect compilation, so every gate stays green while the diff is unreviewable. This makes the mistake structurally impossible rather than a matter of who edits next. `* text=auto` has git store LF in the repository, and the files that are CRLF in the working tree -- the original core headers, the first examples, the two oldest test suites, the UML sources and one dataset -- are pinned with `text eol=crlf` so checkout still gives them CRLF. Everything added since is LF and needs no entry. The effect is that a tool which converts endings now produces no diff at all: the index holds LF either way, and checkout restores the working-tree convention. Verified by repeating the #178 mistake against these rules -- the same operation that produced +8518/-8518 now produces nothing, and checkout restores CRLF. This commit therefore renormalizes what the repository stores for those files. That is a one-time whole-file diff for each, and the last one they will have for this reason. Checked two ways first: every staged change is line-endings only (content compared with CR stripped), and no file's working-tree endings change.
danmcleran
added a commit
that referenced
this pull request
Aug 31, 2026
Editing a CRLF file with a tool that normalizes newlines rewrites the whole file. In #178 a +33/-8 change to nn_unit_test.cpp was recorded as 17,011 lines, which buried the real change and poisoned git blame; #179 undid it by hand. Nothing in CI catches this -- line endings do not affect compilation, so every gate stays green while the diff is unreviewable. This makes the mistake structurally impossible rather than a matter of who edits next. `* text=auto` has git store LF in the repository, and the files that are CRLF in the working tree -- the original core headers, the first examples, the two oldest test suites, the UML sources and one dataset -- are pinned with `text eol=crlf` so checkout still gives them CRLF. Everything added since is LF and needs no entry. The effect is that a tool which converts endings now produces no diff at all: the index holds LF either way, and checkout restores the working-tree convention. Verified by repeating the #178 mistake against these rules -- the same operation that produced +8518/-8518 now produces nothing, and checkout restores CRLF. This commit therefore renormalizes what the repository stores for those files. That is a one-time whole-file diff for each, and the last one they will have for this reason. Checked two ways first: every staged change is line-endings only (content compared with CR stripped), and no file's working-tree endings change.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Addresses the RNG divergence surfaced by the nightly MSan job in #177.
The problem
Several suites initialize network weights from a seeded RNG, then assert on what training converges to.
std::uniform_real_distributiondoes not specify the mapping from engine output to value, andstd::default_random_engineis an implementation-chosen typedef — so libstdc++ and libc++ return different doubles from identical engine state. Different weights, different trained results, failing tolerances. Not a defect; a portability hole.The fix
unit_test/include/portable_test_random.hppfreezes the sequence the tests were already written against, in portable code:minstd_rand0(x = 16807x mod 2^31-1) plus thegenerate_canonical<double, 53>mapping libstdc++ layers on top, which for this engine consumes exactly two draws.Verified bit-identical to libstdc++ over 20,000 draws — 0 mismatches, max difference 0. So every tolerance keeps its current meaning and not one needed changing.
Why not
std::mt19937That was the first choice — its output is standard-specified — and it was rejected on evidence. It is statistically indistinguishable from the current generator (mean ~0, mean|x| ~0.5 over 2000 draws), but it produces a different sequence, and two tests do not survive that:
test_case_lstm_weight_serializationtest_case_rmsprop_fixedpoint_xorLoosening those to accommodate a new generator would weaken two real gates to settle a question neither is asking about.
Locked against silent drift
test_case_portable_uniform_real_matches_frozen_sequenceasserts the first eight draws exactly. Every training test here is calibrated against those numbers, so a future edit to the generator would otherwise silently re-tune all of them. Verified to have teeth by perturbing an expected value and confirming it fails.Verification
unit_test/nnunit_test/kanunit_test/qlearnLeft alone, deliberately
qlearn's
std::uniform_int_distributionfor maze state selection has the same implementation-defined mapping. It is documented rather than changed: the suite passes under the instrumented libc++ today, so there is no observed problem, and changing a passing suite's draw sequence risks its assertions for no benefit.🤖 Generated with Claude Code
https://claude.ai/code/session_019tVgXeXCcfbHqfMjhufWzd