Opened 13 months ago
Closed 11 months ago
#31718 closed enhancement (fixed)
ManifoldSubset: Change some methods to generators
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: | 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.
Follow-up for some methods of Manifold
in #31720.
Change History (15)
comment:1 Changed 13 months ago by
- Branch set to u/mkoeppe/manifoldsubset__manifold__change_some_methods_to_generators
comment:2 Changed 13 months ago by
- Commit set to 186707ba9fe840040c6675421a65f28fc4f3999e
- Description modified (diff)
comment:3 Changed 13 months ago by
- Description modified (diff)
comment:4 Changed 13 months ago by
- Commit changed from 186707ba9fe840040c6675421a65f28fc4f3999e to 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 13 months ago by
- Description modified (diff)
- Status changed from new to needs_review
- Summary changed from ManifoldSubset, Manifold: Change some methods to generators to ManifoldSubset: Change some methods to generators
comment:6 Changed 13 months ago by
- Commit changed from 78cc27a8e439c542c2f03958e8e0888b0551a979 to e026e7a68095577277de1b32df0f8c446b594aa7
Branch pushed to git repo; I updated commit sha1. New commits:
e026e7a | ManifoldSubset.subset_digraph: Use open_covers method
|
comment:7 Changed 13 months ago by
- Description modified (diff)
comment:8 Changed 13 months ago by
- Reviewers set to Eric Gourgoulhon
- Status changed from needs_review to positive_review
LGTM. Thanks!
comment:9 Changed 13 months ago by
Thanks for reviewing!
comment:10 follow-up: ↓ 12 Changed 13 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 13 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 non-subset 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 in reply to: ↑ 10 ; follow-up: ↓ 13 Changed 13 months ago by
Replying to gh-mjungmath:
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 in reply to: ↑ 12 ; follow-up: ↓ 14 Changed 13 months ago by
Replying to mkoeppe:
Replying to gh-mjungmath:
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 in reply to: ↑ 13 Changed 13 months ago by
Replying to gh-mjungmath:
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 11 months ago by
- Branch changed from u/mkoeppe/manifoldsubset__manifold__change_some_methods_to_generators to e026e7a68095577277de1b32df0f8c446b594aa7
- Resolution set to fixed
- Status changed from positive_review to closed
New commits:
ManifoldSubset.subsets: Change to generator