JuliaManifolds / ManifoldsBase.jl

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

Refine documentation #69

Closed kellertuer closed 3 years ago

kellertuer commented 3 years ago

This small PR extends the documentation especially of exp and retract. Maybe this is a good PR to also improve other docstrings, so feel free to propose further changes.

codecov[bot] commented 3 years ago

Codecov Report

Merging #69 (2f884b1) into master (a805e8e) will increase coverage by 0.00%. The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #69   +/-   ##
=======================================
  Coverage   99.70%   99.70%           
=======================================
  Files          11       14    +3     
  Lines        1348     1351    +3     
=======================================
+ Hits         1344     1347    +3     
  Misses          4        4           
Impacted Files Coverage Δ
src/DecoratorManifold.jl 100.00% <ø> (ø)
src/ManifoldsBase.jl 99.08% <ø> (-0.35%) :arrow_down:
src/vector_transport.jl 100.00% <ø> (ø)
src/exp_log_geo.jl 100.00% <100.00%> (ø)
src/projections.jl 100.00% <100.00%> (ø)
src/retractions.jl 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update a805e8e...2f884b1. Read the comment docs.

mateuszbaran commented 3 years ago

Since we already work on this, maybe it would be nice to also point out the difference between a geodesic and a curve that minimizes distance?

kellertuer commented 3 years ago

Good idea, that could best be done in geodesic and shortest_geodesic then.

kellertuer commented 3 years ago

I added more details on the geodesic. Since we are at it, we could also add ApproximateInverseRetraction (and by the way an approximate log would also be neat), and solve JuliaManifold/Manifolds.jl#343 as well by adding the NLsolveInverseRetraction here, too, though then I would maybe add it here and put retractions into their separate section in the interface.html?.

mateuszbaran commented 3 years ago

Sure, I think a general section about retractions, inverse retractions and projections (with some figures as discussed in https://github.com/JuliaNLSolvers/Optim.jl/issues/920 ) would be helpful.

Affie commented 3 years ago

Sure, I think a general section about retractions, inverse retractions and projections (with some figures as discussed in JuliaNLSolvers/Optim.jl#920 ) would be helpful.

That sounds great. The optim.jl documentation gave me the wrong idea that retract and project_tangent were opposites. So it should help users in the future to have retractions and inverse retractions defined together with the projections.

kellertuer commented 3 years ago

Well, Optim.jl uses the retraction as it is defined for topological spaces for a retraction on a manifold, where it has a different meaning usually, for that we can‘t do anything.

For the figure I have to take a look what would be a suitable illustration. Currently thinking of maybe even using just the Circle.

Affie commented 3 years ago

I drew it in my notebook as Circles, I have about 6 of them :-)

kellertuer commented 3 years ago

Hm, I somehow broke something (ExponentialRetraction is not found anymore) though I just copied (carefully) things around. Will take a look later.

edit: just after positing I noticed, it is the usual , missing in exports. Those are really hard to spot.

kellertuer commented 3 years ago

I did a minor bump, since we reorganised files here (and Manifolds.jl has to adapt to that), but it is non-breaking though I also moved two types here (which breaks nothing here and when upgrading manifolds will also break nothing there).

kellertuer commented 3 years ago

Just as a remark, the last commit puts the implementation of the retraction with NLsolve back into Manifolds.jl, since it would introduce a dependency on Reuiqres.jl here.

kellertuer commented 3 years ago

Oh, this was the first time I actually had to carefully do a merge conflict resolution, since we edited the same areas. I hope though that I did not miss anything and we might face something similar in Manifolds.jl then, though there it might be less severe, since I mainly edited docs.

edit: A I might have to rethink my section on main types, which covered Manifold, MPoint, TVector and CoTVector, but maybe the last two are now moved to a vector space section anyways?

mateuszbaran commented 3 years ago

Oh, this was the first time I actually had to carefully do a merge conflict resolution, since we edited the same areas. I hope though that I did not miss anything and we might face something similar in Manifolds.jl then, though there it might be less severe, since I mainly edited docs.

I'll review the changes again :slightly_smiling_face: .

edit: A I might have to rethink my section on main types, which covered Manifold, MPoint, TVector and CoTVector, but maybe the last two are now moved to a vector space section anyways?

Yes, the TVector/CoTVector thing was changed a bit (and we now have AbstractFibreVector). It may make more sense to talk about them in a vector space section.

kellertuer commented 3 years ago

Ah, sure, that makes more sense. I will wait for your manifolds branch to be merged and then also there adapt the docs in my rewrite.

dehann commented 3 years ago

Thanks for all the effort, these documentation refinements really help a lot!

kellertuer commented 3 years ago

I think this one is also good to be merged. I have locally checke both this and the manifolds branch with the new (0.11/0.5) versions and locally all looks good.

mateuszbaran commented 3 years ago

The failure on nightly is probably caused by https://github.com/JuliaLang/julia/pull/33697 .

kellertuer commented 3 years ago

That is a very neat notation they came up with, I just do net yet see why that breaks our shows ;) but yes, that seems related.