Sectional curvature

[mateuszbaran]

Adapted from Manopt.jl, with added functions for lower and upper bounds.

[kellertuer]

This looks very nice! Thanks for adapting that.

If the test coverage is fine, I am ok with this.

[mateuszbaran]

Question: should sectional_curvature return 0 for non-linearly independent vectors X, Y or be undefined? Making it 0 makes it much easier to define sectional_curvature on power and product manifolds but makes it harder to properly define the generic fallback that uses riemann_tensor.

[kellertuer]

Why is that harder for the Riemannian tensor?
I would have thought it is either undefined or zero.

[mateuszbaran]

The formula inner(M, p, R, X) / ((norm(M, p, X)^2 * norm(M, p, Y)^2) - (inner(M, p, X, Y)^2)) is 0 / 0 when X and Y are linearly dependent. We can either detect this situation and return 0 (which requires adding a cut-off due to numerical inaccuracy) or say "don't use linearly dependent vectors".

[kellertuer]

I think the clearer way would be “we do not use linearly dependent vectors”?

[mateuszbaran]

That's how it currently works.

[kellertuer]

Ah, yes. I see. And it is also well-documented that way. I think it is ok to keep it that way.

[mateuszbaran]

OK, then we need a test for linear independence in sectional_curvature of ProductManifold. I don't really have a nice idea for that, maybe something like norm(M, p, X) < eps(...) || norm(M, p, Y) < eps(...) || isapprox(inner(M, p, X, Y), norm(M, p, X)*norm(M, p, Y)) || isapprox(inner(M, p, X, Y), -norm(M, p, X)*norm(M, p, Y))?

[mateuszbaran]

Or we can just leave that not implemented for now.

[kellertuer]

I would thoroughly document that we assume they are linearly independent – and for now not implement a check.

[mateuszbaran]

It doesn't work for product manifolds though. You may have two vectors that are linearly independent, but their projections on one or more of the component manifolds are not linearly independent, so that needs to be handled somewhere since sectional curvature for them is well-defined.

[kellertuer]

But then one could check that on every “manifold factor” and only add that to the min if they are linearly independent and ignore them otherwise?

[mateuszbaran]

Yes, that's what would be needed.

[kellertuer]

Overall this looks fine to me. Just two small remarks.

[kellertuer]

This looks good so far. I am just missing the good old code coverage – did we miss to update the GitHub action? I remember something like this in a recent PR either on Manopt or Manifolds as well that I had to specifically add the secret (again) to the action.

[kellertuer]

ah cf. JuliaManifolds/Manopt.jl@5d7646f

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.96%. Comparing base (b7665ae) to head (08abaef).
Report is 1 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master     #184   +/-   ##
=======================================
  Coverage   99.96%   99.96%           
=======================================
  Files          27       27           
  Lines        3107     3156   +49     
=======================================
+ Hits         3106     3155   +49     
  Misses          1        1           

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

[mateuszbaran]

Right, I somehow didn't notice the lack of coverage report. It works now. I think we can merge this now?