JuliaManifolds / Manifolds.jl

Manifolds.jl provides a library of manifolds aiming for an easy-to-use and fast implementation.
https://juliamanifolds.github.io/Manifolds.jl
MIT License
366 stars 52 forks source link

Implement infectivity radius on Stiefel #722

Closed kellertuer closed 3 months ago

kellertuer commented 3 months ago

cf. Zimmermann & Stoye https://arxiv.org/abs/2405.02268v1 .

codecov[bot] commented 3 months ago

Codecov Report

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

Project coverage is 99.58%. Comparing base (6f54b6e) to head (36a00d1).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #722 +/- ## ======================================= Coverage 99.58% 99.58% ======================================= Files 114 114 Lines 11229 11230 +1 ======================================= + Hits 11182 11183 +1 Misses 47 47 ```

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

mateuszbaran commented 3 months ago

That was quick! The failure is caused by a new ambiguity:

injectivity_radius(M::AbstractDecoratorManifold, m::AbstractRetractionMethod) @ ManifoldsBase ~/.julia/packages/ManifoldsBase/pOL51/src/nested_trait.jl:305,
injectivity_radius(::Stiefel, p) @ Manifolds ~/work/Manifolds.jl/Manifolds.jl/src/manifolds/StiefelEuclideanMetric.jl:112
kellertuer commented 3 months ago

Oh we should maybe check ambiguities on the infectivity radius. – So how do we reduce this? Or do we increase the bound?

mateuszbaran commented 3 months ago

It's real, you can try calling injectivity_radius(Stiefel(5, 2), ExponentialRetraction()) and it would fail. You can probably just delete injectivity_radius(::Stiefel, p) = π though.

kellertuer commented 3 months ago

Then the special point tests fail. But if that is ok, we can do that (that is remove the point ones).

mateuszbaran commented 3 months ago

Then the special point tests fail. But if that is ok, we can do that (that is remove the point ones).

You can limit the type of p in injectivity_radius(::Stiefel, p) to those special point types.

kellertuer commented 3 months ago

or leave them out, we would specialise to arrays or such, and that might not be to useful (we otherwise do not do that I think).

mateuszbaran commented 3 months ago

I think the variant with p should work. You can also disambiguate by defining injectivity_radius(M::Stiefel, m::AbstractRetractionMethod). Or we shouldn't have made injectivity_radius so prone to ambiguities.

kellertuer commented 3 months ago

I omitted the p variants for now, but sure in the long run we should make the infectivity radius a bit less prone to ambiguities.