fix(de): de-duplicate a candidate before registering its learned mutation settings, so a duplicate stops destroying the earlier candidate's record (#775) - #776
Merged
Conversation
…tion settings, so a duplicate stops destroying the earlier candidate's record (#775) Island `de` wrote each candidate's learned-settings record inside `new_individual` (`differential_evolution.py:353`) and resolved a duplicate candidate afterwards, at the call site (`:847-850`). `_trial_settings` is keyed by the `PSet`, so when two candidates of one generation landed on identical parameters the second candidate's write overwrote the first candidate's record at the shared key; the perturbation that followed then popped that key and carried the survivor to the moved one. The first candidate's outcome was gone, and the settings that built it were neither credited nor blamed by the success history. `_perturb_duplicate` existed to prevent exactly this -- its docstring said "the record has to follow the candidate or its outcome is lost" -- and it rescued the key while the record it was protecting had already been clobbered. The fix is an ordering change behind a hook. `new_individual` now calls `_deduplicate` between building the `PSet` and registering the settings. The base implementation is the identity, because `ade` has no in-flight register keyed by the candidate and tolerates a shared key by design (#730, `_note_trial_result`); island `de` overrides it with the `island_map` loop the call site used to run. Both of `de`'s per-candidate registers -- `island_map` for the slot and `_trial_settings` for the settings -- now land under the candidate's final key. The hook sits at the exact point the call-site loop used to run, so the rng is consumed in the same order and every seeded oracle in `test_diff_evolution` keeps its drawn values bit for bit. That was verified before anything else: the file's 94 other tests passed unchanged on the first run, with only the test pinning the old helper behaviour failing. `_perturb_duplicate` is now pure -- it moves the parameters and touches no record, because it runs before any record exists. Its old test asserted the record-carrying that was the misleading half of the bug, so it is replaced by four tests: - the ordering itself: a second candidate on an in-flight candidate's parameters comes out moved, and registering it leaves the first record alone; - the helper is pure; - `ade` keeps the base identity, so its documented tolerance is unchanged; - end to end: a generation whose three candidates all coincide registers three records. The last one is the real guard, and it is deterministic. The rng is scripted so every draw is fixed, which makes `new_individual` a pure function of the population and all three candidates identical. On `main` it reports `assert 1 == 3` -- three candidates, one surviving record. The copy guarantee stays on throughout (`de_adapt_mutation` forces it on regardless of `de_force_mutation`, which is why passing `de_force_mutation = 0` here would have changed nothing but the log): the guarantee keeps a candidate from copying the member it was built FROM and says nothing about two candidates coinciding with EACH OTHER. How this surfaced, and what the rate is not: as a flake in `test_de_flushes_at_the_end_of_each_island_generation`, failing `assert (3 == 3 and 2 == 3)` on the `pytest (py3.14, bngsim-less)` job of #774, and reproduced locally once in 60 unseeded runs of that test. It is rare and the frequency is NOT well determined -- a sweep of `random_seed = 0..599` produced no collision on either tree, and two entropy-sampled harnesses disagreed by an order of magnitude. No rate is claimed anywhere in the code or tests for that reason; the deterministic test replaces the estimate. `_de_config` now pins `random_seed = 1`, the same reproducibility fix #732 applied to the `ade` fixture: unseeded, `SeedSequence(None)` draws fresh OS entropy per construction and a failure cannot be replayed. It is deliberately not the thing that keeps the flush oracle at three records -- after this change that count holds for every draw, seeded or not -- and the comment says so, so the seed is not later mistaken for the guard and the guard is not removed as redundant. #730's closing analysis states that island `de` is immune to this because it "moves the record to the perturbed key, so its own flush oracle is distinct by construction". The candidates do end up distinct; the records did not. That claim is corrected on the issue. Tested: fast tier green, 5371 passed / 25 skipped / 0 failed. ruff clean.
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.
Closes #775.
The bug
Island
deregistered a candidate's learned mutation settings before resolving a duplicate candidate, and_trial_settingsis keyed by thePSet. The two halves sat in different places:differential_evolution.py:353— insidenew_individual:self._trial_settings[new_pset] = (...)differential_evolution.py:847-850— at the call site, afternew_individualreturned:while new_pset in self.island_map: new_pset = self._perturb_duplicate(new_pset)So for candidate B built byte-identical to an earlier candidate A of the same generation, B's write destroyed A's record at the shared key, and the perturbation then carried B's record to the moved key. A's outcome was gone, and the settings that built it were never credited or blamed by the success history.
_perturb_duplicateexisted to prevent this — its docstring said "the record has to follow the candidate or its outcome is lost" — and it rescued the key while the record it was protecting had already been clobbered. Sinceisland_mapis emptied as results arrive, the duplicate is always a sibling of the same generation, so a sibling collision always lost a record.The fix
An ordering change behind a hook.
new_individualcalls_deduplicatebetween building thePSetand registering the settings. The base is the identity —adehas no in-flight register keyed by the candidate and tolerates a shared key by design (#730) — and islanddeoverrides it with theisland_maploop the call site used to run. Both ofde's per-candidate registers now land under the candidate's final key.The hook sits at the exact point the old loop ran, so the change is rng-neutral. Verified first, before anything else: the file's 94 other tests passed unchanged on the first run, with only the test pinning the old helper behaviour failing.
_perturb_duplicateis now pure. Its old test asserted the record-carrying that was the misleading half of the bug, so it is replaced by four tests: the ordering itself, the helper's purity,adekeeping the base identity, and — the real guard — an end-to-end test that a generation whose three candidates all coincide registers three records. That one is deterministic: onmainit reportsassert 1 == 3.On the rate — what I am not claiming
This surfaced as a flake on the
pytest (py3.14, bngsim-less)job of #774, and I reproduced it once in 60 unseeded runs of that test. But the frequency is not well determined: a sweep ofrandom_seed = 0..599produced no collision on either tree, and two entropy-sampled harnesses disagreed by an order of magnitude (2/400 vs 1/2000). I had written "about one generation in two hundred" into a docstring from the first of those; it is removed, and no rate appears anywhere in the code or tests. The deterministic test replaces the estimate.I also had
de_force_mutation=0in that test on the assumption it was what made the candidates coincide. It is not —de_adapt_mutationforces the guarantee on regardless, so the argument did nothing but emit a log line. Removed, and the docstring now says what actually makes them coincide (the scripted rng) and why the guarantee is irrelevant here: it keeps a candidate from copying the member it was built from, not from coinciding with another candidate.Fixture seed
_de_confignow pinsrandom_seed = 1, the reproducibility fix #732 applied to theadefixture. It is deliberately not what keeps the flush oracle at three records — after this change that holds for every draw — and the comment says so, so the seed is not later mistaken for the guard.#730
#730's closing analysis says island
deis immune because it "moves the record to the perturbed key, so its own flush oracle is distinct by construction". The candidates do end up distinct; the records did not. Corrected on that issue.Testing
Fast tier green: 5371 passed, 25 skipped, 0 failed.
ruff@0.15.14 check .clean. Every new test verified to fail against a worktree atorigin/main.