Flaky test: test_ade_flushes_every_population_size_results asserts three distinct in-flight records, but ade tolerates duplicate candidates and the fixture is unseeded

[wshlavacek]

What happens

tests/test_diff_evolution.py::TestLearnedMutationSettings::test_ade_flushes_every_population_size_results asserts, at tests/test_diff_evolution.py:773:

assert len(proposals) == 3 and len(ade._trial_settings) == 3

len(proposals) is always 3 -- one proposal per got_result. len(ade._trial_settings) is 3 only when the three proposed candidates are three distinct PSets. About 0.5% of the time two of them are byte-identical, they collide on the _trial_settings key (it is keyed by the PSet, pybnf/algorithms/optimizers/differential_evolution.py:353), the dict keeps two entries, and the assertion fails with assert (3 == 3 and 2 == 3).

Why it is not deterministic

The test builds its config with config.Configuration(...) directly and never sets random_seed, so it defaults to None (pybnf/config_schema.py:312). Algorithm._init_rng then constructs the stream as np.random.default_rng(np.random.SeedSequence(None)) (pybnf/algorithms/base.py:278-279), and SeedSequence(None) draws fresh OS entropy on every construction. The _ade_config fixture sets mutation_rate = 1.0 -- the suite's standard guard against duplicate proposals (see the setup_class comment at tests/test_diff_evolution.py:20) -- but de_adapt_mutation = 1 overrides the rate with a draw from the success history, which can sit low enough that two candidates coincide, so that guard does not apply here.

Measured on a clean checkout (f3bd79f0, before any #708 work): 2 collisions in 400 constructions = 0.5%.

Reproduction (mirrors the test, run 400 times with fresh entropy):

base = {  # the _ade_config fixture, with de_adapt_mutation on
    'population_size': 3, 'max_iterations': 100, 'mutation_rate': 1.0,
    'mutation_factor': 0.5, 'de_strategy': 'rand1', 'fit_type': 'ade',
    'de_adapt_mutation': 1,
    ('uniform_var', 'v1__FREE'): [-100, 100], ('uniform_var', 'v2__FREE'): [-100, 100],
    ('uniform_var', 'v3__FREE'): [-100, 100],
    'models': {'bngl_files/parabola.bngl'}, 'exp_data': {'bngl_files/par1.exp'},
    'bngl_files/parabola.bngl': ['bngl_files/par1.exp'], 'output_dir': <tmp>}
ade = algorithms.AsynchronousDifferentialEvolution(config.Configuration(base))
start = ade.start_run()
ade.fitnesses = [5.0, 3.0, 7.0]
for ps, sc in zip(start, [5.0, 3.0, 7.0]):
    r = algorithms.Result(ps, d1s, ps.name); r.score = sc
    ade.got_result(r)
# count the runs where len(ade._trial_settings) != 3  ->  ~2 in 400

Why this is a test bug, not a product bug

The production code deliberately tolerates the collision. _note_trial_result documents it in its own docstring (pybnf/algorithms/optimizers/differential_evolution.py:467-468): "A candidate this history did not build (the initial population, or a duplicate whose record another candidate overwrote) records nothing either." So the contract is that a duplicate candidate's learned-settings record may be overwritten and its outcome dropped; the fold still runs correctly on whatever records survive.

Island de goes further and perturbs a duplicate candidate by up to 1e-6 per parameter so it never collides (_perturb_duplicate, called at pybnf/algorithms/optimizers/differential_evolution.py:850 while new_pset in self.island_map), moving the record to the perturbed key. ade's proposal path (pybnf/algorithms/optimizers/differential_evolution.py:1020-1029) does not, by the same documented tolerance. So the == 3 assertion is stricter than the behaviour the code guarantees, and the gap is ade-only: the sibling island-de test at tests/test_diff_evolution.py:739 asserts the same == 3, but island de perturbs duplicates, so it holds there.

Where

tests/test_diff_evolution.py:773 (the assertion). Root cause is the unseeded fixture plus the ade-tolerated PSet collision, not any production defect.

Fix options

• Set random_seed in the de_adapt_mutation test config(s) so the tests are deterministic and reproducible from the seed. This is the smaller change and also removes the entropy dependence from the other de_adapt_mutation = 1 tests in the file.
• Or relax the assertion to the contract the code actually guarantees: that the fold registers a record for each distinct candidate and folds the successes among them, without requiring all three proposals to be distinct.

The first is preferred: one key added to the fixture, deterministic thereafter, and no change to the assertion's intent.

Severity

Low. The test fails loudly (never a silent wrong answer), roughly 1 run in 200 of this test, and only under de_adapt_mutation = 1. It is worth fixing because an unseeded, entropy-dependent test reddens CI at random and is not reproducible from its own output.

[wshlavacek]

Correcting one claim in the closing analysis here, since it turned out to hide a real defect. This issue's write-up says island de is immune:

Island de additionally perturbs a candidate that duplicates one already in flight, by up to 1e-6 per parameter, and moves the record to the perturbed key, so its own flush oracle is distinct by construction and is left alone here.

The candidates do end up distinct. The records do not. _trial_settings is written inside new_individual (differential_evolution.py:353) while the perturbation runs afterwards at the call site (:847-850), so a duplicate candidate's write has already overwritten the earlier candidate's record at the shared key by the time anything moves — and what _perturb_duplicate then carries to the perturbed key is the survivor, not the record it was meant to protect.

So the tolerance documented here for ade — "a duplicate whose record another candidate overwrote" records nothing — applied to island de too, which was not the intent: de perturbs precisely because it does want every candidate to carry a record.

The sibling test at tests/test_diff_evolution.py:739 that was left alone on that reasoning duly failed on CI (the pytest (py3.14, bngsim-less) job of #774), with the same assert (3 == 3 and 2 == 3) signature as this issue.

Filed as #775 and fixed in #776, which changes the ordering rather than relaxing the assertion, and pins _de_config's seed the way #732 pinned _ade_config's. Nothing here needs reopening — the ade analysis and fix in this issue were correct for ade.