Repository navigation
Stopping criterion updates - #31
Merged
Merged
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #31 +/- ##
==========================================
+ Coverage 99.57% 99.60% +0.03%
==========================================
Files 6 6
Lines 234 255 +21
==========================================
+ Hits 233 254 +21
Misses 1 1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
lkdvos
force-pushed
the
ld-stopping-criterion-fixes
branch
from
July 30, 2026 17:17
8d29402 to
0ac592d
Compare
Collaborator
also heavily depends on which tolerance. If you stop because the step got too small (in Manopt The static vs dynamic thing seems to be very much adapted from |
kellertuer
reviewed
Jul 31, 2026
kellertuer
left a comment
kellertuer
left a comment
Collaborator
There was a problem hiding this comment.
Overall a very neat approach; I personally needed (back then) a bit of time to wrap my head around the static “could it indicate if it says to stop?” and “does it actually?” parts, but they are very well mapped here. So I just have a few short questions about the details :)
Thanks for continuing to work on this.
lkdvos
force-pushed
the
ld-stopping-criterion-fixes
branch
2 times, most recently
from
July 31, 2026 13:19
7320c12 to
62b0128
Compare
Four related defects in `StoppingCriterion` handling, all surfaced by trying to express "converged, or did we run out of iterations?" for a `tol | maxiter` criterion: - The two-argument `indicates_convergence` had its condition inverted, claiming convergence exactly when `get_reason` returned `nothing`, i.e. when the criterion had *not* indicated to stop. Its own docstring describes the opposite. Untested, since the only test used `StopAfterIteration`, whose `indicates_convergence` is `false` either way. - There was no way to ask a group which of its children triggered. `indicates_convergence(::StopWhenAny) = all(indicates_convergence, criteria)` is unconditionally `false` as soon as a non-converging fallback such as `StopAfterIteration` is part of the group, so the canonical combination of a convergence criterion with an iteration cap could never report convergence. Adds state-aware group methods that consult only the children that actually indicated to stop. - `is_finished!` for `StopWhenAll`/`StopWhenAny` used `all`/`any`, which short-circuit. Children past the deciding one were skipped, starving stateful criteria of the current iterate and leaving their `at_iteration` unset even when they did indicate to stop -- which the previous item now depends on. Every child is updated exactly once per iteration. - Group `get_reason` rendered non-triggering children as the literal text "nothing", since `join` prints `nothing` rather than skipping it. The `stopping_criterion.md` walkthrough defined the two-argument `indicates_convergence` directly and unconditionally `true`, which bypasses the state check entirely; it now defines only the single-argument variant and explains why that is enough.
The machine-readable question "did this criterion indicate to stop?" was answered by `!isnothing(get_reason(...))`, deriving control flow from a message meant for human consumption. That coupling is what produced the inverted condition and the "nothing"-in-the-reason bugs fixed in 5795dd5, and it left `get_reason` unable to change shape without breaking logic. Introduce `indicated_to_stop`, which answers that question directly and defaults to reading the `at_iteration` property every state in the package already carried. `indicates_convergence(criterion, criterion_state)` is now derived from it, `at_iteration` is documented as part of the `StoppingCriterionState` contract, and `indicated_to_stop` is the override for a state that records its status differently. Route the concrete criteria through it as well, which fixes a disagreement on `StopAfterIteration`: `get_reason` was gated on `at_iteration >= max_iterations` while `Base.summary` was gated on `at_iteration >= 0`, so for a recorded stop below the cap there was no message but a summary claiming the criterion was reached. Finally, give `get_reason` and the single-argument `indicates_convergence` conservative defaults of `nothing` and `false`. Both were declaration-only, so a criterion that implemented neither hit a `MethodError` from the derived path it never opted into. That also makes the explicit `indicates_convergence(::StopAfterIteration) = false` and `(::StopAfter) = false` redundant, so drop them.
`join` over an empty iterator yields `""`, so a `StopWhenAll` or
`StopWhenAny` that indicated to stop while none of its active children had
anything to say returned the empty string rather than `nothing`. Since
`!isnothing("")` holds, that reads as a genuine message to any consumer, and
before the preceding commit it also read as "did stop".
Select the children to report on with `indicated_to_stop` rather than by
filtering out `nothing` afterwards, so the message is built from the ones
that actually triggered instead of from whoever happened to produce text,
and gate the group's `Base.summary` on the same predicate.
Document why the quantifiers invert between the two meta criteria: `All`
only triggers once every child does, so one convergence certificate settles
it, whereas `Any` can trigger on any single child and therefore needs all of
them to be certificates.
Two ways it did not. For `StopWhenAll` and `StopWhenAny` it carried `(k == 0) && (stopping_criterion_states.at_iteration = -1)`, contradicting the documented promise that it "merely inspects the current status without mutation". Inspecting a group at iteration 0 therefore erased its record of having stopped, and with it whatever `get_reason` and `indicates_convergence` would have reported. For `StopAfter` it reported on `stop_after_state.time`, the elapsed time as recorded by the last `is_finished!`, so it could answer "not finished" for a run that had been over its threshold for a while. Nothing forced that: the start instant is already in the state, so the elapsed time is re-derivable at query time without writing anything. Only (re)starting the timer genuinely needs to mutate, and that stays with `is_finished!`. Guard on an uninitialised `start`, since subtracting zero would otherwise make the elapsed time the whole `time_ns()` reading and trip any threshold. Reading the clock makes the query non-idempotent, which is inherent to asking "would this stop right now?" of a time-based criterion. The two test fixtures that faked a stop by writing `time` now move `start` into the past instead, which is what elapsed time means to a clock-reading implementation. Short-circuiting `all`/`any` is kept throughout: with no side effects to perform, a child that is never asked loses nothing, which is why only `is_finished!` needed the non-short-circuiting treatment in 5795dd5.
Nothing in the library consumed `get_reason` or `indicates_convergence`, and a caller had to reach through `algorithm.stopping_criterion` and `state.stopping_criterion_state` by hand to reach them at all. Add `get_active_stopping_criteria`, which reports the `(criterion, criterion_state)` pairs that actually became active. `indicates_convergence` collapses the outcome to a boolean, which is too coarse for the case that motivates asking: a budget running out and a step size collapsing both report `false` while calling for quite different responses. It recurses through `StopWhenAll` and `StopWhenAny` and does not report the groups themselves, since `&` and `|` only flatten against their own type and a mixed `(c1 | c2) & c3` genuinely nests. The default treats a criterion as a leaf, so a future criterion that combines others has to implement this to be recursed into. Add `StopReasonAction`, a `LoggingAction` for the `:Stop` context that prints the reason, the analogue of Manopt.jl's `DebugStoppingCriterion`. It prints nothing while no criterion has indicated to stop, so registering it on another context is harmless rather than misleading. Export the stopping criterion state types. A downstream criterion whose state needs nothing beyond a recorded iteration is expected to reuse `DefaultStoppingCriterionState` rather than define its own, and the test suite had to reach for it with `using AlgorithmsInterface: ...` to do exactly that. All three are already public in the generated documentation.
The stopping criterion page claimed `get_reason` and `indicates_convergence` "are required to interact with the logging system", which was false until `StopReasonAction` existed. Replace that with the distinction the interface actually rests on: `indicated_to_stop` answers whether a criterion triggered and is what the generic reporting is built on, while `get_reason` is for human consumption. Add a section on querying the verdict after a run. It runs the same composed criterion twice, under a generous and a tight iteration cap, so the rendered output shows convergence and budget exhaustion side by side rather than asserting the difference in prose. Also fix the walkthrough's `get_reason`, which re-checked `st.delta < c.tol` on top of the recorded status. That teaches a pattern where a later state update retroactively erases the message, so gate it on the record alone. Drop a stale TODO in the Newton example asking about stopping criterion initialisation, which the interface now answers, and gitignore the `test/test_*.log` files ParallelTestRunner leaves behind.
lkdvos
force-pushed
the
ld-stopping-criterion-fixes
branch
from
July 31, 2026 13:20
62b0128 to
f01d291
Compare
lkdvos
enabled auto-merge (squash)
July 31, 2026 13:20
kellertuer
approved these changes
Jul 31, 2026
kellertuer
left a comment
kellertuer
left a comment
Collaborator
There was a problem hiding this comment.
Nice! Thanks for the constructive discussion even on such a hard topic as naming.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR aims to port over some of the newer Manopt.jl things that didn't fully make it into the package, and improve some of the stopping criterion ergonomics.
The first thing is the ability to ask the question "has this algorithm converged", based on the state of the stopping criteria. The big thing here is that reaching iteration count does not mean convergence, but reaching tolerance criterion does, in other words this is a static property of the specific stopping criterion.
For this, I added
indicates_convergence(criterion)as the static trait, andindicates_convergence(criterion, state)as the actual runtime check.A second thing is that in some of the fixes (see list below), it become apparent that we were using
!isnothing(get_reason(...))as a check for "has this criterion fired". Since there were some small mistakes in there, I factored that out into a more human-readable function that bundles this instead:indicated_to_stopis the machine-readable variant ofget_reason.Then, I actually tied together the stopping and logging system by a
StopReasonAction, which basically just writes the reason for stopping if called.A list of small fixes as well:
anyandallare short-circuiting, so for stateful criteria this could makeis_finished!end up not being called once per iteration on the statesindicates_convergencehadisnothing(get_reason(...)), which indicates non-convergence"nothing"ended up being printed in variousget_reasoncalls for stopping groups.Kept for future PRs: