Opened 20 months ago
Closed 18 months ago
#31718 closed enhancement (fixed)
ManifoldSubset: Change some methods to generators
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:  e026e7a (Commits, GitHub, GitLab)  Commit:  e026e7a68095577277de1b32df0f8c446b594aa7 
Dependencies:  #31680  Stopgaps: 
Description (last modified by )
This ticket proposes to change some methods that currently return lists to generators  like the method open_supersets
added in #31677. This is in line with the changes in the Python standard library when we moved from Python 2 to 3.
ManifoldSubset
:
open_covers()
currently returns a list of lists change to generator of
ManifoldSubsetFiniteFamily
instances  add optional argument
trivial
to simplify the common use case that only needs the nontrivial open covers
 change to generator of
subsets()
currently returns afrozenset
 change to generator of
ManifoldSubset
instances
 change to generator of
These API changes will probably make some updates to sage.manifolds worksheets that are maintained outside of the Sage tree necessary.
Followup for some methods of Manifold
in #31720.
Change History (15)
comment:1 Changed 20 months ago by
Branch:  → u/mkoeppe/manifoldsubset__manifold__change_some_methods_to_generators 

comment:2 Changed 20 months ago by
Commit:  → 186707ba9fe840040c6675421a65f28fc4f3999e 

Description:  modified (diff) 
comment:3 Changed 20 months ago by
Description:  modified (diff) 

comment:4 Changed 20 months ago by
Commit:  186707ba9fe840040c6675421a65f28fc4f3999e → 78cc27a8e439c542c2f03958e8e0888b0551a979 

Branch pushed to git repo; I updated commit sha1. Last 10 new commits:
42abda7  Adding __bool__ for other families.

1f21a8f  Merge #31717

f095988  FiniteManifoldObjectFamily.__bool__: Remove, inherited from superclass after #31717

5d87cee  Manifold{Object,Subset}FiniteFamily: Rename from FiniteManifold{Object,Subset}Family

2f2ace2  src/doc/en/reference/manifolds/manifold.rst: Add sage.manifolds.family

1aff58a  Fix up docstring markup

b922066  ManifoldSubsetFiniteFamily: If all subsets are open, include 'open' in repr

30271af  Fixup doctest

adac07a  Merge #31680

78cc27a  ManifoldSubset.open_covers: Change to generator, add optional arg 'trivial'; update uses

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

Description:  modified (diff) 
Status:  new → needs_review 
Summary:  ManifoldSubset, Manifold: Change some methods to generators → ManifoldSubset: Change some methods to generators 
comment:6 Changed 20 months ago by
Commit:  78cc27a8e439c542c2f03958e8e0888b0551a979 → e026e7a68095577277de1b32df0f8c446b594aa7 

Branch pushed to git repo; I updated commit sha1. New commits:
e026e7a  ManifoldSubset.subset_digraph: Use open_covers method

comment:7 Changed 20 months ago by
Description:  modified (diff) 

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

Status:  needs_review → positive_review 
LGTM. Thanks!
comment:10 followup: 12 Changed 20 months ago by
I assume it has a purpose, but just out of curiosity: why do you make a difference between families of manifold objects and families of manifold subsets? Do you have a further usage in mind other than just subsets? Because all the examples you provide are still with subsets.
comment:11 Changed 20 months ago by
I can imagine this can be useful for frames whose domains cover the manifold? Perhaps this is a more suitable example for the nonsubset case to show the difference?
Some time ago I provided a helper function _get_min_covering
(can be found in manifolds/manifold.py
) to obtain a minimal amount of manifold objects necessary to cover the manifold. It looks like this method is more suited within the family class you just provided.
Another example in the field I could imagine is for orientations. An orientation is given by a family of charts/frames, too, but yet not implemented as such.
comment:12 followup: 13 Changed 20 months ago by
Replying to ghmjungmath:
I assume it has a purpose, but just out of curiosity: why do you make a difference between families of manifold objects and families of manifold subsets? Do you have a further usage in mind other than just subsets?
Yes, in #31732 I use ManifoldObjectFiniteFamily
directly for the family of open covers. Its elements are ManifoldSubsetFiniteFamily
instances.
I agree that charts/frames are likely to benefit from becoming finite families too. This will also need a separate subclass because they are indexed not by names but by coordinate tuples.
comment:13 followup: 14 Changed 20 months ago by
Replying to mkoeppe:
Replying to ghmjungmath:
I assume it has a purpose, but just out of curiosity: why do you make a difference between families of manifold objects and families of manifold subsets? Do you have a further usage in mind other than just subsets?
Yes, in #31732 I use
ManifoldObjectFiniteFamily
directly for the family of open covers. Its elements areManifoldSubsetFiniteFamily
instances.
Alright, that makes sense. Just a personal taste: I think its better to add some distinct examples to ManifoldObjectFiniteFamily
to clarify the difference to ManifoldSubsetFiniteFamily
.
I agree that charts/frames are likely to benefit from becoming finite families too. This will also need a separate subclass because they are indexed not by names but by coordinate tuples.
Right. But don't you think it should rather be a new common parent class both inherit from?
In any case, this is something definitely need, indeed!
comment:14 Changed 20 months ago by
Replying to ghmjungmath:
I think its better to add some distinct examples to
ManifoldObjectFiniteFamily
to clarify the difference toManifoldSubsetFiniteFamily
.
Sure, let's do that when we introduce some more applications to families.
Replying to mkoeppe:
I agree that charts/frames are likely to benefit from becoming finite families too. This will also need a separate subclass because they are indexed not by names but by coordinate tuples.
Right. But don't you think it should rather be a new common parent class both inherit from?
Sure, that makes sense.
In any case, this is something definitely need, indeed!
Let's take this discussion to #31720 (Manifold: Change some methods to generators), which introduces generators for charts. Families could be introduced in the same ticket  but I will need some guidance there what the keys should be.
comment:15 Changed 18 months ago by
Branch:  u/mkoeppe/manifoldsubset__manifold__change_some_methods_to_generators → e026e7a68095577277de1b32df0f8c446b594aa7 

Resolution:  → fixed 
Status:  positive_review → closed 
New commits:
ManifoldSubset.subsets: Change to generator