chore: add docs, fail fast on execute for structural flawed solutions, make ScoreAnalysis not fail fact on structural flawed input - #2609
Conversation
Also add a fail-fast when executing a move that produced a structurally flawed solution.
…ad of raising an exception
There was a problem hiding this comment.
Leaving some comments.
Your comment about keeping the graph forever is noted. It is an implementation downside of a requirement which makes sense for users; we take the pain. Going forward, maybe we refactor the graph algorithm; it is fast becoming marginal, maybe we do not need all the incrementality anymore. Anyway, not for now.
| @NullMarked | ||
| public interface StructuralFlawAnalysis { | ||
| /** | ||
| * Return a collection of {@link ai.timefold.solver.core.api.domain.entity.PlanningEntity} |
| * Return a collection of {@link ai.timefold.solver.core.api.domain.entity.PlanningEntity} | ||
| * that have inconsistent shadow variables. | ||
| */ | ||
| Collection<Object> getInconsistentEntities(); |
There was a problem hiding this comment.
Can we separate this into individual loops?
IMO that would make some sense - it would allow for better debugging of what is looping.
There was a problem hiding this comment.
Also, I would prefer if this returned a SequencedSet - clearer semantics, without having to explain.
| * without recalculating the score for performance reasons. | ||
| * | ||
| * @param move the move to execute | ||
| * @throws IllegalArgumentException if the move causes the solution to have a negative {@link Score#structuralScore()}. |
There was a problem hiding this comment.
IMO it should by explained somewhere why this exception is there and what the user can do to avoid it.
| <Score_ extends Score<Score_>> ScoreAnalysis<Score_> analyze(InnerScore<Score_> state, | ||
| Map<ConstraintRef, ConstraintMatchTotal<Score_>> constraintMatchTotalMap, ScoreAnalysisFetchPolicy fetchPolicy); | ||
| Map<ConstraintRef, ConstraintMatchTotal<Score_>> constraintMatchTotalMap, | ||
| Collection<Object> inconsistentEntities, |
There was a problem hiding this comment.
Since I'd like this to be separated into individual loops, arguably an internal wrapping type would work better.
| * this solution must not be modified any further. | ||
| * @return the result of the consumer | ||
| */ | ||
| <Result_> @Nullable Result_ executeTemporarilyHandlingStructurallyFlawedSolutions(Move<Solution_> move, |
There was a problem hiding this comment.
The Javadoc here is missing param structurallyFlawedSolutionConsumer - what should the consumer do? How does it interact with the other consumer? IMO this method is significantly under-documented.
| } | ||
|
|
||
| /** | ||
| * @return true if the last {@link #updateShadowVariables()} was successful, false otherwise. |
| Map<ConstraintRef, ConstraintMatchTotal<Score_>> constraintMatchTotalMap, ScoreAnalysisFetchPolicy fetchPolicy); | ||
| Map<ConstraintRef, ConstraintMatchTotal<Score_>> constraintMatchTotalMap, | ||
| Collection<Object> inconsistentEntities, | ||
| @Nullable ScoreDefinition<Score_> scoreDefinition, |
There was a problem hiding this comment.
Why nullable? When? IMO should be documented.
| if (innerScore.isStructurallyFlawed()) { | ||
| // If there were a fixed dependency loop, the shadow variable session would fail fast before here | ||
| throw new IllegalStateException( | ||
| "Impossible state: The initial solution passed to the solver is inconsistent even after unassigning involved entities."); |
There was a problem hiding this comment.
Is this actually "impossible state"? If this could be user error, then the "impossible state" prefix should not be there - that is reserved for unforeseen flaws in our own logic.
| [NOTE] | ||
| ==== | ||
| The score analysis of structurally flawed solutions has a structural flaw analysis, | ||
| which can be used to get inconsistent entities. | ||
| Constraint analyses are still available for structurally flawed solutions, but will exclude any matches sourced from an inconsistent entity. | ||
| ==== |
There was a problem hiding this comment.
| [NOTE] | |
| ==== | |
| The score analysis of structurally flawed solutions has a structural flaw analysis, | |
| which can be used to get inconsistent entities. | |
| Constraint analyses are still available for structurally flawed solutions, but will exclude any matches sourced from an inconsistent entity. | |
| ==== | |
| NOTE: The score analysis of structurally flawed solutions has a structural flaw analysis, | |
| which can be used to get inconsistent entities. | |
| Constraint analyses are still available for structurally flawed solutions, but will exclude any matches sourced from an inconsistent entity. |
A more concise alternative where you do not need to remember the closing statement, therefore avoiding bugs from not closing the block.
For these simple blocks, it works well. For the latter blocks of multiple paragraphs, the verbose syntax is much better.
| For example, if `c` depends on `a`, and `a` is in a loop with `b`, then `c` is also considered part of the loop. | ||
|
|
||
| When a declarative shadow variable is inconsistent, it will be set to `null`. | ||
| When a declarative shadow variable is inconsistent, the solver's reaction depends on whether any entity in the model declares a `@ShadowVariablesInconsistent` field. |
There was a problem hiding this comment.
IMO there should be a place (where exactly?) in which we discuss:
- The recommendations around this. (In other words: drop this, get a significantly faster solve.)
- The consequences. (If you have the annotation, you need to have a constraint based on the field. If you do not have the annotation, you do not need any constraint, because you get a structural score.)
Methinks we should slightly restructure the section; guide people from the start towards the annotation-less approach, and only mention the annotation once in a corner somewhere, with the explicit mention of it being deprecated. (In other words: the structural approach is the default, anything else is a legacy behavior which should only be mentioned, not promoted.)
2 participants
For the record, I strongly dislike this change to
ScoreAnalysis(returning with structural flaws instead of throwing an exception), and it will force the old implementation of the graph to live forever in some ways.