#33221 closed enhancement (fixed)
Add example of projective space to manifold catalog
Reported by:  Trevor Karn  Owned by:  

Priority:  major  Milestone:  sage9.6 
Component:  manifolds  Keywords:  projective, manifolds, catalog 
Cc:  Travis Scrimshaw, Trevor Karn, Michael Jung  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: 
Description (last modified by )
Add projective space and charts in the manifold catalog.
See #31249.
Change History (32)
comment:1 Changed 10 months ago by
Branch:  → u/tkarn/33221projectivespace 

Commit:  → af760c9816025fa956b7d9499414d95d22bec6ab 
comment:2 Changed 10 months ago by
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 10 months ago by
Commit:  af760c9816025fa956b7d9499414d95d22bec6ab → 38f2be27a0e38d1d7d54095df5d2965d5b49efa7 

Branch pushed to git repo; I updated commit sha1. New commits:
38f2be2  Fix inverse map and tests, and restrict to real projective space

comment:4 Changed 10 months ago by
Commit:  38f2be27a0e38d1d7d54095df5d2965d5b49efa7 → bdc04413a6874f942feeded5e5da598f6e70c5d1 

Branch pushed to git repo; I updated commit sha1. New commits:
bdc0441  Fix PEP8 compliance

comment:5 Changed 10 months ago by
Status:  new → needs_review 

comment:6 followups: 7 9 Changed 10 months ago by
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 Changed 10 months ago by
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) <ipythoninput3eff7e167b774> 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 10 months ago by
Commit:  bdc04413a6874f942feeded5e5da598f6e70c5d1 → 11cf10d18c07c5701c7c4a1d2849795ba1d56298 

Branch pushed to git repo; I updated commit sha1. New commits:
11cf10d  Add reviewer suggestions

comment:9 followup: 10 Changed 10 months ago by
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 Changed 10 months ago by
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 followup: 12 Changed 10 months ago by
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 Changed 10 months ago by
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 followup: 15 Changed 10 months ago by
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 10 months ago by
Commit:  11cf10d18c07c5701c7c4a1d2849795ba1d56298 → e5be551cb6e4e397fcc9b90949c17dbf7829d692 

comment:15 Changed 10 months ago by
Replying to tscrim: Thanks for all the suggestions! They are incorporated in the latest commit.
comment:16 followup: 18 Changed 10 months ago by
Reviewers:  → 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 10 months ago by
Commit:  e5be551cb6e4e397fcc9b90949c17dbf7829d692 → f18aa29c952e2e40c875b7c3cf541b612d6ada51 

Branch pushed to git repo; I updated commit sha1. New commits:
f18aa29  Fix documentation and field import

comment:18 followup: 19 Changed 10 months ago by
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 followup: 20 Changed 10 months ago by
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 fixedwidth font. Will adding a space between =
and dim
`d=` ``dim``
work for that?
comment:20 followup: 22 Changed 10 months ago by
Replying to tkarn:
Thanks for the catch. I am hoping that
d=
is in math mode anddim
is in fixedwidth font.
Ah yes, I see.
Will adding a space between
=
anddim
`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
.
comment:21 Changed 10 months ago by
Commit:  f18aa29c952e2e40c875b7c3cf541b612d6ada51 → 0eb3b733173f2d1c6f09689d897eec0efdbd3344 

Branch pushed to git repo; I updated commit sha1. New commits:
0eb3b73  Fix documentation by adding space

comment:22 Changed 10 months ago by
Replying to egourgoulhon:
Replying to tkarn:
Thanks for the catch. I am hoping that
d=
is in math mode anddim
is in fixedwidth font.Ah yes, I see.
Will adding a space between
=
anddim
`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 htmlThis 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 followup: 25 Changed 10 months ago by
Instead of using \Bold{R}
, you can just use the Sagespecific macro \RR
(good for uniformity and easy if we decide to change our minds).
comment:24 Changed 10 months ago by
Commit:  0eb3b733173f2d1c6f09689d897eec0efdbd3344 → 316dd9ec8e561479c8f3cdfa014a74a084d2285c 

Branch pushed to git repo; I updated commit sha1. New commits:
316dd9e  \Bold > \RR

comment:25 Changed 10 months ago by
Replying to tscrim:
Instead of using
\Bold{R}
, you can just use the Sagespecific macro\RR
(good for uniformity and easy if we decide to change our minds).
Fixed! Thanks!
comment:26 Changed 10 months ago by
Commit:  316dd9ec8e561479c8f3cdfa014a74a084d2285c → 51f9419e13686805845b04d88b0a071bf32a8f08 

Branch pushed to git repo; I updated commit sha1. New commits:
51f9419  Make RP bold

comment:27 Changed 10 months ago by
Reviewers:  Travis Scrimshaw → Travis Scrimshaw, Eric Gourgoulhon 

Status:  needs_review → 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 10 months ago by
Branch:  u/tkarn/33221projectivespace → 51f9419e13686805845b04d88b0a071bf32a8f08 

Resolution:  → fixed 
Status:  positive_review → closed 
comment:29 followup: 30 Changed 10 months ago by
Commit:  51f9419e13686805845b04d88b0a071bf32a8f08 

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/sage9.5#Manifolds.
comment:30 Changed 8 months ago by
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/sage9.5#Manifolds.
Ping (since the release of Sage 9.6 seems to be close).
comment:31 followup: 32 Changed 8 months ago by
Thanks for the ping. Just took care of it! https://wiki.sagemath.org/ReleaseTours/sage9.6#Projective_spaces
comment:32 Changed 8 months ago by
Replying to tkarn:
Thanks for the ping. Just took care of it! https://wiki.sagemath.org/ReleaseTours/sage9.6#Projective_spaces
Thanks a lot!
Still has an issue with the domain of transition maps causing one test to fail.
New commits:
Add projective space