`hasconverged(::StopWhenAll)` reports `false` for nested criteria

[lkdvos]

Bumped into this when looking a bit more at AlgorithmsInterface.jl:

The "are all criteria active" hasconverged uses a length (non-recursive) and compares it to get_active_stopping_criteria (recursive), so this can mismatch when combining & and |.
Current tests only have s1 & s2 or s1 | s2, not s1 & s2 | s3.

MWE:

s1 = StopWhenGradientNormLess(1.0e-4)   # indicates_convergence == true
s2 = StopAfterIteration(10)             # indicates_convergence == false
cap = StopAfterIteration(20)
sc = (s1 | s2) & cap                    # StopWhenAll(StopWhenAny(s1, s2), cap)

# every leaf active: the group has genuinely stopped, and s1 is a convergence certificate
s1.at_iteration = 1
s2.at_iteration = 11
cap.at_iteration = 21
has_converged(sc)   # false -- expected true
#  active leaves = [s1, s2, cap] -> 3, length(sc.criteria) -> 2, gate fails

# now deactivate s2, which is the criterion that cannot certify anything
s2.at_iteration = -1
has_converged(sc)   # true
#  active leaves = [s1, cap] -> 2 == 2, gate passes

---

Disclaimer: found with assistance of AI

[kellertuer]

Thanks for reporting. Might take quite a while until I find time looking into that since #630 is a super large (hopefully still nonbreaking) refactor for better performance.

[lkdvos]

No worries, I'm not actually using this but I figured it's nice to at least report these things while working on AlgorithmsInterface.jl :)

[kellertuer]

Yes, thanks for reporting it, sure; and sorry, that I currently do not find the time to help on AlgorithmsInteface.jl

[kellertuer]

I noticed another problem here, while trying to fix this – and that is internal inconsistency by uncarefull hacking ;)

There is the function is_active_stopping_criterion(sc) (which one could rename since it is internal to just is_active. This for now always just checks if at_iteration is nonnegative.
For any/all (ay/arr!) at_iteration is set when one of the “children” gets active:

Manopt.jl/src/commons/stopping_criteria.jl

Lines 22 to 29 in c44763c

	function (c::StopWhenAll)(p::AbstractManoptProblem, s::AbstractManoptSolverState, k::Int)
	(k == 0) && (c.at_iteration = -1) # reset on init
	if all(subC -> subC(p, s, k), c.criteria)
	c.at_iteration = k
	return true
	end
	return false
	end

So that when I tried to fix this, it was not enough to “hack” s1,s2,caps at_iteration but actually at iteration 21, scs at_iteration would increase as well.
Sure for and one can always also (a) check the min > 0 and take the max. But for now we double that information (and for any that is not so easy).

The fix (in #630) is hence how

Manopt.jl/src/commons/stopping_criteria.jl

Lines 57 to 60 in c44763c

	function has_converged(c::StopWhenAll)
	# (a) all are active (have converged) and and at least one of them indicates convergence
	return is_active_stopping_criterion(c) && any(has_converged(ci) for ci in c.criteria)
	end

and I made it consistent with the any case (where the check for has_converged is the second part is limited to the ones active).

Manopt.jl/src/commons/stopping_criteria.jl

Line 189 in c44763c

function has_converged(c::StopWhenAny)

and kept an adapted version of your example (that sets sc and s1 | s2s iteration as well).

Thanks again for reporting this :)