Skip to content
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

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

Christopher-Chianelli
Copy link
Contributor

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

Copy link
Contributor

@triceo triceo left a 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?


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

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.

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
Note: it is impossible to test a startup failure due to a runtime
property in Quarkus; see quarkusio/quarkus#45669
for details.
Copy link
Contributor

@triceo triceo left a 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.

@@ -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")
Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor

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?

Copy link

sonarqubecloud bot commented Jan 17, 2025

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Adaptive termination
2 participants