#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: |
Description (last modified by )
Add projective space and charts in the manifold catalog.
See #31249.
Change History (32)
comment:1 Changed 4 months ago by
- Branch set to u/tkarn/33221-projective-space
- Commit set to af760c9816025fa956b7d9499414d95d22bec6ab
comment:2 Changed 4 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 4 months ago by
- Commit changed from af760c9816025fa956b7d9499414d95d22bec6ab to 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 4 months ago by
- Commit changed from 38f2be27a0e38d1d7d54095df5d2965d5b49efa7 to bdc04413a6874f942feeded5e5da598f6e70c5d1
Branch pushed to git repo; I updated commit sha1. New commits:
bdc0441 | Fix PEP8 compliance
|
comment:5 Changed 4 months ago by
- Status changed from new to needs_review
comment:6 follow-ups: ↓ 7 ↓ 9 Changed 4 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 in reply to: ↑ 6 Changed 4 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) <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 4 months ago by
- Commit changed from bdc04413a6874f942feeded5e5da598f6e70c5d1 to 11cf10d18c07c5701c7c4a1d2849795ba1d56298
Branch pushed to git repo; I updated commit sha1. New commits:
11cf10d | Add reviewer suggestions
|
comment:9 in reply to: ↑ 6 ; follow-up: ↓ 10 Changed 4 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 in reply to: ↑ 9 Changed 4 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 follow-up: ↓ 12 Changed 4 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 in reply to: ↑ 11 Changed 4 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 follow-up: ↓ 15 Changed 4 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 4 months ago by
- Commit changed from 11cf10d18c07c5701c7c4a1d2849795ba1d56298 to e5be551cb6e4e397fcc9b90949c17dbf7829d692
comment:15 in reply to: ↑ 13 Changed 4 months ago by
Replying to tscrim: Thanks for all the suggestions! They are incorporated in the latest commit.
comment:16 follow-up: ↓ 18 Changed 4 months ago by
- 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 4 months ago by
- Commit changed from e5be551cb6e4e397fcc9b90949c17dbf7829d692 to f18aa29c952e2e40c875b7c3cf541b612d6ada51
Branch pushed to git repo; I updated commit sha1. New commits:
f18aa29 | Fix documentation and field import
|
comment:18 in reply to: ↑ 16 ; follow-up: ↓ 19 Changed 4 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 in reply to: ↑ 18 ; follow-up: ↓ 20 Changed 4 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 fixed-width font. Will adding a space between =
and dim
`d=` ``dim``
work for that?
comment:20 in reply to: ↑ 19 ; follow-up: ↓ 22 Changed 4 months ago by
Replying to tkarn:
Thanks for the catch. I am hoping that
d=
is in math mode anddim
is in fixed-width 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 4 months ago by
- Commit changed from f18aa29c952e2e40c875b7c3cf541b612d6ada51 to 0eb3b733173f2d1c6f09689d897eec0efdbd3344
Branch pushed to git repo; I updated commit sha1. New commits:
0eb3b73 | Fix documentation by adding space
|
comment:22 in reply to: ↑ 20 Changed 4 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 fixed-width 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 follow-up: ↓ 25 Changed 4 months ago by
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 4 months ago by
- 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 4 months ago by
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 4 months ago by
- Commit changed from 316dd9ec8e561479c8f3cdfa014a74a084d2285c to 51f9419e13686805845b04d88b0a071bf32a8f08
Branch pushed to git repo; I updated commit sha1. New commits:
51f9419 | Make RP bold
|
comment:27 Changed 4 months ago by
- 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 3 months ago by
- 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: ↓ 30 Changed 3 months ago by
- 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 8 weeks 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/sage-9.5#Manifolds.
Ping (since the release of Sage 9.6 seems to be close).
comment:31 follow-up: ↓ 32 Changed 8 weeks ago by
Thanks for the ping. Just took care of it! https://wiki.sagemath.org/ReleaseTours/sage-9.6#Projective_spaces
comment:32 in reply to: ↑ 31 Changed 8 weeks ago by
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!
Still has an issue with the domain of transition maps causing one test to fail.
New commits:
Add projective space