fix(algorithms): the differential evolution population floor is the strategy's donor count, not always three (#708)

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
 """"""""""""""""""""

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

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