DE's `2` strategies (rand2/best2/all2) crash: 5 unique donors are drawn from a population floored at 3

[wshlavacek]

What happens

new_individual sets pickn = 5 for any strategy whose name does not contain '1' (line 288-290: if '1' in self.strategy: pickn = 3 else: pickn = 5), then draws picks = self.rng.choice(len(individuals), pickn, replace=False) at line 295. numpy raises ValueError: Cannot take a larger sample than population when replace is False whenever len(individuals) < 5.

Nothing in the config path prevents that. de_strategy is validated only against the name list at line 229 (options = ('rand1', 'rand2', 'best1', 'best2', 'all1', 'all2')) with no reference to population_size. Both population floors are 3, not 5, and neither knows the strategy: DifferentialEvolution.__init__ line 613-615 does self.num_per_island = int(population_size / islands) then if self.num_per_island < 3: self.num_per_island = 3, and AsynchronousDifferentialEvolution.__init__ line 904-905 does if self.population_size < 3: self.population_size = 3. The comment and the user-facing message both assert the minimum is 3 ("Differential evolution requires a population size of at least 3"), which is only true for the 1 strategies.

_forced_parameter is not the culprit here -- its re-draw pool (len(individuals) - 1) is always big enough for any population that survived line 295 -- so the failure is squarely the pickn = 5 draw.

I confirmed the numpy behaviour in this repo's env: rng.choice(4, 5, replace=False) and rng.choice(3, 5, replace=False) both raise ValueError: Cannot take a larger sample than population when replace is False.

Reproduction

Executed and observed. Read-only; script lives in the scratchpad at /private/tmp/claude-503/-Users-l119605-Code-PyBNF/eb55c5a3-1006-4c5d-9d1e-9d14fe8045f0/scratchpad/repro.py. No repo file was touched.

Config (mirrors the _ade_config helper in tests/test_diff_evolution.py:133, with two keys changed):
'fit_type': 'ade', 'de_strategy': 'best2', 'population_size': 4,
'max_iterations': 100, 'mutation_rate': 1.0, 'mutation_factor': 0.5,
three uniform_var free parameters, models = bngl_files/parabola.bngl, exp_data = bngl_files/par1.exp

Snippet (run with cwd = /Users/l119605/Code/PyBNF/tests so the bngl/exp paths resolve):
c = config.Configuration(base)
alg = algorithms.AsynchronousDifferentialEvolution(c)
inds = alg.start_run()
res = algorithms.Result(inds[0], None, inds[0].name); res.score = 1.0
alg.got_result(res)

Command:
cd /Users/l119605/Code/PyBNF && BNGPATH=/Users/l119605/Code/bionetgen/bng2 uv run --extra tests --extra petab python /repro.py

Observed:
population_size after init: 4
len(individuals): 4
Traceback ... multistart.py:157 got_result -> differential_evolution.py:1012 _search_got_result
-> differential_evolution.py:295 new_individual
picks = self.rng.choice(len(individuals), pickn, replace=False)
ValueError: Cannot take a larger sample than population when replace is False

Note that population_size stays 4: the < 3 clamp never fires, no warning is printed, and the config looks legitimate right up until the first result comes back.

Expected: either the floor is 5 when de_strategy ends in '2' (with the same clamp-and-warn treatment the 3 floor gets), or DifferentialEvolutionBase.__init__ raises a PybnfError naming both de_strategy and population_size.

Island variant (read-verified, not executed for time): fit_type = de, de_strategy = rand2, population_size = 24, islands = 8 gives num_per_island = int(24/8) = 3, which is not < 3, so no clamp and no warning; the first island to finish a generation calls new_individual(self.individuals[island], ...) at line 830-836 with a 3-member list and hits the same line 295.

This is committed code, not the dirty working tree — git status lists no change to pybnf/algorithms/optimizers/differential_evolution.py.

Verification notes

I tried to refute this and could not. Everything in the claim checks out against the code, and I reproduced the crash through the public got_result entry point.

What I verified:

• /Users/l119605/Code/PyBNF/pybnf/algorithms/optimizers/differential_evolution.py:288-295 is exactly as claimed: if '1' in self.strategy: pickn = 3 else: pickn = 5, then picks = self.rng.choice(len(individuals), pickn, replace=False).
• No upstream guard exists. de_strategy is validated only against the name tuple at line 229; I grepped population_size in pybnf/config.py, pybnf/parse.py and pybnf/config_schema.py and found no cross-check against de_strategy, and no occurrence of a 5-member floor anywhere in pybnf/ or tests/.
• Both floors are 3 and neither consults the strategy: DifferentialEvolution.__init__ lines ~613-615 (num_per_island = int(population_size / islands), clamp to 3) and AsynchronousDifferentialEvolution.__init__ lines ~904-909 (clamp to 3).
• The path is reachable with the real population list, not a synthetic one: ADE's _search_start_run builds exactly population_size individuals (line ~944-946) and _search_got_result calls self.new_individual(self.individuals, ...) at lines 1012/1014/1016. The island variant passes self.individuals[island], which has num_per_island members, at lines 830-836.
• Existing tests do NOT pin the current behaviour as correct. tests/test_diff_evolution.py:171 (test_rand2_sums_two_donor_differences) monkeypatches alg.rng with SimpleNamespace(choice=lambda n, k, replace=False: np.array([0,1,2,3,4])), so the real rng.choice never runs, and the test supplies 5 individuals anyway. Every other new_individual test either uses a '1' strategy or the same fake rng. tests/test_config_golden.py:185 uses de_strategy = rand2 but only as a config-serialization fixture. So nothing in the suite covers a '2' strategy against a population smaller than 5 — the gap the claim points at.

The only correction I'd make is severity: this needs a user-chosen population under 5 per island, which is unusual for DE, and it fails loudly rather than producing a wrong answer. Medium, not high. The island case is the nastier one because population_size = 24 looks entirely reasonable and the printed message ("at least 3 per island") actively misinforms.

Where

pybnf/algorithms/optimizers/differential_evolution.py:295 — severity medium, bug class crash.