Don't proliferate the metric switching

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

Hide file tree

Showing 10 changed files with 91 additions and 46 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();
@@ -56,7 +56,7 @@ public void solve(SolverScope<Solution_> solverScope) {
         }
 
         for (var placement : entityPlacer) {
-            var stepScope = new ConstructionHeuristicStepScope<>(phaseScope);
+            var stepScope = phaseScope.buildStepScope();
             stepStarted(stepScope);
             decider.decideNextStep(stepScope, placement);
             if (stepScope.getStep() == null) {
@@ -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/...imefold/solver/core/impl/constructionheuristic/scope/ConstructionHeuristicPhaseScope.java b/...imefold/solver/core/impl/constructionheuristic/scope/ConstructionHeuristicPhaseScope.java
@@ -7,13 +7,21 @@
 /**
  * @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;
 
     public ConstructionHeuristicPhaseScope(SolverScope<Solution_> solverScope, int phaseIndex) {
         super(solverScope, phaseIndex, false);
-        lastCompletedStepScope = new ConstructionHeuristicStepScope<>(this, -1);
+        lastCompletedStepScope = buildStepScope(-1);
+    }
+
+    public ConstructionHeuristicStepScope<Solution_> buildStepScope() {
+        return buildStepScope(getNextStepIndex());
+    }
+
+    protected ConstructionHeuristicStepScope<Solution_> buildStepScope(int stepIndex) {
+        return new ConstructionHeuristicStepScope<>(this, stepIndex);
     }
 
     @Override
@@ -25,8 +33,8 @@ public void setLastCompletedStepScope(ConstructionHeuristicStepScope<Solution_>
         this.lastCompletedStepScope = lastCompletedStepScope;
     }
 
-    // ************************************************************************
-    // Calculated methods
-    // ************************************************************************
+    public void addMoveEvaluationCountPerType(String moveType, long count) {
+        solverScope.addMoveEvaluationCountPerType(moveType, count);
+    }
 
-}
+}
diff --git a/...timefold/solver/core/impl/constructionheuristic/scope/ConstructionHeuristicStepScope.java b/...timefold/solver/core/impl/constructionheuristic/scope/ConstructionHeuristicStepScope.java
@@ -7,7 +7,7 @@
 /**
  * @param <Solution_> the solution type, the class with the {@link PlanningSolution} annotation
  */
-public final class ConstructionHeuristicStepScope<Solution_> extends AbstractStepScope<Solution_> {
+public class ConstructionHeuristicStepScope<Solution_> extends AbstractStepScope<Solution_> {
 
     private final ConstructionHeuristicPhaseScope<Solution_> phaseScope;
 

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,33 @@
+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.constructionheuristic.scope.ConstructionHeuristicStepScope;
+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 ConstructionHeuristicStepScope<Solution_> buildStepScope() {
+        return buildStepScope(getNextStepIndex());
+    }
+
+    @Override
+    protected ConstructionHeuristicStepScope<Solution_> buildStepScope(int stepIndex) {
+        return new RuinRecreateConstructionHeuristicStepScope<>(this, stepIndex);
+    }
+
+    @Override
+    public void addChildThreadsMoveEvaluationCount(long addition) {
+        // Nested phase does not count moves.
+    }
+
+    @Override
+    public void addMoveEvaluationCountPerType(String moveType, long count) {
+        // Nested phase does not count moves.
+    }
+
+}
diff --git a/...core/impl/heuristic/selector/move/generic/RuinRecreateConstructionHeuristicStepScope.java b/...core/impl/heuristic/selector/move/generic/RuinRecreateConstructionHeuristicStepScope.java
@@ -0,0 +1,18 @@
+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.constructionheuristic.scope.ConstructionHeuristicStepScope;
+import ai.timefold.solver.core.impl.heuristic.move.Move;
+
+final class RuinRecreateConstructionHeuristicStepScope<Solution_> extends ConstructionHeuristicStepScope<Solution_> {
+
+    public RuinRecreateConstructionHeuristicStepScope(ConstructionHeuristicPhaseScope<Solution_> phaseScope, int stepIndex) {
+        super(phaseScope, stepIndex);
+    }
+
+    @Override
+    public void incrementMoveEvaluationCount(Move<?> move) {
+        // Nested phase does not count moves.
+    }
+
+}
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
@@ -34,8 +34,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 +91,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
     // ************************************************************************

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
@@ -47,11 +47,10 @@ public void setBestScoreImproved(Boolean 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);
-            }
+        var solverScope = getPhaseScope().getSolverScope();
+        solverScope.addMoveEvaluationCount(1L);
+        if (solverScope.isMetricEnabled(SolverMetric.MOVE_COUNT_PER_TYPE)) {
+            solverScope.incrementMoveEvaluationCountPerType(move);
         }
     }