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: sage-9.4
Component: manifolds Keywords:
Cc: gh-mjungmath, 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:

Status badges

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 gh-mjungmath

Possibly related: #30263.

comment:2 Changed 8 weeks ago by mkoeppe

Yes, hopefully this new feature can be used as a debugging tool

comment:3 Changed 8 weeks ago by mkoeppe

  • Branch set to u/mkoeppe/poset_of_manifold_subsets

comment:4 follow-up: Changed 8 weeks ago by mkoeppe

  • 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:

1b5c153ManifoldSubset.subset_digraph: New

comment:5 Changed 8 weeks ago by git

  • Commit changed from 1b5c153e6c973c6fe0b2040dc2af2f12960b8716 to 91bbc3ecb2b08dd3ffbb0ef1f3c2297de6b33816

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

91bbc3eManifoldSubset.subset_poset: New

comment:6 Changed 8 weeks ago by git

  • Commit changed from 91bbc3ecb2b08dd3ffbb0ef1f3c2297de6b33816 to 858c82de2f280be41c6aa6c3928290c30c97f240

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

858c82dFixup

comment:7 Changed 8 weeks ago by mkoeppe

  • Dependencies set to #31681

comment:8 Changed 8 weeks ago by git

  • Commit changed from 858c82de2f280be41c6aa6c3928290c30c97f240 to a1e21b0b4687f43f4f19b2c4ddac8e1e9493ae81

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

280a82dtrac #31681: fix sorting in layout_acyclic_dummy
a1e21b0Merge #31681

comment:9 Changed 8 weeks ago by mkoeppe

  • Authors set to Matthias Koeppe

comment:10 Changed 8 weeks ago by git

  • Commit changed from a1e21b0b4687f43f4f19b2c4ddac8e1e9493ae81 to 2e2d91ebc69117b7a49e1f099f83993bfd118db8

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

2e2d91eManifoldSubset.{sub,super}set_{digraph,poset}: More options

comment:11 in reply to: ↑ 4 Changed 8 weeks ago by egourgoulhon

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.

Last edited 8 weeks ago by egourgoulhon (previous) (diff)

comment:12 follow-up: Changed 8 weeks ago by gh-mjungmath

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 ; follow-up: Changed 8 weeks ago by mkoeppe

Replying to gh-mjungmath:

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 follow-up 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 high-level view on the existing structures.

comment:14 in reply to: ↑ 13 Changed 8 weeks ago by gh-mjungmath

Replying to mkoeppe:

Replying to gh-mjungmath:

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 follow-up 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 high-level view on the existing structures.

+1

Besides, I really like the way you write code. It's clear and neat!

Last edited 8 weeks ago by gh-mjungmath (previous) (diff)

comment:15 Changed 8 weeks ago by git

  • Commit changed from 2e2d91ebc69117b7a49e1f099f83993bfd118db8 to 02a82f90ccf15540fa8518e3591485092845d4d3

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

02a82f9Fix up handling of {lower,upper}_bound, add examples

comment:16 Changed 8 weeks ago by git

  • Commit changed from 02a82f90ccf15540fa8518e3591485092845d4d3 to 44c8d8c6f7cd8ed6b4055b464e82364e88cc9665

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

44c8d8cInclude example for open_covers=True

comment:17 Changed 8 weeks ago by mkoeppe

  • Status changed from new to needs_review

comment:18 Changed 8 weeks ago by git

  • Commit changed from 44c8d8c6f7cd8ed6b4055b464e82364e88cc9665 to 0571aaa5c5b52e0b7623476da8b7b3d10e3690ab

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

0571aaasrc/sage/manifolds/subset.py: Fix docstring markup

comment:19 Changed 8 weeks ago by egourgoulhon

  • 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?

Last edited 8 weeks ago by egourgoulhon (previous) (diff)

comment:20 Changed 8 weeks ago by mkoeppe

Good idea, I'll add it. This is very useful.

comment:21 Changed 8 weeks ago by git

  • 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:

9d35006Relabel digraphs and posets for plotting

comment:22 Changed 8 weeks ago by mkoeppe

Also added examples for relabeled plotting of the digraphs.

comment:23 Changed 8 weeks ago by mkoeppe

  • 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 git

  • Commit changed from 9d35006855c380d96d776e84ea8bab0caa93227f to 3ae1f9809350bf5d74c65c9020ae1fff4aa793d6

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

4d93fe5src/sage/manifolds/subset_set.py: New
3ae1f98ManifoldSubset.subset_digraph: Make subset nodes (singleton) ManifoldSubsetSet instances, open_cover nodes tuples

comment:25 Changed 8 weeks ago by mkoeppe

  • 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 future-proof 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 git

  • Commit changed from 3ae1f9809350bf5d74c65c9020ae1fff4aa793d6 to 120fe97420a2ef8ebc914ad7c6c5393d1409cb8c

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

120fe97src/sage/manifolds/subset_set.py: Add documentation

comment:27 Changed 7 weeks ago by mkoeppe

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 git

  • Commit changed from 120fe97420a2ef8ebc914ad7c6c5393d1409cb8c to 97ec61d80b555f6b5686987e9be58c366743a73b

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

97ec61dFiniteManifoldObjectFamily: Rename from ManifoldSubsetSet, subclass FiniteFamily instead of frozenset

comment:29 Changed 7 weeks ago by mkoeppe

Ready for review.

comment:30 Changed 7 weeks ago by mkoeppe

  • Dependencies changed from #31681 to #31681, #31717

comment:31 Changed 7 weeks ago by git

  • Commit changed from 97ec61d80b555f6b5686987e9be58c366743a73b to f0959889a79f531474f8fe7a12ebe749db3c2957

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

cbbef90Adding a __bool__ to trivial family.
42abda7Adding __bool__ for other families.
1f21a8fMerge #31717
f095988FiniteManifoldObjectFamily.__bool__: Remove, inherited from superclass after #31717

comment:32 follow-up: Changed 7 weeks ago by egourgoulhon

Thanks for the update. Looks good. I've got two remarks:

  1. Is there any reason for the names FiniteManifoldObjectFamily and FiniteManifoldSubsetFamily instead of ManifoldObjectFiniteFamily and ManifoldSubsetFiniteFamily? The latter ones sound more natural to me, making clear that we are dealing with finite families.
  1. To generate the documentation about these classes in the reference manual, the new entry sage/manifolds/family must be added to src/doc/en/reference/manifolds/manifolds.rst

comment:33 Changed 7 weeks ago by git

  • Commit changed from f0959889a79f531474f8fe7a12ebe749db3c2957 to 2f2ace2e9e9b0580d2b6d93eaa98d14bbc966f77

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

5d87ceeManifold{Object,Subset}FiniteFamily: Rename from FiniteManifold{Object,Subset}Family
2f2ace2src/doc/en/reference/manifolds/manifold.rst: Add sage.manifolds.family

comment:34 Changed 7 weeks ago by git

  • Commit changed from 2f2ace2e9e9b0580d2b6d93eaa98d14bbc966f77 to 1aff58a70849386432f336b5bba37e9545eed6eb

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

1aff58aFix up docstring markup

comment:35 in reply to: ↑ 32 Changed 7 weeks ago by mkoeppe

Replying to egourgoulhon:

  1. Is there any reason for the names FiniteManifoldObjectFamily and FiniteManifoldSubsetFamily instead of ManifoldObjectFiniteFamily and ManifoldSubsetFiniteFamily? The latter ones sound more natural to me, making clear that we are dealing with finite families.

Good idea, done.

  1. To generate the documentation about these classes in the reference manual, the new entry sage/manifolds/family must be added to src/doc/en/reference/manifolds/manifolds.rst

Thanks, done - I always forget to do this.

comment:36 Changed 7 weeks ago by git

  • Commit changed from 1aff58a70849386432f336b5bba37e9545eed6eb to b922066d150a4bdc6e6ee9d52924a7d49f039946

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

b922066ManifoldSubsetFiniteFamily: If all subsets are open, include 'open' in repr

comment:37 Changed 7 weeks ago by mkoeppe

Did one final improvement for more informative printing

comment:38 Changed 7 weeks ago by git

  • Commit changed from b922066d150a4bdc6e6ee9d52924a7d49f039946 to 30271af51d78e4796f1be8b221088ceb2a71b2fa

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

30271afFixup doctest

comment:39 Changed 7 weeks ago by egourgoulhon

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
Last edited 7 weeks ago by egourgoulhon (previous) (diff)

comment:40 Changed 7 weeks ago by git

  • Commit changed from 30271af51d78e4796f1be8b221088ceb2a71b2fa to 84896c4d1ec02b22bed05d2f5dd188d71ee0767d

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

215c378ManifoldSubset.subset_{digraph,poset}: Add plots in documentation, remove # not tested
84896c4ManifoldSubsetFiniteFamily: Use full error message in doctest

comment:41 follow-up: Changed 7 weeks ago by mkoeppe

How's this look?

comment:42 in reply to: ↑ 41 Changed 7 weeks ago by egourgoulhon

  • Status changed from needs_review to positive_review

Replying to mkoeppe:

How's this look?

That's perfect, thanks!

comment:43 Changed 7 weeks ago by mkoeppe

Thanks for the review!

Note: See TracTickets for help on using tickets.