JuliaManifolds / ManifoldsBase.jl

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

Introduce the idea of the more-precise default functions from (#144). #145

Closed kellertuer closed 1 year ago

kellertuer commented 1 year ago

Let me know what you think.

Maybe default_X_method(M, typeof(p)) looks a little clumsy, but I feel to emphasise that it should never depend on the values of p really only on its type. We should not change the default “locally” but based on representation.

The application example from Manifolds.jl is the Grassmann manifold, where we should only set a default for the Array/Stiefel representation (to be ProjectionRetraction) but not for the Projection representation, where no vector transport is implemented.

This is non-breaking, since the 2-argument version falls back to the 1-argument one – but making use of this more-precise dispatch requires a bit of patching in Manifolds and Manopt.

codecov[bot] commented 1 year ago

Codecov Report

Merging #145 (f632b19) into master (c7e2a17) will increase coverage by 0.08%. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #145      +/-   ##
==========================================
+ Coverage   99.79%   99.87%   +0.08%     
==========================================
  Files          19       19              
  Lines        2452     2462      +10     
==========================================
+ Hits         2447     2459      +12     
+ Misses          5        3       -2     
Impacted Files Coverage Δ
src/decorator_trait.jl 100.00% <ø> (ø)
src/PowerManifold.jl 100.00% <100.00%> (+0.37%) :arrow_up:
src/retractions.jl 100.00% <100.00%> (ø)
src/vector_transport.jl 100.00% <100.00%> (ø)

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

mateuszbaran commented 1 year ago

Everything else looks fine here :+1: .

kellertuer commented 1 year ago

I also already ran downstream tests – Manifolds still works on this – but we can refine in a few places (where we use the default and for Grassmann as discussed).