From 6f9e54ff3aa46ba4b3f0cf7d29bc9327d7c487f8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Luk=C3=A1=C5=A1=20Petrovick=C3=BD?= Date: Wed, 4 Dec 2024 08:23:21 +0100 Subject: [PATCH] fix: off-by-one error when choosing element locations 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. --- .../list/ElementLocationRandomIterator.java | 8 ++-- .../list/ElementDestinationSelectorTest.java | 43 +++++++++++++++++++ 2 files changed, 48 insertions(+), 3 deletions(-) diff --git a/core/src/main/java/ai/timefold/solver/core/impl/heuristic/selector/list/ElementLocationRandomIterator.java b/core/src/main/java/ai/timefold/solver/core/impl/heuristic/selector/list/ElementLocationRandomIterator.java index 3ac48f98c2..d340eff2bb 100644 --- a/core/src/main/java/ai/timefold/solver/core/impl/heuristic/selector/list/ElementLocationRandomIterator.java +++ b/core/src/main/java/ai/timefold/solver/core/impl/heuristic/selector/list/ElementLocationRandomIterator.java @@ -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. @@ -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. diff --git a/core/src/test/java/ai/timefold/solver/core/impl/heuristic/selector/list/ElementDestinationSelectorTest.java b/core/src/test/java/ai/timefold/solver/core/impl/heuristic/selector/list/ElementDestinationSelectorTest.java index c6f923c774..aaea1c0492 100644 --- a/core/src/test/java/ai/timefold/solver/core/impl/heuristic/selector/list/ElementDestinationSelectorTest.java +++ b/core/src/test/java/ai/timefold/solver/core/impl/heuristic/selector/list/ElementDestinationSelectorTest.java @@ -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; @@ -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; @@ -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; @@ -202,6 +207,44 @@ 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 random = new Random(0); + + var solutionDescriptor = TestdataAllowsUnassignedValuesListSolution.buildSolutionDescriptor(); + var scoreDirector = mockScoreDirector(solutionDescriptor); + scoreDirector.setWorkingSolution(solution); + + var solverScope = new SolverScope(); + 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(random); + 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");