#31718 closed enhancement (fixed)

ManifoldSubset: Change some methods to generators

Reported by: Matthias Köppe Owned by:
Priority: major Milestone: sage-9.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:

Status badges

Description (last modified by Matthias Köppe)

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
  • subsets() currently returns a frozenset
    • change to generator of ManifoldSubset instances

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 20 months ago by Matthias Köppe

Branch: u/mkoeppe/manifoldsubset__manifold__change_some_methods_to_generators

comment:2 Changed 20 months ago by Matthias Köppe

Commit: 186707ba9fe840040c6675421a65f28fc4f3999e
Description: modified (diff)

New commits:

186707bManifoldSubset.subsets: Change to generator

comment:3 Changed 20 months ago by Matthias Köppe

Description: modified (diff)

comment:4 Changed 20 months ago by git

Commit: 186707ba9fe840040c6675421a65f28fc4f3999e78cc27a8e439c542c2f03958e8e0888b0551a979

Branch pushed to git repo; I updated commit sha1. Last 10 new commits:

42abda7Adding __bool__ for other families.
1f21a8fMerge #31717
f095988FiniteManifoldObjectFamily.__bool__: Remove, inherited from superclass after #31717
5d87ceeManifold{Object,Subset}FiniteFamily: Rename from FiniteManifold{Object,Subset}Family
2f2ace2src/doc/en/reference/manifolds/manifold.rst: Add sage.manifolds.family
1aff58aFix up docstring markup
b922066ManifoldSubsetFiniteFamily: If all subsets are open, include 'open' in repr
30271afFixup doctest
adac07aMerge #31680
78cc27aManifoldSubset.open_covers: Change to generator, add optional arg 'trivial'; update uses

comment:5 Changed 20 months ago by Matthias Köppe

Authors: Matthias Koeppe
Description: modified (diff)
Status: newneeds_review
Summary: ManifoldSubset, Manifold: Change some methods to generatorsManifoldSubset: Change some methods to generators

comment:6 Changed 20 months ago by git

Commit: 78cc27a8e439c542c2f03958e8e0888b0551a979e026e7a68095577277de1b32df0f8c446b594aa7

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

e026e7aManifoldSubset.subset_digraph: Use open_covers method

comment:7 Changed 20 months ago by Matthias Köppe

Description: modified (diff)

comment:8 Changed 20 months ago by Eric Gourgoulhon

Reviewers: Eric Gourgoulhon
Status: needs_reviewpositive_review

LGTM. Thanks!

comment:9 Changed 20 months ago by Matthias Köppe

Thanks for reviewing!

comment:10 Changed 20 months ago by Michael Jung

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 Michael Jung

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.

Last edited 20 months ago by Michael Jung (previous) (diff)

comment:12 in reply to:  10 ; Changed 20 months ago by Matthias Köppe

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 ; Changed 20 months ago by Michael Jung

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 are ManifoldSubsetFiniteFamily 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 20 months ago by Matthias Köppe

Replying to gh-mjungmath:

I think its better to add some distinct examples to ManifoldObjectFiniteFamily to clarify the difference to ManifoldSubsetFiniteFamily.

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 Volker Braun

Branch: u/mkoeppe/manifoldsubset__manifold__change_some_methods_to_generatorse026e7a68095577277de1b32df0f8c446b594aa7
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.