Add example of projective space to manifold catalog

[trevorkarn]

Add projective space and charts in the manifold catalog.

See #31249.

CC: @tscrim @trevorkarn @mjungmath

Component: manifolds

Keywords: projective, catalog

Author: Trevor K. Karn

Branch: 51f9419

Reviewer: Travis Scrimshaw, Eric Gourgoulhon

Issue created by migration from https://trac.sagemath.org/ticket/33221

[trevorkarn]

Commit: af760c9

[trevorkarn]

comment:1

Still has an issue with the domain of transition maps causing one test to fail.

---

New commits:

af760c9

Add projective space

[trevorkarn]

Branch: u/tkarn/33221-projective-space

[mjungmath]

comment:2

For now this approach is all right. I am curious at which dimension this implementation becomes problematic in view of computational times.

[sagetrac-git]

Changed commit from af760c9 to 38f2be2

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. New commits:

38f2be2

Fix inverse map and tests, and restrict to real projective space

[sagetrac-git]

Changed commit from 38f2be2 to bdc0441

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. New commits:

bdc0441

Fix PEP8 compliance

[tscrim]

comment:6

Some trivial comments:

Why not use gens()?

Also, this should be split over 2 lines*

Ci, Cj = P.atlas()[i], P.atlas()[j]

I feel like the more standard latex would be to make it (blackboard?) bold. However, we should match what happens elsewhere, such as for Euclidean space and spheres. (This is more of a bikeshedding comment.) Should this also have a normal name being passed too?

[trevorkarn]

comment:7

Replying to @tscrim:

Some trivial comments:

Why not use gens()?

It appears that no such method exists for charts - am I missing something?

sage: P = manifolds.RealProjectiveSpace(3)
sage: A = P.atlas(); A
[Chart (U0, (x_1, x_2, x_3)),
 Chart (U1, (x_0, x_2, x_3)),
 Chart (U2, (x_0, x_1, x_3)),
 Chart (U3, (x_0, x_1, x_2)),
 Chart (U0_inter_U1, (x_1, x_2, x_3)),
 Chart (U0_inter_U1, (x_0, x_2, x_3)),
 Chart (U0_inter_U2, (x_1, x_2, x_3)),
 Chart (U0_inter_U2, (x_0, x_1, x_3)),
 Chart (U0_inter_U3, (x_1, x_2, x_3)),
 Chart (U0_inter_U3, (x_0, x_1, x_2)),
 Chart (U1_inter_U2, (x_0, x_2, x_3)),
 Chart (U1_inter_U2, (x_0, x_1, x_3)),
 Chart (U1_inter_U3, (x_0, x_2, x_3)),
 Chart (U1_inter_U3, (x_0, x_1, x_2)),
 Chart (U2_inter_U3, (x_0, x_1, x_3)),
 Chart (U2_inter_U3, (x_0, x_1, x_2))]
sage: A[0].gens()
---------------------------------------------------------------------------
AttributeError                            Traceback (most recent call last)
<ipython-input-3-eff7e167b774> in <module>
----> 1 A[Integer(0)].gens()

AttributeError: 'RealChart' object has no attribute 'gens'

Should this also have a normal name being passed too?

I was thinking that it has the normal name "P", unless I misunderstand what you meant by "normal name"

[sagetrac-git]

Changed commit from bdc0441 to 11cf10d

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. New commits:

11cf10d

Add reviewer suggestions

[trevorkarn]

comment:9

Replying to @tscrim:

I feel like the more standard latex would be to make it (blackboard?) bold. However, we should match what happens elsewhere, such as for Euclidean space and spheres.

Spheres do appear to use \mathbb (see https://sagemanifolds.obspm.fr/examples/pdf/SM_sphere_S2.pdf) so I adjusted accordingly, reflected in most recent commit.

[egourgoulhon]

comment:10

Replying to @trevorkarn:

Replying to @tscrim:

I feel like the more standard latex would be to make it (blackboard?) bold. However, we should match what happens elsewhere, such as for Euclidean space and spheres.

Spheres do appear to use \mathbb (see https://sagemanifolds.obspm.fr/examples/pdf/SM_sphere_S2.pdf) so I adjusted accordingly, reflected in most recent commit.

Indeed, at the moment, we have

sage: latex(EuclideanSpace(2))
\mathbb{E}^{2}
sage: latex(manifolds.Sphere(3))
\mathbb{S}^{3}

So please use \mathbb{P} for projective spaces as well.

PS: there is some inconsistency with the above in

sage: latex(manifolds.RealLine())
\Bold{R}

This should probably be fixed/discussed in another ticket. I guess this was to be consistent with

sage: latex(RR)
\Bold{R}

but this may not be necessary, because after all RR and manifolds.RealLine() are two different beasts and having a distinction in the LaTeX output might not be a bad thing. Also in the long term, IMHO we should get rid of this \Bold extra command, which unnecessarily pollutes the Jupyter notebooks.

[egourgoulhon]

comment:11

BTW, thanks for having opened this ticket! I've added it to the metaticket #30525. Feel free to perform such an addition yourself next time you are opening a ticket connected to manifolds. It helps to keep track of what's going on and to prepare the release tour.

[trevorkarn]

comment:12

Replying to @egourgoulhon:

BTW, thanks for having opened this ticket! I've added it to the metaticket #30525. Feel free to perform such an addition yourself next time you are opening a ticket connected to manifolds. It helps to keep track of what's going on and to prepare the release tour.

Will do! And thanks for the advice on latex representation.

[tscrim]

comment:13

Ah right, that was done as a workaround to get the X.<a,b> = M.chart() preparser syntax working. You should just use X[:] instead (which is what _first_ngens() does).

For the standard name, I meant pass a name="RP{}".format(dim). However, as you said, it is already implicitly there with the unnamed "P" input. I had missed that, sorry. For consistency, I think it should follow the latex name.

You also need {} for the latex in case the dimension is at least 10, so latex_name=r"\mathbb{{RP}}^``".format(dim).

I would use a nested for loop instead of combinations:

A = P.atlas()
for i in range(dim+1):
    Ci = A[i]
    gi = Ci[:]
    for j in range(i,dim+1):
        Cj = A[j]
        gj = Cj[:]

and so on. Although being slightly clever, you can combine this with the previous for loop for creating the charts (this might not be the best option in terms of code presentation; only in terms of optimization). I also might not rely on atlas being in a particular order either and instead just store the charts you make in a list you make.

It will also be faster if you explicitly set the inverse rather than having Sage compute it (and you can skip the checks too in the final version).

It might be good to also give a latex name to the subsets of U_{i}.

[sagetrac-git]

Changed commit from 11cf10d to e5be551

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. New commits:

7b6dbf0	Make reviewer changes except for optimizing chart creation
e5be551	Optimize creation of open subsets and charts into a single loop

[trevorkarn]

comment:15

Replying to @tscrim:
Thanks for all the suggestions! They are incorporated in the latest commit.

[tscrim]

Reviewer: Travis Scrimshaw

[tscrim]

comment:16

Other than the fact that this is using the real numbers, making the documentation incorrect (there is no ``field``), I have no further comments. Anyone else?

[sagetrac-git]

Changed commit from e5be551 to f18aa29

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. New commits:

f18aa29

Fix documentation and field import

[egourgoulhon]

comment:18

Replying to @tscrim:

I have no further comments. Anyone else?

A minor typo in the documentation, which prevents the display of dim:

-    - ``P`` -- the projective space `RP^d` where `d =```dim``.
+    - ``P`` -- the projective space `RP^d` where `d =``dim``.

[trevorkarn]

comment:19

Replying to @egourgoulhon:

Replying to @tscrim:

I have no further comments. Anyone else?

A minor typo in the documentation, which prevents the display of dim:

-    - ``P`` -- the projective space `RP^d` where `d =```dim``.
+    - ``P`` -- the projective space `RP^d` where `d =``dim``.

Thanks for the catch. I am hoping that d= is in math mode and dim is in fixed-width font. Will adding a space between = and dim

`d=` ``dim``

work for that?

[egourgoulhon]

comment:20

Replying to @trevorkarn:

Thanks for the catch. I am hoping that d= is in math mode and dim is in fixed-width font.

Ah yes, I see.

Will adding a space between = and dim

`d=` ``dim``

work for that?

I would say yes, but Sphinx is sometimes touchy. You can try by yourself: implement the change and run

sage -b
sage -docbuild reference/manifolds html

This is faster than running make doc.

[sagetrac-git]

Changed commit from f18aa29 to 0eb3b73

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. New commits:

0eb3b73

Fix documentation by adding space

[trevorkarn]

comment:22

Replying to @egourgoulhon:

Replying to @trevorkarn:

Thanks for the catch. I am hoping that d= is in math mode and dim is in fixed-width font.

Ah yes, I see.

Will adding a space between = and dim

`d=` ``dim``

work for that?

I would say yes, but Sphinx is sometimes touchy. You can try by yourself: implement the change and run

sage -b
sage -docbuild reference/manifolds html

This is faster than running make doc.

Adding the space does work, and is reflected in the most recent commit. Thanks for the tip on doc building.

[tscrim]

comment:23

Instead of using \Bold{R}, you can just use the Sage-specific macro \RR (good for uniformity and easy if we decide to change our minds).

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. New commits:

316dd9e

\Bold -> \RR

[sagetrac-git]

Changed commit from 0eb3b73 to 316dd9e

[trevorkarn]

comment:25

Replying to @tscrim:

Instead of using \Bold{R}, you can just use the Sage-specific macro \RR (good for uniformity and easy if we decide to change our minds).

Fixed! Thanks!

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. New commits:

51f9419

Make RP bold

[sagetrac-git]

Changed commit from 316dd9e to 51f9419

[tscrim]

Changed reviewer from Travis Scrimshaw to Travis Scrimshaw, Eric Gourgoulhon

[tscrim]

comment:27

In regards to comment:2, this seems to be reasonably fast up to dimension 10 (I didn't check further):

sage: P4 = manifolds.RealProjectiveSpace(4)
sage: %time P = manifolds.RealProjectiveSpace(3)
CPU times: user 448 ms, sys: 16 ms, total: 464 ms
Wall time: 387 ms
sage: %time P = manifolds.RealProjectiveSpace(4)
CPU times: user 888 ms, sys: 24 ms, total: 912 ms
Wall time: 778 ms
sage: %time P = manifolds.RealProjectiveSpace(5)
CPU times: user 1.53 s, sys: 44 ms, total: 1.57 s
Wall time: 1.35 s
sage: %time P = manifolds.RealProjectiveSpace(6)
CPU times: user 2.47 s, sys: 56 ms, total: 2.53 s
Wall time: 2.17 s
sage: %time P = manifolds.RealProjectiveSpace(7)
CPU times: user 3.81 s, sys: 112 ms, total: 3.92 s
Wall time: 3.38 s
sage: %time P = manifolds.RealProjectiveSpace(8)
CPU times: user 5.44 s, sys: 128 ms, total: 5.56 s
Wall time: 4.78 s
sage: %time P = manifolds.RealProjectiveSpace(9)
CPU times: user 7.54 s, sys: 168 ms, total: 7.71 s
Wall time: 6.6 s
sage: %time P = manifolds.RealProjectiveSpace(10)
CPU times: user 10.2 s, sys: 224 ms, total: 10.4 s
Wall time: 8.95 s

This will be a good placeholder until projective space has its own class.

[vbraun]

Changed branch from u/tkarn/33221-projective-space to 51f9419

[egourgoulhon]

Changed commit from 51f9419 to none

[egourgoulhon]

comment:29

Now that the ticket branch has been merged in Sage 9.6.beta1, there remains a last thing to do: preparing some examples of use for the Sage 9.6 release tour, on the same footing as what has been done for Sage 9.5:
https://wiki.sagemath.org/ReleaseTours/sage-9.5#Manifolds.

[egourgoulhon]

comment:30

Replying to @egourgoulhon:

Now that the ticket branch has been merged in Sage 9.6.beta1, there remains a last thing to do: preparing some examples of use for the Sage 9.6 release tour, on the same footing as what has been done for Sage 9.5:
https://wiki.sagemath.org/ReleaseTours/sage-9.5#Manifolds.

Ping (since the release of Sage 9.6 seems to be close).

[trevorkarn]

comment:31

Thanks for the ping. Just took care of it! https://wiki.sagemath.org/ReleaseTours/sage-9.6#Projective_spaces

[egourgoulhon]

comment:32

Replying to @trevorkarn:

Thanks for the ping. Just took care of it! https://wiki.sagemath.org/ReleaseTours/sage-9.6#Projective_spaces

Thanks a lot!