🎆 Prepare for ManifoldsBase.jl 1.0

[kellertuer]

This is a breaking PR preparing for a nicer function naming in LieGroups.jl

❓

• the “scaling” exp is now called expt. Should we do the same with retract?
  It does however not suffer from the same ambiguities, since we also have the method there as a mandatory element I think.
• Anything else we wanted to break anyways here? Maybe remove (since unused and unclear) vector_transport_along cf 1.0 release planning #211? Done.
• for me a keyword exp(M, p, X; t=...) would be fine, but not necessary. Thoughts on this why this might be a bad idea to have? No.

📋

• rename exp(M, p, X, t) to expt(M; p, X, t)
• rename exp!(M, q, p, X, t) to expt!(M, q, p, X, t)
• rename TVector to AbstractTangentVector and similarly for Validation|Default|NonTVector
• rename CoTVector to AbstractCotangentVector and similarly for Validation|Default|NonTVector
• rename expt to exp_fused and retract_t to retract_fused
• 📚Updated docs (Quarto/Tutorial to Julia 1.11, verified they render on the new 0.16)
• fix Make injectivity_radius less prone to ambiguities #189 on injectivity_radius
• 📈 test coverage

[codecov]

Codecov Report

Attention: Patch coverage is 91.77215% with 13 lines in your changes missing coverage. Please review.

Project coverage is 99.40%. Comparing base (5c4a61e) to head (614ef23).

Files with missing lines	Patch %	Lines
src/ProductManifold.jl	53.84%	6 Missing ⚠️
src/point_vector_fallbacks.jl	90.00%	3 Missing ⚠️
src/ManifoldsBase.jl	0.00%	2 Missing ⚠️
src/retractions.jl	97.40%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #221      +/-   ##
==========================================
- Coverage   99.97%   99.40%   -0.58%     
==========================================
  Files          31       31              
  Lines        3539     3507      -32     
==========================================
- Hits         3538     3486      -52     
- Misses          1       21      +20     

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

[kellertuer]

I just noticed both an advantage of this PR and an argument against the keyword I propose:

With the previous positional t::Number we always went for the “fusing” variant, so if a (maybe beginner) contributor did not think of that, we would still always compute 1*X which in uncareful code is inefficient.
By dividing both, the classical variant now is a bit nicer even. While the keyword with a nothing default does not have that, using that uncareful might have the same effect.

[olivierverdier]

Yes!! I suggested that once and it was dissed, but that is an obvious change to me. Way more robust. Go! 😀

[kellertuer]

You were not dissed; the problem is that this is a huge change so it took some discussions.

For example adapting Manifolds.jl took me most of today (still locally here). So someone has to do all that work then.
And sure in hindsight, you were right earlier. But it will be at least 2-3 further full days of work to adapt Manifolds.jl, and even more for the other packages.

That is why one should only do this after careful considerations.

[kellertuer]

Though about Q1 with retract – I think there the positional parameters especially with the method in the end will avoid the problems we had with exp. So that should be fine.
Also changing retract would be breaking in a lot of places.

[mateuszbaran]

• the “scaling” exp is now called expt. Should we do the same with retract?
  It does however not suffer from the same ambiguities, since we also have the method there as a mandatory element I think.

Yes; for consistency I'd suggest doing the same thing with retract. Maybe retract_t?

• for me a keyword exp(M, p, X; t=...) would be fine, but not necessary. Thoughts on this why this might be a bad idea to have?

I'd prefer to not have the t keyword argument. It doesn't offer anything new and would only cause more maintenance to keep it supported.

• Anything else we wanted to break anyways here? Maybe remove (since unused and unclear) vector_transport_along cf 1.0 release planning #211?

Yes, I'd remove vector_transport_along for now and only reintroduce it if/when we figure out curves.

I'd also like to have #189 resolved for 0.16.

Though about Q1 with retract – I think there the positional parameters especially with the method in the end will avoid the problems we had with exp. So that should be fine.
Also changing retract would be breaking in a lot of places.

How would retract be any less ambiguous than exp? The retraction method argument doesn't affect in any way the earlier arguments, which are exactly the same as in exp. So we have exactly the same problems, only with the method added. Or without it because it's optional, in which case it has the same issues as exp.

[mateuszbaran]

By the way, we could probably go 1.0 in this breaking release? The interface seems stable enough to me.

[kellertuer]

• the “scaling” exp is now called expt. Should we do the same with retract?
  It does however not suffer from the same ambiguities, since we also have the method there as a mandatory element I think.

Yes; for consistency I'd suggest doing the same thing with retract. Maybe retract_t?

Hm, then expt should also be exp_t. The disadvantage that I see is the amount of work both here as well as in all other packages. Until now this here is at least nonbreaking for Manopt, especially because it uses retract mainly.

And reworking exp took me 6 hours, retract is much much much more revolved, so time wise, this would also be a huge effort and (besides consistency) I do not see a good reason why. Ambiguities should not be a problem for retract.

• for me a keyword exp(M, p, X; t=...) would be fine, but not necessary. Thoughts on this why this might be a bad idea to have?

I'd prefer to not have the t keyword argument. It doesn't offer anything new and would only cause more maintenance to keep it supported.

Sure, not problem.

• Anything else we wanted to break anyways here? Maybe remove (since unused and unclear) vector_transport_along cf 1.0 release planning #211?

Yes, I'd remove vector_transport_along for now and only reintroduce it if/when we figure out curves.

I'd also like to have #189 resolved for 0.16.

If you could work on that, that would be great.

How would retract be any less ambiguous than exp?

I do not see it appearing on Lie groups for example

By the way, we could probably go 1.0 in this breaking release? The interface seems stable enough to me.

Sure we could do that, if we even do infectivity radius we have solved all we have in mind. Sounds nice to move the interface to 1.0, sure.

[kellertuer]

While I personally would prefer having metric and connection here before 1.0, I do see the advantage and think it is nice to go to 1.0 with this, when we actually resolve all points from the release plan. And metric and connection could be part of a 1.1 – and then maybe later be used to make LieGroups more precise in its metrics/connections.

[mateuszbaran]

Yes, metric and connection is quite a bit of work still and putting them here is not a breaking change. I'm working on this PR right now as we discussed (retract thing).

[kellertuer]

When exporting them now, which name do we use?

' exp_scaled is explaining a bit what the function mathematically does
' exp_fused what it technically does

both are a bit clumsy, but I have no better idea; I think I personally prefer _fused.

[mateuszbaran]

I think I prefer exp_scaled actually because it describes the mathematical idea better.

[kellertuer]

My motivation for the _fused is that the main reason for the function is not that it is mathematically possible – that can be done with exp as well – but that the reason the function exists is a technical one, not a mathematical one. So that is why I would prefer _fused to emphasise its technical nature/motivation.

But I also think this is a function that “belongs” to you to some extend, you introduced that idea and realised it – I still don‘t fully see how much better it is to use it – so I leave the final naming to you.

[kellertuer]

I just removed all _along methods.

I was shortly thinking about a “How to get started”, but felt, this is maybe a bit complicated, since we already have the tutorial how to implement a manifold in the API.

[mateuszbaran]

OK, then I'll rename it to _fused.

For injectivity radius I'd suggest changing it so that:

• _injectivity_radius always requires AbstractRetractionMethod as the last argument and is what specific manifolds implement.
• injectivity_radius is a convenience wrapper for _injectivity_radius.
• injectivity_radius_exp is removed.
  Does this look fine to you?

[kellertuer]

Thanks for your understanding. I can check that the docs mention both the math part and the advantages in allocations.

The plan for the infectivity radius sounds good, the convenience one would have a keyword argument? Should that default to ExponentialRetraction then?

[mateuszbaran]

The plan for the infectivity radius sounds good, the convenience one would have a keyword argument? Should that default to ExponentialRetraction then?

I'm not sure, I thought about leaving it positional like it is right now. The default should probably be default_retraction_method(M) ?

[kellertuer]

Positional is fine with me as well.

That also sounds reasonable, I was a bit thinking of exp since that is the mathematical one, but sure, consistency is more important in our API, so lets go for the default method.

[kellertuer]

I updated the initial post to reflect the missing things here.

Let me know if I can help somewhere. We should maybe use this nice step to either start a ManifoldsBase Thread on Discourse (I would prefer that) or a general JuliaManifolds announcement thread?

[mateuszbaran]

I will try to finish my changes here during the weekend.

I'm also contemplating other minor change. get_vector_method and get_coordianate_method currently have the basis unpacked and it annoyed me basically every time I had to deal with that. I'd prefer to have it changed to just passing the basis.

I think we can have a new ManifoldsBase thread on discourse, sure.

[kellertuer]

Sure that change sounds reasonable to me. Feel free to work on that as well.

For the thread – I will draft an announcement then on the weekend.

[kellertuer]

Here is three small things I noticed. For retract, I would prefer adapting that to exp, since there the “ease of use” allows to first just implement exp(M, p, X) and only get into the detail later if necessary; retract is now the other way around, one always has to first learn about _fused, and both are now inconsistent.