Skip to content
Draft
Show file tree
Hide file tree
Changes from 11 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions NEWS.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,15 @@ All notable changes to ´Manifolds.jl´ will be documented in this file.
The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).

## [0.11.30] Unreleased

### Added

* `injectivity_radius(M, p, m)` and `injectivity_radius(M, m)` where `m` is an `AbstractInverseRetractionMethod` and `M` is a `Sphere` are introduced.
* `injectivity_radius(M, m)` where `M` is a `Sphere` and `m` is a `LogarithmicInverseRetraction` or a `ProjectionInverseRetraction`.
* `bijectivity_radius(M, p, m)` and `bijectivity_radius(M, m)` are defined for `Sphere`s.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could also add tests for this?

* `Manifolds.Test.test_manifold` now tests injectivity radii for inverse retractions.

## [0.11.29] Unreleased

### Fixed
Expand Down
15 changes: 13 additions & 2 deletions ext/ManifoldsTestExt/ManifoldsTestExt.jl
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ Possible entries of the `expectations` dictionary are
* for `injectivity_radius`
- you can provide the global test for just the function,
- you can provide the radius for a specific point with `(injectivity_radius, p)`
- you can provide a global or local one for retractions as well usinr `(injectivity_radius, rm)` and `injectivity_radius, p, rm`, respectively
- you can provide a global or local one for retractions as well using `(injectivity_radius, rm)` and `injectivity_radius, p, rm`, respectively
* for `get_basis`, the key is a tuple of the function and the basis, e.g. `(get_basis, B) => ...` to the expected basis
* for `get_coordinates` the key is a tuple of the function and the basis, e.g. `(get_coordinates, B) => c`
* for `get_vector` the key is a tuple of the function, the coordinate vector, and the basis, e.g. `(get_vector, c, B) => X`
Expand Down Expand Up @@ -315,7 +315,18 @@ function Manifolds.Test.test_manifold(M::AbstractManifold, properties::Dict, exp
name = "injectivity_radius(M, p, $rm)", # shorten name within large suite
)
end

for irm in inverse_retraction_methods
ismissing(irm) && continue
expected_irm = get_expectation(expectations, (injectivity_radius, points[1], irm))
expected_irm_global = get_expectation(expectations, (injectivity_radius, irm))
Manifolds.Test.test_injectivity_radius(
M, points[1];
expected_value = expected_irm,
expected_global_value = expected_irm_global,
retraction_method = irm,
name = "injectivity_radius(M, p, $irm)",
)
end
end
if (inner in functions) && !ismissing(vector)
expected_inner = get_expectation(expectations, inner)
Expand Down
6 changes: 5 additions & 1 deletion src/Manifolds.jl
Original file line number Diff line number Diff line change
Expand Up @@ -308,6 +308,7 @@ using ManifoldsBase:
SoftmaxRetraction,
SoftmaxInverseRetraction,
StabilizedRetraction,
StabilizedInverseRetraction,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not defined in ManifoldsBase? How can you import it from there then?

StopForwardingType,
TangentSpace,
TangentSpaceType,
Expand Down Expand Up @@ -756,7 +757,10 @@ export AbstractInverseRetractionMethod,
PolarLightInverseRetraction,
ProjectionInverseRetraction,
ShootingInverseRetraction,
SoftmaxInverseRetraction
SoftmaxInverseRetraction,
StabilizedInverseRetraction
# Bijectivity radii are exported here to be tested, but this should be removed if bijectivity radii get moved to ManifoldsBase.
export bijectivity_radius
# Estimation methods for median and mean
export AbstractApproximationMethod,
GradientDescentEstimation,
Expand Down
65 changes: 60 additions & 5 deletions src/manifolds/Sphere.jl
Original file line number Diff line number Diff line change
Expand Up @@ -286,7 +286,7 @@ function get_vector_orthonormal!(M::AbstractSphere{ℝ}, Y, p, c, ::RealNumbers)
end

_doc_injectivity_radius_sphere = raw"""
injectivity_radius(M::AbstractSphere[, p, ::ExponentialRetraction])

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No. We can not do this.
If you replace ExponentialRetraction here with the Stabilized one, the function is now undefined for ExcponentialRetraction. Anyone relying on that because they use it (it is the standard even!) has their code breaking.
One thing we should always avoid is breaking other peoples code. Always under all circumstances. In 6 years (since we started 2019) we had 10 reasons to do so and we did also those only very carefully. So please do not delete the old variant.

You can for sure add a copy that does this for the stabilised one but I would even argue the stablized one just has a numerical check around so it could even just call infectivity radius on its inner retraction.

injectivity_radius(M::AbstractSphere[, p, ::StabilizedRetraction])

Return the injectivity radius for the [`AbstractSphere`](@ref) `M`, which is globally ``π``.
"""
Expand All @@ -301,12 +301,11 @@ _doc_injectivity_radius_sphere_projection = raw"""
injectivity_radius(M::Sphere, p, ::ProjectionRetraction)

Return the injectivity radius for the [`ProjectionRetraction`](@extref `ManifoldsBase.ProjectionRetraction`) on the
[`AbstractSphere`](@ref), which is globally ``\frac{π}{2}``.
[`AbstractSphere`](@ref), which is globally ``\infty``.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we are here we should document this much more precise, what is injective here, we had a long discussion so lets also be precise here.

"""

@doc "$(_doc_injectivity_radius_sphere_projection)"
injectivity_radius(::AbstractSphere, ::ProjectionRetraction)

@doc "$(_doc_injectivity_radius_sphere_projection)"
injectivity_radius(::AbstractSphere, p, ::ProjectionRetraction)

Expand All @@ -317,8 +316,8 @@ end
function injectivity_radius(M::AbstractSphere, p, m::AbstractRetractionMethod)
return _injectivity_radius(M, p, m)
end
_injectivity_radius(::AbstractSphere, ::ExponentialRetraction) = π

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing this definition here is what is actually breaking. Let's not do that, but as mentioned define the stabilised one to return the one of the retraction it stabilises .

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does make much more sense. I will keep it Manifolds.jl so we can run tests before moving it to ManifoldsBase.jl where stabilized retractions are defined.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is our usual path. We introduce new features here in Manifolds.jl and once we notice they are good – well-designed and tested “in the real world”, we move them to ManifoldsBase

_injectivity_radius(::AbstractSphere, ::ProjectionRetraction) = π / 2
_injectivity_radius(::AbstractSphere, ::StabilizedRetraction) = π
_injectivity_radius(::AbstractSphere, ::ProjectionRetraction) = Inf

@doc raw"""
inverse_retract(M::AbstractSphere, p, q, ::ProjectionInverseRetraction)
Expand All @@ -337,6 +336,62 @@ function inverse_retract_project!(::AbstractSphere, X, p, q)
return (X .= q ./ real(dot(p, q)) .- p)
end

_doc_injectivity_radius_sphere_inverse_projection = raw"""
injectivity_radius(M::AbstractSphere, p, ::ProjectionInverseRetraction)

Return the injectivity radius for the [`ProjectionInverseRetraction`](@extref `ManifoldsBase.ProjectionInverseRetraction`) on the [`AbstractSphere`](@ref), which is the largest geodesic distance ``ξ`` such that the inverse projection retraction from `p` is invertible whenever ``d_{𝕊^{d-1}}(p,q) ≤ ξ``. This is globally ``\frac{\pi}{2}``.
"""

@doc "$(_doc_injectivity_radius_sphere_inverse_projection)"
injectivity_radius(::AbstractSphere, ::ProjectionInverseRetraction)

@doc "$(_doc_injectivity_radius_sphere_inverse_projection)"
injectivity_radius(::AbstractSphere, p, ::ProjectionInverseRetraction)

function injectivity_radius(M::AbstractSphere, m::AbstractInverseRetractionMethod)
return _injectivity_radius(M, m)
end
function injectivity_radius(M::AbstractSphere, p, m::AbstractInverseRetractionMethod)
return _injectivity_radius(M, p, m)
end

function _injectivity_radius(M::AbstractSphere, m::LogarithmicInverseRetraction)
return injectivity_radius(M, StabilizedRetraction())

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No!
(a) Log is the mathematically most important thing, so that one should have the number it returns defined specifically
(b) the stabilised one is just a numerical thing
(c) why are you passing from an inverse retraction to a retraction? That looks at least misleading.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(c) I thought this was the right way to implement your suggestion in this message: the injectivity radius for log calls the one for exp. But I agree this is misleading, I will change that.

end
function _injectivity_radius(M::AbstractSphere, p, m::LogarithmicInverseRetraction)
return injectivity_radius(M, p, StabilizedRetraction())
end
_injectivity_radius(::AbstractSphere, ::ProjectionInverseRetraction) = π / 2
function _injectivity_radius(M::AbstractSphere, p, m::ProjectionInverseRetraction)
return _injectivity_radius(M, m)
end

_doc_bijectivity_radius_sphere = raw"""
bijectivity_radius(M::AbstractSphere[, p, m::Union{AbstractRetractionMethod, AbstractInverseRetractionMethod}])

Return the bijectivity radius for the abstract sphere `M` endowed with (inverse) retraction method `m`, which is the smallest radius applicable to both the tangent spaces and the Riemannian metric for which the (inverse) retraction `m` has a well-defined inverse. This radius is computed as the minimum between the injectivity radius of `M` endowed with (inverse) retraction method `m` (at `p`), and the injectivity radius of the inverse of `m` (at `p`).
"""

@doc "$(_doc_bijectivity_radius_sphere)"
bijectivity_radius(::AbstractSphere, ::Union{AbstractRetractionMethod, AbstractInverseRetractionMethod})
@doc"$(_doc_bijectivity_radius_sphere)"
bijectivity_radius(::AbstractSphere, p, ::Union{AbstractRetractionMethod, AbstractInverseRetractionMethod})

function bijectivity_radius(M::AbstractSphere, m::Union{AbstractRetractionMethod, AbstractInverseRetractionMethod})
if m ∈ (StabilizedRetraction(), StabilizedInverseRetraction())

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks very strange and will often not yield what you want
(a) you compare instances. Even if someone comes along with an instance of the type, the instance might be a different one
(b) if you want to go for if you should check types, so that typeof(m) === StabilizedRetraction. The usually best way is – as all examples in the code before – to do that with dispatch.
(c) also here anything stabilised (since it is a computer/implementation/stability thing) should “pass” to the inner retraction (or inverse retraction) it stores.

return injectivity_radius(M)
elseif m ∈ (ProjectionRetraction(), ProjectionInverseRetraction())
return injectivity_radius(M, ProjectionInverseRetraction())
end
end
function bijectivity_radius(M::AbstractSphere, p, m::Union{AbstractRetractionMethod, AbstractInverseRetractionMethod})
if m ∈ (StabilizedRetraction(), StabilizedInverseRetraction())
return injectivity_radius(M, p)
elseif m ∈ (ProjectionRetraction(), ProjectionInverseRetraction())
return injectivity_radius(M, p, ProjectionInverseRetraction())
end
end

"""
is_flat(M::AbstractSphere)

Expand Down
6 changes: 4 additions & 2 deletions test/manifolds/test_sphere.jl
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ using ManifoldDiff
:InvalidVectors => [p],
:NormalVectors => [V],
:Covectors => [ξ],
:RetractionMethods => [ExponentialRetraction(), ProjectionRetraction()],

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sane as before – no. The sphere should absolutely be tested on the ExponentialRetraction. Stabilisation is something extra – and exp is still the default. If you want to test it, add it at the end.

:RetractionMethods => [StabilizedRetraction(), ProjectionRetraction()],
:VectorTransportMethods => [ParallelTransport(), SchildsLadderTransport(), PoleLadderTransport()]
),
# Expectations
Expand All @@ -48,7 +48,9 @@ using ManifoldDiff
(get_vectors, DefaultOrthonormalBasis()) => :Orthonormal,
(get_vectors, DefaultOrthogonalBasis()) => :Orthogonal,
injectivity_radius => π,
(injectivity_radius, ProjectionRetraction()) => π / 2,
(injectivity_radius, ProjectionRetraction()) => Inf,
(injectivity_radius, LogarithmicInverseRetraction()) => π,
(injectivity_radius, ProjectionInverseRetraction()) => π / 2,
is_default_metric => EuclideanMetric(),
log => X, norm => π / 4,
parallel_transport_to => parallel_transport_to(M, p, X, q),
Expand Down
Loading