-
Notifications
You must be signed in to change notification settings - Fork 9
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Introduce AbstractApproximationMethod #177
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #177 +/- ##
=======================================
Coverage 99.96% 99.96%
=======================================
Files 26 27 +1
Lines 3068 3093 +25
=======================================
+ Hits 3067 3092 +25
Misses 1 1 ☔ View full report in Codecov by Sentry. |
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 :) |
# Conflicts: # NEWS.md
Thanks for starting this!
That's fine, though I have never had the need for this much generality.
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
This is also fine, except I wouldn't convert retractions, inverse retractions and vector transports to this scheme because they are not for estimation. |
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, |
Could you give a reference for calling retraction an estimation method? |
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. |
These references just support my point of view that they are approximation methods, not estimation methods. |
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. |
To be precise what I had in mind is (google, dictionary Oxford)
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. |
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 |
Hm, yeah, one could argue that if the mean is the estimator, then we are approximating the estimate ;) I would be fine with |
OK, though I wouldn't use the name |
Yes it sounds as strange as the |
Two further points: Should the For the exact estimator (in the statistical sense) without approximation it would still be nice to have a name. |
I see but maybe it would be better to call it |
I still think it does not hit that completely, but I am also not a statistician. |
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. |
We can keep the old name as a deprecated alias. |
Co-authored-by: Mateusz Baran <[email protected]>
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. |
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. |
# Conflicts: # NEWS.md
# Conflicts: # NEWS.md # src/vector_transport.jl
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. |
I don't really have a strong preference about it being a trait function. I can see arguments both for and against. |
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. |
This PR tackles a discussion from JuliaManifolds/Manifolds.jl#661 and introduces the
AbstractEstimationMethod
already here in ManifoldsBase.jlThere are two changes I modelled here as well while rethinking this concept a bit
ExtrinsicEstimation
now stores another method internally which method to use in the embedding. We could maybe even rename it to EmbeddedEstimationdefault_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.The new method
default_estimation_method
falls back to the existing default methods likedefault_retraction_method
when providingretract
orretract!
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
default_estifmation_method(M[, f, T)