Opened 7 weeks ago
Last modified 4 weeks ago
#31764 positive_review enhancement
ManifoldSubset: union, intersection, declare_{union,intersection} with arbitrary number of arguments
Reported by:  mkoeppe  Owned by:  

Priority:  major  Milestone:  sage9.4 
Component:  manifolds  Keywords:  
Cc:  egourgoulhon, ghmjungmath  Merged in:  
Authors:  Matthias Koeppe  Reviewers:  Eric Gourgoulhon 
Report Upstream:  N/A  Work issues:  
Branch:  u/mkoeppe/manifoldsubset__union__intersection__declare__union_intersection__with_arbitrary_number_of_arguments (Commits, GitHub, GitLab)  Commit:  2d7fda7417de199893f5b58c2e38c5096fbdb5d9 
Dependencies:  #31680, #31718, #31677, #31727, #31732, #31736, #31763  Stopgaps: 
Description
In this ticket, we extend union
and declare_union
as follows:
 support more than two arguments
 optional argument
disjoint
 support 0 arguments (
declare_union
is the same asdeclare_empty()
)  support 1 argument (
declare_union
is the same asdeclare_equal(other)
)
Likewise, we extend declare_intersection
and intersection
.
We also introduce
declare_disjoint(other)
 same asself.intersection(other).declare_empty()
Change History (15)
comment:1 Changed 7 weeks ago by
 Branch set to u/mkoeppe/manifoldsubset__union__intersection__declare__union_intersection__with_arbitrary_number_of_arguments
comment:2 Changed 7 weeks ago by
 Commit set to a08356971af378cabec7cd5a51be8e597ea77a67
 Status changed from new to needs_review
comment:3 Changed 6 weeks ago by
 Commit changed from a08356971af378cabec7cd5a51be8e597ea77a67 to da9165368b08b0115c6e141641a8282f1dcf9f5a
Branch pushed to git repo; I updated commit sha1. New commits:
da91653  ManifoldSubset._union_subset: Fixup

comment:4 Changed 6 weeks ago by
Doctests are missing for the methods _reduce_intersection_members
, _intersection_subset
, _reduce_union_members
and _union_subset
.
comment:5 Changed 5 weeks ago by
 Status changed from needs_review to needs_work
comment:6 Changed 5 weeks ago by
 Commit changed from da9165368b08b0115c6e141641a8282f1dcf9f5a to ee4efc9ad51725bd500380a86333eb8a69e63311
Branch pushed to git repo; I updated commit sha1. New commits:
94edd68  ManifoldSubset.declare_superset: Fix documentation

41826b4  ManifoldSubset.declare_{sub,super}set: Expand docstring

3ef5141  Merge #31763

79a625b  ManifoldSubset._reduce_intersection_members: Add examples, raise TypeError if input is empty family

8e70023  ManifoldSubset._reduce_union_members: Add examples

ee4efc9  ManifoldSubset._{union,intersection}_subset: Do cache the result here; add examples

comment:7 Changed 5 weeks ago by
 Status changed from needs_work to needs_review
comment:8 Changed 5 weeks ago by
What about the Pyflakes error? (local variable 'old' is assigned to but never used)
comment:9 Changed 5 weeks ago by
 Commit changed from ee4efc9ad51725bd500380a86333eb8a69e63311 to 6bbd4ae1295dbc7301571abddccb0e6c7487565d
comment:10 Changed 5 weeks ago by
Ready for review. The error indicated by the patchbot is unrelated to this ticket
comment:11 Changed 5 weeks ago by
Well, pyflakes is still not happy, despite comment:9 commit.
comment:12 Changed 4 weeks ago by
 Commit changed from 6bbd4ae1295dbc7301571abddccb0e6c7487565d to 2d7fda7417de199893f5b58c2e38c5096fbdb5d9
Branch pushed to git repo; I updated commit sha1. New commits:
2d7fda7  src/sage/manifolds/subset.py: Simplify code more to make pyflakes happy

comment:13 Changed 4 weeks ago by
Indeed... thanks for your patience
comment:14 Changed 4 weeks ago by
 Reviewers set to Eric Gourgoulhon
 Status changed from needs_review to positive_review
Sorry for the delay. LGTM. Thanks for this improvement!
comment:15 Changed 4 weeks ago by
Thank you!
Last 10 new commits:
ManifoldSubset.intersection: Handle arbitrary intersections
intersection: Refactor
ManifoldSubset.intersection: Only pass name, latex_name to the final intersection
ManifoldSubset.intersection: Go through ManifoldSubsetFiniteFamily to make the order of operations canonical
ManifoldSubset._union_subset: Factor out from ManifoldSubset.union
ManifoldSubset.union: Handle arbitrary unions
ManifoldSubset.union: Add example and plots
ManifoldSubset.declare_union: Handle arbitrary unions
src/sage/manifolds/subset.py: import itertools
ManifoldSubset._intersection_subset, _union_subset: Use declare_subset, declare_superset; fix doctest