JuliaManifolds / ManifoldsBase.jl

Basic interface for manifolds in Julia
https://juliamanifolds.github.io/ManifoldsBase.jl/
MIT License
87 stars 8 forks source link

Introduce AbstractApproximationMethod #177

Closed kellertuer closed 9 months ago

kellertuer commented 9 months ago

This PR tackles a discussion from https://github.com/JuliaManifolds/Manifolds.jl/issues/661 and introduces the AbstractEstimationMethod already here in ManifoldsBase.jl

There are two changes I modelled here as well while rethinking this concept a bit

The new method default_estimation_method falls back to the existing default methods like default_retraction_method when providing retract or retract! as a function.

If this design is fine (feedback wanted :) ), I can implement the necessary tests (though that is a bit of work I think).

With this available, the mean/median in in manifolds might become nicer, but especially in Manopt one can specify the mean method more fine granular, eg. in Nelder-Mead.

ToDo

codecov[bot] commented 9 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (f5d3189) 99.96% compared to head (8f11a9a) 99.96%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #177 +/- ## ======================================= Coverage 99.96% 99.96% ======================================= Files 26 27 +1 Lines 3068 3093 +25 ======================================= + Hits 3067 3092 +25 Misses 1 1 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

kellertuer commented 9 months ago

I had a bit of time so I did the testing real quick. I think I like the way this turned out, so I will switch the tags :)

mateuszbaran commented 9 months ago

Thanks for starting this!

  • The ExtrinsicEstimation now stores another method internally which method to use in the embedding. We could maybe even rename it to EmbeddedEstimation

That's fine, though I have never had the need for this much generality.

  • the retraction, inverse retraction and vector transport method types are now subtypes of this type, since all of them are estimates (of eco, log and parallel transport, resp.)

Well, I wouldn't call that estimation. Estimation is for inferring parameters of a statistical model. Retractions, inverse retractions and vector transports may be called approximation methods, but even that is not entirely appropriate for ExponentialRetraction for example.

  • there is a new default_estimation_method with 3 parameters: a manifold, a function and a type. The function is to distinguish (possibly) different methods for every function – the T similar to the three methods above to distinguish different point representations.

This is also fine, except I wouldn't convert retractions, inverse retractions and vector transports to this scheme because they are not for estimation.

kellertuer commented 9 months ago

Well, a retraction is an estimation method for the exponential map. I am also not sure this is something I will need somewhen soon, but it fits the scheme quite nicely I think.

And yes, ExponentialRetraction is the one case I thought of a well when I was thinking about how to best write that the mean on Euclidean is exact – where wet would also need an estimation type that specifies that it is exact.

mateuszbaran commented 9 months ago

Well, a retraction is an estimation method for the exponential map. I am also not sure this is something I will need somewhen soon, but it fits the scheme quite nicely I think.

Could you give a reference for calling retraction an estimation method?

kellertuer commented 9 months ago

Absil, Mahony, Sepulchre (2008) write (p. 56, l. 14): “The exponential mapping is, in the geometric sense, the most natural retraction to use” and later in that paragraph (after tiling about the exp to be expensive) “These drawbacks are an invitation to consider alternatives in the form of approximations to the exponential that are computationally cheap without jeopardising the convergence properties of the optimization schemes”

Sure, Boumal (2023) works much more with retractions and the exponential map is just a special retraction (third order).

I would still consider exp/log/parallel transport as the “most natural” way to do plus / minus / move tangents and then you can (cf. check_retraction in Manopt/Matlab here) check whether a retraction is a retraction, when it is a first-order approximation of the exponential map.

mateuszbaran commented 9 months ago

These references just support my point of view that they are approximation methods, not estimation methods.

kellertuer commented 9 months ago

Ah, I misread your post then and for me estimation and approximation are not so far from each other. But sure, if you prefer to keep them separate I can undo that common super type for them all. would just reduce the code lines a bit again.

kellertuer commented 9 months ago

To be precise what I had in mind is (google, dictionary Oxford)

to estimate – an approximate calculation or of the value, number, quantity, or extent of something. "at a rough estimate, staff are recycling a quarter of paper used"

And hence considered approximation a more precise noun where estimate is a super type (since it might also include not-so-mathematical ways of estimation).

edit: And design wise – this would join all our 4 method of estimation/approximation in one super type – generic ones / retractions / inverse retractions / vector transports. I do not directly see a use case for this but it is maybe nice to have.

mateuszbaran commented 9 months ago

For technical terms it's better to use technical definitions, not general dictionaries. For example Wikipedia has a definition of estimation that I agree with: https://en.wikipedia.org/wiki/Estimator#Definition . On the other hand, approximation is used when describing a procedure related to a deterministic function (like retraction/inverse retraction/vector transport). One could argue that different estimation methods may want to approximate the MVU estimator, so just renaming AbstractEstimationMethod to AbstractApproximationMethod would be fine with me. Or we could keep both abstract types separate.

kellertuer commented 9 months ago

Hm, yeah, one could argue that if the mean is the estimator, then we are approximating the estimate ;)

I would be fine with AbstractApproximationMethod with exactly the interpretation above, we could even use ExactEstimator for the mean on Euclidean then.

mateuszbaran commented 9 months ago

OK, though I wouldn't use the name ExactEstimator there.

kellertuer commented 9 months ago

Yes it sounds as strange as the ExponentialRetraction sounds to me, but we need a way to say that the mean-estimator on Euclidean is not approximated further (like the mean manifold estimator is approximated by a gradient descent for example)

kellertuer commented 9 months ago

Two further points: Should the Estimation structs/methods then be called Approximation? I think that would just fit.

For the exact estimator (in the statistical sense) without approximation it would still be nice to have a name.

mateuszbaran commented 9 months ago

, but we need a way to say that the mean-estimator on Euclidean is not approximated further

I see but maybe it would be better to call it EfficientEstimator: https://en.wikipedia.org/wiki/Efficiency_(statistics) ?

kellertuer commented 9 months ago

I still think it does not hit that completely, but I am also not a statistician.

kellertuer commented 9 months ago

I thought a bit about the concrete types, I think to keep Manifolds non-breaking we should maybe keep their names as is.

Since I do not have a better idea, maybe we can also introduce your name as the exact/efficient one and document it accordingly.

mateuszbaran commented 9 months ago

I thought a bit about the concrete types, I think to keep Manifolds non-breaking we should maybe keep their names as is.

We can keep the old name as a deprecated alias.

kellertuer commented 9 months ago

We can keep the old name as a deprecated alias.

There is only two changes, the abstract type changed its name and the extrinsic one now has a field for the method to extrinsically use, since that is nicer to work with – we can use GeodesicIngterpolation as a default since that was the implicit one used for now. Then also that is nonbreaking.

I am also fine with the Estimation names, since they are still closer to what we use the things currently, namely in statistics. But we can also switch to Approximation names – and do the consts as you said – in Manifolds, I am also ok with that.

kellertuer commented 9 months ago

There is two things to check here

Concerning the naming - the new scheme here is now slightly different from the one in Manifolds.jl – but we can just introduce either const's or deprecation warning, depending on what fits better, so the adaption therein can be made non-breaking.

kellertuer commented 9 months ago

For now I did not make the function a trait function. Should it be? That would be the last point to add here before we merge an register.

mateuszbaran commented 9 months ago

I don't really have a strong preference about it being a trait function. I can see arguments both for and against.

kellertuer commented 9 months ago

Since that is easier – we could go for “we make it a trait function when necessary.” and leave it as is for now.

I will check test coverage here, still.