Skip to content

Commit 6cd697f

Browse files
triceotriceo
authored
fix: invert the probability of acceptance in fading tabu (#2358)
The closer the item is to becoming non-tabu, the higher should its acceptance probability be. (This has been a long-standing bug which went unnoticed.) Also made sure that, when the chance is 0 or 1, that the RNG is not called. RNG is expensive, and it affects randomness in other places of the solver that use the same RNG.
1 parent 9bc0b72 commit 6cd697f

8 files changed

Lines changed: 486 additions & 79 deletions

File tree

‎core/src/main/java/ai/timefold/solver/core/impl/localsearch/decider/acceptor/AcceptorFactory.java‎

Lines changed: 71 additions & 65 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717
import ai.timefold.solver.core.impl.localsearch.decider.acceptor.lateacceptance.LateAcceptanceAcceptor;
1818
import ai.timefold.solver.core.impl.localsearch.decider.acceptor.simulatedannealing.SimulatedAnnealingAcceptor;
1919
import ai.timefold.solver.core.impl.localsearch.decider.acceptor.stepcountinghillclimbing.StepCountingHillClimbingAcceptor;
20+
import ai.timefold.solver.core.impl.localsearch.decider.acceptor.tabu.AbstractTabuAcceptor;
2021
import ai.timefold.solver.core.impl.localsearch.decider.acceptor.tabu.EntityTabuAcceptor;
2122
import ai.timefold.solver.core.impl.localsearch.decider.acceptor.tabu.MoveTabuAcceptor;
2223
import ai.timefold.solver.core.impl.localsearch.decider.acceptor.tabu.ValueTabuAcceptor;
@@ -54,15 +55,14 @@ public Acceptor<Solution_> buildAcceptor(HeuristicConfigPolicy<Solution_> config
5455
.collect(Collectors.toList());
5556

5657
if (acceptorList.size() == 1) {
57-
return acceptorList.get(0);
58+
return acceptorList.getFirst();
5859
} else if (acceptorList.size() > 1) {
5960
return new CompositeAcceptor<>(acceptorList);
6061
} else {
61-
throw new IllegalArgumentException(
62-
"The acceptor does not specify any acceptorType (" + acceptorConfig.getAcceptorTypeList()
63-
+ ") or other acceptor property.\n"
64-
+ "For a good starting values,"
65-
+ " see the docs section \"Which optimization algorithms should I use?\".");
62+
throw new IllegalArgumentException("""
63+
The acceptor does not specify any acceptorType (%s) or other acceptor property.
64+
For good starting values, see the docs section "Which optimization algorithms should I use?"."""
65+
.formatted(acceptorConfig.getAcceptorTypeList()));
6666
}
6767
}
6868

@@ -94,34 +94,35 @@ private Optional<StepCountingHillClimbingAcceptor<Solution_>> buildStepCountingH
9494
}
9595

9696
private Optional<EntityTabuAcceptor<Solution_>> buildEntityTabuAcceptor(HeuristicConfigPolicy<Solution_> configPolicy) {
97+
var entityTabuSize = acceptorConfig.getEntityTabuSize();
98+
var entityTabuRatio = acceptorConfig.getEntityTabuRatio();
99+
var fadingEntityTabuSize = acceptorConfig.getFadingEntityTabuSize();
100+
var fadingEntityTabuRatio = acceptorConfig.getFadingEntityTabuRatio();
97101
if (acceptorTypeListsContainsAcceptorType(AcceptorType.ENTITY_TABU)
98-
|| acceptorConfig.getEntityTabuSize() != null || acceptorConfig.getEntityTabuRatio() != null
99-
|| acceptorConfig.getFadingEntityTabuSize() != null || acceptorConfig.getFadingEntityTabuRatio() != null) {
102+
|| entityTabuSize != null || entityTabuRatio != null
103+
|| fadingEntityTabuSize != null || fadingEntityTabuRatio != null) {
100104
var acceptor = new EntityTabuAcceptor<Solution_>(configPolicy.getLogIndentation());
101-
if (acceptorConfig.getEntityTabuSize() != null) {
102-
if (acceptorConfig.getEntityTabuRatio() != null) {
103-
throw new IllegalArgumentException("The acceptor cannot have both acceptorConfig.getEntityTabuSize() ("
104-
+ acceptorConfig.getEntityTabuSize() + ") and acceptorConfig.getEntityTabuRatio() ("
105-
+ acceptorConfig.getEntityTabuRatio() + ").");
105+
if (entityTabuSize != null) {
106+
if (entityTabuRatio != null) {
107+
throw new IllegalArgumentException(
108+
"The acceptor cannot have both entityTabuSize (%d) and entityTabuRatio (%f)."
109+
.formatted(entityTabuSize, entityTabuRatio));
106110
}
107-
acceptor.setTabuSizeStrategy(new FixedTabuSizeStrategy<>(acceptorConfig.getEntityTabuSize()));
108-
} else if (acceptorConfig.getEntityTabuRatio() != null) {
109-
acceptor.setTabuSizeStrategy(new EntityRatioTabuSizeStrategy<>(acceptorConfig.getEntityTabuRatio()));
110-
} else if (acceptorConfig.getFadingEntityTabuSize() == null && acceptorConfig.getFadingEntityTabuRatio() == null) {
111+
acceptor.setTabuSizeStrategy(new FixedTabuSizeStrategy<>(entityTabuSize));
112+
} else if (entityTabuRatio != null) {
113+
acceptor.setTabuSizeStrategy(new EntityRatioTabuSizeStrategy<>(entityTabuRatio));
114+
} else if (fadingEntityTabuSize == null && fadingEntityTabuRatio == null) {
111115
acceptor.setTabuSizeStrategy(new EntityRatioTabuSizeStrategy<>(0.1));
112116
}
113-
if (acceptorConfig.getFadingEntityTabuSize() != null) {
114-
if (acceptorConfig.getFadingEntityTabuRatio() != null) {
117+
if (fadingEntityTabuSize != null) {
118+
if (fadingEntityTabuRatio != null) {
115119
throw new IllegalArgumentException(
116-
"The acceptor cannot have both acceptorConfig.getFadingEntityTabuSize() ("
117-
+ acceptorConfig.getFadingEntityTabuSize()
118-
+ ") and acceptorConfig.getFadingEntityTabuRatio() ("
119-
+ acceptorConfig.getFadingEntityTabuRatio() + ").");
120+
"The acceptor cannot have both fadingEntityTabuSize (%d) and fadingEntityTabuRatio (%f)."
121+
.formatted(fadingEntityTabuSize, fadingEntityTabuRatio));
120122
}
121-
acceptor.setFadingTabuSizeStrategy(new FixedTabuSizeStrategy<>(acceptorConfig.getFadingEntityTabuSize()));
122-
} else if (acceptorConfig.getFadingEntityTabuRatio() != null) {
123-
acceptor.setFadingTabuSizeStrategy(
124-
new EntityRatioTabuSizeStrategy<>(acceptorConfig.getFadingEntityTabuRatio()));
123+
acceptor.setFadingTabuSizeStrategy(new FixedTabuSizeStrategy<>(fadingEntityTabuSize));
124+
} else if (fadingEntityTabuRatio != null) {
125+
acceptor.setFadingTabuSizeStrategy(new EntityRatioTabuSizeStrategy<>(fadingEntityTabuRatio));
125126
}
126127
if (configPolicy.getEnvironmentMode().isFullyAsserted()) {
127128
acceptor.setAssertTabuHashCodeCorrectness(true);
@@ -132,43 +133,47 @@ private Optional<EntityTabuAcceptor<Solution_>> buildEntityTabuAcceptor(Heuristi
132133
}
133134

134135
private Optional<ValueTabuAcceptor<Solution_>> buildValueTabuAcceptor(HeuristicConfigPolicy<Solution_> configPolicy) {
136+
var valueTabuSize = acceptorConfig.getValueTabuSize();
137+
var fadingValueTabuSize = acceptorConfig.getFadingValueTabuSize();
135138
if (acceptorTypeListsContainsAcceptorType(AcceptorType.VALUE_TABU)
136-
|| acceptorConfig.getValueTabuSize() != null || acceptorConfig.getFadingValueTabuSize() != null) {
137-
var acceptor = new ValueTabuAcceptor<Solution_>(configPolicy.getLogIndentation());
138-
if (acceptorConfig.getValueTabuSize() != null) {
139-
acceptor.setTabuSizeStrategy(new FixedTabuSizeStrategy<>(acceptorConfig.getValueTabuSize()));
140-
}
141-
if (acceptorConfig.getFadingValueTabuSize() != null) {
142-
acceptor.setFadingTabuSizeStrategy(new FixedTabuSizeStrategy<>(acceptorConfig.getFadingValueTabuSize()));
143-
}
144-
145-
if (acceptorConfig.getValueTabuSize() != null) {
146-
acceptor.setTabuSizeStrategy(new FixedTabuSizeStrategy<>(acceptorConfig.getValueTabuSize()));
147-
}
148-
if (acceptorConfig.getFadingValueTabuSize() != null) {
149-
acceptor.setFadingTabuSizeStrategy(new FixedTabuSizeStrategy<>(acceptorConfig.getFadingValueTabuSize()));
150-
}
151-
if (configPolicy.getEnvironmentMode().isFullyAsserted()) {
152-
acceptor.setAssertTabuHashCodeCorrectness(true);
139+
|| valueTabuSize != null || fadingValueTabuSize != null) {
140+
if (valueTabuSize == null && fadingValueTabuSize == null) {
141+
throw new IllegalArgumentException(
142+
"The acceptorType (%s) requires either valueTabuSize or fadingValueTabuSize to be configured."
143+
.formatted(AcceptorType.VALUE_TABU));
153144
}
145+
var acceptor = new ValueTabuAcceptor<Solution_>(configPolicy.getLogIndentation());
146+
configureFixedSizeTabuAcceptor(acceptor, configPolicy, valueTabuSize, fadingValueTabuSize);
154147
return Optional.of(acceptor);
155148
}
156149
return Optional.empty();
157150
}
158151

152+
private static <Solution_> void configureFixedSizeTabuAcceptor(AbstractTabuAcceptor<Solution_> acceptor,
153+
HeuristicConfigPolicy<Solution_> configPolicy, Integer tabuSize, Integer fadingTabuSize) {
154+
if (tabuSize != null) {
155+
acceptor.setTabuSizeStrategy(new FixedTabuSizeStrategy<>(tabuSize));
156+
}
157+
if (fadingTabuSize != null) {
158+
acceptor.setFadingTabuSizeStrategy(new FixedTabuSizeStrategy<>(fadingTabuSize));
159+
}
160+
if (configPolicy.getEnvironmentMode().isFullyAsserted()) {
161+
acceptor.setAssertTabuHashCodeCorrectness(true);
162+
}
163+
}
164+
159165
private Optional<MoveTabuAcceptor<Solution_>> buildMoveTabuAcceptor(HeuristicConfigPolicy<Solution_> configPolicy) {
166+
var moveTabuSize = acceptorConfig.getMoveTabuSize();
167+
var fadingMoveTabuSize = acceptorConfig.getFadingMoveTabuSize();
160168
if (acceptorTypeListsContainsAcceptorType(AcceptorType.MOVE_TABU)
161-
|| acceptorConfig.getMoveTabuSize() != null || acceptorConfig.getFadingMoveTabuSize() != null) {
162-
var acceptor = new MoveTabuAcceptor<Solution_>(configPolicy.getLogIndentation());
163-
if (acceptorConfig.getMoveTabuSize() != null) {
164-
acceptor.setTabuSizeStrategy(new FixedTabuSizeStrategy<>(acceptorConfig.getMoveTabuSize()));
165-
}
166-
if (acceptorConfig.getFadingMoveTabuSize() != null) {
167-
acceptor.setFadingTabuSizeStrategy(new FixedTabuSizeStrategy<>(acceptorConfig.getFadingMoveTabuSize()));
168-
}
169-
if (configPolicy.getEnvironmentMode().isFullyAsserted()) {
170-
acceptor.setAssertTabuHashCodeCorrectness(true);
169+
|| moveTabuSize != null || fadingMoveTabuSize != null) {
170+
if (moveTabuSize == null && fadingMoveTabuSize == null) {
171+
throw new IllegalArgumentException(
172+
"The acceptorType (%s) requires either moveTabuSize or fadingMoveTabuSize to be configured."
173+
.formatted(AcceptorType.MOVE_TABU));
171174
}
175+
var acceptor = new MoveTabuAcceptor<Solution_>(configPolicy.getLogIndentation());
176+
configureFixedSizeTabuAcceptor(acceptor, configPolicy, moveTabuSize, fadingMoveTabuSize);
172177
return Optional.of(acceptor);
173178
}
174179
return Optional.empty();
@@ -181,9 +186,9 @@ private Optional<MoveTabuAcceptor<Solution_>> buildMoveTabuAcceptor(HeuristicCon
181186
var acceptor = new SimulatedAnnealingAcceptor<Solution_>();
182187
if (acceptorConfig.getSimulatedAnnealingStartingTemperature() == null) {
183188
// TODO Support SA without a parameter
184-
throw new IllegalArgumentException("The acceptorType (" + AcceptorType.SIMULATED_ANNEALING
185-
+ ") currently requires a acceptorConfig.getSimulatedAnnealingStartingTemperature() ("
186-
+ acceptorConfig.getSimulatedAnnealingStartingTemperature() + ").");
189+
throw new IllegalArgumentException(
190+
"The acceptorType (%s) requires non-null acceptorConfig.getSimulatedAnnealingStartingTemperature()."
191+
.formatted(AcceptorType.SIMULATED_ANNEALING));
187192
}
188193
acceptor.setStartingTemperature(
189194
configPolicy.getScoreDefinition().parseScore(acceptorConfig.getSimulatedAnnealingStartingTemperature()));
@@ -221,19 +226,20 @@ private Optional<GreatDelugeAcceptor<Solution_>> buildGreatDelugeAcceptor(Heuris
221226
var acceptor = new GreatDelugeAcceptor<Solution_>();
222227
if (acceptorConfig.getGreatDelugeWaterLevelIncrementScore() != null) {
223228
if (acceptorConfig.getGreatDelugeWaterLevelIncrementRatio() != null) {
224-
throw new IllegalArgumentException("The acceptor cannot have both a "
225-
+ "acceptorConfig.getGreatDelugeWaterLevelIncrementScore() ("
226-
+ acceptorConfig.getGreatDelugeWaterLevelIncrementScore()
227-
+ ") and a acceptorConfig.getGreatDelugeWaterLevelIncrementRatio() ("
228-
+ acceptorConfig.getGreatDelugeWaterLevelIncrementRatio() + ").");
229+
throw new IllegalArgumentException("""
230+
The acceptor cannot have both acceptorConfig.getGreatDelugeWaterLevelIncrementScore() (%s) \
231+
and acceptorConfig.getGreatDelugeWaterLevelIncrementRatio() (%s)."""
232+
.formatted(acceptorConfig.getGreatDelugeWaterLevelIncrementScore(),
233+
acceptorConfig.getGreatDelugeWaterLevelIncrementRatio()));
229234
}
230235
acceptor.setWaterLevelIncrementScore(
231236
configPolicy.getScoreDefinition().parseScore(acceptorConfig.getGreatDelugeWaterLevelIncrementScore()));
232237
} else if (acceptorConfig.getGreatDelugeWaterLevelIncrementRatio() != null) {
233238
if (acceptorConfig.getGreatDelugeWaterLevelIncrementRatio() <= 0.0) {
234-
throw new IllegalArgumentException("The acceptorConfig.getGreatDelugeWaterLevelIncrementRatio() ("
235-
+ acceptorConfig.getGreatDelugeWaterLevelIncrementRatio()
236-
+ ") must be positive because the water level should increase.");
239+
throw new IllegalArgumentException("""
240+
The acceptorConfig.getGreatDelugeWaterLevelIncrementRatio() (%s) must be positive \
241+
because the water level should increase."""
242+
.formatted(acceptorConfig.getGreatDelugeWaterLevelIncrementRatio()));
237243
}
238244
acceptor.setWaterLevelIncrementRatio(acceptorConfig.getGreatDelugeWaterLevelIncrementRatio());
239245
} else {

‎core/src/main/java/ai/timefold/solver/core/impl/localsearch/decider/acceptor/tabu/AbstractTabuAcceptor.java‎

Lines changed: 21 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -142,16 +142,14 @@ public boolean isAccepted(LocalSearchMoveScope<Solution_> moveScope) {
142142
logIndentation, moveScope.getMove());
143143
return false;
144144
}
145-
var acceptChance = calculateFadingTabuAcceptChance(tabuStepCount - workingTabuSize);
146-
var accepted = moveScope.getWorkingRandom().nextDouble() < acceptChance;
145+
var decision = decideFadingTabuAcceptance(moveScope, tabuStepCount - workingTabuSize);
146+
var accepted = decision > 0;
147147
if (accepted) {
148148
logger.trace("{} Proposed move ({}) is fading tabu with acceptChance ({}) and is accepted.",
149-
logIndentation,
150-
moveScope.getMove(), acceptChance);
149+
logIndentation, moveScope.getMove(), Math.abs(decision));
151150
} else {
152151
logger.trace("{} Proposed move ({}) is fading tabu with acceptChance ({}) and is not accepted.",
153-
logIndentation,
154-
moveScope.getMove(), acceptChance);
152+
logIndentation, moveScope.getMove(), Math.abs(decision));
155153
}
156154
return accepted;
157155
}
@@ -185,12 +183,24 @@ private int locateMaximumTabuStepIndex(LocalSearchMoveScope<Solution_> moveScope
185183

186184
/**
187185
* @param fadingTabuStepCount {@code 0 < fadingTabuStepCount <= fadingTabuSize}
188-
* @return {@code 0.0 < acceptChance < 1.0}
186+
* @return in absolute value, the accept chance;
187+
* negative signum or 0 means not accepted, positive signum means accepted.
188+
* This is hacky, but we can represent 2 things with one number
189+
* and avoid allocation new types for this multiple return.
189190
*/
190-
protected double calculateFadingTabuAcceptChance(int fadingTabuStepCount) {
191-
// The + 1's are because acceptChance should not be 0.0 or 1.0
192-
// when (fadingTabuStepCount == 0) or (fadingTabuStepCount + 1 == workingFadingTabuSize)
193-
return (workingFadingTabuSize - fadingTabuStepCount) / ((double) (workingFadingTabuSize + 1));
191+
private double decideFadingTabuAcceptance(LocalSearchMoveScope<Solution_> moveScope, int fadingTabuStepCount) {
192+
// Invert the chance; the longer the element is in the tabu list, the higher the chance should be.
193+
var numerator = workingFadingTabuSize - fadingTabuStepCount;
194+
if (numerator <= 0) { // The inverted chance would be >= 1.
195+
return 1.0d;
196+
}
197+
var denominator = workingFadingTabuSize + 1;
198+
if (numerator >= denominator) { // The inverted chance would be <= 0.
199+
return 0.0d;
200+
}
201+
var acceptChance = 1.0d - (numerator / (double) denominator);
202+
var accepted = moveScope.getWorkingRandom().nextDouble() < acceptChance;
203+
return accepted ? acceptChance : -acceptChance;
194204
}
195205

196206
/**

‎core/src/main/java/ai/timefold/solver/core/impl/localsearch/decider/acceptor/tabu/size/FixedTabuSizeStrategy.java‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -8,9 +8,9 @@ public final class FixedTabuSizeStrategy<Solution_> extends AbstractTabuSizeStra
88

99
public FixedTabuSizeStrategy(int tabuSize) {
1010
this.tabuSize = tabuSize;
11-
if (tabuSize < 0) {
12-
throw new IllegalArgumentException("The tabuSize (" + tabuSize
13-
+ ") cannot be negative.");
11+
if (tabuSize < 1) {
12+
throw new IllegalArgumentException("The tabuSize (%d) must be at least 1."
13+
.formatted(tabuSize));
1414
}
1515
}
1616

‎core/src/test/java/ai/timefold/solver/core/impl/localsearch/decider/acceptor/AcceptorFactoryTest.java‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -110,4 +110,22 @@ <Solution_> void diversifiedLateAcceptanceAcceptor() {
110110
AcceptorFactory<Solution_> badAcceptorFactory = AcceptorFactory.create(localSearchAcceptorConfig);
111111
assertThatIllegalStateException().isThrownBy(() -> badAcceptorFactory.buildAcceptor(heuristicConfigPolicy));
112112
}
113+
114+
@Test
115+
<Solution_> void valueTabuWithoutSizes_throwsException() {
116+
var config = new LocalSearchAcceptorConfig()
117+
.withAcceptorTypeList(List.of(AcceptorType.VALUE_TABU));
118+
var factory = AcceptorFactory.create(config);
119+
assertThatIllegalArgumentException()
120+
.isThrownBy(() -> factory.buildAcceptor(mock(HeuristicConfigPolicy.class)));
121+
}
122+
123+
@Test
124+
<Solution_> void moveTabuWithoutSizes_throwsException() {
125+
var config = new LocalSearchAcceptorConfig()
126+
.withAcceptorTypeList(List.of(AcceptorType.MOVE_TABU));
127+
var factory = AcceptorFactory.create(config);
128+
assertThatIllegalArgumentException()
129+
.isThrownBy(() -> factory.buildAcceptor(mock(HeuristicConfigPolicy.class)));
130+
}
113131
}

0 commit comments

Comments
 (0)