Add atol to ConstrainedManifoldObjective

[JoshuaLampert]

As discussed in #540, this adds a field atol to ConstrainedManifoldObjective, which is used in is_feasible to test if the constraints are satisfied. Before, the code tested for floating point equality, which is usually very prone. I used 1e-13 as default atol because this is what is used in DebugFeasibility:

Manopt.jl/src/plans/debug.jl

Line 461 in c10dec8

function DebugFeasibility(format = ["feasible: ", :Feasible]; io::IO = stdout, atol = 1.0e-13)

[kellertuer]

Here is a few comments.

Currently also the tests fail, in short, because one function accessing atol is not careful about that the objective (similar to a state= might be wrapped . and the wrapped variant does not have the field atol of course.
I can try to take a look, but first I have some other duties to follow up on

Thanks for this contribution; numerically that is quite useful to have.

[JoshuaLampert]

Currently also the tests fail, in short, because one function accessing atol is not careful about that the objective (similar to a state= might be wrapped . and the wrapped variant does not have the field atol of course.

Ah, I see: is_feasible is also called for a ReturnManifoldObjective and not only for ConstrainedManifoldObjective (this is what the docstring states; so probably this should be more generic in the docstring). In the case of a ReturnManifoldObjective we would probably access atol as cmo.objective.atol, right? In this case, I think one solution would be to add something like get_atol(cmo::ConstrainedManifoldObjective) = cmo.atol and get_atol(cmo::ReturnManifoldObjective{E, O2, O1}) where {E, O2, O1 <: ConstrainedManifoldObjective} = cmo.objective.atol?

[kellertuer]

Sure the doc string could be updated. The “lazy” solution is to directly document that is works on arbitrary decorators, I.e. for https://manoptjl.org/stable/plans/objective/#Manopt.AbstractDecoratedManifoldObjective s,
and for now I did not write that many such accessors, but mainly use the “unwrap” approach: https://manoptjl.org/stable/plans/problem/#Manopt.get_objective

[JoshuaLampert]

Ah, get_objective is much nicer. Thanks for the hint! Give me a second to adapt that.

[JoshuaLampert]

Ok, I unwrap the cmo now using get_objective and adapted the docstring. Is this what you had in mind? Hopefully, this fixes CI.

[kellertuer]

Nearly, I would have added a second line of the function signature, the first one with emo::Constrained... the second one with the decorator. That makes it implicitly clearer that inside the decorator the constrained one is expected

[JoshuaLampert]

Nearly, I would have added a second line of the function signature, the first one with emo::Constrained... the second one with the decorator. That makes it implicitly clearer that inside the decorator the constrained one is expected

Makes sense. Like 61630cd?

[kellertuer]

We are getting close. One small thing that is now formally breaking (but I think no one used it probably) is that removing the keyword atol is breaking.

We could maybe keep it (but no longer document it), default to nan, and if it is not nan we add a warning stating that that is deprecated and for now ignored.
The word deprecated would be important here, because on a next breaking release I would remove it then.
Also that can be done in one line as

isnan(atol) && (@warn "Providing atol= directly to DebugFeasibility is deprecated. Use the keyword for the ConstrainedObjective instead. The value provided here ($(atol)) is ignored")

That way it does not affect code cov, we could document that as well and then it is nice and nonbreaking.
(Yes I hate breaking code in nonbreaking releases, one of my larges waste of time in other peoples code, so I personally approach that super carefully)

[JoshuaLampert]

Ok, I added that in the last commit.

[kellertuer]

We then also do no longer have to store atol ;) For the rest, I would just have to check the changelog, the rest looks good

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.00%. Comparing base (7571c84) to head (7212758).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff            @@
##            master      #545   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           90        90           
  Lines         9967      9970    +3     
=========================================
+ Hits          9967      9970    +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.