Enhances solver with plateau detection, adds missing Python options #80
Conversation
merschformann
requested review from
dirkschumacher,
nilsbeck and
sebastian-quintero
merschformann
changed the title
solve_terminate.go
Outdated
| } | ||
|
|
||
| // IsStop returns true if the solver should stop due to a detected plateau. | ||
| func (t *plateauTracker) IsStop(iterations int, elapsed time.Duration) bool { |
There was a problem hiding this comment.
Maybe rename the function to ShouldStop? Also because we have the concept of a Stop in nextroute. But what the function does is to decide if the solver should stop.
There was a problem hiding this comment.
Yeah, good call. 👍
There was a problem hiding this comment.
I renamed it to ShouldTerminate to stay away from the Stop double meaning. 😊
solve_solver_parallel.go
Outdated
| @@ -351,6 +363,12 @@ func (s *parallelSolverImpl) Solve( | |||
| if totalIterations.Add(1) >= int64(interpretedParallelSolveOptions.Iterations) { | |||
| cancel() | |||
| } | |||
| if s.plateauTracker.IsStop( | |||
There was a problem hiding this comment.
This gets called very often. Maybe we can add a benchmark test that ensures that IsStop never allocates and does totalIterations.Load() cost a lot? Because we can reuse the result of totalIterations.Add(1) then we don't have to lock again (if Load() locks).
There was a problem hiding this comment.
I was thinking the same thing. Now that you mention it, I think avoiding the double atomic call is better. I doubt that it makes concurrency issues worse (which I was worried about before for some reason), but may even make it better. For sure, it will avoid extra cost (however small they may be).
There was a problem hiding this comment.
Just to clarify, you are only worried about a potential performance loss for when a user activates plateau tracking, right? (it's off by default - so shouldn't cause a regression)
There was a problem hiding this comment.
Yes, I think the code in the hot path of the solver needs to be as fast as possible and ideally should not allocate (which it does not, unless some interface related hidden allocations are triggered). With the change of how the iteration count is retrieved the tracker will have minimal overhead if it is disabled.
dirkschumacher
deleted the
merschformann/adding-early-termination-criteria
branch
Description
Introduced plateau detection for solvers, allowing termination based on lack of improvement over time or iterations.
Changes
PlateauOptionsstruct for plateau detection configuration.github.com/nextmv-io/sdkdependency to include an upstream golden file test fix.Preview
This PR adds the option to define stopping criteria when the solve process (value progression / improvement) starts to stagnate / converge.
This can be done via the typical
nextroute.ParallelSolveOptionsstruct or simply via the options (applicable for most cases):Nextmv Hybrid Optimization Platform Usage: ... -solve.plateau.absolutethreshold float absolute threshold for significant improvement (env SOLVE_PLATEAU_ABSOLUTE_THRESHOLD) (default -1) -solve.plateau.duration duration maximum duration without (significant) improvement (env SOLVE_PLATEAU_DURATION) -solve.plateau.iterations int maximum number of iterations without (significant) improvement (env SOLVE_PLATEAU_ITERATIONS) -solve.plateau.relativethreshold float relative threshold for significant improvement (env SOLVE_PLATEAU_RELATIVE_THRESHOLD) -solve.rundeterministically