Opened 8 weeks ago
Last modified 7 weeks ago
#31680 positive_review enhancement
Poset of manifold subsets
Reported by:  mkoeppe  Owned by:  

Priority:  major  Milestone:  sage9.4 
Component:  manifolds  Keywords:  
Cc:  ghmjungmath, egourgoulhon, tscrim  Merged in:  
Authors:  Matthias Koeppe  Reviewers:  Eric Gourgoulhon 
Report Upstream:  N/A  Work issues:  
Branch:  u/mkoeppe/poset_of_manifold_subsets (Commits, GitHub, GitLab)  Commit:  84896c4d1ec02b22bed05d2f5dd188d71ee0767d 
Dependencies:  #31681, #31717  Stopgaps: 
Description
The declared subsets or supersets of a ManifoldSubset
form finite posets. We add methods to expose them as instances of FinitePoset
.
Change History (43)
comment:1 Changed 8 weeks ago by
comment:2 Changed 8 weeks ago by
Yes, hopefully this new feature can be used as a debugging tool
comment:3 Changed 8 weeks ago by
 Branch set to u/mkoeppe/poset_of_manifold_subsets
comment:4 followup: ↓ 11 Changed 8 weeks ago by
 Commit set to 1b5c153e6c973c6fe0b2040dc2af2f12960b8716
Here's a first step: Building a digraph from the defined subsets.
(Plotting needs dot2tex
and looks bad because of the long labels. I don't know if it is possible to replace the labels by the latex names)
New commits:
1b5c153  ManifoldSubset.subset_digraph: New

comment:5 Changed 8 weeks ago by
 Commit changed from 1b5c153e6c973c6fe0b2040dc2af2f12960b8716 to 91bbc3ecb2b08dd3ffbb0ef1f3c2297de6b33816
Branch pushed to git repo; I updated commit sha1. New commits:
91bbc3e  ManifoldSubset.subset_poset: New

comment:6 Changed 8 weeks ago by
 Commit changed from 91bbc3ecb2b08dd3ffbb0ef1f3c2297de6b33816 to 858c82de2f280be41c6aa6c3928290c30c97f240
Branch pushed to git repo; I updated commit sha1. New commits:
858c82d  Fixup

comment:7 Changed 8 weeks ago by
 Dependencies set to #31681
comment:8 Changed 8 weeks ago by
 Commit changed from 858c82de2f280be41c6aa6c3928290c30c97f240 to a1e21b0b4687f43f4f19b2c4ddac8e1e9493ae81
comment:9 Changed 8 weeks ago by
comment:10 Changed 8 weeks ago by
 Commit changed from a1e21b0b4687f43f4f19b2c4ddac8e1e9493ae81 to 2e2d91ebc69117b7a49e1f099f83993bfd118db8
Branch pushed to git repo; I updated commit sha1. New commits:
2e2d91e  ManifoldSubset.{sub,super}set_{digraph,poset}: More options

comment:11 in reply to: ↑ 4 Changed 8 weeks ago by
Replying to mkoeppe:
Here's a first step: Building a digraph from the defined subsets.
I'm happy to see this happening! At the beginning of the SageManifolds project, it was pretty clear that graphs would be the proper way to deal with subsets, but I was not familiar with this part of Sage and wanted to move fast...
(Plotting needs
dot2tex
and looks bad because of the long labels. I don't know if it is possible to replace the labels by the latex names)
This would be nice, indeed. Playing with the doctest examples,
sage: view(latex(D)) sage: view(latex(P))
yields nice figures, with the latex names.
comment:12 followup: ↓ 13 Changed 8 weeks ago by
This digraph approach might also be useful to improve restrictions of tensor fields and make them even faster, no?
comment:13 in reply to: ↑ 12 ; followup: ↓ 14 Changed 8 weeks ago by
Replying to ghmjungmath:
This digraph approach might also be useful to improve restrictions of tensor fields and make them even faster, no?
Yes, I think there are many more digraph / poset structures in sage.manifolds that could be exposed by similar methods in followup tickets.
On this ticket I am not making an attempt to actually change any of the internal representations of these structures. Just providing some tools that give a highlevel view on the existing structures.
comment:14 in reply to: ↑ 13 Changed 8 weeks ago by
Replying to mkoeppe:
Replying to ghmjungmath:
This digraph approach might also be useful to improve restrictions of tensor fields and make them even faster, no?
Yes, I think there are many more digraph / poset structures in sage.manifolds that could be exposed by similar methods in followup tickets.
On this ticket I am not making an attempt to actually change any of the internal representations of these structures. Just providing some tools that give a highlevel view on the existing structures.
+1
Besides, I really like the way you write code. It's clear and neat!
comment:15 Changed 8 weeks ago by
 Commit changed from 2e2d91ebc69117b7a49e1f099f83993bfd118db8 to 02a82f90ccf15540fa8518e3591485092845d4d3
Branch pushed to git repo; I updated commit sha1. New commits:
02a82f9  Fix up handling of {lower,upper}_bound, add examples

comment:16 Changed 8 weeks ago by
 Commit changed from 02a82f90ccf15540fa8518e3591485092845d4d3 to 44c8d8c6f7cd8ed6b4055b464e82364e88cc9665
Branch pushed to git repo; I updated commit sha1. New commits:
44c8d8c  Include example for open_covers=True

comment:17 Changed 8 weeks ago by
 Status changed from new to needs_review
comment:18 Changed 8 weeks ago by
 Commit changed from 44c8d8c6f7cd8ed6b4055b464e82364e88cc9665 to 0571aaa5c5b52e0b7623476da8b7b3d10e3690ab
Branch pushed to git repo; I updated commit sha1. New commits:
0571aaa  src/sage/manifolds/subset.py: Fix docstring markup

comment:19 Changed 8 weeks ago by
 Reviewers set to Eric Gourgoulhon
 Status changed from needs_review to positive_review
LGTM.
Side note: it's a pity that the subsets latex names cannot be used for the plots.
I've not found such an option in the plot
method of graphs; for posets, one can use the option element_labels
as follows:
sage: M = Manifold(3, 'M') sage: U = M.open_subset('U'); V = M.open_subset('V'); W = M.open_subset('W') sage: VW = V.union(W) sage: P = M.subset_poset() sage: P.plot(element_labels={s: s._name for s in M.subsets()})
Do you think it is worth to include this trick in the documentation?
comment:20 Changed 8 weeks ago by
Good idea, I'll add it. This is very useful.
comment:21 Changed 8 weeks ago by
 Commit changed from 0571aaa5c5b52e0b7623476da8b7b3d10e3690ab to 9d35006855c380d96d776e84ea8bab0caa93227f
 Status changed from positive_review to needs_review
Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:
9d35006  Relabel digraphs and posets for plotting

comment:22 Changed 8 weeks ago by
Also added examples for relabeled plotting of the digraphs.
comment:23 Changed 8 weeks ago by
 Status changed from needs_review to needs_work
In light of #30263, I am going to revise the definition of the poset.
comment:24 Changed 8 weeks ago by
 Commit changed from 9d35006855c380d96d776e84ea8bab0caa93227f to 3ae1f9809350bf5d74c65c9020ae1fff4aa793d6
comment:25 Changed 8 weeks ago by
 Status changed from needs_work to needs_review
Here's a new version in which the vertices of graph and poset representing manifold subsets are wrapped in an extra layer of a frozenset
. This new format is intended to be futureproof for the changes necessary to fix #30263: Basically, one will need to quotient out by the equality relation of subsets.
In fact, instead of using frozenset
directly, I define a subclass called ManifoldSubsetSet
, which is convenient for printing and sorting.
comment:26 Changed 7 weeks ago by
 Commit changed from 3ae1f9809350bf5d74c65c9020ae1fff4aa793d6 to 120fe97420a2ef8ebc914ad7c6c5393d1409cb8c
Branch pushed to git repo; I updated commit sha1. New commits:
120fe97  src/sage/manifolds/subset_set.py: Add documentation

comment:27 Changed 7 weeks ago by
This new class ManifoldSubsetSet
could obviously be generalized to other finite sets of manifold objects. Would this be of interest?
comment:28 Changed 7 weeks ago by
 Commit changed from 120fe97420a2ef8ebc914ad7c6c5393d1409cb8c to 97ec61d80b555f6b5686987e9be58c366743a73b
Branch pushed to git repo; I updated commit sha1. New commits:
97ec61d  FiniteManifoldObjectFamily: Rename from ManifoldSubsetSet, subclass FiniteFamily instead of frozenset

comment:29 Changed 7 weeks ago by
Ready for review.
comment:30 Changed 7 weeks ago by
 Dependencies changed from #31681 to #31681, #31717
comment:31 Changed 7 weeks ago by
 Commit changed from 97ec61d80b555f6b5686987e9be58c366743a73b to f0959889a79f531474f8fe7a12ebe749db3c2957
comment:32 followup: ↓ 35 Changed 7 weeks ago by
Thanks for the update. Looks good. I've got two remarks:
 Is there any reason for the names
FiniteManifoldObjectFamily
andFiniteManifoldSubsetFamily
instead ofManifoldObjectFiniteFamily
andManifoldSubsetFiniteFamily
? The latter ones sound more natural to me, making clear that we are dealing with finite families.
 To generate the documentation about these classes in the reference manual, the new entry
sage/manifolds/family
must be added tosrc/doc/en/reference/manifolds/manifolds.rst
comment:33 Changed 7 weeks ago by
 Commit changed from f0959889a79f531474f8fe7a12ebe749db3c2957 to 2f2ace2e9e9b0580d2b6d93eaa98d14bbc966f77
comment:34 Changed 7 weeks ago by
 Commit changed from 2f2ace2e9e9b0580d2b6d93eaa98d14bbc966f77 to 1aff58a70849386432f336b5bba37e9545eed6eb
Branch pushed to git repo; I updated commit sha1. New commits:
1aff58a  Fix up docstring markup

comment:35 in reply to: ↑ 32 Changed 7 weeks ago by
Replying to egourgoulhon:
 Is there any reason for the names
FiniteManifoldObjectFamily
andFiniteManifoldSubsetFamily
instead ofManifoldObjectFiniteFamily
andManifoldSubsetFiniteFamily
? The latter ones sound more natural to me, making clear that we are dealing with finite families.
Good idea, done.
 To generate the documentation about these classes in the reference manual, the new entry
sage/manifolds/family
must be added tosrc/doc/en/reference/manifolds/manifolds.rst
Thanks, done  I always forget to do this.
comment:36 Changed 7 weeks ago by
 Commit changed from 1aff58a70849386432f336b5bba37e9545eed6eb to b922066d150a4bdc6e6ee9d52924a7d49f039946
Branch pushed to git repo; I updated commit sha1. New commits:
b922066  ManifoldSubsetFiniteFamily: If all subsets are open, include 'open' in repr

comment:37 Changed 7 weeks ago by
Did one final improvement for more informative printing
comment:38 Changed 7 weeks ago by
 Commit changed from b922066d150a4bdc6e6ee9d52924a7d49f039946 to 30271af51d78e4796f1be8b221088ceb2a71b2fa
Branch pushed to git repo; I updated commit sha1. New commits:
30271af  Fixup doctest

comment:39 Changed 7 weeks ago by
Thanks for the changes. One minor remark: shouldn't
TypeError: all... subsets must have the same manifold
be replaced by
TypeError: all open subsets must have the same manifold
in the output of the doctest
sage: ManifoldSubsetFiniteFamily([M, N])
in ManifoldSubsetFiniteFamily
? Both doctests passed, but the latter is identical to the actual message get by the user.
Besides, do you think it is worth to add PLOT
directives like
.. PLOT:: M = Manifold(3, 'M') U = M.open_subset('U'); V = M.open_subset('V'); W = M.open_subset('W') VW = V.union(W) P = M.subset_poset() g = P.plot(element_labels={element: element._name for element in P}) sphinx_plot(g)
immediately after the doctest
sage: P.plot(element_labels={element: element._name for element in P})
to generate plots of posets and digraphs in the reference manual?
In any case, I would suggest that the doctest markers # not tested
are removed from the plot
examples:
 sage: P.plot(element_labels={element: element._name for element in P}) # not tested + sage: P.plot(element_labels={element: element._name for element in P}) + Graphics object consisting of 10 graphics primitives
comment:40 Changed 7 weeks ago by
 Commit changed from 30271af51d78e4796f1be8b221088ceb2a71b2fa to 84896c4d1ec02b22bed05d2f5dd188d71ee0767d
comment:41 followup: ↓ 42 Changed 7 weeks ago by
How's this look?
comment:42 in reply to: ↑ 41 Changed 7 weeks ago by
 Status changed from needs_review to positive_review
comment:43 Changed 7 weeks ago by
Thanks for the review!
Possibly related: #30263.