Seed the two chi-square uniformity tests so they cannot fail by chance - #730
Open
ciaranra wants to merge 1 commit into
Open
Seed the two chi-square uniformity tests so they cannot fail by chance#730ciaranra wants to merge 1 commit into
ciaranra wants to merge 1 commit into
Conversation
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
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.
Problem
test_choice_uniformityandtest_randint_uniformitydraw 10,000 unseeded samples, run a chi-square test for uniformity, and assertp_value > 0.01. Under the null hypothesis the p-value is uniform on [0, 1), so each test fails roughly one run in a hundred by construction, regardless of whether anything is wrong. Together they cost about two percent of CI runs.This is not hypothetical. It failed the
pr-core-python (rest)shard on an unrelated Rust-only PR and blocked it until the job was rerun:A p-value of 0.0093 is an ordinary draw, not a defect in
choice.Change
Both tests now seed the generator first, matching the convention already used elsewhere in the same file (
test_random_uniformity_ks_testseeds with 42,test_binomial_momentsparametrizes over seeds).The chosen seeds give the assertion a wide margin rather than merely clearing the threshold:
test_choice_uniformitytest_randint_uniformityBoth are about two orders of magnitude above the 0.01 threshold, so ordinary numerical drift cannot flip them.
The tests keep their diagnostic value. They are now deterministic: a genuine loss of uniformity in
choiceorrandintdrives the p-value sharply down and fails the assertion every time, instead of failing one run in a hundred whether or not a regression exists.Scope
A repository-wide search for statistical assertions found exactly these two tests lacking a seed. Every other test that performs a chi-square, KS, or moment comparison already seeds its generator.
Seeding pins each test to the current RNG stream, so a deliberate change to the underlying generator will require re-selecting the seeds. That is the same property the already-seeded tests in this file have.
Verification
pre-commiton the changed file: all hooks passed.TestPerformanceComparisontests fail identically on unmodifieddevin the same local environment, so they are unrelated to this change; they are timing comparisons against NumPy and are sensitive to build profile and machine load.