Skip to content

Commit a906e68

Browse files
Christopher-Chianellitriceo
Christopher-Chianelli
authored and
triceo
committed
fix: Update looped status of entity even if no variables changed
We previously only added a non-looped entity to affected entities if one of its declarative variables changed. However, if null is a valid value (such as for unassigned entities), then it might be the case the looped status of an entity changed from looped to non-looped. We now add an entity to the affected entities list if its loop status changed.
1 parent 2a1fe87 commit a906e68

9 files changed

Lines changed: 236 additions & 13 deletions

File tree

‎core/src/main/java/ai/timefold/solver/core/impl/domain/variable/declarative/AffectedEntitiesUpdater.java‎

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -118,13 +118,18 @@ private boolean updateShadowVariable(EntityVariablePair<Solution_> entityVariabl
118118
var entity = entityVariable.entity();
119119
var shadowVariableReference = entityVariable.variableReference();
120120
var oldValue = shadowVariableReference.memberAccessor().executeGetter(entity);
121+
var loopDescriptor = shadowVariableReference.shadowVariableLoopedDescriptor();
122+
if (loopDescriptor != null) {
123+
var oldLooped = (boolean) loopDescriptor.getValue(entity);
124+
if (oldLooped != isLooped) {
125+
// Loop status change; add to affected entities
126+
affectedEntities.add(entityVariable);
127+
}
128+
}
121129

122130
if (isLooped) {
123-
// null might be a valid value, and thus it could be the case
124-
// that is was not looped and null, then turned to looped and null,
125-
// which is still considered a change.
126-
affectedEntities.add(entityVariable);
127131
if (oldValue != null) {
132+
affectedEntities.add(entityVariable);
128133
changeShadowVariableAndNotify(shadowVariableReference, entity, null);
129134
}
130135
return true;

‎core/src/main/java/ai/timefold/solver/core/impl/domain/variable/declarative/DefaultShadowVariableSession.java‎

Lines changed: 14 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -2,25 +2,35 @@
22

33
import ai.timefold.solver.core.impl.domain.variable.descriptor.VariableDescriptor;
44
import ai.timefold.solver.core.impl.domain.variable.supply.Supply;
5+
import ai.timefold.solver.core.preview.api.domain.metamodel.VariableMetaModel;
56

67
import org.jspecify.annotations.NullMarked;
78

89
@NullMarked
9-
public class DefaultShadowVariableSession<Solution_> implements Supply {
10-
10+
public final class DefaultShadowVariableSession<Solution_> implements Supply {
1111
final VariableReferenceGraph<Solution_> graph;
1212

1313
public DefaultShadowVariableSession(VariableReferenceGraph<Solution_> graph) {
1414
this.graph = graph;
1515
}
1616

1717
public void beforeVariableChanged(VariableDescriptor<Solution_> variableDescriptor, Object entity) {
18-
graph.beforeVariableChanged(variableDescriptor.getVariableMetaModel(),
18+
beforeVariableChanged(variableDescriptor.getVariableMetaModel(),
1919
entity);
2020
}
2121

2222
public void afterVariableChanged(VariableDescriptor<Solution_> variableDescriptor, Object entity) {
23-
graph.afterVariableChanged(variableDescriptor.getVariableMetaModel(),
23+
afterVariableChanged(variableDescriptor.getVariableMetaModel(),
24+
entity);
25+
}
26+
27+
public void beforeVariableChanged(VariableMetaModel<Solution_, ?, ?> variableMetaModel, Object entity) {
28+
graph.beforeVariableChanged(variableMetaModel,
29+
entity);
30+
}
31+
32+
public void afterVariableChanged(VariableMetaModel<Solution_, ?, ?> variableMetaModel, Object entity) {
33+
graph.afterVariableChanged(variableMetaModel,
2434
entity);
2535
}
2636

‎core/src/main/java/ai/timefold/solver/core/impl/domain/variable/listener/support/VariableListenerSupport.java‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -354,6 +354,12 @@ public void triggerVariableListenersInNotificationQueues() {
354354
}
355355
if (shadowVariableSession != null) {
356356
shadowVariableSession.updateVariables();
357+
// Some internal variable listeners (such as those used
358+
// to check for solution corruption) might have a declarative
359+
// shadow variable as a source and need to be triggered here.
360+
for (var notifiable : notifiableRegistry.getAll()) {
361+
notifiable.triggerAllNotifications();
362+
}
357363
}
358364
notificationQueuesAreEmpty = true;
359365
}

‎core/src/main/java/ai/timefold/solver/core/impl/domain/variable/listener/support/violation/SolutionTracker.java‎

Lines changed: 17 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -111,9 +111,15 @@ private List<String> getEntitiesMissingBeforeAfterEvents(VariableSnapshotTotal<S
111111
return out;
112112
}
113113

114-
public String buildScoreCorruptionMessage() {
114+
public record SolutionCorruptionResult(boolean isCorrupted, String message) {
115+
public static SolutionCorruptionResult untracked() {
116+
return new SolutionCorruptionResult(false, "");
117+
}
118+
}
119+
120+
public SolutionCorruptionResult buildSolutionCorruptionResult() {
115121
if (beforeMoveSolution == null) {
116-
return "";
122+
return new SolutionCorruptionResult(false, "");
117123
}
118124

119125
StringBuilder out = new StringBuilder();
@@ -161,11 +167,17 @@ public String buildScoreCorruptionMessage() {
161167
""".formatted(formatList(missingEventsBackward)));
162168
}
163169

164-
if (out.isEmpty()) {
165-
return "Genuine and shadow variables agree with from scratch calculation after the undo move and match the state prior to the move.";
170+
var isCorrupted = !out.isEmpty();
171+
if (isCorrupted) {
172+
return new SolutionCorruptionResult(isCorrupted, out.toString());
173+
} else {
174+
return new SolutionCorruptionResult(false,
175+
"Genuine and shadow variables agree with from scratch calculation after the undo move and match the state prior to the move.");
166176
}
177+
}
167178

168-
return out.toString();
179+
public String buildScoreCorruptionMessage() {
180+
return buildSolutionCorruptionResult().message;
169181
}
170182

171183
static <Solution_> List<String> getVariableChangedViolations(

‎core/src/main/java/ai/timefold/solver/core/impl/score/director/AbstractScoreDirector.java‎

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -781,6 +781,36 @@ that could cause the scoreDifference (%s)."""
781781
}
782782
}
783783

784+
public SolutionTracker.SolutionCorruptionResult getSolutionCorruptionAfterUndo(Move<Solution_> move,
785+
InnerScore<Score_> undoInnerScore) {
786+
var trackingWorkingSolution = solutionTracker != null;
787+
if (trackingWorkingSolution) {
788+
solutionTracker.setAfterUndoSolution(workingSolution);
789+
}
790+
// Precondition: assert that there are probably no corrupted constraints
791+
var undoMoveToString = "Undo(%s)".formatted(move);
792+
assertWorkingScoreFromScratch(undoInnerScore, undoMoveToString);
793+
// Precondition: assert that shadow variables aren't stale after doing the undoMove
794+
assertShadowVariablesAreNotStale(undoInnerScore, undoMoveToString);
795+
if (trackingWorkingSolution) {
796+
// Recalculate all shadow variables from scratch.
797+
// We cannot set all shadow variables to null, since some variable listeners
798+
// may expect them to be non-null.
799+
// Instead, we just simulate a change to all genuine variables.
800+
variableListenerSupport.forceTriggerAllVariableListeners(workingSolution);
801+
solutionTracker.setUndoFromScratchSolution(workingSolution);
802+
803+
// Also calculate from scratch for the before solution, since it might
804+
// have been corrupted but was only detected now
805+
solutionTracker.restoreBeforeSolution();
806+
variableListenerSupport.forceTriggerAllVariableListeners(workingSolution);
807+
solutionTracker.setBeforeFromScratchSolution(workingSolution);
808+
809+
return solutionTracker.buildSolutionCorruptionResult();
810+
}
811+
return SolutionTracker.SolutionCorruptionResult.untracked();
812+
}
813+
784814
/**
785815
* @param uncorruptedScoreDirector never null
786816
* @param predicted true if the score was predicted and might have been calculated on another thread

‎core/src/test/java/ai/timefold/solver/core/impl/solver/MoveAssertScoreDirector.java‎

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,47 @@
1+
package ai.timefold.solver.core.impl.solver;
2+
3+
import java.util.Map;
4+
5+
import ai.timefold.solver.core.api.score.Score;
6+
import ai.timefold.solver.core.api.score.constraint.ConstraintMatchTotal;
7+
import ai.timefold.solver.core.api.score.constraint.Indictment;
8+
import ai.timefold.solver.core.impl.score.constraint.ConstraintMatchPolicy;
9+
import ai.timefold.solver.core.impl.score.director.AbstractScoreDirector;
10+
import ai.timefold.solver.core.impl.score.director.InnerScore;
11+
12+
public class MoveAssertScoreDirector<Solution_, Score_ extends Score<Score_>>
13+
extends AbstractScoreDirector<Solution_, Score_, MoveAssertScoreDirectorFactory<Solution_, Score_>> {
14+
15+
protected MoveAssertScoreDirector(MoveAssertScoreDirectorFactory<Solution_, Score_> scoreDirectorFactory,
16+
boolean lookUpEnabled,
17+
ConstraintMatchPolicy constraintMatchPolicy,
18+
boolean expectShadowVariablesInCorrectState) {
19+
super(scoreDirectorFactory, lookUpEnabled, constraintMatchPolicy, expectShadowVariablesInCorrectState);
20+
}
21+
22+
@Override
23+
public void setWorkingSolution(Solution_ workingSolution) {
24+
super.setWorkingSolution(workingSolution, ignored -> {
25+
});
26+
}
27+
28+
@Override
29+
public InnerScore<Score_> calculateScore() {
30+
return InnerScore.fullyAssigned(scoreDirectorFactory.getScoreDefinition().getZeroScore());
31+
}
32+
33+
@Override
34+
public Map<String, ConstraintMatchTotal<Score_>> getConstraintMatchTotalMap() {
35+
return Map.of();
36+
}
37+
38+
@Override
39+
public Map<Object, Indictment<Score_>> getIndictmentMap() {
40+
return Map.of();
41+
}
42+
43+
@Override
44+
public boolean requiresFlushing() {
45+
return false;
46+
}
47+
}

‎core/src/test/java/ai/timefold/solver/core/impl/solver/MoveAssertScoreDirectorFactory.java‎

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,38 @@
1+
package ai.timefold.solver.core.impl.solver;
2+
3+
import ai.timefold.solver.core.api.score.Score;
4+
import ai.timefold.solver.core.impl.domain.solution.descriptor.SolutionDescriptor;
5+
import ai.timefold.solver.core.impl.score.director.AbstractScoreDirector;
6+
import ai.timefold.solver.core.impl.score.director.AbstractScoreDirectorFactory;
7+
8+
import org.jspecify.annotations.NullMarked;
9+
10+
@NullMarked
11+
public class MoveAssertScoreDirectorFactory<Solution_, Score_ extends Score<Score_>>
12+
extends AbstractScoreDirectorFactory<Solution_, Score_, MoveAssertScoreDirectorFactory<Solution_, Score_>> {
13+
public MoveAssertScoreDirectorFactory(
14+
SolutionDescriptor<Solution_> solutionDescriptor) {
15+
super(solutionDescriptor);
16+
}
17+
18+
@Override
19+
public AbstractScoreDirector.AbstractScoreDirectorBuilder<Solution_, Score_, ?, ?> createScoreDirectorBuilder() {
20+
return new MoveAssertScoreDirectorBuilder(this);
21+
}
22+
23+
public class MoveAssertScoreDirectorBuilder extends
24+
AbstractScoreDirector.AbstractScoreDirectorBuilder<Solution_, Score_, MoveAssertScoreDirectorFactory<Solution_, Score_>, MoveAssertScoreDirectorBuilder> {
25+
26+
protected MoveAssertScoreDirectorBuilder(MoveAssertScoreDirectorFactory<Solution_, Score_> scoreDirectorFactory) {
27+
super(scoreDirectorFactory);
28+
}
29+
30+
@Override
31+
public MoveAssertScoreDirector<Solution_, Score_> build() {
32+
return new MoveAssertScoreDirector<>(scoreDirectorFactory,
33+
lookUpEnabled,
34+
constraintMatchPolicy,
35+
expectShadowVariablesInCorrectState);
36+
}
37+
}
38+
}

‎core/src/test/java/ai/timefold/solver/core/impl/solver/MoveAsserter.java‎

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,37 @@
1+
package ai.timefold.solver.core.impl.solver;
2+
3+
import ai.timefold.solver.core.api.score.Score;
4+
import ai.timefold.solver.core.impl.domain.solution.descriptor.SolutionDescriptor;
5+
import ai.timefold.solver.core.impl.score.director.InnerScore;
6+
import ai.timefold.solver.core.preview.api.move.Move;
7+
8+
public class MoveAsserter<Solution_> {
9+
private final SolutionDescriptor<Solution_> solutionDescriptor;
10+
11+
private MoveAsserter(SolutionDescriptor<Solution_> solutionDescriptor) {
12+
this.solutionDescriptor = solutionDescriptor;
13+
}
14+
15+
public static <Solution_> MoveAsserter<Solution_> create(SolutionDescriptor<Solution_> solutionDescriptor) {
16+
return new MoveAsserter<>(solutionDescriptor);
17+
}
18+
19+
public void assertMove(Solution_ solution, Move<Solution_> move) {
20+
var scoreDirectorFactory = new MoveAssertScoreDirectorFactory<>(solutionDescriptor);
21+
scoreDirectorFactory.setTrackingWorkingSolution(true);
22+
try (var scoreDirector = scoreDirectorFactory.createScoreDirectorBuilder()
23+
.withLookUpEnabled(false)
24+
.buildDerived()) {
25+
var innerScore = InnerScore.fullyAssigned((Score) scoreDirector.getScoreDefinition().getZeroScore());
26+
scoreDirector.setWorkingSolution(solution);
27+
scoreDirector.executeTemporaryMove(move, true);
28+
var corruptionResult = scoreDirector.getSolutionCorruptionAfterUndo(move, innerScore);
29+
if (corruptionResult.isCorrupted()) {
30+
throw new AssertionError("""
31+
Solution corruption caused by move (%s) or its undo.
32+
Analysis:
33+
%s""".formatted(move, corruptionResult.message()));
34+
}
35+
}
36+
}
37+
}

‎core/src/test/java/ai/timefold/solver/core/preview/api/variable/declarative/dependent/DependencyValuesShadowVariableTest.java‎

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,12 +6,17 @@
66
import java.time.LocalDateTime;
77
import java.time.ZoneOffset;
88
import java.util.List;
9+
import java.util.Set;
910

1011
import ai.timefold.solver.core.api.solver.SolverFactory;
1112
import ai.timefold.solver.core.config.solver.EnvironmentMode;
1213
import ai.timefold.solver.core.config.solver.PreviewFeature;
1314
import ai.timefold.solver.core.config.solver.SolverConfig;
1415
import ai.timefold.solver.core.config.solver.termination.TerminationConfig;
16+
import ai.timefold.solver.core.impl.domain.solution.descriptor.SolutionDescriptor;
17+
import ai.timefold.solver.core.impl.move.streams.generic.move.ListAssignMove;
18+
import ai.timefold.solver.core.impl.solver.MoveAsserter;
19+
import ai.timefold.solver.core.preview.api.domain.metamodel.PlanningListVariableMetaModel;
1520
import ai.timefold.solver.core.testdomain.declarative.dependency.TestdataDependencyConstraintProvider;
1621
import ai.timefold.solver.core.testdomain.declarative.dependency.TestdataDependencyEntity;
1722
import ai.timefold.solver.core.testdomain.declarative.dependency.TestdataDependencySolution;
@@ -69,4 +74,37 @@ void testSolve() {
6974
}
7075
}
7176
}
77+
78+
@Test
79+
void testLoopStatusOfEntityIsUpdatedEvenIfNoVariablesOnTheEntityChanged() {
80+
var baseTime = LocalDateTime.ofEpochSecond(0, 0, ZoneOffset.UTC);
81+
var entityA = new TestdataDependencyEntity(baseTime);
82+
var entityB = new TestdataDependencyEntity(baseTime);
83+
var entityC = new TestdataDependencyEntity(baseTime);
84+
85+
var valueA = new TestdataDependencyValue("A", Duration.ofHours(5), null);
86+
var valueB = new TestdataDependencyValue("B", Duration.ofHours(6), null);
87+
var valueC = new TestdataDependencyValue("C", Duration.ofHours(7), List.of(valueA, valueB));
88+
89+
var schedule = new TestdataDependencySolution(
90+
List.of(entityA, entityB, entityC),
91+
List.of(valueA, valueB, valueC));
92+
93+
entityA.getValues().add(valueB);
94+
entityA.getValues().add(valueA);
95+
96+
var solutionDescriptor = SolutionDescriptor.buildSolutionDescriptor(Set.of(PreviewFeature.DECLARATIVE_SHADOW_VARIABLES),
97+
TestdataDependencySolution.class, TestdataDependencyEntity.class, TestdataDependencyValue.class);
98+
var moveAsserter = MoveAsserter.create(solutionDescriptor);
99+
100+
// Tests the move [A, B] -> [C, A, B].
101+
// Since C depends on A and B, this is an invalid solution,
102+
// and C.startTime/C.endTime remains null and C.isLooped is true.
103+
// When the move is undone, C.startTime/C.endTime remains null,
104+
// and C.isLooped is false.
105+
moveAsserter.assertMove(schedule, new ListAssignMove<>(
106+
(PlanningListVariableMetaModel<TestdataDependencySolution, ? super TestdataDependencyEntity, ? super TestdataDependencyValue>) solutionDescriptor
107+
.getListVariableDescriptor().getVariableMetaModel(),
108+
valueC, entityA, 0));
109+
}
72110
}

0 commit comments

Comments
 (0)