Opened 20 months ago
Closed 18 months ago
#31680 closed enhancement (fixed)
Poset of manifold subsets
Reported by:  Matthias Köppe  Owned by:  

Priority:  major  Milestone:  sage9.4 
Component:  manifolds  Keywords:  
Cc:  Michael Jung, Eric Gourgoulhon, Travis Scrimshaw  Merged in:  
Authors:  Matthias Koeppe  Reviewers:  Eric Gourgoulhon 
Report Upstream:  N/A  Work issues:  
Branch:  84896c4 (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 (44)
comment:1 Changed 20 months ago by
comment:3 Changed 20 months ago by
Branch:  → u/mkoeppe/poset_of_manifold_subsets 

comment:4 followup: 11 Changed 20 months ago by
Commit:  → 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 20 months ago by
Commit:  1b5c153e6c973c6fe0b2040dc2af2f12960b8716 → 91bbc3ecb2b08dd3ffbb0ef1f3c2297de6b33816 

Branch pushed to git repo; I updated commit sha1. New commits:
91bbc3e  ManifoldSubset.subset_poset: New

comment:6 Changed 20 months ago by
Commit:  91bbc3ecb2b08dd3ffbb0ef1f3c2297de6b33816 → 858c82de2f280be41c6aa6c3928290c30c97f240 

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

comment:7 Changed 20 months ago by
Dependencies:  → #31681 

comment:8 Changed 20 months ago by
Commit:  858c82de2f280be41c6aa6c3928290c30c97f240 → a1e21b0b4687f43f4f19b2c4ddac8e1e9493ae81 

comment:9 Changed 20 months ago by
Authors:  → Matthias Koeppe 

comment:10 Changed 20 months ago by
Commit:  a1e21b0b4687f43f4f19b2c4ddac8e1e9493ae81 → 2e2d91ebc69117b7a49e1f099f83993bfd118db8 

Branch pushed to git repo; I updated commit sha1. New commits:
2e2d91e  ManifoldSubset.{sub,super}set_{digraph,poset}: More options

comment:11 Changed 20 months 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 20 months ago by
This digraph approach might also be useful to improve restrictions of tensor fields and make them even faster, no?
comment:13 followup: 14 Changed 20 months 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 Changed 20 months 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 20 months ago by
Commit:  2e2d91ebc69117b7a49e1f099f83993bfd118db8 → 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 20 months ago by
Commit:  02a82f90ccf15540fa8518e3591485092845d4d3 → 44c8d8c6f7cd8ed6b4055b464e82364e88cc9665 

Branch pushed to git repo; I updated commit sha1. New commits:
44c8d8c  Include example for open_covers=True

comment:17 Changed 20 months ago by
Status:  new → needs_review 

comment:18 Changed 20 months ago by
Commit:  44c8d8c6f7cd8ed6b4055b464e82364e88cc9665 → 0571aaa5c5b52e0b7623476da8b7b3d10e3690ab 

Branch pushed to git repo; I updated commit sha1. New commits:
0571aaa  src/sage/manifolds/subset.py: Fix docstring markup

comment:19 Changed 20 months ago by
Reviewers:  → Eric Gourgoulhon 

Status:  needs_review → 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:21 Changed 20 months ago by
Commit:  0571aaa5c5b52e0b7623476da8b7b3d10e3690ab → 9d35006855c380d96d776e84ea8bab0caa93227f 

Status:  positive_review → 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:23 Changed 20 months ago by
Status:  needs_review → needs_work 

In light of #30263, I am going to revise the definition of the poset.
comment:24 Changed 20 months ago by
Commit:  9d35006855c380d96d776e84ea8bab0caa93227f → 3ae1f9809350bf5d74c65c9020ae1fff4aa793d6 

comment:25 Changed 20 months ago by
Status:  needs_work → 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 20 months ago by
Commit:  3ae1f9809350bf5d74c65c9020ae1fff4aa793d6 → 120fe97420a2ef8ebc914ad7c6c5393d1409cb8c 

Branch pushed to git repo; I updated commit sha1. New commits:
120fe97  src/sage/manifolds/subset_set.py: Add documentation

comment:27 Changed 20 months 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 20 months ago by
Commit:  120fe97420a2ef8ebc914ad7c6c5393d1409cb8c → 97ec61d80b555f6b5686987e9be58c366743a73b 

Branch pushed to git repo; I updated commit sha1. New commits:
97ec61d  FiniteManifoldObjectFamily: Rename from ManifoldSubsetSet, subclass FiniteFamily instead of frozenset

comment:30 Changed 20 months ago by
Dependencies:  #31681 → #31681, #31717 

comment:31 Changed 20 months ago by
Commit:  97ec61d80b555f6b5686987e9be58c366743a73b → f0959889a79f531474f8fe7a12ebe749db3c2957 

comment:32 followup: 35 Changed 20 months 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 20 months ago by
Commit:  f0959889a79f531474f8fe7a12ebe749db3c2957 → 2f2ace2e9e9b0580d2b6d93eaa98d14bbc966f77 

comment:34 Changed 20 months ago by
Commit:  2f2ace2e9e9b0580d2b6d93eaa98d14bbc966f77 → 1aff58a70849386432f336b5bba37e9545eed6eb 

Branch pushed to git repo; I updated commit sha1. New commits:
1aff58a  Fix up docstring markup

comment:35 Changed 20 months 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 20 months ago by
Commit:  1aff58a70849386432f336b5bba37e9545eed6eb → b922066d150a4bdc6e6ee9d52924a7d49f039946 

Branch pushed to git repo; I updated commit sha1. New commits:
b922066  ManifoldSubsetFiniteFamily: If all subsets are open, include 'open' in repr

comment:38 Changed 20 months ago by
Commit:  b922066d150a4bdc6e6ee9d52924a7d49f039946 → 30271af51d78e4796f1be8b221088ceb2a71b2fa 

Branch pushed to git repo; I updated commit sha1. New commits:
30271af  Fixup doctest

comment:39 Changed 20 months 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 20 months ago by
Commit:  30271af51d78e4796f1be8b221088ceb2a71b2fa → 84896c4d1ec02b22bed05d2f5dd188d71ee0767d 

comment:42 Changed 20 months ago by
Status:  needs_review → positive_review 

comment:44 Changed 18 months ago by
Branch:  u/mkoeppe/poset_of_manifold_subsets → 84896c4d1ec02b22bed05d2f5dd188d71ee0767d 

Resolution:  → fixed 
Status:  positive_review → closed 
Possibly related: #30263.