feat: add new adaptive termination type #1313

Christopher-Chianelli · 2025-01-10T21:06:32Z

The adaptive termination type has the concept of a grace period G and minimum improvement ratio R.

The adaptive termination type works as follows.

When in the grace period, do not terminate.
If a non-lowest score level changes at any point, reset the grace period (or enter a new one if the grace period is finished).
After the duration G of the grace period passes, record the difference of final best score and the starting best score as Y_0. If this is 0, terminate.
On each step, calculate the difference between the current best score and the best score G moments ago as Y_n. If Y_n / Y < R (i.e. the score did not improve by at least R% of the improvement in the grace window), terminate.

The adaptive termination type has the concept of a grace period G and minimum improvement ratio R. The adaptive termination type works as follows. - When in the grace period, do not terminate. - If a non-lowest score level changes at any point, reset the grace period (or enter a new one if the grace period is finished). - After the duration G of the grace period passes, record the difference of final best score and the starting best score as Y_0. If this is 0, terminate. - On each step, calculate the difference between the current best score and the best score G moments ago as Y_n. If Y_n / Y < R (i.e. the score did not improve by at least R% of the improvement in the grace window), terminate.

…heck

triceo

That was quick! Leaving suggestions for improvement.

Maybe we introduce some TRACE-level logging? This termination is doing smart things, and some visibility into the mechanism may be necessary. We should find a decent way of logging the decrease of the ratio - maybe log every time in halves?

Also required:

Documentation. Preferrably with a visualization as above.
Naming. (Already started a conversation on Slack.)
Add this to quickstarts?

triceo · 2025-01-11T09:04:36Z

core/src/main/java/ai/timefold/solver/core/impl/solver/termination/AdaptiveTermination.java

+    public AdaptiveTermination(long gracePeriodMillis, double minimumImprovementRatio) {
+        this.gracePeriodMillis = gracePeriodMillis;
+        this.minimumImprovementRatio = minimumImprovementRatio;
+        this.scoresByTime = new TreeMap<>();


I was skeptical of this approach, as fast-stepping algorithms will easily put tens of thousands of entries to this map per second, and I was expecting that to be both CPU-intensive and memory-intensive.

However, in the experiments I ran, this did not materialize. Impact of the termination is barely visible in the data. Interesting.

I wonder though if the TreeMap is bringing any tangible benefits. Consider this:

We don't actually need sorting, do we? The values are sequential.

We are forced to box long into Long, causing some unnecessary GC pressure.

I wonder if we could convert this into List<Score_>. The index would have to become relative to the last reset, so starting with zero. But it may cause large gaps, and those would be a problem in their own right. So maybe not.

triceo · 2025-01-11T09:06:48Z

core/src/main/java/ai/timefold/solver/core/impl/solver/termination/AdaptiveTermination.java

+        this.scoresByTime = new TreeMap<>();
+    }
+
+    private record LevelScoreDiff(boolean harderScoreChanged, double softestScoreDiff) {


Typically, I would be all for a carrier type like this. In this case, and on the hot path, I'd rather store the two variables individually and avoid allocating instances of the wrapper.

In fact, this is the only thing that actually made a measurable impact in my experiments - LevelScoreDiff was ~10th most allocated object on the heap.

triceo · 2025-01-11T09:12:56Z

core/src/main/java/ai/timefold/solver/core/impl/solver/termination/AdaptiveTermination.java

+    @Override
+    @SuppressWarnings("unchecked")
+    public boolean isSolverTerminated(SolverScope<Solution_> solverScope) {
+        return isTerminated(System.currentTimeMillis(), (Score_) solverScope.getBestScore());


I wonder if we should use nanotime instead, and truncate it to millis.
The precision of currentTimeMillis is questionable, see Javadoc:

Note that while the unit of time of the return value is a millisecond, the granularity of the value depends on the underlying operating system and may be larger. For example, many operating systems measure time in units of tens of milliseconds.

Also... should this terminate the solver even in, for example, a CH phase? Arguably it should not, the CH phase generally doesn't improve.

The way how unimproved terminations get around it is by forcing themselves to be only admissible in local search phase. Maybe we do something like that too?

Although I really don't like that, because it causes config pain. (You need to define LS phase, and put the termination inside of it.) Maybe we refactor the terminations a bit, so that it is possible for an unimproved termination to be defined on the solver-level, but never terminate a CH? Would need a discussion, I think.

(My ultimate goal is to eventually have only one termination. This one. And have it be the default, without any configuration. And this quirk is preventing that from happening, bwd compat issues aside.)

triceo · 2025-01-11T09:18:15Z

core/src/main/java/ai/timefold/solver/core/impl/solver/termination/AdaptiveTermination.java

+
+    @Override
+    public double calculateSolverTimeGradient(SolverScope<Solution_> solverScope) {
+        return -1.0;


I wonder if there's a way to estimate this based on how close we are to the minimum necessary improvement.

triceo · 2025-01-11T09:38:15Z

core/src/main/java/ai/timefold/solver/core/impl/solver/termination/AdaptiveTermination.java

+         */
+        public static <Score_ extends Score<Score_>> @Nullable LevelScoreDiff between(@NonNull Score_ start,
+                @NonNull Score_ end) {
+            var scoreDiffs = end.subtract(start).toLevelDoubles();


Arguably, you can save yourself some of this work. compareTo() the two scores; if no better, no need to do anything anymore. The comparison works directly on the underlying data types, so no need to convert them to doubles and create a new array of them.

triceo · 2025-01-11T09:42:10Z

core/src/main/java/ai/timefold/solver/core/impl/solver/termination/AdaptiveTermination.java

+import org.jspecify.annotations.NonNull;
+import org.jspecify.annotations.Nullable;


You can use @NullMarked on the class, which means that everything is @NonNull unless marked as @Nullable. In my experience, this saves a lot of hassle.

- Use a signal exception to avoid the need for a carrier type, allowing use to return double instead of Double or LevelScoreDiff - Use a pair of ring buffers to represent the TreeMap<> to avoid creating boxed types and to optimize on the fact the entries we are looking for are often near the start, and we are always appending to the end

sonarqubecloud · 2025-01-13T22:35:40Z

Quality Gate failed

Failed conditions
53.9% Coverage on New Code (required ≥ 70%)
C Reliability Rating on New Code (required ≥ A)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

triceo · 2025-01-14T06:41:13Z

core/src/main/java/ai/timefold/solver/core/impl/solver/termination/AdaptiveTermination.java

+        var softestLevel = scoreDiffs.length - 1;
+        for (int i = 0; i < softestLevel; i++) {
+            if (scoreDiffs[i] != 0.0) {
+                throw new HardLevelImprovedSignal();


May I suggest we return an impossible value instead of using exceptions for control flow? Perhaps NaN?

I actually found using exceptions made the logic a bit cleaner, since there are two separate places the method gets called and need to do the same logic (the difference being in what old score the new score is being compared to). It also forces handling the exception/signal, whereas it can be easy to forget checking an impossible value.

triceo · 2025-01-14T06:43:33Z

core/src/main/java/ai/timefold/solver/core/impl/solver/termination/AdaptiveTermination.java

+
+    private static <Score_ extends Score<Score_>> double softImprovement(@NonNull Score_ start,
+            @NonNull Score_ end) throws HardLevelImprovedSignal {
+        if (start.equals(end)) {


Can the score be worse here? Arguably, it can - sometimes the solver picks a deteriorating move to try to get out of a local optima.

No, it uses best score, not step score. Best score should never decrease.

stepScope.getPhaseScope().getBestScore()

Christopher-Chianelli requested a review from triceo January 10, 2025 21:06

Christopher-Chianelli temporarily deployed to internal January 10, 2025 21:06 — with GitHub Actions Inactive

Christopher-Chianelli deployed to documentation (preview) January 10, 2025 21:06 — with GitHub Actions View deployment

fix: set adaptive config to null and include it in the isConfigured c…

a2d314f

…heck

Christopher-Chianelli temporarily deployed to internal January 10, 2025 21:28 — with GitHub Actions Inactive

Christopher-Chianelli deployed to documentation (preview) January 10, 2025 21:28 — with GitHub Actions View deployment

triceo reviewed Jan 11, 2025

View reviewed changes

triceo mentioned this pull request Jan 11, 2025

feat: add new termination config based on the best solution improvement #1234

Closed

Christopher-Chianelli deployed to internal January 13, 2025 22:11 — with GitHub Actions Active

Christopher-Chianelli deployed to documentation (preview) January 13, 2025 22:12 — with GitHub Actions View deployment

triceo reviewed Jan 14, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add new adaptive termination type #1313

feat: add new adaptive termination type #1313

Christopher-Chianelli commented Jan 10, 2025

triceo left a comment •

edited

Loading

triceo Jan 11, 2025

triceo Jan 11, 2025 •

edited

Loading

triceo Jan 11, 2025 •

edited

Loading

triceo Jan 11, 2025

triceo Jan 11, 2025 •

edited

Loading

triceo Jan 11, 2025

triceo Jan 11, 2025 •

edited

Loading

triceo Jan 11, 2025 •

edited

Loading

sonarqubecloud bot commented Jan 13, 2025

triceo Jan 14, 2025

Christopher-Chianelli Jan 14, 2025

triceo Jan 14, 2025 •

edited

Loading

Christopher-Chianelli Jan 14, 2025

		import org.jspecify.annotations.NonNull;
		import org.jspecify.annotations.Nullable;

feat: add new adaptive termination type #1313

Are you sure you want to change the base?

feat: add new adaptive termination type #1313

Conversation

Christopher-Chianelli commented Jan 10, 2025

triceo left a comment • edited Loading

Choose a reason for hiding this comment

triceo Jan 11, 2025

Choose a reason for hiding this comment

triceo Jan 11, 2025 • edited Loading

Choose a reason for hiding this comment

triceo Jan 11, 2025 • edited Loading

Choose a reason for hiding this comment

triceo Jan 11, 2025

Choose a reason for hiding this comment

triceo Jan 11, 2025 • edited Loading

Choose a reason for hiding this comment

triceo Jan 11, 2025

Choose a reason for hiding this comment

triceo Jan 11, 2025 • edited Loading

Choose a reason for hiding this comment

triceo Jan 11, 2025 • edited Loading

Choose a reason for hiding this comment

sonarqubecloud bot commented Jan 13, 2025

Quality Gate failed

triceo Jan 14, 2025

Choose a reason for hiding this comment

Christopher-Chianelli Jan 14, 2025

Choose a reason for hiding this comment

triceo Jan 14, 2025 • edited Loading

Choose a reason for hiding this comment

Christopher-Chianelli Jan 14, 2025

Choose a reason for hiding this comment

triceo left a comment •

edited

Loading

triceo Jan 11, 2025 •

edited

Loading

triceo Jan 11, 2025 •

edited

Loading

triceo Jan 11, 2025 •

edited

Loading

triceo Jan 11, 2025 •

edited

Loading

triceo Jan 11, 2025 •

edited

Loading

triceo Jan 14, 2025 •

edited

Loading