JuliaManifolds / Manopt.jl

πŸ”οΈManopt. jl – Optimization on Manifolds in Julia
http://manoptjl.org
Other
314 stars 40 forks source link

Use `sectional_curvature` from ManifoldsBase #365

Closed mateuszbaran closed 5 months ago

mateuszbaran commented 5 months ago

Ideally it should use sectional_curvature_max but it's not generally implemented in Manifolds.jl yet.

I've also removed the unused k_min keyword argument.

codecov[bot] commented 5 months ago

Codecov Report

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

Project coverage is 99.73%. Comparing base (15194b0) to head (94122a8).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #365 +/- ## ========================================== - Coverage 99.73% 99.73% -0.01% ========================================== Files 73 73 Lines 6822 6819 -3 ========================================== - Hits 6804 6801 -3 Misses 18 18 ```

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

mateuszbaran commented 5 months ago

Documenter.jl thinks ManifoldsBase.jl 0.15.8 doesn't exist but CI tests use it without issue... weird.

kellertuer commented 5 months ago

Maybe the docs need a small bump? But I wanted to check real quick the interlinks anyways, now that ManifoldsBase has run on Documenter 1.3 and has a first such index to use for the inter-docs :)

kellertuer commented 5 months ago

This looks good to me. Maybe we can check whether the docs are ok when merged with #366 .

hajg-ijk commented 5 months ago

Looks good to me, too. The only note here is that ΞΆ_1 (which is not used anywhere at the moment) in theory takes in the lower bound on the curvature k_min as an input.

mateuszbaran commented 5 months ago

Now we have a different error:

β”Œ Error: On "src/solvers/convex_bundle_method.md", cannot resolve external link: Cannot find "`ManifoldsBase.sectional_curvature`" in any InterLinks inventory: ["ManifoldsBase"]
β”‚   node =
β”‚    @ast MarkdownAST.Link("@extref", "") do
β”‚      MarkdownAST.Code("ManifoldsBase.sectional_curvature")
β”‚    end
β”‚    
β”” @ DocumenterInterLinks ~/.julia/packages/DocumenterInterLinks/jzyZp/src/expand_extrefs.jl:93

I thought that would work, like https://github.com/JuliaManifolds/Manopt.jl/pull/366/files#diff-6b82ea9e272b78f9938c76cb57acca981b149e4c4a319e5be73944701d1d693bR29 ?

kellertuer commented 5 months ago

Had to take a look, the main problem is, that the method has 3 signatures (abstract manifold, power, product) so you have to specify the signature. That is what I fixed in my commit.

Might look a bit complicated, but I also have no idea how InterLink could make that better.

kellertuer commented 5 months ago

In other places I do not use the ManifoldsBase prefix in the link text – should we keep that here, what do you think?

mateuszbaran commented 5 months ago

I see, it's different for types and methods. I think we can remove the prefix then.

kellertuer commented 5 months ago

One tipp that Michael also gave is, to using DocumenterInterLinks and then load the

links = InterLinks(
    "ManifoldsBase" => ("https://juliamanifolds.github.io/ManifoldsBase.jl/stable/")
)

then you can query

julia> links["ManifoldsBase"]("sectional_curvature-")
3-element Vector{DocInventories.InventoryItem}:
 InventoryItem(":jl:method:`ManifoldsBase.sectional_curvature-Tuple{AbstractManifold, Any, Any, Any}`" => "functions/#ManifoldsBase.sectional_curvature-Tuple%7BAbstractManifold%2C%20Any%2C%20Any%2C%20Any%7D")
 InventoryItem(":jl:method:`ManifoldsBase.sectional_curvature-Tuple{AbstractPowerManifold, Any, Any, Any}`" => "metamanifolds/#ManifoldsBase.sectional_curvature-Tuple%7BAbstractPowerManifold%2C%20Any%2C%20Any%2C%20Any%7D")
 InventoryItem(":jl:method:`ManifoldsBase.sectional_curvature-Tuple{ProductManifold, Any, Any, Any}`" => "metamanifolds/#ManifoldsBase.sectional_curvature-Tuple%7BProductManifold%2C%20Any%2C%20Any%2C%20Any%7D")

and everything after the method: is what you copy (you might guess this can also indicate types or sections or such)

mateuszbaran commented 5 months ago

Thanks, that's a good tip.

kellertuer commented 5 months ago

I see, it's different for types and methods.

Yes, since types are uniquely identified by their name, methods need the full signature with parameters, but then the list from above is helpful.