fix: match count in score analysis diff need not match the match list size (#1454)

triceo · web-flow · commit 90f02f45856c · 2025-03-14T13:01:47.000+01:00
diff --git a/core/src/main/java/ai/timefold/solver/core/api/score/analysis/ConstraintAnalysis.java b/core/src/main/java/ai/timefold/solver/core/api/score/analysis/ConstraintAnalysis.java
@@ -31,9 +31,19 @@
  *        non-empty if constraint has matches.
  *        This is a {@link List} to simplify access to individual elements,
  *        but it contains no duplicates just like {@link HashSet} wouldn't.
- * @param matchCount -1 if analysis not available,
+ * @param matchCount
+ *        <ul>
+ *        <li>For regular constraint analysis:
+ *        -1 if analysis not available,
  *        0 if constraint has no matches,
  *        positive if constraint has matches.
+ *        Equal to the size of the {@link #matches} list.</li>
+ *        <li>For a {@link ScoreAnalysis#diff(ScoreAnalysis) diff of constraint analyses}:
+ *        positive if the constraint has more matches in the new analysis,
+ *        zero if the number of matches is the same in both,
+ *        negative otherwise.
+ *        Need not be equal to the size of the {@link #matches} list.</li>
+ *        </ul>
  */
 public record ConstraintAnalysis<Score_ extends Score<Score_>>(@NonNull ConstraintRef constraintRef, @NonNull Score_ weight,
         @NonNull Score_ score, @Nullable List<MatchAnalysis<Score_>> matches, int matchCount) {
@@ -56,20 +66,22 @@ public ConstraintAnalysis(@NonNull ConstraintRef constraintRef, @NonNull Score_
                 .formatted(DefaultConstraintMatchTotal.class.getSimpleName(),
                         ConstraintMatchAwareIncrementalScoreCalculator.class.getSimpleName()));
         Objects.requireNonNull(score);
-        if (matches != null && matchCount != matches.size()) {
-            throw new IllegalArgumentException("The match count must be equal to the size of the matches list.");
-        }
     }
 
     @NonNull
     ConstraintAnalysis<Score_> negate() {
+        // Only used to compute diff; use semantics for non-diff.
+        // A negative match count is only allowed within these semantics when matches == null.
         if (matches == null) {
+            // At this point, matchCount is already negative, as matches == null.
             return new ConstraintAnalysis<>(constraintRef, weight.negate(), score.negate(), null, matchCount);
         } else {
-            var negatedMatchAnalyses = matches.stream()
+            // Within these semantics, match count == list size.
+            var negatedMatchAnalysesList = matches.stream()
                     .map(MatchAnalysis::negate)
                     .toList();
-            return new ConstraintAnalysis<>(constraintRef, weight.negate(), score.negate(), negatedMatchAnalyses);
+            return new ConstraintAnalysis<>(constraintRef, weight.negate(), score.negate(), negatedMatchAnalysesList,
+                    matchCount);
         }
     }
 
@@ -99,19 +111,19 @@ ConstraintAnalysis<Score_> negate() {
         var constraintWeightDifference = constraintAnalysis.weight().subtract(otherConstraintAnalysis.weight());
         var scoreDifference = constraintAnalysis.score().subtract(otherConstraintAnalysis.score());
         if (matchAnalyses == null) {
-            var leftCount = constraintAnalysis.matchCount();
-            var rightCount = otherConstraintAnalysis.matchCount();
-            if ((leftCount == -1 && rightCount != -1) || (leftCount != -1 && rightCount == -1)) {
+            var leftHasMatchCount = hasMatchCount(constraintAnalysis);
+            var rightHasMatchCount = hasMatchCount(otherConstraintAnalysis);
+            if ((!leftHasMatchCount && rightHasMatchCount) || (leftHasMatchCount && !rightHasMatchCount)) {
                 throw new IllegalStateException(
                         "Impossible state: One of the score analyses (%s, %s) provided no match count for a constraint (%s)."
                                 .formatted(constraintAnalysis, otherConstraintAnalysis, constraintRef));
             }
             return new ConstraintAnalysis<>(constraintRef, constraintWeightDifference, scoreDifference, null,
-                    leftCount - rightCount);
+                    getMatchCount(constraintAnalysis, otherConstraintAnalysis));
         }
         var matchAnalysisMap = mapMatchesToJustifications(matchAnalyses);
         var otherMatchAnalysisMap = mapMatchesToJustifications(otherMatchAnalyses);
-        var result = Stream.concat(matchAnalysisMap.keySet().stream(), otherMatchAnalysisMap.keySet().stream())
+        var matchAnalysesList = Stream.concat(matchAnalysisMap.keySet().stream(), otherMatchAnalysisMap.keySet().stream())
                 .distinct()
                 .map(justification -> {
                     var matchAnalysis = matchAnalysisMap.get(justification);
@@ -133,7 +145,16 @@ ConstraintAnalysis<Score_> negate() {
                     }
                 })
                 .toList();
-        return new ConstraintAnalysis<>(constraintRef, constraintWeightDifference, scoreDifference, result);
+        return new ConstraintAnalysis<>(constraintRef, constraintWeightDifference, scoreDifference, matchAnalysesList,
+                getMatchCount(constraintAnalysis, otherConstraintAnalysis));
+    }
+
+    private static boolean hasMatchCount(ConstraintAnalysis<?> analysis) {
+        return analysis.matchCount >= 0;
+    }
+
+    private static int getMatchCount(ConstraintAnalysis<?> analysis, ConstraintAnalysis<?> otherAnalysis) {
+        return analysis.matchCount() - otherAnalysis.matchCount();
     }
 
     private static <Score_ extends Score<Score_>> Map<ConstraintJustification, MatchAnalysis<Score_>>
diff --git a/core/src/test/java/ai/timefold/solver/core/api/solver/SolutionManagerTest.java b/core/src/test/java/ai/timefold/solver/core/api/solver/SolutionManagerTest.java
@@ -636,6 +636,120 @@ void recommendAssignmentWithUnassigned(SolutionManagerSource SolutionManagerSour
         });
     }
 
+    @ParameterizedTest
+    @EnumSource(SolutionManagerSource.class)
+    void recommendAssignmentWithUnassignedMatchCount(SolutionManagerSource SolutionManagerSource) {
+        int valueSize = 3;
+        var solution = TestdataAllowsUnassignedSolution.generateSolution(valueSize, 3);
+        var uninitializedEntity = solution.getEntityList().get(2);
+        uninitializedEntity.setValue(null);
+
+        // At this point, entity 0 and entity 2 are unassigned.
+        // Entity 1 is assigned to value #1.
+        // But only entity2 should be processed for recommendations.
+        var solutionManager = SolutionManagerSource.createSolutionManager(SOLVER_FACTORY_UNASSIGNED);
+        assertThat(solutionManager).isNotNull();
+        var recommendationList =
+                solutionManager.recommendAssignment(solution, uninitializedEntity, TestdataAllowsUnassignedEntity::getValue,
+                        ScoreAnalysisFetchPolicy.FETCH_MATCH_COUNT);
+
+        // Three values means there need to be four recommendations, one extra for unassigned.
+        assertThat(recommendationList).hasSize(valueSize + 1);
+        /*
+         * The calculator penalizes how many entities have the same value as another entity.
+         * Therefore the recommendation to assign value 0 and value 2 need to come first and in the order of the placer,
+         * as it means two entities no longer share a value, improving the score.
+         */
+        var recommendation1 = recommendationList.get(0);
+        assertSoftly(softly -> {
+            softly.assertThat(recommendation1.proposition()).isEqualTo(solution.getValueList().get(0));
+            // Two entities no longer share null value; two less matches.
+            softly.assertThat(recommendation1.scoreAnalysisDiff()
+                    .score()).isEqualTo(SimpleScore.of(2));
+            softly.assertThat(recommendation1.scoreAnalysisDiff().constraintMap())
+                    .extractingFromEntries(e -> e.getValue().matchCount())
+                    .first()
+                    .isEqualTo(-2);
+        });
+        var recommendation2 = recommendationList.get(1);
+        assertSoftly(softly -> {
+            softly.assertThat(recommendation2.proposition()).isEqualTo(solution.getValueList().get(2));
+            softly.assertThat(recommendation2.scoreAnalysisDiff()
+                    .score()).isEqualTo(SimpleScore.of(2));
+        });
+        // The other two recommendations need to come in order of the placer; so null, then value #1.
+        var recommendation3 = recommendationList.get(2);
+        assertSoftly(softly -> {
+            softly.assertThat(recommendation3.proposition()).isEqualTo(null);
+            softly.assertThat(recommendation3.scoreAnalysisDiff()
+                    .score()).isEqualTo(SimpleScore.ZERO);
+        });
+        var recommendation4 = recommendationList.get(3);
+        assertSoftly(softly -> {
+            softly.assertThat(recommendation4.proposition()).isEqualTo(solution.getValueList().get(1));
+            softly.assertThat(recommendation4.scoreAnalysisDiff()
+                    .score()).isEqualTo(SimpleScore.ZERO);
+        });
+        // Ensure the original solution is in its original state.
+        assertSoftly(softly -> {
+            softly.assertThat(uninitializedEntity.getValue()).isNull();
+            softly.assertThat(solution.getEntityList().get(0).getValue()).isNull();
+            softly.assertThat(solution.getEntityList().get(1).getValue()).isEqualTo(solution.getValueList().get(1));
+            softly.assertThat(solution.getEntityList().get(2).getValue()).isNull();
+            softly.assertThat(solution.getScore()).isNull();
+        });
+    }
+
+    @ParameterizedTest
+    @EnumSource(SolutionManagerSource.class)
+    void recommendAssignmentWithUnassignedFetchAllVsFetchMatchCount(SolutionManagerSource SolutionManagerSource) {
+        int valueSize = 3;
+        var solution = TestdataAllowsUnassignedSolution.generateSolution(valueSize, 3);
+        var uninitializedEntity = solution.getEntityList().get(2);
+        uninitializedEntity.setValue(null);
+
+        // At this point, entity 0 and entity 2 are unassigned.
+        // Entity 1 is assigned to value #1.
+        // But only entity2 should be processed for recommendations.
+        var solutionManager = SolutionManagerSource.createSolutionManager(SOLVER_FACTORY_UNASSIGNED);
+        assertThat(solutionManager).isNotNull();
+        var matchCountRecommendationList =
+                solutionManager.recommendAssignment(solution, uninitializedEntity, TestdataAllowsUnassignedEntity::getValue,
+                        ScoreAnalysisFetchPolicy.FETCH_MATCH_COUNT);
+
+        var justificationsRecommendationList =
+                solutionManager.recommendAssignment(solution, uninitializedEntity, TestdataAllowsUnassignedEntity::getValue,
+                        ScoreAnalysisFetchPolicy.FETCH_ALL);
+
+        // Three values means there need to be four recommendations, one extra for unassigned.
+        assertThat(matchCountRecommendationList).hasSize(valueSize + 1);
+        assertThat(justificationsRecommendationList).hasSize(valueSize + 1);
+        /*
+         * The calculator penalizes how many entities have the same value as another entity.
+         * Therefore the recommendation to assign value 0 and value 2 need to come first and in the order of the placer,
+         * as it means two entities no longer share a value, improving the score.
+         */
+        var matchCountRecommendation1 = matchCountRecommendationList.get(0);
+        var justificationsRecommendation1 = justificationsRecommendationList.get(0);
+
+        assertSoftly(softly -> {
+            softly.assertThat(matchCountRecommendation1.proposition()).isEqualTo(solution.getValueList().get(0));
+            softly.assertThat(justificationsRecommendation1.proposition()).isEqualTo(solution.getValueList().get(0));
+
+            softly.assertThat(matchCountRecommendation1.scoreAnalysisDiff()
+                    .score()).isEqualTo(SimpleScore.of(2)); // Two entities no longer share null value.
+            softly.assertThat(justificationsRecommendation1.scoreAnalysisDiff()
+                    .score()).isEqualTo(SimpleScore.of(2)); // Two entities no longer share null value.
+
+            // The matchCount is expected to be the same in the case of both FETCH_ALL and FETCH_MATCH_COUNT
+            int matchCountForFetchMatchCount = matchCountRecommendation1.scoreAnalysisDiff()
+                    .constraintMap().values().iterator().next().matchCount();
+            int matchCountForFetchAll = justificationsRecommendation1.scoreAnalysisDiff()
+                    .constraintMap().values().iterator().next().matchCount();
+            softly.assertThat(matchCountForFetchMatchCount).isEqualTo(matchCountForFetchAll);
+        });
+    }
+
     @ParameterizedTest
     @EnumSource(SolutionManagerSource.class)
     void recommendAssignmentWithAllAssigned(SolutionManagerSource SolutionManagerSource) {
diff --git a/python/python-core/src/main/python/score/_score_analysis.py b/python/python-core/src/main/python/score/_score_analysis.py
@@ -352,7 +352,7 @@ def constraint_match_total_map(self) -> dict[str, ConstraintMatchTotal]:
             e.getKey()[len(_user_package_prefix):]: ConstraintMatchTotal(
                 constraint_ref=ConstraintRef(
                     package_name=e.getValue().getConstraintRef().packageName()[len(_user_package_prefix):],
-                                             constraint_name=e.getValue().getConstraintRef().constraintName()),
+                    constraint_name=e.getValue().getConstraintRef().constraintName()),
                 constraint_match_count=e.getValue().getConstraintMatchCount(),
                 constraint_match_set=_map_constraint_match_set(e.getValue().getConstraintMatchSet()),
                 constraint_weight=to_python_score(e.getValue().getConstraintWeight()),
@@ -464,7 +464,16 @@ class ConstraintAnalysis(Generic[Score_]):
         Returns a diagnostic text
         that explains part of the score quality through the ConstraintAnalysis API.
     match_count : int
-        Return the match count of the constraint.
+        For regular constraint analysis:
+        -1 if analysis not available,
+        0 if constraint has no matches,
+        positive if constraint has matches.
+        Equal to the size of the matches list.</li>
+        For a diff of constraint analyses:
+        positive if the constraint has more matches in the new analysis,
+        zero if the number of matches is the same in both,
+        negative otherwise.
+        Need not be equal to the size of the matches list.
     """
     _delegate: '_JavaConstraintAnalysis[Score_]'
 
