From 88e9a98008ebe858f914e432e851bc0e9e9abd5c Mon Sep 17 00:00:00 2001 From: Bill Hlavacek Date: Thu, 17 Sep 2026 11:31:34 -0600 Subject: [PATCH] fix(algorithms): the differential evolution population floor is the strategy's donor count, not always three (#708) The '2' strategies (rand2, best2, all2) build each candidate from five distinct parameter sets -- a base and two donor pairs -- where the '1' strategies use three. new_individual draws exactly that many at once with numpy's rng.choice(..., replace=False), which raises "ValueError: Cannot take a larger sample than population when replace is False" whenever the population it draws from holds fewer than five. Both population floors were hardcoded to three and neither consulted de_strategy, so a '2' strategy with a small enough population crashed on the first proposal -- not with a wrong answer, but with an unhandled exception partway into the run. Two entry points reached it. AsynchronousDifferentialEvolution clamped population_size up to three, so population_size = 4 with best2 passed construction untouched and crashed when the first result came back. The island DifferentialEvolution clamped num_per_island up to three, so population_size = 24 over 8 islands gave 3 per island, looked entirely reasonable, drew no warning, and crashed the first island to finish a generation -- and the message it would have printed ("at least 3 times the number of islands") actively misinformed. The fix ties the floor to the draw. min_population is computed once in DifferentialEvolutionBase.__init__, right after de_strategy is validated: three for a '1' strategy, five for a '2'. new_individual now draws min_population donors instead of recomputing three-or-five, so the count a candidate needs and the count the population is floored at are the same value by construction and cannot drift apart. Both subclasses clamp up to min_population with the same clamp-and-warn treatment the old floor of three already had, and the warning now names de_strategy and the required size rather than asserting a flat minimum of three. This is the issue's first suggested fix, clamp and warn, chosen over raising, so that a too-small population behaves for a '2' strategy exactly as it does for a '1' strategy today: increased to the minimum with a warning rather than aborting the run. docs/algorithms.rst notes the requirement where it already explains that the '2' strategies draw five parameter sets. Tests: the two population-floor tests become parametrized over all six strategies, pinning the floor at three for '1' and five for '2', single- and multi-island; two regression tests drive a below-five '2' population through got_result -- the async path at population 4 and the island path at population 24 over 8 islands -- both of which raised before and now complete a generation. The six '1'-strategy cases pass against the old code as controls; the eight '2'-strategy cases fail against it. Signed-off-by: Bill Hlavacek --- docs/algorithms.rst | 2 +- .../optimizers/differential_evolution.py | 50 +++++++------ tests/test_diff_evolution.py | 70 ++++++++++++++++--- 3 files changed, 90 insertions(+), 32 deletions(-) diff --git a/docs/algorithms.rst b/docs/algorithms.rst index 7287e2e2..f57ad6d1 100644 --- a/docs/algorithms.rst +++ b/docs/algorithms.rst @@ -113,7 +113,7 @@ Implementation details We maintain a list of ``population_size`` current parameter sets, and in each iteration, ``population_size`` new parameter sets are proposed. The method to propose a new parameter set is specified by the config key ``de_strategy``. The default setting ``rand1`` works best for most problems, and runs as follows: We choose 3 random parameter sets p1, p2, and p3 in the current population. For each free parameter P, the new parameter set is assigned the value p1[P] + ``mutation_factor`` * (p2[P]-p3[P]) with probability ``mutation_rate``. Otherwise it keeps p1[P], or, with ``de_cross_with_target = 1``, the value held by the parameter set it will replace, the one with the same index in the current population, as published differential evolution does (see :ref:`alg-de-target`). The new parameter set replaces the parameter set with the same index in the current population if it has a lower objective value. -With ``de_strategy`` of ``best1`` or ``best2``, we force the above p1 to be the parameter set with the lowest objective value. With ``de_strategy`` of ``all1`` or ``all2``, we force p1 to be the parameter set at the same index we are proposing to replace. The ``best`` strategy results in fast convergence to what is likely only a local optimum. The ``all`` strategy converges more slowly, and prevents the entire population from converging to the same value. However, there is still a risk of each member of the population becoming stuck in its own local minimum. For the ``de_strategy``\ s ending in ``2``, we instead choose a total of 5 parameter sets, p1 through p5, and set the new parameter value as p1[P] + ``mutation_factor`` * (p2[P]-p3[P] + p4[P]-p5[P]) +With ``de_strategy`` of ``best1`` or ``best2``, we force the above p1 to be the parameter set with the lowest objective value. With ``de_strategy`` of ``all1`` or ``all2``, we force p1 to be the parameter set at the same index we are proposing to replace. The ``best`` strategy results in fast convergence to what is likely only a local optimum. The ``all`` strategy converges more slowly, and prevents the entire population from converging to the same value. However, there is still a risk of each member of the population becoming stuck in its own local minimum. For the ``de_strategy``\ s ending in ``2``, we instead choose a total of 5 parameter sets, p1 through p5, and set the new parameter value as p1[P] + ``mutation_factor`` * (p2[P]-p3[P] + p4[P]-p5[P]). Because these are drawn as distinct parameter sets, a ``2`` strategy needs a ``population_size`` of at least 5, where the ``1`` strategies need only 3 (per island, for the island-based version); a smaller ``population_size`` is increased to that minimum with a warning. Asynchronous version """""""""""""""""""" diff --git a/pybnf/algorithms/optimizers/differential_evolution.py b/pybnf/algorithms/optimizers/differential_evolution.py index 61c55371..33bcc453 100644 --- a/pybnf/algorithms/optimizers/differential_evolution.py +++ b/pybnf/algorithms/optimizers/differential_evolution.py @@ -230,6 +230,14 @@ def __init__(self, config): if self.strategy not in options: raise PybnfError('Invalid differential evolution strategy "{}". Options are: {}'.format(self.strategy, ','.join(options))) + # How many distinct members each candidate is built from, which is the floor on + # the population (per island): the '1' strategies draw a base and one donor pair + # (3), the '2' strategies a base and two donor pairs (5). new_individual draws + # exactly this many with replace=False, so a smaller population makes that draw + # raise (#708); the subclasses clamp population_size up to it and each is the + # single source of truth for the other. + self.min_population = 3 if '1' in self.strategy else 5 + # The learned mutation settings (#667, ADR-0142): whether to learn them, how much # to remember, one success history per island (``ade``: one), and the settings of # every candidate still in flight, keyed by the candidate, since ``ade`` returns @@ -283,12 +291,11 @@ def new_individual(self, individuals, base_index=None, island=0, target_index=No """ # Choose a starting parameter set (either a random one or the base_index specified) - # and others to cross over (always random) - - if '1' in self.strategy: - pickn = 3 - else: - pickn = 5 + # and others to cross over (always random). The number of distinct members a + # candidate needs -- a base plus one donor pair ('1') or two ('2') -- is the same + # count the population is floored at (see min_population), so the subclasses + # guarantee this draw has enough to draw from (#708). + pickn = self.min_population # Choose pickn random unique indices, or if base_index was given, choose base_index followed by pickn-1 unique # indices @@ -611,16 +618,18 @@ def __init__(self, config): self.num_islands = config.config['islands'] self.num_per_island = int(config.config['population_size'] / self.num_islands) - if self.num_per_island < 3: - self.num_per_island = 3 + if self.num_per_island < self.min_population: + self.num_per_island = self.min_population if self.num_islands == 1: - print1('Differential evolution requires a population size of at least 3. Increased the population size ' - 'to 3.') - logger.warning('Increased population size to minimum allowed value of 3') + print1('Differential evolution with de_strategy "%s" requires a population size of at least %i. ' + 'Increased the population size to %i.' + % (self.strategy, self.min_population, self.min_population)) + logger.warning('Increased population size to minimum allowed value of %i' % self.min_population) else: - print1('Island-based differential evolution requires a population size of at least 3 times ' - 'the number of islands. Increased the population size to %i.' % (3*self.num_islands)) - logger.warning('Increased population size to minimum allowed value of 3 per island') + print1('Island-based differential evolution with de_strategy "%s" requires a population size of at ' + 'least %i times the number of islands. Increased the population size to %i.' + % (self.strategy, self.min_population, self.min_population * self.num_islands)) + logger.warning('Increased population size to minimum allowed value of %i per island' % self.min_population) if config.config['population_size'] % config.config['islands'] != 0: logger.warning('Reduced population_size to %i to evenly distribute it over %i islands' % (self.num_islands * self.num_per_island, self.num_islands)) @@ -901,12 +910,13 @@ def __init__(self, config): super().__init__(config) self.population_size = config.config['population_size'] - if self.population_size < 3: - self.population_size = 3 - self.config.config['population_size'] = 3 - print1('Asynchronous differential evolution requires a population size of at least 3. ' - 'Increasing the population size to 3.') - logger.warning('Increased population_size to the minimum allowed value of 3') + if self.population_size < self.min_population: + self.population_size = self.min_population + self.config.config['population_size'] = self.min_population + print1('Asynchronous differential evolution with de_strategy "%s" requires a population size of at least ' + '%i. Increasing the population size to %i.' + % (self.strategy, self.min_population, self.min_population)) + logger.warning('Increased population_size to the minimum allowed value of %i' % self.min_population) self.sims_completed = 0 self.individuals = [] # List of individuals diff --git a/tests/test_diff_evolution.py b/tests/test_diff_evolution.py index 88c2d8ab..8c5741a0 100644 --- a/tests/test_diff_evolution.py +++ b/tests/test_diff_evolution.py @@ -255,13 +255,38 @@ class TestDifferentialEvolutionPlumbing: d1s.data = d1s._read_file_lines( ['# time v1_result v2_result v3_result\n', ' 1 2.1 3.1 6.1\n'], r'\s+') - def test_population_floored_to_three_per_island(self, tmp_path): - """Oracle (minimum population): DE needs >= 3 individuals per island, so a - too-small population is bumped to 3 per island, single- or multi-island.""" - assert algorithms.DifferentialEvolution(_de_config(tmp_path, population_size=2, - islands=1)).num_per_island == 3 - assert algorithms.DifferentialEvolution(_de_config(tmp_path, population_size=2, - islands=2)).num_per_island == 3 + @pytest.mark.parametrize("strategy, floor", [ + ('rand1', 3), ('best1', 3), ('all1', 3), + ('rand2', 5), ('best2', 5), ('all2', 5), + ]) + def test_population_floored_to_strategy_minimum_per_island(self, tmp_path, strategy, floor): + """Oracle (minimum population, #708): DE needs the strategy's donor count per + island -- 3 for a '1' strategy, 5 for a '2' -- so a too-small population is + bumped up to that floor per island, single- or multi-island.""" + assert algorithms.DifferentialEvolution( + _de_config(tmp_path, de_strategy=strategy, population_size=2, + islands=1)).num_per_island == floor + assert algorithms.DifferentialEvolution( + _de_config(tmp_path, de_strategy=strategy, population_size=2, + islands=2)).num_per_island == floor + + def test_two_strategy_per_island_floor_catches_reasonable_population(self, tmp_path): + """Regression (#708, island variant): population_size=24 over 8 islands gives 3 + per island -- above the old floor of 3, so no clamp fired and the printed + message misinformed -- yet a '2' strategy draws 5 donors per island. The 5 floor + now bumps it, so an island can complete a generation instead of crashing its + first proposal's donor draw.""" + de = algorithms.DifferentialEvolution( + _de_config(tmp_path, de_strategy='rand2', population_size=24, islands=8)) + assert de.num_per_island == 5 + start = de.start_run() + island0 = [ps for ps in start if de.island_map[ps][0] == 0] + assert len(island0) == 5 + out = None + for ps in island0: + res = algorithms.Result(ps, self.d1s, ps.name); res.score = 5.0 + out = de.got_result(res) + assert isinstance(out, list) and len(out) == 5 # island 0's next generation def test_population_reduced_to_divide_islands(self, tmp_path): """Oracle (even split): num_per_island = floor(population_size/islands), so @@ -397,10 +422,33 @@ class TestAsyncDifferentialEvolution: d1s.data = d1s._read_file_lines( ['# time v1_result v2_result v3_result\n', ' 1 2.1 3.1 6.1\n'], r'\s+') - def test_population_floored_to_three(self, tmp_path): - """Oracle (minimum population): a population below 3 is bumped to 3.""" - ade = algorithms.AsynchronousDifferentialEvolution(_ade_config(tmp_path, population_size=2)) - assert ade.population_size == 3 + @pytest.mark.parametrize("strategy, floor", [ + ('rand1', 3), ('best1', 3), ('all1', 3), + ('rand2', 5), ('best2', 5), ('all2', 5), + ]) + def test_population_floored_to_strategy_minimum(self, tmp_path, strategy, floor): + """Oracle (minimum population, #708): each candidate is built from a base plus + one donor pair ('1', 3 members) or two donor pairs ('2', 5 members), so a + population below that count is bumped up to it. The '2' floor of 5 is the fix: + before it, a '2' strategy with a population of 2, 3 or 4 crashed the first + replacement's donor draw (new_individual's rng.choice).""" + ade = algorithms.AsynchronousDifferentialEvolution( + _ade_config(tmp_path, de_strategy=strategy, population_size=2)) + assert ade.population_size == floor + + def test_two_strategy_below_five_survives_first_result(self, tmp_path): + """Regression (#708): a '2' strategy with a population of 4 -- above the old + floor of 3, below the 5 donors it draws -- used to raise ValueError from + rng.choice on the first result. The clamp to 5 makes the whole population large + enough, so driving a result through got_result now spawns a replacement instead + of crashing.""" + ade = algorithms.AsynchronousDifferentialEvolution( + _ade_config(tmp_path, de_strategy='best2', population_size=4)) + start = ade.start_run() + assert len(start) == 5 + res = algorithms.Result(start[0], self.d1s, start[0].name); res.score = 1.0 + out = ade.got_result(res) + assert isinstance(out, list) and len(out) == 1 def test_reset_clears_state(self, tmp_path): """Oracle (reset invariant): reset() empties the population and fitness