add segre and warped segre manifold

[sjacobsson]

No description provided.

[kellertuer]

Thanks for getting this started!
If you grant me access to your repository/fork, I can also push a few small things along the way.

A very short first look

• Signatures of functions in the doc string can also just be one line
• we usually added doc-strings to the non-inlace variant (e.g exp) while implementing the mutating one (e.g. exp!).
• doc strings would be great with a formula for most of the functions (inner, exp, log, norm,...)
• you already added the reference, you can refer to that in the doc string as well.
• I can help setting up the following as well
  • a separate page in the documentation for the new manifold
  • a bit for file structures see the other manifolds different files for different metrics
  • a first idea for testing

[sjacobsson]

Thanks for getting this started! If you grant me access to your repository/fork, I can also push a few small things along the way.

A very short first look

* Signatures of functions in the doc string can also just be one line

* we usually added doc-strings to the non-inlace variant (e.g `exp`) while implementing the mutating one (e.g. `exp!`).

* doc strings would be great with a formula for most of the functions (inner, exp, log, norm,...)

* you already added the reference, you can refer to that in the doc string as well.

* I can help setting up the following as well
  
  * a separate page in the documentation for the new manifold
  * a bit for file structures see the other manifolds different files for different metrics
  * a first idea for testing

Thanks, you should now be added as a collaborator to the fork.

I'll take a look at the doc stuff and try to conform to the rest of the repo. For the testing, I never figured out how to run the tests locally so I just commited my best guess. But once tests run, they should at least guarantee that the methods that Segre implements are consistent with each other.

[kellertuer]

No worries! Some parts of such a PR are a bit involved. I will both set up the tests for you and help you running them locally :)
On the other hand – tests are quite important to make sure the code continues to work as we expect, so we should definetly add them – also because we have a few checks here, that require to have a certain fraction (quite large part actually)of your code covered by tests.
But again, no worries, I am confident we will manage that together!

[sjacobsson]

Hi again,
Sorry, is there some way to view the documentation that I'm writing? I tried to run docs/make.jl but it ran into some errors.

[kellertuer]

HI,
an easy, but slow way, is to make sure Manifolds.jl compiles at least and then you can (now with the label added) render the docs here If you want.

Locally on your machine you have to set the package into development mode and then call docs/make.jl (either from shell as it is a shell script or including it because it is also a Julia script).

The development mode is maybe the tricky one (see https://pkgdocs.julialang.org/v1/managing-packages/#developing), running from shell should be a bit easier. We wrote the docs in its own environment so that should work fine.
It might be that running the make fiel for the docs itself errors, that happens whenever there is documented functions/types that are not yet added to the documentation. But as I said, when I find time I can create the page for your manifold and the metric.

If you can not get it to run you could post the error message here or we can check (after I found time) in a video call.

[sjacobsson]

Thanks, I'll try those things. No stress!

[mateuszbaran]

Thanks for the contribution! I see there is an error with an unrelated test, I will fix it soon in a separate PR.

[kellertuer]

I allowed myself to fix a few small things here

• merged master so the last fix from Mateusz also works here (not related to your code, but made CI fail)
• created a page in the docs.
• made sure docs work
• made Manifolds.jl compile – you had a few undeclared parameters. Not using them, one can even remove them
  • usually our field 𝔽 is the first parameter, I switched the order in Segre to make it first there as well since it confused me and makes a few function signatures nicer in the metric file.
  • I simplified said signatures – V could be removed in most places
  • removes all @assert within a function. We take the approach “input is correct/valid“ as far as possible – there is the ValidatioNManifold to activate those tests on a global level for a manifold. The advantage their is you can even choose the level of check – do you want to see an error for example or just an info
  • I removed the exp for now since that default we have implemented as well – if so we might want to write the corresponding allocate_result (https://github.com/JuliaManifolds/ManifoldsBase.jl/blob/7b52f311f3cc5bcd4671edb5479f798250c02876/src/exp_log_geo.jl#L16) – then much more functions also only need their !-variants.
• it would maybe be nicer to document is_point instead of `check_vector´ – it would also be great to have a bit more of a desciption there.
• checked that tests are included
  • they currently still fail, since you initialise some AlphaWarpedMetric? Was that renamed?
• ran JuliaFormatter (the format check here, run it with using JuliaFormatter; format(".") in the base folder of Manifolds.jl)
• started an entry in the news.
• Commented out tests for now, since then we get a first code coverage report.
• made them “standalone” they can just be run with include("test/manifolds/segre.jl") instead of running all tests – load other packages you need also there (and check they are in the extras of the project toml)

What would be great

• continue with the documentation, the formulae deyail in the beginning looks great (will read them carefully somewhen)
• check performance
• check test coverage
• add you to the contributors also in the zenodo info file for example
• I did not yet read any of the docs or code more closely but will definetly do that to learn about the new manifold as well

Besides all these comments – super cool that you got this started!

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.42%. Comparing base (80541bb) to head (a2413c5).
Report is 1 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #755      +/-   ##
==========================================
+ Coverage   96.37%   96.42%   +0.05%     
==========================================
  Files         125      127       +2     
  Lines       11625    11790     +165     
==========================================
+ Hits        11204    11369     +165     
  Misses        421      421              

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

[sjacobsson]

Thanks for fixing those things!

The problem is indeed a rename of AlphaWarpedMetric. I still don't know if the current name, WarpedMetric, is a good choice. The problem is that the euclidean metric is already a warped product metric, but then you can have a whole family of metrics based on your warping parameter and most of those are not euclidean...

Sorry, I still can't find where to view the documentation.

[kellertuer]

Ah it might be that as an external PR-opener this does not give you push rights, so the preview button does not appear, I have to check.
But still, just “out of the box” our docs/make.jl (either run via include on Julia REPL or as a shell script) should produce the docs as well. It activates its own environment and adds Manifolds.jl in development mode from the parent folder, so it should always generate the docs of the state you have on your hard drive.

For the name, try to find a name that fits well. For me WarpedMetric is fine but maybe a bit too generic (and the alpha does not help much then). Still, if you favour WarpedMetric as the type name for the metric, go for it :)

[sjacobsson]

I shall meditate on the name for today and tomorrow, otherwise let's just go with WarpedMetric.

I also tried running make.jl but I get the following error: ERROR: LoadError: 'tutorials/getstarted.md' is not an existing page!
Here is the full output:

simonj@beltrami:~/Workspace/Manifolds.jl$ julia
               _
   _       _ _(_)_     |  Documentation: https://docs.julialang.org
  (_)     | (_) (_)    |
   _ _   _| |_  __ _   |  Type "?" for help, "]?" for Pkg help.
  | | | | | | |/ _` |  |
  | | |_| | | | (_| |  |  Version 1.9.3 (2023-08-24)
 _/ |\__'_|_|_|\__'_|  |  Official https://julialang.org/ release
|__/                   |

julia> using Manifolds

help?> Segre
search: Segre NestedReplacingPowerRepresentation base_group isinteger HeisenbergGroup SymplecticGrassmann

      \mathcal{S} = \operatorname{Seg}(𝔽^{n_1} \times \dots \times 𝔽^{n_d})

  is the space of rank-one tensors in 𝔽^{n_1} \otimes \dots \otimes 𝔽^{n_d}.

  When 𝔽 = ℝ, the Segre manifold is represented as

      \mathcal{S} \sim ℝ^{+} \times S^{n_1 - 1} \times \dots \times S^{n_d - 1}.

  This is a local diffeomorphism, and the manifold is a locally a warped product
  (https://en.wikipedia.org/wiki/Warped_product) of ℝ^{+} and S^{n_1 - 1} \times \dots \times S^{n_d -
  1}. The tuple (n_1, \dots, n_d) is called the valence of the manifold.

  The geometry is summarized in JacobssonSwijsenVandervekenVannieuwenhoven:2024 (@cite).

  Constructor
  ≡≡≡≡≡≡≡≡≡≡≡≡≡

  Segre(valence::NTuple{V, Int}; field::AbstractNumbers=ℝ)

  Generate a valence V Segre manifold.

julia> include("docs/make.jl")
  Activating project at `~/Workspace/Manifolds.jl/docs`
   Resolving package versions...
    Updating `~/Workspace/Manifolds.jl/docs/Project.toml`
  [1cead3c2] ~ Manifolds v0.10.4 `..` ⇒ v0.10.4 `~/Workspace/Manifolds.jl`
    Updating `~/Workspace/Manifolds.jl/docs/Manifest.toml`
  [1cead3c2] ~ Manifolds v0.10.4 `..` ⇒ v0.10.4 `~/Workspace/Manifolds.jl`
  No Changes to `~/Workspace/Manifolds.jl/docs/Project.toml`
  No Changes to `~/Workspace/Manifolds.jl/docs/Manifest.toml`
[ Info: Precompiling ManifoldsRecipesBaseExt [37da849e-34ab-54fd-a5a4-b22599bd6cb0]
    CondaPkg Found dependencies: /home/simonj/Workspace/Manifolds.jl/docs/CondaPkg.toml
    CondaPkg Found dependencies: /home/simonj/.julia/packages/PythonCall/Nr75f/CondaPkg.toml
    CondaPkg Found dependencies: /home/simonj/.julia/packages/PythonPlot/469aA/CondaPkg.toml
    CondaPkg Dependencies already up to date
[ Info: Precompiling ManifoldsRecursiveArrayToolsExt [e29539f8-2f71-57d7-85ce-957fb5efd4f1]
[ Info: Precompiling ManifoldsOrdinaryDiffEqExt [95431769-3d64-508c-9576-79148c2c82de]
[ Info: Precompiling ManifoldsBoundaryValueDiffEqExt [eb713886-0b96-5c41-a09d-5e8967e02f7d]
[ Info: Precompiling ManifoldsNLsolveExt [b22937d9-c54a-59ec-962e-7448ad3eacb1]
[ Info: Precompiling ManifoldsOrdinaryDiffEqDiffEqCallbacksExt [f484757d-caa2-58a4-a95c-7868e5350983]
[ Info: SetupBuildDirectory: setting up build directory.
ERROR: LoadError: 'tutorials/getstarted.md' is not an existing page!
Stacktrace:
  [1] error(s::String)
    @ Base ./error.jl:35
  [2] walk_navpages(visible::Bool, title::String, src::String, children::Vector{Any}, parent::Documenter.NavNode, doc::Documenter.Document)
    @ Documenter ~/.julia/packages/Documenter/C1XEF/src/builder_pipeline.jl:185
  [3] walk_navpages
    @ ~/.julia/packages/Documenter/C1XEF/src/builder_pipeline.jl:200 [inlined]
  [4] walk_navpages
    @ ~/.julia/packages/Documenter/C1XEF/src/builder_pipeline.jl:202 [inlined]
  [5] #81
    @ ./none:0 [inlined]
  [6] iterate
    @ ./generator.jl:47 [inlined]
  [7] collect(itr::Base.Generator{Vector{Pair{String, String}}, Documenter.var"#81#82"{Documenter.NavNode, Documenter.Document}})
    @ Base ./array.jl:782
  [8] walk_navpages
    @ ~/.julia/packages/Documenter/C1XEF/src/builder_pipeline.jl:203 [inlined]
  [9] walk_navpages
    @ ~/.julia/packages/Documenter/C1XEF/src/builder_pipeline.jl:190 [inlined]
 [10] walk_navpages(title::String, children::Vector{Pair{String, String}}, parent::Nothing, doc::Documenter.Document)
    @ Documenter ~/.julia/packages/Documenter/C1XEF/src/builder_pipeline.jl:199
 [11] walk_navpages(p::Pair{String, Any}, parent::Nothing, doc::Documenter.Document)
    @ Documenter ~/.julia/packages/Documenter/C1XEF/src/builder_pipeline.jl:202
 [12] (::Documenter.var"#81#82"{Nothing, Documenter.Document})(p::Pair{String, Any})
    @ Documenter ./none:0
 [13] iterate
    @ ./generator.jl:47 [inlined]
 [14] collect_to!(dest::Vector{Documenter.NavNode}, itr::Base.Generator{Vector{Any}, Documenter.var"#81#82"{Nothing, Documenter.Document}}, offs::Int64, st::Int64)
    @ Base ./array.jl:840
 [15] collect_to_with_first!
    @ ./array.jl:818 [inlined]
 [16] collect(itr::Base.Generator{Vector{Any}, Documenter.var"#81#82"{Nothing, Documenter.Document}})
    @ Base ./array.jl:792
 [17] walk_navpages
    @ ~/.julia/packages/Documenter/C1XEF/src/builder_pipeline.jl:203 [inlined]
 [18] runner(#unused#::Type{Documenter.Builder.SetupBuildDirectory}, doc::Documenter.Document)
    @ Documenter ~/.julia/packages/Documenter/C1XEF/src/builder_pipeline.jl:133
 [19] dispatch(#unused#::Type{Documenter.Builder.DocumentPipeline}, x::Documenter.Document)
    @ Documenter.Selectors ~/.julia/packages/Documenter/C1XEF/src/utilities/Selectors.jl:170
 [20] #89
    @ ~/.julia/packages/Documenter/C1XEF/src/makedocs.jl:272 [inlined]
 [21] withenv(::Documenter.var"#89#91"{Documenter.Document}, ::Pair{String, Nothing}, ::Vararg{Pair{String, Nothing}})
    @ Base ./env.jl:197
 [22] #88
    @ ~/.julia/packages/Documenter/C1XEF/src/makedocs.jl:271 [inlined]
 [23] cd(f::Documenter.var"#88#90"{Documenter.Document}, dir::String)
    @ Base.Filesystem ./file.jl:112
 [24] #makedocs#87
    @ ~/.julia/packages/Documenter/C1XEF/src/makedocs.jl:271 [inlined]
 [25] top-level scope
    @ ~/Workspace/Manifolds.jl/docs/make.jl:95
 [26] include(fname::String)
    @ Base.MainInclude ./client.jl:478
 [27] top-level scope
    @ REPL[3]:1
in expression starting at /home/simonj/Workspace/Manifolds.jl/docs/make.jl:95

julia> 

[kellertuer]

Oh, sorry! I forgot about the (bit involved) Quarto examples. You either have to install Quarto (and add --quarto) to render them or pass --exclude-tutorials to the arguments, and yeah sorry that means for now it does not just work by include, since that both are command line args.

Sorry forgot about those nitty gritty details. And sure, for best of cases we should come up with something that “just works”, but for now rendering Quarto with pure Julia is still not yet there (and we would depend on that).
So for now you would have to call it from terminal then, sorry. (in your case as docs/make.jl --exclude-tutorials)

[sjacobsson]

No worries! Sorry for bothering you when you said you were busy, I just wanted to be able to see the docs so that I could work on them in the meantime.

I get the same error again with docs/make.jl --exclude-tutorials :(
When I look in make.jl there is an if "--quarto" ∈ ARGS, but I don't see any lines with --exclude-tutorials.

[kellertuer]

Ah, hm, yes. That comes from “I am busy but I can answer this in 2 minutes so let's just do it”-answers (mine, not yours).

What I wrote does hold for Manopt.jl, but not yet for Manifolds.jl (but then we should introduce this soon!) – so then, sorry, you would have to run first with --quarto to generate the files Documenter complains about and for that you have to install quarto,...
so maybe – you were right from the start – you can currently only render the docs with a bit of effort locally.
But since Manopt.jl has a solution for that I will transfer it here as soon as time permits.

[sjacobsson]

I got it to work now by commenting out lines 109-114 in make.jl:

            "🚀 Get Started with `Manifolds.jl`" => "tutorials/getstarted.md",
            "work in charts" => "tutorials/working-in-charts.md",
            "perform Hand gesture analysis" => "tutorials/hand-gestures.md",
            "integrate on manifolds and handle probability densities" => "tutorials/integration.md",
            "explore curvature without coordinates" => "tutorials/exploring-curvature.md",
            "work with groups" => "tutorials/groups.md",

Thanks for the help! Now I'll try to write a nice documentation page like the ones for the Stiefel etc :)

[kellertuer]

Ah, yeah, the ARG --exclude-tutorials in principle does the same thing ;) Great that you got it to work, just remember not to commit the commenting out.

I also got you started already on such a page, see docs/src/manifolds/segre.md, which automatically includes all docs from both files you write – but sure a bit more structure like in the Stiefel page or the SPD case are also nice to have, especially with a default metric and a second (the warped) one.

[sjacobsson]

I see the segre.md that you added, very nice! I also see some texing errors that I made along the way. Thanks!

[kellertuer]

HI,
it was a bit busy the last month on my side. How is your implementation going, anything I can help with?

[sjacobsson]

Hi Ronny,
For me it has been busy as well. Sorry for not getting back to you until now. I think there are lots of small things that I need help with, but right now the biggest thing is getting the tests to run. I have a bunch of tests that I wrote as a script, and if you have the time maybe you could help me get them into the test suite.

[kellertuer]

Sure, maybe not the next few days - busy finishing a semester, two talks and some travels - but turning the script into tests should not be too complicated.

You can also get inspired by what the other manifold have in tests. Otherwise I'll find some time next week

[kellertuer]

Oh I noticed that also the docs are not yet fully correct. I was too used to docs failing when doc strings are not included. But here we have switched that to just warning (instead of errors). Will work through that and fix the docs somewhen the next days.
You can give it a try as well – with the current version on master (finally!) running docs/make.jl should “just work” also if you do not have (or intend to run) quarto.

[sjacobsson]

Hi, I have a couple of thoughts:

• The dys could maybe be code golfed a bit. It was generated as [rand(4 * manifold_dimension(M)) for M in Ms] but really it just needs to be a list of vectors of the correct length that are generic enough, so not ones(4 * manifold_dimension(M)) but anything that is more "random" than that would be fine.
• Equality of points on the Segre isn't p == q but rather embed(M, p) == embed(M, q), but embedding is super expensive, so that's why I wrote the tests to use points that are close enough that p == q also holds. It's maybe not the nicest solution but that's how it's implemented right now.
• I didn't manage to compile the docs, but it's a me problem. I get some error message like "LoadError: Unsatisfiable requirements detected for package ManifoldDiff"... I'll try to fix this so that I can compile the docs locally.

[kellertuer]

I carefully read the rendered docs and did a bit of formatting – and e.g. ordered the function definitions alphabetically in every file and such unifying things

Final remarks:

1. now both the sphere and the Segre manifold are denoted \mathcal S in their docs – the sphere in Segre is just S.
2. usually tangent vectors are denoted X and Y in variables and docs, here it is u and v
3. simiarily coordinates are denoted c (sometimes d)

Would it maybe be ok to unify / adapt this a bit? That would be nice.

4. if we could do a 10-line variant of dys as well that would be great. Could I do the same with them as with the us? There I shortened all numbers to one digit after the . and if it were too many, the remaining ones are zeros. Would that be valid for the dys as well?
5. there is 5 new lines not yet covered by tests – it would be great (though CI is fine with the current coverage) – if those could be covered as well :)

[sjacobsson]

1. The Sphere is denoted \mathbb{S}? Sure, this could be updated in the Segre docs. If you still want to free up \mathcal{S}, we can use \mathrm{Seg} for the Segre.

2. and 3. sounds good, I can change it tomorrow :)

_4. maybe we can do something like [0.1, 0.3, -0.1, zeros(5)..., -0.5, 0.7, zeros(3)...]? The problem with only right-padding with zeros is that that corresponds to some of the control points never moving. But the actual vector dy can in principle be anything (I don't know why it only has positive entries atm...), the digits of pi, pi^2, ... or something like that but then at that point maybe we're just reimplementing the wheel.

_5. I'll go through the warnings tomorrow :)

[kellertuer]

1. you are right, the sphere is $\mathbb S^d$, where $d$ is the dimension of the sphere. If you could unify that, that would be great! No, we do not have to free up $\mathcal S$ then, I misread my own latex code, so we can keep that for Segre – or any other (not yet used) Symbol you like.
2. thanks :)
3. thanks :)
4. sure, anything that we can fit in a line is fine with me. I just want to avoid several hundred lines of numbers.
5. nice!

[sjacobsson]

I managed to compile the docs, I was just trying to do it with the wrong julia version earlier (:

Now I just have to complete the code coverage and then I think I'm happy.

[kellertuer]

The commit looks very nice! Yes, once I see the new code cov – I can already approve this PR. nice work!

[sjacobsson]

Thanks, happy to be able to contribute!

[sjacobsson]

Sorry, did we lose the codecov check?

[kellertuer]

No, it is only triggered when I push, not when you do. The simple reason is, that external contributors might otherwise “inject” code. I will do a small commit soon :)

[kellertuer]

Interesting, now your last commit has coverage, mine does not ;) Maybe it is bit delayed.

[sjacobsson]

I'll make another commit. For good measure :).

[kellertuer]

Nice! Even with full code coverage. This is done. I can merge it and register a new version tomorrow.

[sjacobsson]

Okay perfect! Thanks for helping me getting it merge-ready. The next manifold will be a breeze :)

[sjacobsson]

And happy holidays!

[kellertuer]

Same to you; god jul / frohe Weihnachten! 🎄