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

Implement ConditionSet #9696

Merged
merged 22 commits into from Jul 30, 2015

Conversation

Projects
None yet
4 participants
@hargup
Copy link
Member

hargup commented Jul 20, 2015

TODO:

  • Test Args
  • printer
  • Rename CondSet to ConditionSet
    • Class Name
    • __init__.py
    • docstring
    • tests
    • test_args
    • printers: (pretty and latex)
    • printer tests: (pretty and latex)
  • Change attribute lambda -> condition
  • Tests for inequalities
  • Coverage (100%)
  • Update Release Notes

hargup added some commits Jul 20, 2015

Add CondSet
Signed-off-by: Harsh Gupta <gupta.harsh96@gmail.com>
Add Test Args for CondSet
Signed-off-by: Harsh Gupta <gupta.harsh96@gmail.com>
Fix property name in CondSet
Signed-off-by: Harsh Gupta <gupta.harsh96@gmail.com>
Add LaTeX printing for CondSet
Signed-off-by: Harsh Gupta <gupta.harsh96@gmail.com>
Add unicode and ascii pretty printer for CondSet
Signed-off-by: Harsh Gupta <gupta.harsh96@gmail.com>
@hargup

This comment has been minimized.

Copy link
Member

hargup commented Jul 20, 2015

@aktech have a look. I intend to use CondSet as the unevaluated solve class.

@hargup hargup added the solvers label Jul 20, 2015

@hargup hargup self-assigned this Jul 20, 2015

@hargup hargup added the sets label Jul 20, 2015

@moorepants

This comment has been minimized.

Copy link
Member

moorepants commented Jul 21, 2015

Quick note from the stands: "CondSet" isn't an informative name. What does "Cond" stand for?

@moorepants

This comment has been minimized.

Copy link
Member

moorepants commented Jul 21, 2015

Maybe "ConditionSet" is better?

@hargup

This comment has been minimized.

Copy link
Member

hargup commented Jul 21, 2015

Maybe "ConditionSet" is better?

yup, that would be better.

def _contains(self, other):
# XXX: probably we should check if self.cond is returning only true or
# false
return self.cond(other)

This comment has been minimized.

@aktech

aktech Jul 21, 2015

Member

self.cond(other) --> self.lamda(other)

This comment has been minimized.

@aktech

aktech Jul 21, 2015

Member

It could rather be:

        return And(self.lamda(other),
                   FiniteSet(other).is_subset(self.base_set))
@aktech

This comment has been minimized.

Copy link
Member

aktech commented Jul 21, 2015

For def _contains(self, other): , we should also check if other belongs to base_set, otherwise:

In [28]: sin_sols = CondSet(Lambda(x, Eq(sin(x), 0)), Interval(0, 2*pi))

In [29]: sin_sols
Out[29]: CondSet(Lambda(x, Eq(sin(x), 0)), [0, 2*pi])

In [30]: 3*pi in sin_sols
Out[30]: True
@aktech

This comment has been minimized.

Copy link
Member

aktech commented Jul 21, 2015

I intend to use CondSet as the unevaluated solve class.

I think it's a good idea.

Rename CondSet to ConditionSet
Signed-off-by: Harsh Gupta <gupta.harsh96@gmail.com>
@hargup

This comment has been minimized.

Copy link
Member

hargup commented Jul 26, 2015

For def _contains(self, other): , we should also check if other belongs to base_set

Nice catch Thanks!

hargup added some commits Jul 26, 2015

ConditionSet: fix contains
Signed-off-by: Harsh Gupta <gupta.harsh96@gmail.com>
Remove redundant _is_multivariate in ConditionSet
Signed-off-by: Harsh Gupta <gupta.harsh96@gmail.com>
ConditionSet: fix doctest
Signed-off-by: Harsh Gupta <gupta.harsh96@gmail.com>
ConditionSet: Add additional tests
Signed-off-by: Harsh Gupta <gupta.harsh96@gmail.com>
@hargup

This comment has been minimized.

Copy link
Member

hargup commented Jul 26, 2015

@aktech can you do a review.

>>> from sympy import Symbol, S, ConditionSet, Lambda, pi, Eq, sin
>>> x = Symbol('x')
>>> sin_sols = ConditionSet(Lambda(x, Eq(sin(x), 0)), Interval(0, 2*pi))

This comment has been minimized.

@aktech

aktech Jul 26, 2015

Member

import Interval

True
>>> pi/2 in sin_sols
False
>>> 3*pi in sin_sols

This comment has been minimized.

@aktech

aktech Jul 26, 2015

Member

output for this.

@aktech

This comment has been minimized.

Copy link
Member

aktech commented Jul 26, 2015

I think you forgot to run doctests.

True
"""
def __new__(cls, lamda, base_set):
return Basic.__new__(cls, lamda, base_set)

This comment has been minimized.

@aktech

aktech Jul 26, 2015

Member

lamda --> condition
It's good to have consistent naming.

ConditionSet: Fix doctest
Signed-off-by: Harsh Gupta <gupta.harsh96@gmail.com>
>>> 5 in ConditionSet(Lambda(x, x**2 > 4), S.Reals)
True
"""
def __new__(cls, lamda, base_set):

This comment has been minimized.

@aktech
Fix naming in ConditionSet
Signed-off-by: Harsh Gupta <gupta.harsh96@gmail.com>
@hargup

This comment has been minimized.

Copy link
Member

hargup commented Jul 26, 2015

I think you forgot to run doctests.

Thanks! fortunately we have travis.

else:
inn = 'in'
_and = 'and'
variables = self._print_seq(ts.lamda.variables)

This comment has been minimized.

@aktech

aktech Jul 26, 2015

Member

lamda --> condition

inn = 'in'
_and = 'and'
variables = self._print_seq(ts.lamda.variables)
cond = self._print(ts.lamda.expr)

This comment has been minimized.

@aktech
@@ -3101,6 +3101,14 @@ def test_pretty_sets():
assert upretty(Range(-2, -oo, -1)) == ucode_str


def test_pretty_CondSet():
from sympy import CondSet

This comment has been minimized.

@aktech
ascii_str = '{x | x in (-oo, oo) and sin(x) = 0}'
ucode_str = u'{x | x ∊ ℝ ∧ sin(x) = 0}'
assert pretty(CondSet(Lambda(x, Eq(sin(x), 0)), S.Reals)) == ascii_str
assert upretty(CondSet(Lambda(x, Eq(sin(x), 0)), S.Reals)) == ucode_str

This comment has been minimized.

@aktech
@@ -1567,6 +1567,14 @@ def _print_ImageSet(self, s):
', '.join([self._print(var) for var in s.lamda.variables]),
self._print(s.base_set))

def _print_CondSet(self, s):
vars_print = ', '.join([self._print(var) for var in s.lamda.variables])

This comment has been minimized.

@aktech

aktech Jul 26, 2015

Member

s.lamda --> s.condition

@hargup hargup changed the title Implement CondSet Implement ConditionSet Jul 27, 2015

hargup added some commits Jul 27, 2015

CondSet -> ConditionSet in printers
Signed-off-by: Harsh Gupta <gupta.harsh96@gmail.com>
Printers: ConditionSet.{lambda -> condition}
Signed-off-by: Harsh Gupta <gupta.harsh96@gmail.com>
test_args: CondSet -> ConditionSet
Signed-off-by: Harsh Gupta <gupta.harsh96@gmail.com>

hargup and others added some commits Jul 28, 2015

Fix test_args: CondSet -> ConditionSet
Signed-off-by: Harsh Gupta <gupta.harsh96@gmail.com>
don't use unicode_literals
Signed-off-by: Harsh Gupta <gupta.harsh96@gmail.com>
remove unicode_literals import test_pretty
Signed-off-by: Harsh Gupta <gupta.harsh96@gmail.com>
Remove unused imports in conditionset.py
Signed-off-by: AMiT Kumar <dtu.amit@gmail.com>
@aktech

This comment has been minimized.

Copy link
Member

aktech commented Jul 29, 2015

I have sent you a PR #11

Merge pull request #11 from aktech/condset
Remove unused imports in conditionset.py
@aktech

This comment has been minimized.

Copy link
Member

aktech commented Jul 29, 2015

+1

Merge pull request #12 from aktech/condset
Merge branch 'master' into condset and Fix Merge conflicts

aktech added a commit that referenced this pull request Jul 30, 2015

Merge pull request #9696 from hargup/condset
Implement ConditionSet

@aktech aktech merged commit 583545e into sympy:master Jul 30, 2015

1 check passed

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

@hargup hargup deleted the hargup:condset branch Aug 2, 2015

@asmeurer

This comment has been minimized.

Copy link
Member

asmeurer commented Aug 24, 2015

If the condition is always a Lambda why not make it ConditionSet(x, Eq(sin(x), 0), Interval(0, 2*pi))?

@hargup

This comment has been minimized.

Copy link
Member

hargup commented Aug 25, 2015

If the condition is always a Lambda why not make it ConditionSet(x, Eq(sin(x), 0), Interval(0, 2*pi))?

Agreed, this would be simpler interface. Fortunately there was no release after this merge so we can change the api without deprecation warnings.

@hargup

This comment has been minimized.

Copy link
Member

hargup commented Aug 25, 2015

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