fix(algorithms): the differential evolution population floor is the strategy's donor count, not always three (#708) - #731
Merged
Conversation
…trategy'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 <hlavacek@lanl.gov>
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.
Fixes #708.
Problem
The
2differential-evolution strategies (rand2,best2,all2) build each candidate from five distinct parameter sets, but both population floors were hardcoded to three and never consultedde_strategy.new_individualdraws five donors withrng.choice(..., replace=False), which raisesValueError: Cannot take a larger sample than population when replace is Falsewhenever the population it draws from (per island) holds fewer than five. The failure is an unhandled exception on the first proposal, not a wrong answer.Two paths reached it:
population_size = 4withbest2cleared the< 3clamp untouched and crashed when the first result returned.population_size = 24over 8 islands gives 3 per island -- which looks reasonable, draws no warning, and crashes the first island to finish a generation. The message it would have printed ("at least 3 times the number of islands") misinformed.Fix
min_populationis computed once inDifferentialEvolutionBase.__init__(three for a1strategy, five for a2), right afterde_strategyis validated.new_individualnow drawsmin_populationdonors 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 tomin_populationwith the same clamp-and-warn treatment the old floor of three already had, and the warning now namesde_strategyand the required size.This is the issue's first suggested fix, clamp and warn, chosen over raising so that a too-small population behaves for a
2strategy exactly as it does for a1strategy today.Tests
1and five for2, single- and multi-island.2population throughgot_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.1-strategy cases pass (controls); the eight2-strategy cases fail.docs/algorithms.rstnotes the requirement where it already explains that the2strategies draw five parameter sets.