Don't proliferate the metric switching

TimefoldAI · Sep 26, 2024 · 71f1327 · 71f1327
1 parent 0ebfa91
commit 71f1327
Show file tree

Hide file tree

Showing 12 changed files with 71 additions and 60 deletions.
diff --git a/...ai/timefold/solver/core/impl/constructionheuristic/DefaultConstructionHeuristicPhase.java b/...ai/timefold/solver/core/impl/constructionheuristic/DefaultConstructionHeuristicPhase.java
@@ -40,7 +40,7 @@ public String getPhaseTypeString() {
     // ************************************************************************
     @Override
     public void solve(SolverScope<Solution_> solverScope) {
-        var phaseScope = new ConstructionHeuristicPhaseScope<>(solverScope, phaseIndex);
+        var phaseScope = buildPhaseScope(solverScope, phaseIndex);
         phaseStarted(phaseScope);
 
         var solutionDescriptor = solverScope.getSolutionDescriptor();
@@ -94,6 +94,10 @@ public void solve(SolverScope<Solution_> solverScope) {
         phaseEnded(phaseScope);
     }
 
+    protected ConstructionHeuristicPhaseScope<Solution_> buildPhaseScope(SolverScope<Solution_> solverScope, int phaseIndex) {
+        return new ConstructionHeuristicPhaseScope<>(solverScope, phaseIndex);
+    }
+
     private void doStep(ConstructionHeuristicStepScope<Solution_> stepScope) {
         var step = stepScope.getStep();
         step.doMoveOnly(stepScope.getScoreDirector());

diff --git a/.../core/impl/constructionheuristic/decider/forager/DefaultConstructionHeuristicForager.java b/.../core/impl/constructionheuristic/decider/forager/DefaultConstructionHeuristicForager.java
@@ -39,7 +39,8 @@ public void stepEnded(ConstructionHeuristicStepScope<Solution_> stepScope) {
     @Override
     public void addMove(ConstructionHeuristicMoveScope<Solution_> moveScope) {
         selectedMoveCount++;
-        moveScope.getStepScope().incrementMoveEvaluationCount(moveScope.getMove());
+        moveScope.getStepScope().getPhaseScope()
+                .addMoveEvaluationCount(moveScope.getMove(), 1L);
         checkPickEarly(moveScope);
         if (maxScoreMoveScope == null || moveScope.getScore().compareTo(maxScoreMoveScope.getScore()) > 0) {
             maxScoreMoveScope = moveScope;

diff --git a/...imefold/solver/core/impl/constructionheuristic/scope/ConstructionHeuristicPhaseScope.java b/...imefold/solver/core/impl/constructionheuristic/scope/ConstructionHeuristicPhaseScope.java
@@ -7,7 +7,7 @@
 /**
  * @param <Solution_> the solution type, the class with the {@link PlanningSolution} annotation
  */
-public final class ConstructionHeuristicPhaseScope<Solution_> extends AbstractPhaseScope<Solution_> {
+public class ConstructionHeuristicPhaseScope<Solution_> extends AbstractPhaseScope<Solution_> {
 
     private ConstructionHeuristicStepScope<Solution_> lastCompletedStepScope;
 
@@ -25,8 +25,4 @@ public void setLastCompletedStepScope(ConstructionHeuristicStepScope<Solution_>
         this.lastCompletedStepScope = lastCompletedStepScope;
     }
 
-    // ************************************************************************
-    // Calculated methods
-    // ************************************************************************
-
-}
+}
diff --git a/...n/java/ai/timefold/solver/core/impl/exhaustivesearch/decider/ExhaustiveSearchDecider.java b/...n/java/ai/timefold/solver/core/impl/exhaustivesearch/decider/ExhaustiveSearchDecider.java
@@ -105,7 +105,8 @@ public void expandNode(ExhaustiveSearchStepScope<Solution_> stepScope) {
         manualEntityMimicRecorder.setRecordedEntity(expandingNode.getEntity());
 
         int moveIndex = 0;
-        ExhaustiveSearchLayer moveLayer = stepScope.getPhaseScope().getLayerList().get(expandingNode.getDepth() + 1);
+        var phaseScope = stepScope.getPhaseScope();
+        ExhaustiveSearchLayer moveLayer = phaseScope.getLayerList().get(expandingNode.getDepth() + 1);
         for (Move<?> move : moveSelector) {
             ExhaustiveSearchNode moveNode = new ExhaustiveSearchNode(moveLayer, expandingNode);
             moveIndex++;
@@ -114,10 +115,10 @@ public void expandNode(ExhaustiveSearchStepScope<Solution_> stepScope) {
             // If the original value is null and the variable allows unassigned values,
             // the move to null must be done too.
             doMove(stepScope, moveNode);
-            stepScope.incrementMoveEvaluationCount(move);
+            phaseScope.addMoveEvaluationCount(move, 1);
             // TODO in the lowest level (and only in that level) QuitEarly can be useful
             // No QuitEarly because lower layers might be promising
-            stepScope.getPhaseScope().getSolverScope().checkYielding();
+            phaseScope.getSolverScope().checkYielding();
             if (termination.isPhaseTerminated(stepScope.getPhaseScope())) {
                 break;
             }

diff --git a/...ver/core/impl/heuristic/selector/move/generic/RuinRecreateConstructionHeuristicPhase.java b/...ver/core/impl/heuristic/selector/move/generic/RuinRecreateConstructionHeuristicPhase.java
@@ -4,6 +4,8 @@
 import ai.timefold.solver.core.impl.constructionheuristic.DefaultConstructionHeuristicPhase;
 import ai.timefold.solver.core.impl.constructionheuristic.scope.ConstructionHeuristicPhaseScope;
 import ai.timefold.solver.core.impl.constructionheuristic.scope.ConstructionHeuristicStepScope;
+import ai.timefold.solver.core.impl.phase.scope.AbstractStepScope;
+import ai.timefold.solver.core.impl.solver.scope.SolverScope;
 
 final class RuinRecreateConstructionHeuristicPhase<Solution_>
         extends DefaultConstructionHeuristicPhase<Solution_>
@@ -13,6 +15,16 @@ final class RuinRecreateConstructionHeuristicPhase<Solution_>
         super(builder);
     }
 
+    @Override
+    protected void collectMetrics(AbstractStepScope<Solution_> stepScope) {
+        // Nested phase doesn't collect metrics.
+    }
+
+    @Override
+    protected ConstructionHeuristicPhaseScope<Solution_> buildPhaseScope(SolverScope<Solution_> solverScope, int phaseIndex) {
+        return new RuinRecreateConstructionHeuristicPhaseScope<>(solverScope, phaseIndex);
+    }
+
     @Override
     protected void processWorkingSolutionDuringStep(ConstructionHeuristicStepScope<Solution_> stepScope) {
         // Ruin and Recreate CH doesn't process the working solution, it is a nested phase.

diff --git a/...e/impl/heuristic/selector/move/generic/RuinRecreateConstructionHeuristicPhaseBuilder.java b/...e/impl/heuristic/selector/move/generic/RuinRecreateConstructionHeuristicPhaseBuilder.java
@@ -58,7 +58,6 @@ public EntityPlacer<Solution_> getEntityPlacer() {
 
     @Override
     public DefaultConstructionHeuristicPhase<Solution_> build() {
-        disableMetricCollection();
         return new RuinRecreateConstructionHeuristicPhase<>(this);
     }
 }
diff --git a/...ore/impl/heuristic/selector/move/generic/RuinRecreateConstructionHeuristicPhaseScope.java b/...ore/impl/heuristic/selector/move/generic/RuinRecreateConstructionHeuristicPhaseScope.java
@@ -0,0 +1,23 @@
+package ai.timefold.solver.core.impl.heuristic.selector.move.generic;
+
+import ai.timefold.solver.core.impl.constructionheuristic.scope.ConstructionHeuristicPhaseScope;
+import ai.timefold.solver.core.impl.heuristic.move.Move;
+import ai.timefold.solver.core.impl.solver.scope.SolverScope;
+
+final class RuinRecreateConstructionHeuristicPhaseScope<Solution_> extends ConstructionHeuristicPhaseScope<Solution_> {
+
+    public RuinRecreateConstructionHeuristicPhaseScope(SolverScope<Solution_> solverScope, int phaseIndex) {
+        super(solverScope, phaseIndex);
+    }
+
+    @Override
+    public void addChildThreadsMoveEvaluationCount(long addition) {
+        // Nested phase does not count moves.
+    }
+
+    @Override
+    public void addMoveEvaluationCount(Move<?> move, long count) {
+        // Nested phase does not count moves.
+    }
+
+}
diff --git a/.../ai/timefold/solver/core/impl/localsearch/decider/forager/AcceptedLocalSearchForager.java b/.../ai/timefold/solver/core/impl/localsearch/decider/forager/AcceptedLocalSearchForager.java
@@ -74,7 +74,7 @@ public boolean supportsNeverEndingMoveSelector() {
     @Override
     public void addMove(LocalSearchMoveScope<Solution_> moveScope) {
         selectedMoveCount++;
-        moveScope.getStepScope().incrementMoveEvaluationCount(moveScope.getMove());
+        moveScope.getStepScope().getPhaseScope().addMoveEvaluationCount(moveScope.getMove(), 1);
         if (moveScope.getAccepted()) {
             acceptedMoveCount++;
             checkPickEarly(moveScope);

diff --git a/core/src/main/java/ai/timefold/solver/core/impl/phase/AbstractPhase.java b/core/src/main/java/ai/timefold/solver/core/impl/phase/AbstractPhase.java
@@ -34,8 +34,6 @@ public abstract class AbstractPhase<Solution_> implements Phase<Solution_> {
     protected final boolean assertShadowVariablesAreNotStaleAfterStep;
     protected final boolean triggerFirstInitializedSolutionEvent;
 
-    protected final boolean enableCollectMetrics;
-
     /** Used for {@link #addPhaseLifecycleListener(PhaseLifecycleListener)}. */
     protected PhaseLifecycleSupport<Solution_> phaseLifecycleSupport = new PhaseLifecycleSupport<>();
 
@@ -48,7 +46,6 @@ protected AbstractPhase(Builder<Solution_> builder) {
         assertStepScoreFromScratch = builder.assertStepScoreFromScratch;
         assertExpectedStepScore = builder.assertExpectedStepScore;
         assertShadowVariablesAreNotStaleAfterStep = builder.assertShadowVariablesAreNotStaleAfterStep;
-        enableCollectMetrics = builder.enableCollectMetrics;
         triggerFirstInitializedSolutionEvent = builder.triggerFirstInitializedSolutionEvent;
     }
 
@@ -106,7 +103,6 @@ public void solvingEnded(SolverScope<Solution_> solverScope) {
     @Override
     public void phaseStarted(AbstractPhaseScope<Solution_> phaseScope) {
         phaseScope.startingNow();
-        phaseScope.setEnableCollectMetrics(enableCollectMetrics);
         phaseScope.reset();
         solver.phaseStarted(phaseScope);
         phaseTermination.phaseStarted(phaseScope);
@@ -160,15 +156,13 @@ protected <Score_ extends Score<Score_>> void predictWorkingStepScore(AbstractSt
     @Override
     public void stepEnded(AbstractStepScope<Solution_> stepScope) {
         solver.stepEnded(stepScope);
-        if (enableCollectMetrics) {
-            collectMetrics(stepScope);
-        }
+        collectMetrics(stepScope);
         phaseTermination.stepEnded(stepScope);
         phaseLifecycleSupport.fireStepEnded(stepScope);
     }
 
-    private void collectMetrics(AbstractStepScope<Solution_> stepScope) {
-        SolverScope<Solution_> solverScope = stepScope.getPhaseScope().getSolverScope();
+    protected void collectMetrics(AbstractStepScope<Solution_> stepScope) {
+        var solverScope = stepScope.getPhaseScope().getSolverScope();
         if (solverScope.isMetricEnabled(SolverMetric.STEP_SCORE) && stepScope.getScore().isSolutionInitialized()) {
             SolverMetric.registerScoreMetrics(SolverMetric.STEP_SCORE,
                     solverScope.getMonitoringTags(),
@@ -231,8 +225,6 @@ protected abstract static class Builder<Solution_> {
         private boolean assertExpectedStepScore = false;
         private boolean assertShadowVariablesAreNotStaleAfterStep = false;
 
-        private boolean enableCollectMetrics = true;
-
         protected Builder(int phaseIndex, String logIndentation, Termination<Solution_> phaseTermination) {
             this(phaseIndex, false, logIndentation, phaseTermination);
         }
@@ -257,10 +249,6 @@ public void setAssertShadowVariablesAreNotStaleAfterStep(boolean assertShadowVar
             this.assertShadowVariablesAreNotStaleAfterStep = assertShadowVariablesAreNotStaleAfterStep;
         }
 
-        public void disableMetricCollection() {
-            this.enableCollectMetrics = false;
-        }
-
         protected abstract AbstractPhase<Solution_> build();
     }
 }
diff --git a/core/src/main/java/ai/timefold/solver/core/impl/phase/scope/AbstractPhaseScope.java b/core/src/main/java/ai/timefold/solver/core/impl/phase/scope/AbstractPhaseScope.java
@@ -4,7 +4,9 @@
 
 import ai.timefold.solver.core.api.domain.solution.PlanningSolution;
 import ai.timefold.solver.core.api.score.Score;
+import ai.timefold.solver.core.config.solver.monitoring.SolverMetric;
 import ai.timefold.solver.core.impl.domain.solution.descriptor.SolutionDescriptor;
+import ai.timefold.solver.core.impl.heuristic.move.Move;
 import ai.timefold.solver.core.impl.score.director.InnerScoreDirector;
 import ai.timefold.solver.core.impl.solver.scope.SolverScope;
 
@@ -34,8 +36,6 @@ public abstract class AbstractPhaseScope<Solution_> {
 
     protected int bestSolutionStepIndex;
 
-    protected boolean enableCollectMetrics = true;
-
     /**
      * As defined by #AbstractPhaseScope(SolverScope, int, boolean)
      * with the phaseSendingBestSolutionEvents parameter set to true.
@@ -93,20 +93,6 @@ public void setBestSolutionStepIndex(int bestSolutionStepIndex) {
 
     public abstract AbstractStepScope<Solution_> getLastCompletedStepScope();
 
-    /**
-     * @return true, if the metrics collection, such as
-     *         {@link ai.timefold.solver.core.config.solver.monitoring.SolverMetric#MOVE_COUNT_PER_TYPE MOVE_COUNT_PER_TYPE},
-     *         is enabled.
-     *         This is disabled for nested phases, such as Construction heuristics in Ruin and Recreate.
-     */
-    public boolean isMetricCollectionEnabled() {
-        return enableCollectMetrics;
-    }
-
-    public void setEnableCollectMetrics(boolean enableCollectMetrics) {
-        this.enableCollectMetrics = enableCollectMetrics;
-    }
-
     // ************************************************************************
     // Calculated methods
     // ************************************************************************
@@ -159,6 +145,23 @@ public void addChildThreadsMoveEvaluationCount(long addition) {
         childThreadsMoveEvaluationCount += addition;
     }
 
+    public void addMoveEvaluationCount(Move<?> move, long count) {
+        solverScope.addMoveEvaluationCount(1);
+        addMoveEvaluationCountPerType(move, count);
+    }
+
+    public void addMoveEvaluationCountPerType(Move<?> move, long count) {
+        if (solverScope.isMetricEnabled(SolverMetric.MOVE_COUNT_PER_TYPE)) {
+            solverScope.addMoveEvaluationCountPerType(move.getSimpleMoveTypeDescription(), count);
+        }
+    }
+
+    public void addMoveEvaluationCountPerType(String moveDescription, long count) {
+        if (solverScope.isMetricEnabled(SolverMetric.MOVE_COUNT_PER_TYPE)) {
+            solverScope.addMoveEvaluationCountPerType(moveDescription, count);
+        }
+    }
+
     public long getPhaseScoreCalculationCount() {
         return endingScoreCalculationCount - startingScoreCalculationCount + childThreadsScoreCalculationCount;
     }

diff --git a/core/src/main/java/ai/timefold/solver/core/impl/phase/scope/AbstractStepScope.java b/core/src/main/java/ai/timefold/solver/core/impl/phase/scope/AbstractStepScope.java
@@ -4,8 +4,6 @@
 
 import ai.timefold.solver.core.api.domain.solution.PlanningSolution;
 import ai.timefold.solver.core.api.score.Score;
-import ai.timefold.solver.core.config.solver.monitoring.SolverMetric;
-import ai.timefold.solver.core.impl.heuristic.move.Move;
 import ai.timefold.solver.core.impl.score.director.InnerScoreDirector;
 
 /**
@@ -46,15 +44,6 @@ public void setBestScoreImproved(Boolean bestScoreImproved) {
         this.bestScoreImproved = bestScoreImproved;
     }
 
-    public void incrementMoveEvaluationCount(Move<?> move) {
-        if (getPhaseScope().isMetricCollectionEnabled()) {
-            getPhaseScope().getSolverScope().addMoveEvaluationCount(1L);
-            if (getPhaseScope().getSolverScope().isMetricEnabled(SolverMetric.MOVE_COUNT_PER_TYPE)) {
-                getPhaseScope().getSolverScope().incrementMoveEvaluationCountPerType(move);
-            }
-        }
-    }
-
     // ************************************************************************
     // Calculated methods
     // ************************************************************************

diff --git a/core/src/main/java/ai/timefold/solver/core/impl/solver/scope/SolverScope.java b/core/src/main/java/ai/timefold/solver/core/impl/solver/scope/SolverScope.java
@@ -18,7 +18,6 @@
 import ai.timefold.solver.core.api.solver.Solver;
 import ai.timefold.solver.core.config.solver.monitoring.SolverMetric;
 import ai.timefold.solver.core.impl.domain.solution.descriptor.SolutionDescriptor;
-import ai.timefold.solver.core.impl.heuristic.move.Move;
 import ai.timefold.solver.core.impl.phase.scope.AbstractPhaseScope;
 import ai.timefold.solver.core.impl.score.definition.ScoreDefinition;
 import ai.timefold.solver.core.impl.score.director.InnerScoreDirector;
@@ -376,10 +375,6 @@ public void destroyYielding() {
         }
     }
 
-    public void incrementMoveEvaluationCountPerType(Move<?> move) {
-        addMoveEvaluationCountPerType(move.getSimpleMoveTypeDescription(), 1L);
-    }
-
     public void addMoveEvaluationCountPerType(String moveType, long count) {
         moveEvaluationCountPerTypeMap.compute(moveType, (key, counter) -> {
             if (counter == null) {