Add a numerical checks for retractions and their inverses

[kellertuer]

This aims to resolve JuliaManifolds/Manifolds.jl#599,
but my straight forward idea does not yet yield the right results I fear.

Since the check reuses a lot of methods that are already defined in Manopt.jl, I decided to move all of that up here to ManifoldsBase.
That way, Manopt can later import them from here, and the check is already available when just loading ManifoldsBase

Roadmap / ToDo

• Maybe reduce all the redefinition warnings in the test by moving the test definitions into a submodule?
• documentation
• figure out why the test currently fails even for the exponential map, it seems my idea is not yet fully correct.
• move throw_error to the new approach with error= and symbols (to warn or info instead as well)

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.96%. Comparing base (830c5a1) to head (25cc6c9).

Additional details and impacted files

@@           Coverage Diff           @@
##           master     #187   +/-   ##
=======================================
  Coverage   99.96%   99.96%           
=======================================
  Files          27       30    +3     
  Lines        3156     3241   +85     
=======================================
+ Hits         3155     3240   +85     
  Misses          1        1           

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

[kellertuer]

Instead of multiple imports we now have a small ManifoldsBaseTestUtils module that is loaded. There are one or two small things missing, but in general we now have nearly no warning left.

edit: Unified all definitions that are used more than once in that package now, it'ß path is added to the local path if not yet done before, then the module can be loaded just with using; I think this is a nice way to “reuse” things like the NonManifold.

[kellertuer]

The last 3 points should not be that much of work, so I would like to have first a bit of feedback on this approach (both the slight rework of the tests and the new check), since the two remaining functions are nearly copy paste of the existing check.

• for the inverse retraction using log and a norm in the tangent space
• for vector_transport_to using parallel transport and a norm in the tangent space.

though I have not yet seen the order of error for vector transports in books.

[kellertuer]

Let's keep this a bit simpler and just do one check in this PR. I have to think about the other checks in theory first.

[kellertuer]

I just still found out a good idea to check vector transports.

[mateuszbaran]

Instead of multiple imports we now have a small ManifoldsBaseTestUtils module that is loaded

edit: Unified all definitions that are used more than once in that package now, it'ß path is added to the local path if not yet done before, then the module can be loaded just with using; I think this is a nice way to “reuse” things like the NonManifold.

That is a good idea 👍 .

[mateuszbaran]

I've tried your test with check_retraction(Stiefel(5, 3), PolarRetraction(), plot=true) and maybe it should trim horizontal range when the vertical one reaches +- machine precision?

[mateuszbaran]

At least trim for best slope fitting.

[kellertuer]

Or to be precise you would do (I guessed the wrong keyword in the last post)

check_retraction(Stiefel(5, 3), PolarRetraction(), plot=true, limits=(-5,0))

and you are fine. I would usually expect a method to be fine until 1e-8, so I feel this is a good standard;
choosing that automatically might also lead to a very short range (maybe even (-1,0) or so) such that I would say – yes the slope is fine but the method suffers a lot from underflow. That would be something I would still prefer to report to a user?

[mateuszbaran]

I see, maybe indeed finding nicer default limit would be too much work. I just thought it kind of not nice that it returns false for one of our retractions that is accurate enough in practice. Let's keep it as is then.

[mateuszbaran]

I've added some types to make it easier to see what is supposed to be what, and fixed a few issues in docstrings.

[kellertuer]

Sure, I think we do not necessarily need to do reviews here for typos (and type annotations). Thanks :)

Also I really was a bit lazy to move that to an extension, but I think I nearly did that by now. Will also check that it loads a bit faster now

[kellertuer]

I just thought it kind of not nice that it returns false for one of our retractions that is accurate enough in practice.

Yes deciding on a good tradeoff there is maybe a bit hard, the -8 comes from the idea that about the sqrt of machine precision is what I would prefer to still have before too much errors kick in (also underflow ones).

[kellertuer]

In principle the last commit does the rework to put Statistics in a weak dependency (It will be a direct on in Manifolds then and is one in Manopt already).

In practice there is some strange error still that I do not understand that I might try to fix somewhen, but it took me nearly an hour already to get to this (hence my phrasing above: I am too lazy) – getting weak dependencies to work is still involving a bit too much magic for me in the first set up.

[kellertuer]

Hm, Julia 1.6 now still fails using the (previously working) Plots extension. Hmpf.
It would rally be nice if extensions would not involve magic powers and guesswork (and only work on full moon nights).

[kellertuer]

I now spent nearly 2 hours and the one thing I can not get (back) to work is the Plots extension on 1.6. It worked before I added the Statistics extension. Since then it randomly chooses on my machine to work and not to work.

[kellertuer]

Since we run Aqua checks, can we remove the manual ambiguity check we have in the beginning of the runtests? To me it seems Aqua is more fine granular anyways.

Besides that we lose one line of code coverage (99.97 -> 99.94) since we have a second static line by now (for the registration of hints).

[mateuszbaran]

Thanks for changing it to an extension! I've excluded that one line from coverage.

[kellertuer]

Thanks for changing it to an extension! I've excluded that one line from coverage.

Was a bit of work, but you were right, it is worth it spending that.
Thanks for adding that and sad that the other missing line is some quirk in a docstring. but at least that way we stay at just missing one line, which I think is still super impressive.

[mateuszbaran]

Since we run Aqua checks, can we remove the manual ambiguity check we have in the beginning of the runtests? To me it seems Aqua is more fine granular anyways.

If we had no exclusions in Aqua, those ambiguity checks would indeed be unnecessary. runtests ambiguity checks currently check if there is not too much ambiguity for those 3 excluded functions.