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

Fix Intersection's of FiniteSet with symbolic elements #9540

Merged
merged 8 commits into from Jul 3, 2015

Conversation

Projects
None yet
4 participants
@aktech
Copy link
Member

aktech commented Jun 18, 2015

@aktech

This comment has been minimized.

Copy link
Member Author

aktech commented Jun 18, 2015

I don't have enough idea about the failing tests.
There are some failing tests in sympy.stats .

@hargup

This comment has been minimized.

Copy link
Member

hargup commented Jun 18, 2015

These intersection or union should remain unevaluated only if its elements are symbols and the behaviour shouldn't change for elements which are fixed values.

In [3]: f1 = FiniteSet(a, b) # Elements are symbols

In [4]: f2 = FiniteSet(b, c)

In [5]: f1.intersect(f2)
Out[5]: {b} U Intersection({a}, {b, c}) # Correct

In [6]: f1 = FiniteSet('a', 'b') # Elements are strings

In [7]: f2 = FiniteSet('b', 'c')

In [8]: f1.intersect(f2)
Out[8]: {b} U Intersection({a}, {b, c}) # Wrong; should be {b}


@aktech aktech changed the title Fix Intersection's of FiniteSet with symbolic elements [WIP] Fix Intersection's of FiniteSet with symbolic elements Jun 18, 2015

@aktech

This comment has been minimized.

Copy link
Member Author

aktech commented Jun 19, 2015

@hargup
Strings doesn't have any special significance in FiniteSet, they behave identical to Symbol when mapped into a SymPy object.

In [1]: from sympy import *

In [2]: sympy_object = sympify('string')

In [3]: type(sympy_object)
Out[3]: sympy.core.symbol.Symbol

See here in FiniteSet definition.
Hence: FiniteSet(a, b, c) & FiniteSet('a', 'b', 'c') are equivalent.

In [4]: f1 = FiniteSet('a', 'b', 'c')

In [5]: a, b, c = symbols('a, b, c')

In [6]: f2 = FiniteSet(a, b, c)

In [7]: f1 == f2
Out[7]: True

@hargup hargup added the sets label Jun 19, 2015

@aktech

This comment has been minimized.

Copy link
Member Author

aktech commented Jun 21, 2015

@hargup
Please have a look at this commit. I am trying to fix failing test.

@aktech

This comment has been minimized.

Copy link
Member Author

aktech commented Jun 21, 2015

@hargup @flacjacket

In [16]: x, y, z = symbols('x y z', integer=True)

In [19]: f1 = FiniteSet(x, y)

In [20]: f2 = FiniteSet(x, z)

In Master:

In [23]: f1.intersect(f2)
Out[23]: {x}

This PR

In [5]: f1.intersect(f2)
Out[5]: {x} U Intersection({y}, {x, z})

Clearly the behavior of FiniteSet in Master is non-acceptable, since in the above example x, y, z are integer symbols, so they can be any integer, but in the Master , they are assumed to be distinct, which is wrong.
There are such failing tests in test_wester.py here, which should probably be corrected?

@jksuom

This comment has been minimized.

Copy link
Member

jksuom commented Jun 22, 2015

More explicitly f1.intersect(f2) could be Piecewise(({x, y}, y == z), ({x}, True)), but maybe that would be hard to implement in general.

@aktech

This comment has been minimized.

Copy link
Member Author

aktech commented Jun 22, 2015

@jksuom

This comment has been minimized.

Copy link
Member

jksuom commented Jun 25, 2015

test_given fails because it tests the stochastic independence of variables in the construction of B by simply comparing their symbols (pspace_independent)

A quick-and-dirty solution could be adding a keyword to intersect telling if to fall back to the old behaviour. A cleaner solution might be changing all symbols to Python sets (and modifying intersect accordingly).

@aktech aktech closed this Jun 27, 2015

@aktech aktech reopened this Jun 27, 2015

@aktech

This comment has been minimized.

Copy link
Member Author

aktech commented Jun 27, 2015

A cleaner solution might be changing all symbols to Python sets (and modifying intersect accordingly).

Is there a way to represent cases like this?

@jksuom

This comment has been minimized.

Copy link
Member

jksuom commented Jun 27, 2015

I'm afraid I don't quite understand the issue. Changing the type of probability space symbols (i.e. the set of variable names) should not have any consequences here.

@aktech

This comment has been minimized.

Copy link
Member Author

aktech commented Jun 27, 2015

@jksuom
Sorry I misinterpreted your last comment, It's perfectly fine. See changes here: ae5fc1e

@jksuom

This comment has been minimized.

Copy link
Member

jksuom commented Jun 27, 2015

Looks good to me. But maybe the type of other probability space symbols should also be changed (e.g. product space).

@aktech aktech force-pushed the aktech:fs-int branch from ae5fc1e to 4129c66 Jun 28, 2015

@aktech

This comment has been minimized.

Copy link
Member Author

aktech commented Jun 28, 2015

@jksuom
I just noticed that, we can't change the type of symbols to python sets for returning a class object in sympy, since all the args (obj.args) should be an instance of Basic in sympy. (otherwise sympy/core/tests/test_args.py would fail as here.)
So we should only change the @property symbols for internal use. See changes here.

@jksuom

This comment has been minimized.

Copy link
Member

jksuom commented Jun 28, 2015

That looks promising.

@jksuom

This comment has been minimized.

Copy link
Member

jksuom commented Jun 28, 2015

Another possibility, with no effect on the interface, would be just turning a_symbols and b_symbols to Python sets in pspace_independent.

@aktech

This comment has been minimized.

Copy link
Member Author

aktech commented Jun 29, 2015

It sounds good. Made changes here: aktech@c68c9da

@@ -1374,12 +1374,15 @@ def flatten(arg):
if len(args) == 0:
raise TypeError("Intersection expected at least one argument")

try:
args = list(ordered(args, Set._infimum_key))
except TypeError:

This comment has been minimized.

@hargup

hargup Jun 29, 2015

Member

Which scenarios raise this error?

This comment has been minimized.

@aktech
@@ -1431,8 +1434,14 @@ def reduce(args):
# all other sets in the intersection
for s in args:
if s.is_FiniteSet:
return s.func(*[x for x in s
if all(other.contains(x) == True for other in args)])
args = [a for a in args if a != s]

This comment has been minimized.

@hargup

hargup Jun 29, 2015

Member

better name: other_args

return s.func(*[x for x in s
if all(other.contains(x) == True for other in args)])
args = [a for a in args if a != s]
res = s.func(*[x for x in s

This comment has been minimized.

@hargup

hargup Jun 29, 2015

Member

res = FiniteSet(*[...]) will be more readable, it also goes with the Zen of Python #1 Explicit is better than Implicit

@@ -238,7 +238,15 @@ def test_intersect():
assert Union(Interval(0, 1), Interval(2, 3)).intersect(S.EmptySet) == \
S.EmptySet
assert Union(Interval(0, 5), FiniteSet('ham')).intersect(FiniteSet(2, 3, 4, 5, 6)) == \
FiniteSet(2, 3, 4, 5)
Union(FiniteSet(2, 3, 4, 5),
Intersection(FiniteSet(6), Union(Interval(0, 5),

This comment has been minimized.

@hargup

hargup Jun 29, 2015

Member

I guess you can ignore PEP8 here, this line break looks ugly.

@hargup hargup changed the title [WIP] Fix Intersection's of FiniteSet with symbolic elements Fix Intersection's of FiniteSet with symbolic elements Jun 29, 2015

@aktech aktech force-pushed the aktech:fs-int branch from a9a7cab to db09035 Jun 29, 2015

@aktech

This comment has been minimized.

Copy link
Member Author

aktech commented Jun 30, 2015

@jksuom @hargup is there anything else to be addressed?

@hargup

This comment has been minimized.

Copy link
Member

hargup commented Jun 30, 2015

+1, except #9540 (comment), can give me self and other sets on which is_superset function is called?

 File "/home/travis/virtualenv/pypy-2.5.0/site-packages/sympy-0.7.7.dev0-py2.7.egg/sympy/combinatorics/tests/test_partitions.py", line 26, in test_partition
    assert (a > b) is False
  File "/home/travis/virtualenv/pypy-2.5.0/site-packages/sympy-0.7.7.dev0-py2.7.egg/sympy/sets/sets.py", line 1857, in __gt__
    return self.is_proper_superset(other)
  File "/home/travis/virtualenv/pypy-2.5.0/site-packages/sympy-0.7.7.dev0-py2.7.egg/sympy/sets/sets.py", line 382, in is_proper_superset
    return self != other and self.is_superset(other)
  File "/home/travis/virtualenv/pypy-2.5.0/site-packages/sympy-0.7.7.dev0-py2.7.egg/sympy/sets/sets.py", line 357, in is_superset
    return other.is_subset(self)
  File "/home/travis/virtualenv/pypy-2.5.0/site-packages/sympy-0.7.7.dev0-py2.7.egg/sympy/sets/sets.py", line 313, in is_subset
    return self.intersect(other) == self
  File "/home/travis/virtualenv/pypy-2.5.0/site-packages/sympy-0.7.7.dev0-py2.7.egg/sympy/sets/sets.py", line 103, in intersect
    return Intersection(self, other)
  File "/home/travis/virtualenv/pypy-2.5.0/site-packages/sympy-0.7.7.dev0-py2.7.egg/sympy/sets/sets.py", line 1377, in __new__
    args = list(ordered(args, Set._infimum_key))
  File "/home/travis/virtualenv/pypy-2.5.0/site-packages/sympy-0.7.7.dev0-py2.7.egg/sympy/core/compatibility.py", line 678, in ordered
    for v in d[k]:
  File "/home/travis/virtualenv/pypy-2.5.0/site-packages/sympy-0.7.7.dev0-py2.7.egg/sympy/core/compatibility.py", line 678, in ordered
    for v in d[k]:
  File "/home/travis/virtualenv/pypy-2.5.0/site-packages/sympy-0.7.7.dev0-py2.7.egg/sympy/core/compatibility.py", line 659, in ordered
    d[f(a)].append(a)
TypeError: 'list' objects are unhashable
@aktech

This comment has been minimized.

Copy link
Member Author

aktech commented Jun 30, 2015

can give me self and other sets on which is_superset function is called?

It can be simulated by the following:

from sympy import *
from sympy.combinatorics import Partition
p1 = Partition([1, 2], [3, 4])
p2 = Partition([4], [1, 2, 3])
p2.is_proper_superset(p1)
@jksuom

This comment has been minimized.

Copy link
Member

jksuom commented Jun 30, 2015

The stats part is fine, but I didn't look into test_partitions. I understood that it was fixed. +1

@aktech aktech force-pushed the aktech:fs-int branch 2 times, most recently from 972c4c0 to 218b04b Jul 2, 2015

@aktech aktech force-pushed the aktech:fs-int branch from 218b04b to fce49df Jul 2, 2015

@hargup

This comment has been minimized.

Copy link
Member

hargup commented Jul 3, 2015

+1

hargup added a commit that referenced this pull request Jul 3, 2015

Merge pull request #9540 from aktech/fs-int
Fix Intersection's of FiniteSet with symbolic elements

@hargup hargup merged commit 91c7ead into sympy:master Jul 3, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@aktech aktech deleted the aktech:fs-int branch Jul 25, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment