Rework high level interfaces

[kellertuer]

This is an approach to fix #381

🛣️ Roadmap

• move the “point is a number” case not on the highest level but when creating the replacement functions internally
• check what's left with TCG
• 📚check that the high-level interfaces in the new format are updated as well
• 🛠️ rename get/set_manopt_paramater to just get/set_parameter since it is no longer exported – and it makes the code a bit more readable
• 🛠️change the keyword staibilize=keyword in Quasi newton to a project!= keyword as in the other solvers.
• 🛠️unify naming and also call update_stopping_criterion! set_parameter, since the dispatch by the first type should be good enough.
• 🛠️ unify sub solver initialisation for solver states: accept both problem and state or accept a single function and the evaluation keyword; make p and X keywords
  • ALM
  • ARC
  • EPM
  • DoC
  • DCPPA
  • FW
  • PBM
  • CBM
  • TR
• Resolve Initialise states based on point type already and not requiring an actual point #400 here as well
  an idea could be to put the semantic explanation with the “easy to use” part (like AverageGradient()) and put the details like fields of the structs as well as a reference to the interface into the AverageGradientState docs.
  • gradient update rules
  • step sizes (the following factories should be created)
    • Armijo
    • Constant
    • Decreasing
    • Polyak
    • WolfePowellBinary
    • WolfePowell
    • Nonmonotone
    • AdaptiveWNGradient
  • stopping Criteria (will not be changed): the following have a manifold for defaults
    • StopWhenChangeLess
    • StopWhenGradientChangeLess
    • so the effort should not be that large, but I have to think about a naming that is easy.
    • ah maybe these are indeed to complicated, because we could not store them in the StopWhenAny as factories and “producing” them before is a bit too tough, so I will leave this as is for now.
• 📈 test coverage.
• rename inequality_constrains and equality_constrains keyword arguments to inequality_constraints and equality_constraints respectively. This ensures they are correctly passed to the ConstrainedManifoldObjective constructor

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.90%. Comparing base (c8564b8) to head (949457a).
Report is 1 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #392      +/-   ##
==========================================
+ Coverage   99.76%   99.90%   +0.13%     
==========================================
  Files          77       77              
  Lines        8256     8220      -36     
==========================================
- Hits         8237     8212      -25     
+ Misses         19        8      -11     

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

[kellertuer]

The ::Number ambiguities with types points are all removed.

TCG has some strange ambiguities, that I still have to look at. but all other high-level interfaces do not have ambiguities any longer.
The trick was to slightly redefine the methods that “generated” array-functions for number type data.

[kellertuer]

Small update: For now any single of the remaining 6 ambiguities, when trying to fix them, the number only goes further up. I am a bit los on this by now, since trying to reduce ambiguities only increases them (with my limited knowledge).

Some are also due to a deprecated signature so we can maybe leave these out in the end.

Maybe the reasonable solution is to make a breaking release that sets the Hessian to be mandatory, then the user would have to pass the approxHessian themselves.

[kellertuer]

The code with mandatory Hessian is so much cleaner. So this will be part of 0.5.0 – so nothing urgent and I will wait for a few other issues to be resolved and check whether we have other breaking changes we should consider / include then.

[kellertuer]

Oh tests might be a bit tricky, because we first have to bump version on ManoptExamples.jl it seems for the tests.

[kellertuer]

This is mainly waiting for JuliaManifolds/ManoptExamples.jl#16 and is (again) not urgent. So we can also think a bit about what else “to break” to improve.

[kellertuer]

Oh ManoptExamples only works with Julia 1.8, so I would either have to raise the Julia version here as well or run the tests with manoptExamples only on Julia 1.8 but not on 1.6

[kellertuer]

@mateuszbaran I do not seem able here to get RecursiveArrayTools (now a weak dependency here as well) to work fine with Statistics. Any ideas?

Today was rally not my day. My progress over several hours can bee strictly upper bounded by zero; and locally all works fine.

edit: and now I do not know why it works now. Well,...

[mateuszbaran]

Ah, I see you've figured it out already.

[kellertuer]

My main problem is, I neither know what I actually did wrong nor how I fixed it (in basically total confusion). But it works now and for this PR I am even only 2 stepsize rules aways from finishing all rework.

[kellertuer]

I think this is good to go (was a bit more work than I thought) – and turned out quite nice I think

@bvdmitri could you maybe check whether the new docs that provide the new interface are understandable? These are

• Step size factories at https://manoptjl.org/previews/PR392/plans/stepsize/#Manopt.ArmijoLinesearch (and the following ones); I left the math now in those, and moved the internal fields and technical descriptions to the corresponding XStepsize structs below, see https://manoptjl.org/previews/PR392/plans/stepsize/#Manopt.AdaptiveWNGradientStepsize and following
• Similarly the gradient update rules at https://manoptjl.org/previews/PR392/solvers/gradient_descent/#Manopt.AverageGradient and following have also been updated
• ...and also all conjugate gradient descent coefficients at https://manoptjl.org/previews/PR392/solvers/conjugate_gradient_descent/#cg-coeffs

If you have time, maybe give this new version a test run – if not, that is of course also fine

@mateuszbaran if you have time – the high level interfaces have now been reworked, so there are no warnings from Aqua any longer, especially no ambiguities, start points p are set to rand as default last positional argument whenever reasonable. Throughout I hope this is now consistent – and at least Aqua is happy :)

I would like to merge this maybe towards the end of the week, so if you have time to comment on these changes, that would be great – I think this still works as intended; I hope I have documented all breaking changes. Overall this breaking change is mainly unification of documentation and keyword names and improvements on Usability, since M is less often needed.

[bvdmitri]

I noticed some bad renders in the docs, but otherwise seems good to me :)
I also noticed that math doesn't render nicely very often in many docstrings, not sure why is this the case...

Here the English is broken IMO

How about

This function generates a `ManifoldDefaultsFactory`for `ConstantStepsize`.
The manifold specification is optional; if not provided, the manifold will be automatically determined from the optimization procedure. This affects all arguments and keyword arguments with defaults that depend on the manifold, unless explicitly specified here.

Also sometimes its inconsistent as here

(also in other places, e.g. Polyak and PolyakStepsize)

But just let me reiterate that this is a great amount of work you did and its quite impressive!

[kellertuer]

Ah, the info box is a function (with that type as a variable, exactly because I want to just fix the English in one place ;) and maybe I set that in copy. paste not correctly. Will check – also your other findings; thanks for checking!

edit: And thanks again for the nice feedback – that does still not excuse me being unconcentrated when doing all this copy-pase-work – I should probably read the docs one more time myself as well. The typos you found are already fixed :)

[kellertuer]

I might consider in a later PR to also introduce the factory at least to some stopping criteria (StopWhenChangeLess or StopWhenGradientChangeLess or such), but that would also require StopWhenAny and StopWhenAll to be able to handle those, so it is maybe a bit more work – should be doable nonbreaking still I think.

thanks again for all the ideas and contributions!