Opened 5 months ago

Closed 5 months ago

Last modified 3 months ago

#33221 closed enhancement (fixed)

Add example of projective space to manifold catalog

Reported by: tkarn Owned by:
Priority: major Milestone: sage-9.6
Component: manifolds Keywords: projective, manifolds, catalog
Cc: tscrim, tkarn, gh-mjungmath Merged in:
Authors: Trevor K. Karn Reviewers: Travis Scrimshaw, Eric Gourgoulhon
Report Upstream: N/A Work issues:
Branch: 51f9419 (Commits, GitHub, GitLab) Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by gh-mjungmath)

Add projective space and charts in the manifold catalog.

See #31249.

Change History (32)

comment:1 Changed 5 months ago by tkarn

  • Branch set to u/tkarn/33221-projective-space
  • Commit set to af760c9816025fa956b7d9499414d95d22bec6ab

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


New commits:

af760c9Add projective space

comment:2 Changed 5 months ago by gh-mjungmath

  • Description modified (diff)

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

comment:3 Changed 5 months ago by git

  • Commit changed from af760c9816025fa956b7d9499414d95d22bec6ab to 38f2be27a0e38d1d7d54095df5d2965d5b49efa7

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

38f2be2Fix inverse map and tests, and restrict to real projective space

comment:4 Changed 5 months ago by git

  • Commit changed from 38f2be27a0e38d1d7d54095df5d2965d5b49efa7 to bdc04413a6874f942feeded5e5da598f6e70c5d1

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

bdc0441Fix PEP8 compliance

comment:5 Changed 5 months ago by tkarn

  • Status changed from new to needs_review

comment:6 follow-ups: Changed 5 months ago by tscrim

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?

comment:7 in reply to: ↑ 6 Changed 5 months ago by tkarn

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"

comment:8 Changed 5 months ago by git

  • Commit changed from bdc04413a6874f942feeded5e5da598f6e70c5d1 to 11cf10d18c07c5701c7c4a1d2849795ba1d56298

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

11cf10dAdd reviewer suggestions

comment:9 in reply to: ↑ 6 ; follow-up: Changed 5 months ago by tkarn

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.

comment:10 in reply to: ↑ 9 Changed 5 months ago by egourgoulhon

Replying to tkarn:

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.

comment:11 follow-up: Changed 5 months ago by 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.

comment:12 in reply to: ↑ 11 Changed 5 months ago by tkarn

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.

comment:13 follow-up: Changed 5 months ago by tscrim

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}.

comment:14 Changed 5 months ago by git

  • Commit changed from 11cf10d18c07c5701c7c4a1d2849795ba1d56298 to e5be551cb6e4e397fcc9b90949c17dbf7829d692

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

7b6dbf0Make reviewer changes except for optimizing chart creation
e5be551Optimize creation of open subsets and charts into a single loop

comment:15 in reply to: ↑ 13 Changed 5 months ago by tkarn

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

comment:16 follow-up: Changed 5 months ago by tscrim

  • Reviewers set to Travis Scrimshaw

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?

comment:17 Changed 5 months ago by git

  • Commit changed from e5be551cb6e4e397fcc9b90949c17dbf7829d692 to f18aa29c952e2e40c875b7c3cf541b612d6ada51

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

f18aa29Fix documentation and field import

comment:18 in reply to: ↑ 16 ; follow-up: Changed 5 months ago by 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``.

comment:19 in reply to: ↑ 18 ; follow-up: Changed 5 months ago by tkarn

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?

comment:20 in reply to: ↑ 19 ; follow-up: Changed 5 months ago by egourgoulhon

Replying to tkarn:

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?

Maybe; 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.

Version 0, edited 5 months ago by egourgoulhon (next)

comment:21 Changed 5 months ago by git

  • Commit changed from f18aa29c952e2e40c875b7c3cf541b612d6ada51 to 0eb3b733173f2d1c6f09689d897eec0efdbd3344

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

0eb3b73Fix documentation by adding space

comment:22 in reply to: ↑ 20 Changed 5 months ago by tkarn

Replying to egourgoulhon:

Replying to tkarn:

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.

comment:23 follow-up: Changed 5 months ago by 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).

comment:24 Changed 5 months ago by git

  • Commit changed from 0eb3b733173f2d1c6f09689d897eec0efdbd3344 to 316dd9ec8e561479c8f3cdfa014a74a084d2285c

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

316dd9e\Bold -> \RR

comment:25 in reply to: ↑ 23 Changed 5 months ago by tkarn

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!

comment:26 Changed 5 months ago by git

  • Commit changed from 316dd9ec8e561479c8f3cdfa014a74a084d2285c to 51f9419e13686805845b04d88b0a071bf32a8f08

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

51f9419Make RP bold

comment:27 Changed 5 months ago by tscrim

  • Reviewers changed from Travis Scrimshaw to Travis Scrimshaw, Eric Gourgoulhon
  • Status changed from needs_review to positive_review

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.

comment:28 Changed 5 months ago by vbraun

  • Branch changed from u/tkarn/33221-projective-space to 51f9419e13686805845b04d88b0a071bf32a8f08
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:29 follow-up: Changed 4 months ago by egourgoulhon

  • Commit 51f9419e13686805845b04d88b0a071bf32a8f08 deleted

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.

comment:30 in reply to: ↑ 29 Changed 3 months ago by egourgoulhon

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).

comment:31 follow-up: Changed 3 months ago by tkarn

comment:32 in reply to: ↑ 31 Changed 3 months ago by egourgoulhon

Replying to tkarn:

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

Thanks a lot!

Note: See TracTickets for help on using tickets.