🍩 #533

mateuszbaran · 2022-09-21T12:38:23Z

Some initial work on implementing a curved torus discussed in #532 .
Currently it's possible to draw a torus:

as well as do some basic conversions to/from an atlas.

@FedeClaudi you can take a look how get_chart_index is used to select a chart around a given point, get_parameters! and get_point! covert to and from parameters in a chart and get_vector_induced_basis! converts tangent vectors from a representation in $\mathbb{R}^3$ to an in-chart representation. Does that look somewhat understandable? Higher-genus tori would most likely have to implement the same functions, except the embedding can be skipped if explicit transition maps are provided. Currently by default transition maps are defined using an embedding but that's not required.

I've started work on a fully in-chart geodesic solver but I need to think about it a bit more, so that's not ready for review.

src/manifolds/GeneralizedTorus.jl

kellertuer · 2022-09-21T12:47:35Z

Just two first feedback points:

Should we do a new manifold? Its still the torus? I would switch the points/tangent vector types, since it is another representation (cg. Hyperbolic).
The example is nice, if it is a Pluto-Notebook we could even include that into the docs.

FedeClaudi · 2022-09-21T12:49:38Z

First of all, outstanding PR naming game, bravo 💯 !

That looks excellent. I had actually just started working on an atlas for the n-torus here: https://github.com/FedeClaudi/Manifolds.jl/blob/master/torus_wip.jl
I took a different approach and was trying to do something that wasn't dependent on the embedding. I tend to think of manifolds without them being embedded somewhere, but I think Manifolds.jl mostly deals with embedded manifolds right?

Anyway, I'll have a look at the code in a second, but it looks great. I wonder how it would generalize to torii of genus > 1 for which there's no nice parametrization?

Also, I will steal your visualization code if that's okay, that's such a pretty way to visualize surfaces!

codecov · 2022-09-21T13:25:44Z

Codecov Report

Merging #533 (22db56a) into master (c5b4990) will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #533      +/-   ##
==========================================
+ Coverage   98.93%   98.95%   +0.02%     
==========================================
  Files          96       99       +3     
  Lines        9278     9495     +217     
==========================================
+ Hits         9179     9396     +217     
  Misses         99       99

Impacted Files	Coverage Δ
src/Manifolds.jl	`100.00% <100.00%> (ø)`
src/atlases.jl	`94.87% <100.00%> (+1.22%)`	⬆️
src/differentiation/bvp.jl	`100.00% <100.00%> (ø)`
src/differentiation/ode_callback.jl	`100.00% <100.00%> (ø)`
src/manifolds/EmbeddedTorus.jl	`100.00% <100.00%> (ø)`

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

mateuszbaran · 2022-09-21T15:34:57Z

Should we do a new manifold? Its still the torus? I would switch the points/tangent vector types, since it is another representation (cg. Hyperbolic).

Good point, it's topologically the same as the current torus, just with a different metric and point/tangent vector representation.

2. The example is nice, if it is a Pluto-Notebook we could even include that into the docs.

Thanks, I'm going to add a geodesic curve to that picture, then I will ask for some help turning it into a notebook 🙂 . It would be cool to have sliders for selecting tangent vector for geodesic in the docs.

That looks excellent. I had actually just started working on an atlas for the n-torus here: https://github.com/FedeClaudi/Manifolds.jl/blob/master/torus_wip.jl
I took a different approach and was trying to do something that wasn't dependent on the embedding. I tend to think of manifolds without them being embedded somewhere, but I think Manifolds.jl mostly deals with embedded manifolds right?

Cool, that's also a good approach to making an atlas. Manifolds.jl currently focuses on working in an embedding, yes, but the API allows for working without it. Your get_parameters and get_point should work fine.

In this PR I will add a way to compute geodesics fully in charts. Maybe with chart switching if there is an easy way to do it in OrdinaryDiffEq.jl. Also maybe with sensitivity analysis for a gradient-descent-based logarithmic map.

I wonder how it would generalize to torii of genus > 1 for which there's no nice parametrization?

I did a quick search and maybe this would help: https://cs.brown.edu/people/jhughes/papers/Grimm-PNT-2003/paper.pdf ? We would still need to define a metric in some way though, in case of the torus in this PR I simply take the metric from the embedding.

Also, I will steal your visualization code if that's okay, that's such a pretty way to visualize surfaces!

Sure, I stole most of it from here: https://beautiful.makie.org/dev/examples/generated/2d/surfaces/gabriels_horn/ 😄 .

FedeClaudi · 2022-09-21T15:43:13Z

That's great, I'll have a look at that paper.

I'd be happy to help making it an interactive example, it'd be very useful and it would get me to try and understand better the code you're working on.

Looking forward to seeing what comes out of it!

kellertuer · 2022-09-21T15:51:12Z

Should we do a new manifold? Its still the torus? I would switch the points/tangent vector types, since it is another representation (cg. Hyperbolic).

Good point, it's topologically the same as the current torus, just with a different metric and point/tangent vector representation.

Oh also a different metric, interesting – then you could
a) use a metric manifold and still just arrays
b) use a metric manifold and point/tangent types

but I would avoid c) then (just use points/tangent types) because that might be mistaken of a different representation for the default metric we have.

The example is nice, if it is a Pluto-Notebook we could even include that into the docs.

Thanks, I'm going to add a geodesic curve to that picture, then I will ask for some help turning it into a notebook 🙂 . It would be cool to have sliders for selecting tangent vector for geodesic in the docs.

Sure, no problem, I can help turning that into a Pluto Notebook (and maybe even adding it to the docs, since we should start more tutorials I think).

FedeClaudi · 2022-09-21T16:19:21Z

since we should start more tutorials I think

As a new user: yep.

I really like Manifolds.jl and appreciate the work you're doing with it and this PR in particular. Let me know if I can help with anything!

mateuszbaran · 2022-09-23T11:14:40Z

Going between charts seems to work in the version I've just uploaded but most likely I have an error with Christoffel symbols for torus (geodesic shouldn't converge to a circle)

mateuszbaran · 2022-09-23T17:19:06Z

I've made a nice interactive visualization of geodesics in Makie:

mateuszbaran · 2022-09-23T17:24:50Z

To try this you just need to run plot_thing from the generalized_torus.jl file. Sliders set the initial conditions for the geodesic.

test/manifolds/generalized_torus.jl

kellertuer · 2022-09-24T09:34:57Z

Nice! That looks great!

mateuszbaran · 2022-09-24T13:12:15Z

test/manifolds/generalized_torus.jl

+    X = get_vector(M, p, X_p0x, B)
+    @test get_coordinates(M, p, X, B) ≈ X_p0x
+
+    @test_broken norm(X) ≈ norm(M, p0x, TFVector(X_p0x, B))


@kellertuer Could you check if this test actually makes sense? This is supposed to test that the vector X in embedding of the torus has the same norm as calculated using its coordinates in the induced basis and the metric tensor. I'm not sure where I could have a mistake.

Does this one error? If so – by how much?
Also just norm(X)is a little surprising, is that already in the embedding?

Yes, X is in the embedding and the difference is large:

Expression: norm(X) ≈ norm(M, p0x, TFVector(X_p0x, B)) Evaluated: 2.6832815729997477 ≈ 1.4422205101855958

I'd expect the embedding to be isometric because the metric is actually defined through that embedding.

yes then I would expect that as well. But for the next few days I do not have time to check the code too much I fear.

No problem, it can wait.

Ok, but I hope that in the best case the code works fine after you fixed that 🤓

I've found the errors but one of them looks like an issue with API. Currently local_metric expects point in the embedding while arguably it would make sense to pass there coordinate in a basis. It would be nice to be able to tell these two cases apart by method signatures. Do you have an idea for that?

Cool, that you found it

Which local_metricdo you refer to? The one on the sphere

Manifolds.jl/src/manifolds/Sphere.jl

Lines 285 to 291 in 71d9d81

@doc raw"""

local_metric(M::Sphere{n}, p, ::DefaultOrthonormalBasis)

return the local representation of the metric in a [`DefaultOrthonormalBasis`](https://juliamanifolds.github.io/ManifoldsBase.jl/stable/bases.html#ManifoldsBase.DefaultOrthonormalBasis), namely

the diagonal matrix of size ``n×n`` with ones on the diagonal, since the metric is obtained

from the embedding by restriction to the tangent space ``T_p\mathcal M`` at ``p``.

"""

Does what I would expect – the point p on the manifold (or would you prefer that in charts/coordinates?)

Because there G = local_metric(M, p,B) returns the metric as a matrix such that for tangent vectors given in coordinates with respect to B we have c1'*G*c2 which is what I would expect.

One could write one that is g = local_metric(M,p) such that X'*g*Y works for vectors (not coordinates).

So I am not sure what you mean by "point in a basis", since we do not have a basis on the manifold.

Sorry, it was a bit late -- I meant passing point as parameters in a chart. So probably something like local_metric(M::AbstractManifold, A::AbstractAtlas, i, a) for coefficients of the metric tensor at point with parametrization a in chart i from atlas A in basis induced by that chart at that point.

What I'd like to develop here a bit is working on a manifold without an embedding.

Ah, but then A solves your thing here :) I like the signature you propose.

What I also meant was, that there is a (though only subtle and small) difference between point-representation and embedding. But sure in charts is what you meant.

mateuszbaran · 2022-09-24T17:25:33Z

And now we also have parallel transport along geodesics and Gaussian curvature:

kellertuer

I think we should document the new torus together with the old torus on the same documentation page, what do you think?

src/manifolds/CurvedTorus.jl

test/manifolds/torus_example.jl

mateuszbaran · 2022-09-26T14:25:54Z

I think we should document the new torus together with the old torus on the same documentation page, what do you think?

Sure, I can more documentation there.

mateuszbaran · 2022-09-28T12:10:06Z

I've added computing log/distance/geodesic between two points by solving a BVP in a chart. So I would consider this PR feature-complete, and it just needs some docs and a bit of polishing. I'm quite happy that we have now a decent way to work in charts, and what we have here should nicely generalized to higher-genus tori.

FedeClaudi · 2022-09-28T13:01:13Z

looks great, looking forward to playing around with it!

mateuszbaran · 2022-09-29T17:58:08Z

@kellertuer I think this is ready for review. One major point is the new transition_map_diff and in-chart affine_connection, inner and norm, this is the beginning of the new API for working (fully) in charts. The new solvers in ode_callback.jl and bvp.jl are supposed to be somewhat internal and I would prefer to expose them in an API later, maybe after they are validated on higher genus tori.

kellertuer · 2022-09-29T18:12:04Z

Thanks for the summary, I will be travelling the weekend but try to find time to check stuff.

mateuszbaran · 2022-09-29T18:51:53Z

No problem, we can finish this next week 🙂 .

kellertuer

Really nice PR; I have a few comments, but nothing too serious.

Could you also check for code coverage a little bit? We are not far away from the required rate, just a few lines shy I think.

docs/Project.toml

docs/make.jl

docs/src/manifolds/torus.md

src/Manifolds.jl

src/atlases.jl

src/manifolds/CurvedTorus.jl

mateuszbaran · 2022-10-02T17:20:17Z

Really nice PR; I have a few comments, but nothing too serious.

Could you also check for code coverage a little bit? We are not far away from the required rate, just a few lines shy I think

Thanks, I will check the coverage after addressing your comments.

Co-authored-by: Ronny Bergmann <[email protected]>

mateuszbaran · 2022-10-03T13:17:31Z

I think I've resolved all issues 🙂 .

Curved torus

b8e6ddf

github-actions bot reviewed Sep 21, 2022

View reviewed changes

src/manifolds/GeneralizedTorus.jl Outdated Show resolved Hide resolved

formatting

82f5cbe

mateuszbaran added 3 commits September 21, 2022 21:15

general improvements

ad8a723

some work on tests and geodesics

252303b

partially working chart stitching

3216bce

mateuszbaran added 2 commits September 23, 2022 19:20

geodesic visualization

59914d3

small fix

5b74e60

github-actions bot reviewed Sep 23, 2022

View reviewed changes

test/manifolds/generalized_torus.jl Outdated Show resolved Hide resolved

mateuszbaran mentioned this pull request Sep 24, 2022

Change of basis JuliaManifolds/ManifoldsBase.jl#124

Merged

cleanup

251138d

mateuszbaran commented Sep 24, 2022

View reviewed changes

faster solver

15aeec3

mateuszbaran added 5 commits September 24, 2022 19:27

parallel transport

1811287

fix

9dfa11c

cleanup

8c9d961

move test down

831ee2f

improve test coverage

b393e12

mateuszbaran added the preview docs Add this label if you want to see a PR-preview of the documentation label Sep 26, 2022

polishing torus

e4dbaf6

kellertuer reviewed Sep 26, 2022

View reviewed changes

src/manifolds/CurvedTorus.jl Outdated Show resolved Hide resolved

test/manifolds/torus_example.jl Show resolved Hide resolved

mateuszbaran added 2 commits September 26, 2022 21:50

small restructuring

e17fa95

log/distance using BVP in a chart

e89ebb2

mateuszbaran added 3 commits September 28, 2022 19:35

Merge branch 'master' into mbaran/tori

10fc982

geodesic on torus in Makie

52ba424

add some docs

d2026cd

mateuszbaran marked this pull request as ready for review September 29, 2022 17:25

kellertuer reviewed Oct 1, 2022

View reviewed changes

mateuszbaran and others added 4 commits October 2, 2022 19:25

Apply suggestions from code review

e3469b9

Co-authored-by: Ronny Bergmann <[email protected]>

Addressing some review comments

d9fc798

addressing review

ddd9fb7

improve docs

8e811de

kellertuer approved these changes Oct 4, 2022

View reviewed changes

This was referenced Oct 5, 2022

📚 #534

Merged

Fix #539 #540

Merged

mateuszbaran added 3 commits October 5, 2022 19:08

Merge branch 'master' into mbaran/tori

36cfa25

bump version

17874c0

minor improvements

22db56a

mateuszbaran merged commit e3af13e into master Oct 6, 2022

kellertuer deleted the mbaran/tori branch May 4, 2024 17:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🍩 #533

🍩 #533

mateuszbaran commented Sep 21, 2022

kellertuer commented Sep 21, 2022

FedeClaudi commented Sep 21, 2022

codecov bot commented Sep 21, 2022 •

edited

Loading

mateuszbaran commented Sep 21, 2022

FedeClaudi commented Sep 21, 2022

kellertuer commented Sep 21, 2022

FedeClaudi commented Sep 21, 2022

mateuszbaran commented Sep 23, 2022

mateuszbaran commented Sep 23, 2022

mateuszbaran commented Sep 23, 2022

kellertuer commented Sep 24, 2022

mateuszbaran Sep 24, 2022

kellertuer Sep 24, 2022

mateuszbaran Sep 24, 2022

kellertuer Sep 24, 2022

mateuszbaran Sep 24, 2022

kellertuer Sep 24, 2022

mateuszbaran Sep 24, 2022

kellertuer Sep 25, 2022

mateuszbaran Sep 25, 2022

kellertuer Sep 25, 2022

mateuszbaran commented Sep 24, 2022

kellertuer left a comment

mateuszbaran commented Sep 26, 2022

mateuszbaran commented Sep 28, 2022

FedeClaudi commented Sep 28, 2022

mateuszbaran commented Sep 29, 2022

kellertuer commented Sep 29, 2022

mateuszbaran commented Sep 29, 2022

kellertuer left a comment

mateuszbaran commented Oct 2, 2022

mateuszbaran commented Oct 3, 2022

	@doc raw"""
	local_metric(M::Sphere{n}, p, ::DefaultOrthonormalBasis)

	return the local representation of the metric in a [`DefaultOrthonormalBasis`](https://juliamanifolds.github.io/ManifoldsBase.jl/stable/bases.html#ManifoldsBase.DefaultOrthonormalBasis), namely
	the diagonal matrix of size ``n×n`` with ones on the diagonal, since the metric is obtained
	from the embedding by restriction to the tangent space ``T_p\mathcal M`` at ``p``.
	"""

🍩 #533

🍩 #533

Conversation

mateuszbaran commented Sep 21, 2022

kellertuer commented Sep 21, 2022

FedeClaudi commented Sep 21, 2022

codecov bot commented Sep 21, 2022 • edited Loading

Codecov Report

mateuszbaran commented Sep 21, 2022

FedeClaudi commented Sep 21, 2022

kellertuer commented Sep 21, 2022

FedeClaudi commented Sep 21, 2022

mateuszbaran commented Sep 23, 2022

mateuszbaran commented Sep 23, 2022

mateuszbaran commented Sep 23, 2022

kellertuer commented Sep 24, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mateuszbaran commented Sep 24, 2022

kellertuer left a comment

Choose a reason for hiding this comment

mateuszbaran commented Sep 26, 2022

mateuszbaran commented Sep 28, 2022

FedeClaudi commented Sep 28, 2022

mateuszbaran commented Sep 29, 2022

kellertuer commented Sep 29, 2022

mateuszbaran commented Sep 29, 2022

kellertuer left a comment

Choose a reason for hiding this comment

mateuszbaran commented Oct 2, 2022

mateuszbaran commented Oct 3, 2022

codecov bot commented Sep 21, 2022 •

edited

Loading