Adjust injectivity radii for (inverse) retractions on the Sphere (#900) by sblelong · Pull Request #915 · JuliaManifolds/Manifolds.jl

sblelong · 2026-07-06T08:46:14Z

Hi!

Here's a first attempt to address the issue raised in #900 about injectivity radii for retractions and their inverse.

I tried to follow the unit testing pattern on the sphere but I'm requesting approval before proceeding with tests for the bijectivity_radius.

…rojectionInverseRetraction on the Sphere

…olds#900)

…ractSphere (external ref to `StabilizedInverseRetraction` is pending typo fix in ManifoldsBase)

kellertuer

Thanks for the start. Here are some first preliminary comments.

The test failure on CI does seem more related too SciML destroying something, so for comments on that we have to wait for that to work again (I wrote on their channel on slack)

kellertuer · 2026-07-06T13:44:25Z

 end

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


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.

kellertuer · 2026-07-06T13:45:25Z


 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``.


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.

kellertuer · 2026-07-06T13:46:21Z

 function injectivity_radius(M::AbstractSphere, p, m::AbstractRetractionMethod)
    return _injectivity_radius(M, p, m)
 end
-_injectivity_radius(::AbstractSphere, ::ExponentialRetraction) = π


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 .

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.

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

kellertuer · 2026-07-06T13:48:03Z

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


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.

(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.

kellertuer · 2026-07-06T13:51:31Z

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


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.

kellertuer · 2026-07-06T13:52:24Z

    SoftmaxRetraction,
    SoftmaxInverseRetraction,
    StabilizedRetraction,
+    StabilizedInverseRetraction,


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

kellertuer · 2026-07-06T13:53:25Z

            :InvalidVectors => [p],
            :NormalVectors => [V],
            :Covectors => [ξ],
-            :RetractionMethods => [ExponentialRetraction(), ProjectionRetraction()],


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.

kellertuer · 2026-07-06T13:54:38Z

+
+* `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.


You could also add tests for this?

codecov · 2026-07-06T14:12:53Z

Codecov Report

❌ Patch coverage is 0% with 33 lines in your changes missing coverage. Please review.
✅ Project coverage is 7.97%. Comparing base (2821510) to head (048e253).

Files with missing lines	Patch %	Lines
src/manifolds/Sphere.jl	0.00%	23 Missing ⚠️
ext/ManifoldsTestExt/ManifoldsTestExt.jl	0.00%	6 Missing ⚠️
src/Manifolds.jl	0.00%	4 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (2821510) and HEAD (048e253). Click for more details.

HEAD has 2 uploads less than BASE

Flag BASE (2821510) HEAD (048e253)

4 2

Additional details and impacted files

@@             Coverage Diff             @@
##            master    #915       +/-   ##
===========================================
- Coverage   100.00%   7.97%   -92.03%     
===========================================
  Files           98      98               
  Lines         9830    9712      -118     
===========================================
- Hits          9830     775     -9055     
- Misses           0    8937     +8937

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

sblelong · 2026-07-06T14:50:07Z

Thank you for this detailed feedback, I'll address all changes!

kellertuer · 2026-07-06T15:47:18Z

As a concrete example – I could convince the tests a bit to run and this

https://github.com/JuliaManifolds/Manifolds.jl/actions/runs/28779489714/job/85391973067?pr=915#step:6:544

should never happen. You did not touch Stiefel at all, but something you changed made that fail now.

You did change expected numbers in the tests (and whether that is breaking is something we should discuss/conclude in the end) – but nothing else should change – otherwise we have a breaking change.

You can also test this locally. Do you know about development mode and ] test Manifolds to run the tests locally?
If you feel a bit lost, we can check to have a short meeting, but not this week, since officially I am on vacation.

sblelong · 2026-07-07T15:33:42Z

I was indeed negligent with running tests locally: I ran the The Spheres testset only. I do know about the ] test Manifolds command, but wrongly decided not to test thoroughly this time. My mistake.

The error encountered in the CI brings an interesting question though: as soon as an inverse retraction is defined for a manifold, should we expect the injectivity radius for this inverse retraction to be implemented?

kellertuer · 2026-07-07T15:59:22Z

No worries, errors are also always a good occasion to learn.

The Spheres testset only.

Yes, we made the possible on purpose, since the overall tests run for a while, one can focus on a single one of on does work only on these. You however also introduce a new signature globally!

as soon as an inverse retraction is defined for a manifold, should we expect the injectivity radius for this inverse retraction to be implemented?

That is an interesting point that affects the test suite (so not whether something is breaking) – For consistency, I think it would be good if it were implemented for all manifolds we have that now run the new tests, and I think a reasonable start is to set it to the same value as the retraction case – but explicitly to that number and add a doctoring to that implementation as well.
That is a path that takes a bit more time to implement, but does not require so much “thinking” in the sense that you have to prove something or look something up (mainly copy a function and its docs and adapt it)
Would you be available to do that?

The alternative is to make the new tests inactive by default and introduce a boolean keyword – though that might also lead to us “forgetting” to add these functions. But it is less work for sure.

sblelong · 2026-07-07T16:04:26Z

Yes, we made the possible on purpose, since the overall tests run for a while, one can focus on a single one of on does work only on these. You however also introduce a new signature globally!

And that would have been a good reason for me to be more patient and run the tests entirely before opening the PR! One lesson learned. :)

That is an interesting point that affects the test suite (so not whether something is breaking) – For consistency, I think it would be good if it were implemented for all manifolds we have that now run the new tests, and I think a reasonable start is to set it to the same value as the retraction case – but explicitly to that number and add a doctoring to that implementation as well. That is a path that takes a bit more time to implement, but does not require so much “thinking” in the sense that you have to prove something or look something up (mainly copy a function and its docs and adapt it) Would you be available to do that?

That sounds feasible. I am available and will work on that in the upcoming days, while addressing the other issues raised in this PR.

kellertuer · 2026-07-07T16:27:01Z

That sounds feasible. I am available and will work on that in the upcoming days, while addressing the other issues raised in this PR.

Nice, thanks! And no worries, a PR is nearly never a sprint, I always prefer thoroughness and completeness (and less bugs) over speed.

sblelong and others added 11 commits June 27, 2026 15:38

Add injectivity radius for the LogarithmicInverseRetraction and the P…

dcfdab3

…rojectionInverseRetraction on the Sphere

Fix docs and tests

68af73f

Introducing bijectivity radii for the Sphere (according to JuliaManif…

610082e

…olds#900)

Make StabilizedInverseRetraction the default retraction for an Abst…

8369cbd

…ractSphere (external ref to `StabilizedInverseRetraction` is pending typo fix in ManifoldsBase)

Fix a typo in the doc of test_manifold

f8bd474

Unit tests for injectivity radii of inverse retractions

f3016ac

LogarithmicInverseRetraction() is default again for the Sphere

5470f33

Adjust expected values for unit tests

6e57ba6

Update changelog

becf285

Merge branch 'master' into injectivity-radii

ecf8f64

Update changelog

cce40b4

kellertuer requested changes Jul 6, 2026

View reviewed changes

Injectivity radius for StabilizedRetraction

048e253

Uh oh!

Conversation

sblelong commented Jul 6, 2026

Uh oh!

kellertuer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov Bot commented Jul 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

sblelong commented Jul 6, 2026

Uh oh!

kellertuer commented Jul 6, 2026

Uh oh!

sblelong commented Jul 7, 2026

Uh oh!

kellertuer commented Jul 7, 2026

Uh oh!

sblelong commented Jul 7, 2026

Uh oh!

kellertuer commented Jul 7, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

codecov Bot commented Jul 6, 2026 •

edited

Loading