Skip to content

Commit

Permalink
fix: off-by-one error when choosing element locations
Browse files Browse the repository at this point in the history
The error caused the skipping of one entity's list variable as a possible location. In the case of only one entity being available, this would lead to not assigning any value to any list variable - which is how the bug was discovered.
  • Loading branch information
triceo committed Dec 4, 2024
1 parent 07cc41d commit 9d7a502
Show file tree
Hide file tree
Showing 2 changed files with 46 additions and 3 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -52,8 +52,10 @@ public ElementLocation next() {
// This code operates under the assumption that the entity selector already filtered out all immovable entities.
// At this point, entities are only partially pinned, or not pinned at all.
var entitySize = entitySelector.getSize();
// If we support unassigned values, we have to add 1 to all the sizes
// to account for the unassigned destination, which is an extra element.
var entityBoundary = allowsUnassignedValues ? entitySize + 1 : entitySize;
long random = RandomUtils.nextLong(workingRandom, totalSize);
var random = RandomUtils.nextLong(workingRandom, allowsUnassignedValues ? totalSize + 1 : totalSize);
if (allowsUnassignedValues && random == 0) {
// We have already excluded all unassigned elements,
// the only way to get an unassigned destination is to explicitly add it.
Expand All @@ -75,11 +77,11 @@ public ElementLocation next() {
// This skews the selection probability towards entities with fewer unpinned values,
// but at this point, there is no other thing we could possibly do.
var entity = entityIterator.next();
int unpinnedSize = listVariableDescriptor.getUnpinnedSubListSize(entity);
var unpinnedSize = listVariableDescriptor.getUnpinnedSubListSize(entity);
if (unpinnedSize == 0) { // Only the last destination remains.
return ElementLocation.of(entity, listVariableDescriptor.getListSize(entity));
} else { // +1 to include the destination after the final element in the list.
int randomIndex = workingRandom.nextInt(unpinnedSize + 1);
var randomIndex = workingRandom.nextInt(unpinnedSize + 1);
return ElementLocation.of(entity, listVariableDescriptor.getFirstUnpinnedIndex(entity) + randomIndex);
}
} else { // +1 to include the destination after the final element in the list.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
import static ai.timefold.solver.core.impl.heuristic.selector.SelectorTestUtils.phaseStarted;
import static ai.timefold.solver.core.impl.heuristic.selector.SelectorTestUtils.solvingStarted;
import static ai.timefold.solver.core.impl.heuristic.selector.SelectorTestUtils.stepStarted;
import static ai.timefold.solver.core.impl.testdata.domain.list.TestdataListUtils.getAllowsUnassignedvaluesListVariableDescriptor;
import static ai.timefold.solver.core.impl.testdata.domain.list.TestdataListUtils.getListVariableDescriptor;
import static ai.timefold.solver.core.impl.testdata.domain.list.TestdataListUtils.getPinnedAllowsUnassignedvaluesListVariableDescriptor;
import static ai.timefold.solver.core.impl.testdata.domain.list.TestdataListUtils.getPinnedListVariableDescriptor;
Expand All @@ -19,6 +20,7 @@

import java.util.Collections;
import java.util.List;
import java.util.Random;

import ai.timefold.solver.core.config.heuristic.selector.common.SelectionCacheType;
import ai.timefold.solver.core.impl.heuristic.selector.entity.FromSolutionEntitySelector;
Expand All @@ -27,6 +29,9 @@
import ai.timefold.solver.core.impl.testdata.domain.list.TestdataListEntity;
import ai.timefold.solver.core.impl.testdata.domain.list.TestdataListSolution;
import ai.timefold.solver.core.impl.testdata.domain.list.TestdataListValue;
import ai.timefold.solver.core.impl.testdata.domain.list.allows_unassigned.TestdataAllowsUnassignedValuesListEntity;
import ai.timefold.solver.core.impl.testdata.domain.list.allows_unassigned.TestdataAllowsUnassignedValuesListSolution;
import ai.timefold.solver.core.impl.testdata.domain.list.allows_unassigned.TestdataAllowsUnassignedValuesListValue;
import ai.timefold.solver.core.impl.testdata.domain.list.allows_unassigned.pinned.TestdataPinnedUnassignedValuesListEntity;
import ai.timefold.solver.core.impl.testdata.domain.list.allows_unassigned.pinned.TestdataPinnedUnassignedValuesListSolution;
import ai.timefold.solver.core.impl.testdata.domain.list.allows_unassigned.pinned.TestdataPinnedUnassignedValuesListValue;
Expand Down Expand Up @@ -202,6 +207,42 @@ void randomPartiallyPinnedAndUnassigned() {
"D[3]");
}

@Test
void randomUnassignedSingleEntity() {
var unassignedValue = new TestdataAllowsUnassignedValuesListValue("3");
var a = TestdataAllowsUnassignedValuesListEntity.createWithValues("A");

var solution = new TestdataAllowsUnassignedValuesListSolution();
solution.setEntityList(List.of(a));
solution.setValueList(List.of(unassignedValue));

var solutionDescriptor = TestdataAllowsUnassignedValuesListSolution.buildSolutionDescriptor();
var scoreDirector = mockScoreDirector(solutionDescriptor);
scoreDirector.setWorkingSolution(solution);

var solverScope = new SolverScope<TestdataAllowsUnassignedValuesListSolution>();
solverScope.setScoreDirector(scoreDirector);
// This needs to use a real Random instance, otherwise the test never covers the situation where
// the random value of 1 needs to be produced to get to the entity.
solverScope.setWorkingRandom(new Random(0));
var entitySelector = new FromSolutionEntitySelector<>(
solutionDescriptor.findEntityDescriptorOrFail(TestdataAllowsUnassignedValuesListEntity.class),
SelectionCacheType.PHASE, true);
entitySelector.solvingStarted(solverScope);
entitySelector.phaseStarted(new LocalSearchPhaseScope<>(solverScope, 0));

var valueSelector = mockEntityIndependentValueSelector(
getAllowsUnassignedvaluesListVariableDescriptor(scoreDirector),
unassignedValue);
var selector = new ElementDestinationSelector<>(entitySelector, valueSelector, true);
selector.solvingStarted(solverScope);
selector.phaseStarted(new LocalSearchPhaseScope<>(solverScope, 0));

assertCodesOfNeverEndingIterator(selector.iterator(),
"A[0]", // Ensure the entity is accessible.
"UnassignedLocation"); // As well as the unassigned location.
}

@Test
void randomFullyPinned() {
var v1 = new TestdataPinnedWithIndexListValue("1");
Expand Down

0 comments on commit 9d7a502

Please sign in to comment.