Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Poset of manifold subsets #31680

Closed
mkoeppe opened this issue Apr 17, 2021 · 62 comments
Closed

Poset of manifold subsets #31680

mkoeppe opened this issue Apr 17, 2021 · 62 comments

Comments

@mkoeppe
Copy link
Contributor

mkoeppe commented Apr 17, 2021

The declared subsets or supersets of a ManifoldSubset form finite posets. We add methods to expose them as instances of FinitePoset.

Depends on #31681
Depends on #31717

CC: @mjungmath @egourgoulhon @tscrim

Component: manifolds

Author: Matthias Koeppe

Branch/Commit: 84896c4

Reviewer: Eric Gourgoulhon

Issue created by migration from https://trac.sagemath.org/ticket/31680

@mkoeppe mkoeppe added this to the sage-9.4 milestone Apr 17, 2021
@mjungmath
Copy link

comment:1

Possibly related: #30263.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Apr 17, 2021

comment:2

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

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Apr 17, 2021

Branch: u/mkoeppe/poset_of_manifold_subsets

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Apr 17, 2021

comment:4

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

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Apr 17, 2021

Commit: 1b5c153

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 18, 2021

Changed commit from 1b5c153 to 91bbc3e

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 18, 2021

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

91bbc3eManifoldSubset.subset_poset: New

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 18, 2021

Changed commit from 91bbc3e to 858c82d

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 18, 2021

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

858c82dFixup

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Apr 18, 2021

Dependencies: #31681

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 18, 2021

Changed commit from 858c82d to a1e21b0

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 18, 2021

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

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

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Apr 18, 2021

Author: Matthias Koeppe

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 18, 2021

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

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 18, 2021

Changed commit from a1e21b0 to 2e2d91e

@egourgoulhon
Copy link
Member

comment:11

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.

@mjungmath
Copy link

comment:12

This digraph approach might also be useful to improve restrictions of tensor fields and make them even faster, no?

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Apr 18, 2021

comment:13

Replying to @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.

@mjungmath
Copy link

comment:14

Replying to @mkoeppe:

Replying to @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!

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 18, 2021

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

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 18, 2021

Changed commit from 2e2d91e to 02a82f9

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 18, 2021

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

44c8d8cInclude example for open_covers=True

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 18, 2021

Changed commit from 02a82f9 to 44c8d8c

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 19, 2021

Changed commit from 44c8d8c to 0571aaa

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 19, 2021

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

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 22, 2021

Changed commit from 120fe97 to 97ec61d

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Apr 22, 2021

comment:29

Ready for review.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Apr 23, 2021

Changed dependencies from #31681 to #31681, #31717

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 23, 2021

Changed commit from 97ec61d to f095988

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 23, 2021

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

@egourgoulhon
Copy link
Member

comment:32

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.

  2. 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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 23, 2021

Changed commit from f095988 to 2f2ace2

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 23, 2021

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 23, 2021

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

1aff58aFix up docstring markup

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 23, 2021

Changed commit from 2f2ace2 to 1aff58a

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Apr 23, 2021

comment:35

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.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 23, 2021

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

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 23, 2021

Changed commit from 1aff58a to b922066

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Apr 23, 2021

comment:37

Did one final improvement for more informative printing

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 23, 2021

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

30271afFixup doctest

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 23, 2021

Changed commit from b922066 to 30271af

@egourgoulhon
Copy link
Member

comment:39

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 24, 2021

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 24, 2021

Changed commit from 30271af to 84896c4

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Apr 24, 2021

comment:41

How's this look?

@egourgoulhon
Copy link
Member

comment:42

Replying to @mkoeppe:

How's this look?

That's perfect, thanks!

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Apr 25, 2021

comment:43

Thanks for the review!

@vbraun
Copy link
Member

vbraun commented Jun 21, 2021

Changed branch from u/mkoeppe/poset_of_manifold_subsets to 84896c4

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants