-
Notifications
You must be signed in to change notification settings - Fork 216
chore: make score analysis/recommendations/update fail fast on inconsistent solution #2444
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: no-loops
Are you sure you want to change the base?
Changes from all commits
04655ec
af16cb2
83abb4e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,27 @@ | ||
| package ai.timefold.solver.core.api.domain.variable; | ||
|
|
||
| import java.util.List; | ||
|
|
||
| import org.jspecify.annotations.NullMarked; | ||
|
|
||
| @NullMarked | ||
| public class InconsistentSolutionException extends RuntimeException { | ||
| private final Object solution; | ||
| private final List<Object> involvedEntityList; | ||
|
|
||
| public InconsistentSolutionException(String feature, Object solution, List<Object> involvedEntityList) { | ||
| super("The solution (%s) is inconsistent. %s requires a consistent solution.".formatted(solution, feature)); | ||
| this.solution = solution; | ||
| this.involvedEntityList = involvedEntityList; | ||
| } | ||
|
|
||
| @SuppressWarnings("unchecked") | ||
| public <T> T getSolution() { | ||
| return (T) solution; | ||
| } | ||
|
|
||
| @SuppressWarnings("unchecked") | ||
| public <T> List<T> getInvolvedEntityList() { | ||
| return (List<T>) involvedEntityList; | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -75,7 +75,7 @@ public abstract class AbstractScoreDirector<Solution_, Score_ extends Score<Scor | |
| private final @Nullable LookupManager lookUpManager; | ||
| protected final ConstraintMatchPolicy constraintMatchPolicy; | ||
| private boolean expectShadowVariablesInCorrectState; | ||
| private boolean ignoreInconsistentSolutions; | ||
| private final boolean ignoreInconsistentSolutions; | ||
| private final VariableDescriptorCache<Solution_> variableDescriptorCache; | ||
| protected final VariableListenerSupport<Solution_> variableListenerSupport; | ||
| private final @Nullable SolutionTracker<Solution_> solutionTracker; // Null when tracking disabled. | ||
|
|
@@ -111,7 +111,7 @@ protected AbstractScoreDirector(AbstractScoreDirectorBuilder<Solution_, Score_, | |
| this.lookUpManager = lookUpEnabled ? new LookupManager(solutionDescriptor.getLookUpStrategyResolver()) : null; | ||
| this.constraintMatchPolicy = builder.constraintMatchPolicy; | ||
| this.expectShadowVariablesInCorrectState = builder.expectShadowVariablesInCorrectState; | ||
| this.ignoreInconsistentSolutions = builder.ignoreInconsistentSolutions; | ||
| this.ignoreInconsistentSolutions = !solutionDescriptor.hasAnyShadowVariablesInconsistentMember(); | ||
| this.variableDescriptorCache = new VariableDescriptorCache<>(solutionDescriptor); | ||
| this.variableListenerSupport = VariableListenerSupport.create(this); | ||
| this.variableListenerSupport.linkVariableListeners(); | ||
|
|
@@ -344,8 +344,12 @@ protected void afterSetWorkingSolution() { | |
| // Do nothing | ||
| } | ||
|
|
||
| public List<Object> getInconsistentEntities() { | ||
| return variableListenerSupport.getInconsistentEntities(); | ||
| } | ||
|
|
||
| public void unassignInconsistentEntities() { | ||
| var inconsistentEntities = variableListenerSupport.getInconsistentEntities(); | ||
| var inconsistentEntities = getInconsistentEntities(); | ||
| if (listVariableStateSupply != null) { | ||
| var listVariableDescriptor = listVariableStateSupply.getSourceVariableDescriptor(); | ||
| var listElementClass = listVariableStateSupply.getSourceVariableDescriptor().getElementType(); | ||
|
|
@@ -490,6 +494,10 @@ public void triggerVariableListeners() { | |
| lastVariableUpdateWasSuccessful = variableListenerSupport.triggerVariableListenersInNotificationQueues(); | ||
| } | ||
|
|
||
| public boolean isLastVariableUpdateWasSuccessful() { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. IMO works equally as well, even better, without the "was" there. |
||
| return lastVariableUpdateWasSuccessful; | ||
| } | ||
|
|
||
| /** | ||
| * This function clears all listener events that have been generated without triggering any of them. | ||
| * Using this method requires caution because clearing the event queue can lead to inconsistent states. | ||
|
|
@@ -517,7 +525,6 @@ public InnerScoreDirector<Solution_, Score_> createChildThreadScoreDirector(Chil | |
| var childThreadScoreDirector = scoreDirectorFactory.createScoreDirectorBuilder() | ||
| .withLookUpEnabled(lookUpEnabled) | ||
| .withConstraintMatchPolicy(constraintMatchPolicy) | ||
| .withIgnoreInconsistentSolutions(ignoreInconsistentSolutions) | ||
| .buildDerived(); | ||
| // ScoreCalculationCountTermination takes into account previous phases | ||
| // but the calculationCount of partitions is maxed, not summed. | ||
|
|
@@ -527,7 +534,6 @@ public InnerScoreDirector<Solution_, Score_> createChildThreadScoreDirector(Chil | |
| var childThreadScoreDirector = scoreDirectorFactory.createScoreDirectorBuilder() | ||
| .withLookUpEnabled(true) | ||
| .withConstraintMatchPolicy(constraintMatchPolicy) | ||
| .withIgnoreInconsistentSolutions(ignoreInconsistentSolutions) | ||
| .buildDerived(); | ||
| childThreadScoreDirector.setWorkingSolution(cloneWorkingSolution()); | ||
| return childThreadScoreDirector; | ||
|
|
@@ -800,7 +806,6 @@ private void assertScoreFromScratch(InnerScore<Score_> innerScore, Object comple | |
| // Most score directors don't need derived status; CS will override this. | ||
| try (var uncorruptedScoreDirector = assertionScoreDirectorFactory.createScoreDirectorBuilder() | ||
| .withConstraintMatchPolicy(ConstraintMatchPolicy.ENABLED) | ||
| .withIgnoreInconsistentSolutions(ignoreInconsistentSolutions) | ||
| .buildDerived()) { | ||
| uncorruptedScoreDirector.setWorkingSolution(workingSolution); | ||
| var uncorruptedInnerScore = uncorruptedScoreDirector.calculateScore(); | ||
|
|
@@ -999,7 +1004,6 @@ public abstract static class AbstractScoreDirectorBuilder<Solution_, Score_ exte | |
| protected ConstraintMatchPolicy constraintMatchPolicy = ConstraintMatchPolicy.DISABLED; | ||
| protected boolean lookUpEnabled = false; | ||
| protected boolean expectShadowVariablesInCorrectState = true; | ||
| protected boolean ignoreInconsistentSolutions = false; | ||
|
|
||
| protected AbstractScoreDirectorBuilder(Factory_ scoreDirectorFactory) { | ||
| this.scoreDirectorFactory = Objects.requireNonNull(scoreDirectorFactory); | ||
|
|
@@ -1023,12 +1027,6 @@ public Builder_ withExpectShadowVariablesInCorrectState(boolean expectShadowVari | |
| return (Builder_) this; | ||
| } | ||
|
|
||
| @SuppressWarnings("unchecked") | ||
| public Builder_ withIgnoreInconsistentSolutions(boolean ignoreInconsistentSolutions) { | ||
| this.ignoreInconsistentSolutions = ignoreInconsistentSolutions; | ||
| return (Builder_) this; | ||
| } | ||
|
|
||
| public abstract AbstractScoreDirector<Solution_, Score_, Factory_> build(); | ||
|
|
||
| /** | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,6 +5,7 @@ | |
| import java.util.function.Function; | ||
|
|
||
| import ai.timefold.solver.core.api.domain.solution.PlanningSolution; | ||
| import ai.timefold.solver.core.api.domain.variable.InconsistentSolutionException; | ||
| import ai.timefold.solver.core.api.score.Score; | ||
| import ai.timefold.solver.core.api.score.analysis.ScoreAnalysis; | ||
| import ai.timefold.solver.core.api.solver.RecommendedAssignment; | ||
|
|
@@ -51,14 +52,15 @@ public ScoreDirectorFactory<Solution_, Score_> getScoreDirectorFactory() { | |
| @Override | ||
| public Score_ update(Solution_ solution, SolutionUpdatePolicy solutionUpdatePolicy) { | ||
| if (solutionUpdatePolicy == SolutionUpdatePolicy.NO_UPDATE) { | ||
| throw new IllegalArgumentException("Can not call " + this.getClass().getSimpleName() | ||
| + ".update() with this solutionUpdatePolicy (" + solutionUpdatePolicy + ")."); | ||
| throw new IllegalArgumentException( | ||
| "Can not call %s.update() with this solutionUpdatePolicy (%s)." | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I know this is not actually new, but maybe we add a hint as to why they cannot call this? Like this, the exception message is not very helpful. |
||
| .formatted(this.getClass().getSimpleName(), solutionUpdatePolicy)); | ||
| } | ||
| return callScoreDirector(solution, solutionUpdatePolicy, | ||
| return callScoreDirector("Solution update", solution, solutionUpdatePolicy, | ||
| s -> s.getSolutionDescriptor().getScore(s.getWorkingSolution()), ConstraintMatchPolicy.DISABLED, false); | ||
| } | ||
|
|
||
| private <Result_> Result_ callScoreDirector(Solution_ solution, SolutionUpdatePolicy solutionUpdatePolicy, | ||
| private <Result_> Result_ callScoreDirector(String feature, Solution_ solution, SolutionUpdatePolicy solutionUpdatePolicy, | ||
| Function<InnerScoreDirector<Solution_, Score_>, Result_> function, ConstraintMatchPolicy constraintMatchPolicy, | ||
| boolean cloneSolution) { | ||
| var isShadowVariableUpdateEnabled = solutionUpdatePolicy.isShadowVariableUpdateEnabled(); | ||
|
|
@@ -82,7 +84,14 @@ private <Result_> Result_ callScoreDirector(Solution_ solution, SolutionUpdatePo | |
| Maybe use Constraint Streams instead of Easy or Incremental score calculator?"""); | ||
| } | ||
| if (solutionUpdatePolicy.isScoreUpdateEnabled()) { | ||
| scoreDirector.calculateScore(); | ||
| var score = scoreDirector.calculateScore(); | ||
| if (score.isInvalid()) { | ||
| var inconsistentEntities = scoreDirector.getInconsistentEntities(); | ||
| throw new InconsistentSolutionException(feature, nonNullSolution, inconsistentEntities); | ||
| } | ||
| } else if (!scoreDirector.isLastVariableUpdateWasSuccessful()) { | ||
| var inconsistentEntities = scoreDirector.getInconsistentEntities(); | ||
| throw new InconsistentSolutionException(feature, nonNullSolution, inconsistentEntities); | ||
| } | ||
| return function.apply(scoreDirector); | ||
| } | ||
|
|
@@ -112,7 +121,7 @@ public ScoreAnalysis<Score_> analyze(Solution_ solution, ScoreAnalysisFetchPolic | |
| var enterpriseService = | ||
| TimefoldSolverEnterpriseService.loadOrFail(TimefoldSolverEnterpriseService.Feature.SCORE_ANALYSIS); | ||
| var currentScore = (Score_) scoreDirectorFactory.getSolutionDescriptor().getScore(solution); | ||
| var analysis = callScoreDirector(solution, solutionUpdatePolicy, | ||
| var analysis = callScoreDirector("Solution analysis", solution, solutionUpdatePolicy, | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we call it "Score analysis", don't we? |
||
| scoreDirector -> enterpriseService.analyze(scoreDirector.calculateScore(), | ||
| scoreDirector.getConstraintMatchTotalMap(), fetchPolicy), | ||
| ConstraintMatchPolicy.match(fetchPolicy), false); | ||
|
|
@@ -134,7 +143,7 @@ public <In_, Out_> List<RecommendedAssignment<Out_, Score_>> recommendAssignment | |
| ScoreAnalysisFetchPolicy fetchPolicy) { | ||
| var enterpriseService = | ||
| TimefoldSolverEnterpriseService.loadOrFail(TimefoldSolverEnterpriseService.Feature.RECOMMENDATIONS); | ||
| return callScoreDirector( | ||
| return callScoreDirector("Recommended assignment", | ||
| solution, SolutionUpdatePolicy.UPDATE_ALL, enterpriseService.buildRecommender(solverFactory, solution, | ||
| evaluatedEntityOrElement, propositionFunction, fetchPolicy), | ||
| ConstraintMatchPolicy.match(fetchPolicy), true); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think a more fitting name would be
computeInconsistentEntitiesto make it clear that this is expensive.