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

declare_union yields wrong results #30263

Open
mjungmath opened this issue Aug 1, 2020 · 34 comments
Open

declare_union yields wrong results #30263

mjungmath opened this issue Aug 1, 2020 · 34 comments

Comments

@mjungmath
Copy link

(for the visualization of the posets, use #31680 and install the dot2tex package - without that package, the layout is quite misleading)

sage: M = Manifold(3, 'M')
sage: U = M.open_subset('U'); V = M.open_subset('V'); W = M.open_subset('W')
sage: M.declare_union(U, V, W)
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
<ipython-input-3-c7f527dcc1ea> in <module>()
----> 1 M.declare_union(U, V, W)

TypeError: declare_union() takes 3 positional arguments but 4 were given

sage: def label(element): 
....:     try: 
....:         return element._name 
....:     except AttributeError: 
....:         return '[' + ', '.join(sorted(x._name for x in element)) + ']' 

sage: P = M.subset_poset(open_covers=True); P.plot(element_labels={element: label(element) for element in P})

sage: M.declare_union(U, V.union(W))
sage: P = M.subset_poset(open_covers=True); P.plot(element_labels={element: label(element) for element in P})

sage: U.union(V).union(W)
Open subset U_union_V_union_W of the 3-dimensional differentiable manifold M
sage: P = M.subset_poset(open_covers=True); P.plot(element_labels={element: label(element) for element in P})

sage: U.union(V.union(W))
3-dimensional differentiable manifold M
sage: P = M.subset_poset(open_covers=True); P.plot(element_labels={element: label(element) for element in P})  # same

sage: M.is_subset(U.union(V).union(W))                                                         
False

#31764 adds support for arbitrary unions. But the main issue here is the failing associativity of the union.

We fix it as follows.

When creating a union, we update the open covers of the supersets of the members, replacing the union members by the union.

Depends on #31764

CC: @egourgoulhon @tscrim

Component: manifolds

Keywords: sets, union

Author: Matthias Koeppe

Branch/Commit: u/mkoeppe/declare_union_yields_wrong_results @ 743f114

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

@mjungmath mjungmath added this to the sage-9.2 milestone Aug 1, 2020
@mkoeppe mkoeppe modified the milestones: sage-9.2, sage-9.3 Oct 24, 2020
@mkoeppe
Copy link
Member

mkoeppe commented Apr 7, 2021

comment:2

Sage development has entered the release candidate phase for 9.3. Setting a new milestone for this ticket based on a cursory review.

@mkoeppe mkoeppe modified the milestones: sage-9.3, sage-9.4 Apr 7, 2021
@mkoeppe
Copy link
Member

mkoeppe commented Apr 18, 2021

comment:3

Unions of several subsets also appear in another form - as open covers (which are merely lists of subsets and not manifold subsets in their own right). One might ask whether this could be unified as well.

@mkoeppe
Copy link
Member

mkoeppe commented Apr 21, 2021

Dependencies: #31680

@mkoeppe

This comment has been minimized.

@mkoeppe

This comment has been minimized.

@mkoeppe
Copy link
Member

mkoeppe commented Apr 21, 2021

comment:5

An issue with the existing code is that through a sequence of declare_union calls, it is possible to obtain a situation in which two subsets must be the same, although declare_union makes a (not very effective) attempt to stop the user from doing so (by checking whether the two arguments are the same).

In terms of the poset of subsets that is defined by the _subsets (and _supersets) attributes (which is visualized by the code from #31680), this is quite problematic.

This could be solved by introducing equivalence classes of subsets - for example by keeping the _subsets relation acyclic and introducing an _equal_sets attribute; loops that update subsets will have to be modified.
The elements of the poset introduced in #31680 then would be equivalence classes of subsets rather than subsets.

@mkoeppe

This comment has been minimized.

@mkoeppe
Copy link
Member

mkoeppe commented Apr 23, 2021

Changed dependencies from #31680 to #31680, #31718

@mkoeppe
Copy link
Member

mkoeppe commented Apr 24, 2021

@mkoeppe
Copy link
Member

mkoeppe commented Apr 24, 2021

Last 10 new commits:

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
e026e7aManifoldSubset.subset_digraph: Use open_covers method
fca7e89ManifoldSubset.declare_empty etc.

@mkoeppe
Copy link
Member

mkoeppe commented Apr 24, 2021

Author: Matthias Koeppe

@mkoeppe
Copy link
Member

mkoeppe commented Apr 24, 2021

Commit: fca7e89

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 24, 2021

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

215c378ManifoldSubset.subset_{digraph,poset}: Add plots in documentation, remove # not tested
84896c4ManifoldSubsetFiniteFamily: Use full error message in doctest
3364468Merge #31680
6c8374eManifoldSubset.open_covers: Add option supersets; use it to fix is_empty
28650bcFix doctests
26401ddManifoldSubset.declare_empty: Add plot

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 24, 2021

Changed commit from fca7e89 to 26401dd

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 24, 2021

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

fee8b99ManifoldSubsetFiniteFamily.from_subsets_or_families: New constructor
ba51202ManifoldSubset.declare_union: Add option 'disjoint'

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 24, 2021

Changed commit from 26401dd to ba51202

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 25, 2021

Changed commit from ba51202 to 4cf4a52

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 25, 2021

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

9ca80d2ManifoldSubset.{equal_subsets,supersets,superset_family}: New
55d2aa8ManifoldSubset.{subset,superset}_digraph: New option quotient; use it for {subset,superset}_poset
4cf4a52ManifoldSubset.declare_equal: New; use it for 1-arg ManifoldSubset.declare_union

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 25, 2021

Changed commit from 4cf4a52 to 55a28ce

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 25, 2021

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

0f037ffManifoldSubset.intersection: Handle arbitrary intersections
9c75d18intersection: Refactor
8d63c32ManifoldSubset.intersection: Only pass name, latex_name to the final intersection
0867b0cManifoldSubset.intersection: Go through ManifoldSubsetFiniteFamily to make the order of operations canonical
de5f928ManifoldSubset._union_subset: Factor out from ManifoldSubset.union
4698da1ManifoldSubset.union: Handle arbitrary unions
51a89d0ManifoldSubset.union: Add example and plots
55a28ceManifoldSubset.declare_union: Handle arbitrary unions

@mkoeppe
Copy link
Member

mkoeppe commented Apr 26, 2021

Work Issues: rebase on #31732, #31727

@mkoeppe
Copy link
Member

mkoeppe commented Apr 27, 2021

Changed work issues from rebase on #31732, #31727 to rebase on #31732, #31727, #31736

@mkoeppe
Copy link
Member

mkoeppe commented May 2, 2021

Changed branch from u/mkoeppe/declare_union_yields_wrong_results to none

@mkoeppe
Copy link
Member

mkoeppe commented May 2, 2021

Changed commit from 55a28ce to none

@mkoeppe
Copy link
Member

mkoeppe commented May 2, 2021

Changed dependencies from #31680, #31718 to #31764

@mkoeppe
Copy link
Member

mkoeppe commented May 2, 2021

Changed author from Matthias Koeppe to none

@mkoeppe
Copy link
Member

mkoeppe commented May 2, 2021

Changed work issues from rebase on #31732, #31727, #31736 to none

@mkoeppe

This comment has been minimized.

@mkoeppe

This comment has been minimized.

@mkoeppe
Copy link
Member

mkoeppe commented May 3, 2021

@mkoeppe
Copy link
Member

mkoeppe commented May 3, 2021

Commit: 743f114

@mkoeppe
Copy link
Member

mkoeppe commented May 3, 2021

Last 10 new commits:

0032bf9ManifoldSubset.intersection: Go through ManifoldSubsetFiniteFamily to make the order of operations canonical
282010bManifoldSubset._union_subset: Factor out from ManifoldSubset.union
6772570ManifoldSubset.union: Handle arbitrary unions
edfea33ManifoldSubset.union: Add example and plots
fdd0abaManifoldSubset.declare_union: Handle arbitrary unions
c2f484csrc/sage/manifolds/subset.py: import itertools
a083569ManifoldSubset._intersection_subset, _union_subset: Use declare_subset, declare_superset; fix doctest
5d41b46Merge #31764
aceb0e8ManifoldSubset._union_subset: Do cache store the union here
743f114ManifoldSubset._union_subset: Update open covers

@mkoeppe
Copy link
Member

mkoeppe commented May 3, 2021

Author: Matthias Koeppe

@mkoeppe

This comment has been minimized.

@mkoeppe mkoeppe modified the milestones: sage-9.4, sage-9.5 Jul 19, 2021
@mkoeppe mkoeppe modified the milestones: sage-9.5, sage-9.6 Dec 18, 2021
@mkoeppe mkoeppe modified the milestones: sage-9.6, sage-9.7 Mar 5, 2022
@mkoeppe mkoeppe modified the milestones: sage-9.7, sage-9.8 Aug 31, 2022
@mkoeppe mkoeppe modified the milestones: sage-9.8, sage-9.9 Jan 7, 2023
@mkoeppe mkoeppe removed this from the sage-10.0 milestone Mar 16, 2023
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

2 participants