Skip to content

Commit 2bc6995

Browse files
zepfredzepfred
authored
fix: generate list unassigned moves fairly (#2219)
The list change move selector may be prematurely removed from the union move selector iterator during a given step when it picks a random value and all reachable entities from that value are pinned, leaving no valid entities available. We are not verifying whether any values are assigned, so at least one valid destination could be generated. This PR updates the `hasNext` logic of the iterator to address that. One downside of configuring the list change selector to return true whenever there are any assigned values is that it can generate no-change moves, which waste CPU cycles. This situation can occur if the planning value is unassigned and all of its reachable entities are pinned.
1 parent b96aa54 commit 2bc6995

File tree

7 files changed

+283
-10
lines changed

7 files changed

+283
-10
lines changed

‎core/src/main/java/ai/timefold/solver/core/impl/heuristic/selector/common/iterator/UpcomingSelectionIterator.java‎

Lines changed: 14 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -22,14 +22,24 @@ public abstract class UpcomingSelectionIterator<S> extends SelectionIterator<S>
2222

2323
protected boolean upcomingCreated = false;
2424
protected boolean hasUpcomingSelection = true;
25+
private boolean recheckUpcomingSelection = false;
2526
protected S upcomingSelection;
2627

27-
@Override
28-
public boolean hasNext() {
28+
private void readAndRecheckUpcomingValue() {
2929
if (!upcomingCreated) {
3030
upcomingSelection = createUpcomingSelection();
3131
upcomingCreated = true;
32+
if (recheckUpcomingSelection) {
33+
// We ensure that the variable remains consistent if "discardUpcomingSelection" was called previously.
34+
hasUpcomingSelection = upcomingSelection != null;
35+
recheckUpcomingSelection = false;
36+
}
3237
}
38+
}
39+
40+
@Override
41+
public boolean hasNext() {
42+
readAndRecheckUpcomingValue();
3343
return hasUpcomingSelection;
3444
}
3545

@@ -38,9 +48,7 @@ public S next() {
3848
if (!hasUpcomingSelection) {
3949
throw new NoSuchElementException();
4050
}
41-
if (!upcomingCreated) {
42-
upcomingSelection = createUpcomingSelection();
43-
}
51+
readAndRecheckUpcomingValue();
4452
upcomingCreated = false;
4553
return upcomingSelection;
4654
}
@@ -54,6 +62,7 @@ protected S noUpcomingSelection() {
5462

5563
public void discardUpcomingSelection() {
5664
upcomingCreated = false;
65+
recheckUpcomingSelection = true;
5766
}
5867

5968
@Override

‎core/src/main/java/ai/timefold/solver/core/impl/heuristic/selector/list/ElementDestinationSelector.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -128,7 +128,7 @@ public Iterator<ElementPosition> iterator() {
128128
var totalSize = Math.addExact(entitySelector.getSize(), totalValueSize);
129129
return new ElementPositionRandomIterator<>(listVariableStateSupply, entitySelector,
130130
replayingValueSelector != null ? replayingValueSelector.iterator() : null, valueSelector, workingRandom,
131-
totalSize, allowsUnassignedValues);
131+
totalSize, allowsUnassignedValues, allowsUnassignedValues && totalValueSize > 0);
132132
} else {
133133
if (entitySelector.getSize() == 0) {
134134
return Collections.emptyIterator();

‎core/src/main/java/ai/timefold/solver/core/impl/heuristic/selector/list/ElementPositionRandomIterator.java‎

Lines changed: 17 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
package ai.timefold.solver.core.impl.heuristic.selector.list;
22

33
import java.util.Iterator;
4+
import java.util.NoSuchElementException;
45
import java.util.random.RandomGenerator;
56

67
import ai.timefold.solver.core.impl.domain.variable.ListVariableStateSupply;
@@ -23,14 +24,15 @@ final class ElementPositionRandomIterator<Solution_> implements Iterator<Element
2324
private final RandomGenerator workingRandom;
2425
private final long totalSize;
2526
private final boolean allowsUnassignedValues;
27+
private final boolean maybeMovableValues;
2628
private Iterator<Object> valueIterator;
2729
private Object selectedValue;
2830
private boolean hasNextValue = false;
2931

3032
public ElementPositionRandomIterator(ListVariableStateSupply<Solution_, Object, Object> listVariableStateSupply,
3133
EntitySelector<Solution_> entitySelector, Iterator<Object> replayingValueIterator,
3234
IterableValueSelector<Solution_> valueSelector, RandomGenerator workingRandom, long totalSize,
33-
boolean allowsUnassignedValues) {
35+
boolean allowsUnassignedValues, boolean maybeMovableValues) {
3436
this.listVariableStateSupply = listVariableStateSupply;
3537
this.listVariableDescriptor = listVariableStateSupply.getSourceVariableDescriptor();
3638
this.entitySelector = entitySelector;
@@ -44,6 +46,7 @@ public ElementPositionRandomIterator(ListVariableStateSupply<Solution_, Object,
4446
.formatted(totalSize));
4547
}
4648
this.allowsUnassignedValues = allowsUnassignedValues;
49+
this.maybeMovableValues = maybeMovableValues;
4750
this.valueIterator = null;
4851
}
4952

@@ -72,9 +75,16 @@ private boolean hasNextValue() {
7275
// and the entity iterator must discard the previous entity
7376
tryUpdateEntityIterator();
7477
}
75-
return entityIterator.hasNext();
78+
// There will be a valid destination if the entity iterator has a next element
79+
// or if there is at least one non-pinned assigned value,
80+
// which would result in an unassigning move.
81+
return entityIterator.hasNext() || maybeMovableValues;
7682
}
77-
return selectedValue != null && entityIterator.hasNext();
83+
// There will be a valid destination if the entity iterator has a next element
84+
// or if there is at least one non-pinned assigned value,
85+
// which would result in an unassigning move.
86+
return selectedValue != null
87+
&& (entityIterator.hasNext() || maybeMovableValues);
7888
}
7989

8090
@Override
@@ -88,6 +98,9 @@ public boolean hasNext() {
8898

8999
@Override
90100
public ElementPosition next() {
101+
if (!hasNextValue) {
102+
throw new NoSuchElementException();
103+
}
91104
this.hasNextValue = false;
92105
// This code operates under the assumption that the entity selector already filtered out all immovable entities.
93106
// At this point, entities are only partially pinned, or not pinned at all.
@@ -96,7 +109,7 @@ public ElementPosition next() {
96109
// to account for the unassigned destination, which is an extra element.
97110
var entityBoundary = allowsUnassignedValues ? entitySize + 1 : entitySize;
98111
var random = RandomUtils.nextLong(workingRandom, allowsUnassignedValues ? totalSize + 1 : totalSize);
99-
if (allowsUnassignedValues && random == 0) {
112+
if (allowsUnassignedValues && (random == 0 || !entityIterator.hasNext())) {
100113
// We have already excluded all unassigned elements,
101114
// the only way to get an unassigned destination is to explicitly add it.
102115
return ElementPosition.unassigned();

‎core/src/test/java/ai/timefold/solver/core/impl/heuristic/selector/list/ElementDestinationSelectorTest.java‎

Lines changed: 107 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@
1212
import static ai.timefold.solver.core.testdomain.list.TestdataListUtils.mockEntitySelector;
1313
import static ai.timefold.solver.core.testdomain.list.TestdataListUtils.mockIterableFromEntityPropertyValueSelector;
1414
import static ai.timefold.solver.core.testdomain.list.TestdataListUtils.mockIterableValueSelector;
15+
import static ai.timefold.solver.core.testdomain.list.TestdataListUtils.mockUpcomingSelectionIterator;
1516
import static ai.timefold.solver.core.testutil.PlannerAssert.assertAllCodesOfIterableSelector;
1617
import static ai.timefold.solver.core.testutil.PlannerAssert.assertAllCodesOfIterator;
1718
import static ai.timefold.solver.core.testutil.PlannerAssert.assertCodesOfNeverEndingIterableSelector;
@@ -21,25 +22,33 @@
2122
import static ai.timefold.solver.core.testutil.PlannerAssert.verifyPhaseLifecycle;
2223
import static ai.timefold.solver.core.testutil.PlannerTestUtils.mockScoreDirector;
2324
import static org.assertj.core.api.AssertionsForClassTypes.assertThat;
25+
import static org.assertj.core.api.AssertionsForClassTypes.assertThatCode;
2426
import static org.mockito.Mockito.doReturn;
2527
import static org.mockito.Mockito.mock;
2628
import static org.mockito.Mockito.times;
2729
import static org.mockito.Mockito.verify;
2830

2931
import java.util.Collections;
3032
import java.util.List;
33+
import java.util.NoSuchElementException;
3134
import java.util.Random;
3235

3336
import ai.timefold.solver.core.api.solver.SolutionManager;
3437
import ai.timefold.solver.core.config.heuristic.selector.common.SelectionCacheType;
38+
import ai.timefold.solver.core.impl.heuristic.selector.common.decorator.SelectionFilter;
3539
import ai.timefold.solver.core.impl.heuristic.selector.common.iterator.UpcomingSelectionIterator;
3640
import ai.timefold.solver.core.impl.heuristic.selector.entity.FromSolutionEntitySelector;
3741
import ai.timefold.solver.core.impl.heuristic.selector.entity.decorator.FilteringEntityByValueSelector;
42+
import ai.timefold.solver.core.impl.heuristic.selector.entity.decorator.FilteringEntitySelector;
3843
import ai.timefold.solver.core.impl.heuristic.selector.value.IterableValueSelector;
3944
import ai.timefold.solver.core.impl.heuristic.selector.value.decorator.FilteringValueRangeSelector;
45+
import ai.timefold.solver.core.impl.heuristic.selector.value.mimic.ManualValueMimicRecorder;
46+
import ai.timefold.solver.core.impl.heuristic.selector.value.mimic.MimicReplayingValueSelector;
4047
import ai.timefold.solver.core.impl.localsearch.scope.LocalSearchPhaseScope;
4148
import ai.timefold.solver.core.impl.score.director.InnerScoreDirector;
4249
import ai.timefold.solver.core.impl.solver.scope.SolverScope;
50+
import ai.timefold.solver.core.preview.api.domain.metamodel.ElementPosition;
51+
import ai.timefold.solver.core.testdomain.TestdataValue;
4352
import ai.timefold.solver.core.testdomain.list.TestdataListEntity;
4453
import ai.timefold.solver.core.testdomain.list.TestdataListSolution;
4554
import ai.timefold.solver.core.testdomain.list.TestdataListUtils;
@@ -56,6 +65,8 @@
5665
import ai.timefold.solver.core.testdomain.list.valuerange.TestdataListEntityProvidingEntity;
5766
import ai.timefold.solver.core.testdomain.list.valuerange.TestdataListEntityProvidingSolution;
5867
import ai.timefold.solver.core.testdomain.list.valuerange.TestdataListEntityProvidingValue;
68+
import ai.timefold.solver.core.testdomain.list.valuerange.unassignedvar.pinned.TestdataListUnassignedPinnedEntityProvidingEntity;
69+
import ai.timefold.solver.core.testdomain.list.valuerange.unassignedvar.pinned.TestdataListUnassignedPinnedEntityProvidingSolution;
5970
import ai.timefold.solver.core.testutil.TestRandom;
6071

6172
import org.junit.jupiter.api.Test;
@@ -472,6 +483,65 @@ void randomFullyPinned() {
472483
"B[0]");
473484
}
474485

486+
@Test
487+
void refreshReachableEntities() {
488+
var v1 = new TestdataValue("1");
489+
var v2 = new TestdataValue("2");
490+
var a = new TestdataListUnassignedPinnedEntityProvidingEntity("A", List.of(v1)); // Pinned
491+
var b = new TestdataListUnassignedPinnedEntityProvidingEntity("B", List.of(v2)); // Not pinned
492+
// a is pinned
493+
a.setPinned(true);
494+
a.setValueList(List.of(v1));
495+
b.setValueList(List.of(v2));
496+
var solution = new TestdataListUnassignedPinnedEntityProvidingSolution();
497+
solution.setEntityList(List.of(a, b));
498+
SolutionManager.updateShadowVariables(solution);
499+
500+
var scoreDirector = mockScoreDirector(TestdataListUnassignedPinnedEntityProvidingSolution.buildSolutionDescriptor());
501+
scoreDirector.setWorkingSolution(solution);
502+
503+
// Value selector
504+
var listVariableDescriptor = TestdataListUnassignedPinnedEntityProvidingEntity.buildVariableDescriptorForValueList();
505+
var iterableValueSelector = mockIterableValueSelector(listVariableDescriptor, v1, v2);
506+
var mimicRecorder = new ManualValueMimicRecorder<>(iterableValueSelector);
507+
var replayingValueSelector = new MimicReplayingValueSelector<>(mimicRecorder);
508+
// Entity selector with non-pinned entity filtered by value
509+
var entityDescriptor = TestdataListUnassignedPinnedEntityProvidingEntity.buildEntityDescriptor();
510+
var entitySelector = new FromSolutionEntitySelector<>(entityDescriptor, SelectionCacheType.PHASE, true);
511+
var filteringEntity = new FilteringEntityByValueSelector<>(entitySelector, replayingValueSelector, true, false);
512+
var pinningFilterFunction = entityDescriptor.getEffectiveMovableEntityFilter();
513+
var nonPinnedEntitySelector = FilteringEntitySelector.of(filteringEntity, SelectionFilter
514+
.compose((director, selection) -> pinningFilterFunction.test(director.getWorkingSolution(), selection)));
515+
// Destination selector with non-pinned entity selector filtered by value
516+
var selector =
517+
new ElementDestinationSelector<>(nonPinnedEntitySelector, replayingValueSelector, iterableValueSelector, true,
518+
false);
519+
520+
// First, we select v1, which is pinned, and the entity iterator does not return a feasible destination.
521+
// However, the value selector has another assigned value,
522+
// which makes maybeMovableValues in ElementDestinationSelector to be set to true
523+
// We always return 0 to meet the bailout size,
524+
// and then return 1 to ensure the entity is selected by destination iterator
525+
var random = new TestRandom(0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1);
526+
var solverScope = solvingStarted(selector, scoreDirector, random);
527+
phaseStarted(selector, solverScope);
528+
var iterator = selector.iterator();
529+
mimicRecorder.setRecordedValue(v1);
530+
assertThat(iterator.hasNext()).isTrue();
531+
// The expected position is unassigned as v1 has no feasible destination
532+
assertThat(iterator.next()).isSameAs(ElementPosition.unassigned());
533+
// Next, we select v2, which is not pinned, and the entity iterator returns a feasible destination.
534+
// This will cause the iterator to call tryUpdateEntityIterator, and reload the entity list
535+
// b is the only reachable non-pinned entity for v2
536+
mimicRecorder.setRecordedValue(v2);
537+
assertThat(iterator.hasNext()).isTrue();
538+
var position = iterator.next().ensureAssigned();
539+
var entity = position.entity();
540+
var index = position.index();
541+
assertThat(entity).isSameAs(b);
542+
assertThat(index).isSameAs(0);
543+
}
544+
475545
@Test
476546
void emptyIfThereAreNoEntities() {
477547
var v1 = new TestdataListValue("1");
@@ -614,4 +684,41 @@ void discardOldValues() {
614684
// the entity iterator must discard the previous entity during the hasNext() calls
615685
verify(entityIterator, times(1)).discardUpcomingSelection();
616686
}
687+
688+
@Test
689+
void discardOldValuesAndResetState() {
690+
var v1 = new TestdataListEntityProvidingValue("V1");
691+
var v2 = new TestdataListEntityProvidingValue("V2");
692+
var a = new TestdataListEntityProvidingEntity("A", List.of(), List.of());
693+
var solution = new TestdataListEntityProvidingSolution();
694+
solution.setEntityList(List.of(a));
695+
696+
var scoreDirector = mockScoreDirector(TestdataListEntityProvidingSolution.buildSolutionDescriptor());
697+
scoreDirector.setWorkingSolution(solution);
698+
699+
var entitySelector = mockEntitySelector(a);
700+
var entityIterator = mockUpcomingSelectionIterator(a, null, a);
701+
doReturn(entityIterator).when(entitySelector).iterator();
702+
var valueSelector = mockIterableValueSelector(getEntityRangeListVariableDescriptor(scoreDirector), v1);
703+
IterableValueSelector<TestdataListEntityProvidingSolution> replayingValueSelector =
704+
mockReplayingValueSelector(getEntityRangeListVariableDescriptor(scoreDirector), v1, v1, v2);
705+
706+
var selector = new ElementDestinationSelector<>(entitySelector, replayingValueSelector, valueSelector, true, false);
707+
// Value 0 makes the iterator to always request an entity from the related iterator
708+
var random = new TestRandom(0, 0);
709+
solvingStarted(selector, scoreDirector, random);
710+
var iterator = selector.iterator();
711+
// entityIterator returns a
712+
assertThat(iterator.hasNext()).isTrue();
713+
assertThat(iterator.next()).isNotNull();
714+
// entityIterator gets null and call noUpcomingSelection
715+
assertThat(iterator.hasNext()).isFalse();
716+
assertThatCode(iterator::next).isInstanceOf(NoSuchElementException.class);
717+
// replayingValueSelector returns v2, discardUpcomingSelection is called, and entityIterator returns a again
718+
assertThat(iterator.hasNext()).isTrue();
719+
assertThat(iterator.next()).isNotNull();
720+
// iterator exhausted again
721+
assertThat(iterator.hasNext()).isFalse();
722+
assertThatCode(iterator::next).isInstanceOf(NoSuchElementException.class);
723+
}
617724
}

‎core/src/test/java/ai/timefold/solver/core/testdomain/list/TestdataListUtils.java‎

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99
import java.util.Collections;
1010
import java.util.Iterator;
1111
import java.util.List;
12+
import java.util.Objects;
1213

1314
import ai.timefold.solver.core.config.heuristic.selector.common.SelectionCacheType;
1415
import ai.timefold.solver.core.config.heuristic.selector.entity.EntitySelectorConfig;
@@ -21,6 +22,7 @@
2122
import ai.timefold.solver.core.impl.heuristic.HeuristicConfigPolicy;
2223
import ai.timefold.solver.core.impl.heuristic.selector.SelectorTestUtils;
2324
import ai.timefold.solver.core.impl.heuristic.selector.common.decorator.SelectionFilter;
25+
import ai.timefold.solver.core.impl.heuristic.selector.common.iterator.UpcomingSelectionIterator;
2426
import ai.timefold.solver.core.impl.heuristic.selector.entity.EntitySelector;
2527
import ai.timefold.solver.core.impl.heuristic.selector.list.DestinationSelector;
2628
import ai.timefold.solver.core.impl.heuristic.selector.list.DestinationSelectorFactory;
@@ -215,6 +217,10 @@ public static DestinationSelector<TestdataPinnedWithIndexListSolution> mockPinne
215217
return mockNeverEndingDestinationSelector(locationsInList.length, locationsInList);
216218
}
217219

220+
public static <Selection_> MockedUpcomingSelectionIterator<Selection_> mockUpcomingSelectionIterator(Selection_... values) {
221+
return new MockedUpcomingSelectionIterator<>(values);
222+
}
223+
218224
public static <Solution_> DestinationSelector<Solution_> mockNeverEndingDestinationSelector(long size,
219225
ElementPosition... locationsInList) {
220226
var destinationSelector = mock(DestinationSelector.class);
@@ -372,6 +378,25 @@ public static <S> DestinationSelector<S> getEntityValueRangeDestinationSelector(
372378
.buildDestinationSelector(configPolicy, SelectionCacheType.JUST_IN_TIME, randomSelection, "any", false);
373379
}
374380

381+
public static class MockedUpcomingSelectionIterator<Selection_> extends UpcomingSelectionIterator<Selection_> {
382+
383+
private final Selection_[] values;
384+
private int index = 0;
385+
386+
public MockedUpcomingSelectionIterator(Selection_[] values) {
387+
this.values = Objects.requireNonNull(values);
388+
}
389+
390+
@Override
391+
protected Selection_ createUpcomingSelection() {
392+
if (index >= values.length || values[index] == null) {
393+
index++;
394+
return noUpcomingSelection();
395+
}
396+
return values[index++];
397+
}
398+
}
399+
375400
private static <T> Iterator<T> cyclicIterator(List<T> elements) {
376401
if (elements.isEmpty()) {
377402
return Collections.emptyIterator();

0 commit comments

Comments
 (0)