-
Notifications
You must be signed in to change notification settings - Fork 99
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: add new adaptive termination type #1313
base: main
Are you sure you want to change the base?
feat: add new adaptive termination type #1313
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
core/src/main/java/ai/timefold/solver/core/impl/solver/termination/AdaptiveTermination.java
Outdated
Show resolved
Hide resolved
core/src/main/java/ai/timefold/solver/core/impl/solver/termination/AdaptiveTermination.java
Outdated
Show resolved
Hide resolved
core/src/main/java/ai/timefold/solver/core/impl/solver/termination/AdaptiveTermination.java
Outdated
Show resolved
Hide resolved
|
||
@Override | ||
public double calculateSolverTimeGradient(SolverScope<Solution_> solverScope) { | ||
return -1.0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if there's a way to estimate this based on how close we are to the minimum necessary improvement.
core/src/main/java/ai/timefold/solver/core/impl/solver/termination/AdaptiveTermination.java
Outdated
Show resolved
Hide resolved
core/src/main/java/ai/timefold/solver/core/impl/solver/termination/AdaptiveTermination.java
Outdated
Show resolved
Hide resolved
core/src/main/java/ai/timefold/solver/core/impl/solver/termination/AdaptiveTermination.java
Outdated
Show resolved
Hide resolved
core/src/main/java/ai/timefold/solver/core/impl/solver/termination/AdaptiveTermination.java
Outdated
Show resolved
Hide resolved
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.
- 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
81980ad
to
4cbb5aa
Compare
core/src/main/java/ai/timefold/solver/core/impl/solver/termination/AdaptiveScoreRingBuffer.java
Outdated
Show resolved
Hide resolved
Note: it is impossible to test a startup failure due to a runtime property in Quarkus; see quarkusio/quarkus#45669 for details.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good!
Still need to add documentation. Since this termination is non-trivial, we'll need an illustration (akin to what I provided above) together with an explanation of the concepts, and broad strokes of the algorithm.
...va/ai/timefold/solver/core/config/solver/termination/DiminishedReturnsTerminationConfig.java
Show resolved
Hide resolved
...n/java/ai/timefold/solver/core/impl/solver/termination/DiminishedReturnsScoreRingBuffer.java
Show resolved
Hide resolved
...n/java/ai/timefold/solver/core/impl/solver/termination/DiminishedReturnsScoreRingBuffer.java
Show resolved
Hide resolved
...n/java/ai/timefold/solver/core/impl/solver/termination/DiminishedReturnsScoreRingBuffer.java
Show resolved
Hide resolved
...n/java/ai/timefold/solver/core/impl/solver/termination/DiminishedReturnsScoreRingBuffer.java
Show resolved
Hide resolved
core/src/main/java/ai/timefold/solver/core/impl/solver/termination/TerminationFactory.java
Outdated
Show resolved
Hide resolved
@@ -48,6 +48,9 @@ private static String getRequiredProperty(String name) { | |||
.overrideConfigKey("quarkus.timefold.solver.termination.best-score-limit", "0") | |||
.overrideConfigKey("quarkus.timefold.solver.move-thread-count", "4") | |||
.overrideConfigKey("quarkus.timefold.solver-manager.parallel-solver-count", "1") | |||
.overrideConfigKey("quarkus.timefold.solver.termination.diminished-returns.enabled", "false") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Arguably this is not necessary, if either of the other two properties is present?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The other two properties are optional, which make it hard to use the defaults unless there a third property.
Additionally, if you want to use diminishing returns by default, but have the option to disable it, you need to somehow delete the defined properties if there is no enabled property.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see the reason for the property existing - I'm just arguing you don't need to specify it, if you specify any of the others.
Does it get confusing as you switch from one to the other? Probably. But how often does that actually happen?
...ment/src/test/java/ai/timefold/solver/quarkus/TimefoldProcessorXMLDiminishedReturnsTest.java
Outdated
Show resolved
Hide resolved
.../runtime/src/main/java/ai/timefold/solver/quarkus/config/DiminishedReturnsRuntimeConfig.java
Outdated
Show resolved
Hide resolved
.../runtime/src/main/java/ai/timefold/solver/quarkus/config/DiminishedReturnsRuntimeConfig.java
Outdated
Show resolved
Hide resolved
Quality Gate passedIssues Measures |
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.
Fixes #1311