Inequalities: return BooleanAtoms instead of bools. #2984

Merged
merged 1 commit into from Mar 15, 2014

3 participants

@randyheydon

This is the continuation of PR #2844, where Relationals were changed. Now any usage of <, <=, >, or >= will result in a BooleanAtom (S.true or S.false) being returned. To handle this, many pieces of code were changed so they no longer expect True or False.

These two pull requests also create a potential backwards-compatibility concern, as evidenced by all the code that needed correcting. Many places use (a < b) is True that had to be changed to (a < b) == True to continue working properly with this change (not to mention all the subtler issues that cropped up). Given the prevalence of this issue in SymPy itself, I imagine downstream code could have similar problems.

@randyheydon randyheydon Inequalities: return BooleanAtoms instead of bools.
Extension of a similar change to Relational objects.  Now any usage of <, <=,
>, or >= will result in a BooleanAtom (S.true or S.false) being returned.
To handle this, many pieces of code were changed so they no longer expect True
or False; some downstream code may need similar treatment.
8658f2f
@asmeurer
SymPy member

We should just add it to https://github.com/sympy/sympy/wiki/release-notes-for-0.7.6 when it is merged, as a backwards compatibility break.

@asmeurer
SymPy member

I see no new tests were added, only old tests changed. Are you convinced that the old tests cover all the cases? If not, you should add more.

Other than that, I am +1.

@randyheydon

I can't be 100% sure that all cases are covered, because I'm not 100% familiar with all of the modules affected. To the best of my knowledge, no extra tests should be needed, but I would love to get some more input on that from anyone who might know the affected areas better.

@skirpichev skirpichev commented on the diff Mar 2, 2014
sympy/core/containers.py
@@ -94,10 +94,10 @@ def _to_mpmath(self, prec):
return tuple([a._to_mpmath(prec) for a in self.args])
def __lt__(self, other):
- return self.args < other.args
+ return sympify(self.args < other.args)

Shouldn't this be Boolean(self.args < other.args) instead (more specific)?

Boolean (and BooleanAtom) don't have this functionality. Neither specifies a __new__ or __init__ method, so there's no conversion to the singleton objects.

@smichr
SymPy member
smichr added a note Mar 3, 2014

I saw elsewhere someone using _sympify -- that might be a better option.

@smichr
SymPy member
smichr added a note Mar 3, 2014

nevermind -- _sympify won't work with None

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

I was mainly worried aboud testing all combinations of <, <=, >, and >=.

@randyheydon

Well, test_expr.py, test_numbers.py, and test_relational.py are pretty comprehensive already; is there anything else you think should be added?

@smichr
SymPy member

I still don't understand why (... < ...) == True is used in some places and (... < ... ) is S.true in other places.

@randyheydon

I used is S.true in test code, and == True in library code (except for one place where I had to use == S.true). My thought being that, in tests, you want to know exactly what coming out of your function to make sure nothing changes unexpectedly, whereas in regular code you just want to make sure that it's not False and not a Relational - the exact object type is less important (see here for reference).

@randyheydon

Has everyone had a good look at this? Are there any further issues? I hope that this can be pulled in soon, before any other pull requests inadvertently add more is True lines.

@randyheydon

It's been over a week since the last comment. Are there any others, or is this ready to be pulled?

@asmeurer asmeurer merged commit 348981e into sympy:master Mar 15, 2014

1 check passed

Details default The Travis CI build passed
@asmeurer
SymPy member

Sorry, we are a little swamped with stuff due to GSoC stuff right now.

@randyheydon

That's okay. Thanks Aaron!

@randyheydon randyheydon deleted the randyheydon:number_ineq_STrue branch Mar 15, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment