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

ManifoldSubset: Change some methods to generators #31718

Closed
mkoeppe opened this issue Apr 22, 2021 · 21 comments
Closed

ManifoldSubset: Change some methods to generators #31718

mkoeppe opened this issue Apr 22, 2021 · 21 comments

Comments

@mkoeppe
Copy link
Contributor

mkoeppe commented Apr 22, 2021

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.

Depends on #31680

CC: @mjungmath @egourgoulhon @tscrim

Component: manifolds

Author: Matthias Koeppe

Branch/Commit: e026e7a

Reviewer: Eric Gourgoulhon

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

@mkoeppe mkoeppe added this to the sage-9.4 milestone Apr 22, 2021
@mkoeppe
Copy link
Contributor Author

mkoeppe commented Apr 23, 2021

@mkoeppe

This comment has been minimized.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Apr 23, 2021

New commits:

186707bManifoldSubset.subsets: Change to generator

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Apr 23, 2021

Commit: 186707b

@mkoeppe

This comment has been minimized.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 23, 2021

Changed commit from 186707b to 78cc27a

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 23, 2021

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

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Apr 23, 2021

Author: Matthias Koeppe

@mkoeppe

This comment has been minimized.

@mkoeppe mkoeppe changed the title ManifoldSubset, Manifold: Change some methods to generators ManifoldSubset: Change some methods to generators Apr 23, 2021
@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 23, 2021

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

e026e7aManifoldSubset.subset_digraph: Use open_covers method

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 23, 2021

Changed commit from 78cc27a to e026e7a

@mkoeppe

This comment has been minimized.

@egourgoulhon
Copy link
Member

Reviewer: Eric Gourgoulhon

@egourgoulhon
Copy link
Member

comment:8

LGTM. Thanks!

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Apr 25, 2021

comment:9

Thanks for reviewing!

@mjungmath
Copy link

comment:10

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.

@mjungmath
Copy link

comment:11

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.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Apr 26, 2021

comment:12

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

@mjungmath
Copy link

comment:13

Replying to @mkoeppe:

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

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Apr 27, 2021

comment:14

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

@vbraun
Copy link
Member

vbraun commented Jun 21, 2021

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