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

Relational: bool(Relational) should raise #2350

Merged
merged 10 commits into from
Aug 7, 2013
Merged

Conversation

jrioux
Copy link
Member

@jrioux jrioux commented Aug 4, 2013

Continuation of #1646. See issue 2832.

@asmeurer
Copy link
Member

asmeurer commented Aug 4, 2013

Nice. There are test failures, but once we get them all ironed out, let's get this in.

@jrioux
Copy link
Member Author

jrioux commented Aug 5, 2013

SymPy Bot Summary: 🔴 Failed after merging jrioux/relational (b6ea43b) into master (e5dbc80).
@jrioux: Please fix the test failures.
PyPy 2.0.0-beta-1; 2.7.3-final-42: pass
Python 2.7.2-final-0: pass
🔴 Python 3.2.1-final-0: fail
Sphinx 1.1.3: pass
Doc Coverage: unchanged
36.103% of functions have doctests (compared to 36.103% in master)
41.509% of functions are imported into Sphinx (compared to 41.509% in master)

@jrioux
Copy link
Member Author

jrioux commented Aug 5, 2013

SymPy Bot Summary: ✅ Passed after merging jrioux/relational (abf77e5) into master (c18f063).
PyPy 2.0.0-beta-1; 2.7.3-final-42: pass
Python 2.7.2-final-0: pass
Python 3.2.1-final-0: pass
Sphinx 1.1.3: pass
Doc Coverage: unchanged
36.097% of functions have doctests (compared to 36.097% in master)
41.502% of functions are imported into Sphinx (compared to 41.502% in master)

@asmeurer
Copy link
Member

asmeurer commented Aug 6, 2013

I guess this behavior should be explicitly tested somewhere as well.

@jrioux
Copy link
Member Author

jrioux commented Aug 6, 2013

SymPy Bot Summary: ✅ Passed after merging jrioux/relational (4c85b3f) into master (7291221).
PyPy 2.0.0-beta-1; 2.7.3-final-42: pass
Python 2.7.2-final-0: pass
Python 3.2.1-final-0: pass
Sphinx 1.1.3: pass
Doc Coverage: unchanged
36.065% of functions have doctests (compared to 36.065% in master)
41.552% of functions are imported into Sphinx (compared to 41.552% in master)

@asmeurer
Copy link
Member

asmeurer commented Aug 7, 2013

+1.

@asmeurer
Copy link
Member

asmeurer commented Aug 7, 2013

There is some stuff in the Greater docstring about chained inequalities that needs to be updated.

@jrioux
Copy link
Member Author

jrioux commented Aug 7, 2013

What would you like to see changed? Everything in that docstring still applies, doesn't it?

@asmeurer
Copy link
Member

asmeurer commented Aug 7, 2013

I saw at least one part that indicated that relational evaluate to booleans (the numbered list).

@certik
Copy link
Member

certik commented Aug 7, 2013

+1 from me.

@asmeurer
Copy link
Member

asmeurer commented Aug 7, 2013

This is just waiting on that fix to the docstring.

@jrioux
Copy link
Member Author

jrioux commented Aug 7, 2013

SymPy Bot Summary: ✅ Passed after merging jrioux/relational (9138eb7) into master (3a92446).
PyPy 2.0.0-beta-1; 2.7.3-final-42: pass
Python 2.7.2-final-0: pass
Python 3.2.1-final-0: pass
Sphinx 1.1.3: pass
Doc Coverage: unchanged
36.059% of functions have doctests (compared to 36.059% in master)
41.562% of functions are imported into Sphinx (compared to 41.562% in master)

jrioux added a commit that referenced this pull request Aug 7, 2013
Relational: bool(Relational) should raise

Continuation of #1646. See issue 2832.
@jrioux jrioux merged commit 0ef7411 into sympy:master Aug 7, 2013
@asmeurer
Copy link
Member

asmeurer commented Aug 7, 2013

Awesome. Thanks for doing this. Can you update the release notes for the next release?

@jrioux jrioux mentioned this pull request Aug 17, 2013
@asmeurer
Copy link
Member

I should have reviewed this more closely. Several of the if a < b should have been left alone instead of changed to if (a < b) is True, because if they are symbolic, then it is a bug. This is definitely the case for the example in rde.py.

@@ -374,7 +374,7 @@ def spde(a, b, c, n, DE):
pow_a = 0

while True:
if n < 0:
if (n < 0) is True:
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean this change?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. n should always be an integer here. I've no doubt this same issue is the case in many other places as well. I would say to not use is True unless the tests specifically demonstrate the symbolic arguments are possible.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's exactly what I did. I did not automatically change (a > b) into (a > b) is True, but I did so when it would fix a failing test, and the logic of the code seemed to work this way.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This also means that lines that were not covered by tests (or only covered by slow tests) could still raise the symbolic error.

@jrioux
Copy link
Member Author

jrioux commented Aug 19, 2013

If you do mean the above change, changing it back will get a test failure in test_rde.py. Since you added the test, you're probably the best person to decide if the test was wrong.

@asmeurer
Copy link
Member

Ah OK, I stand corrected then. That test can probably be removed. I was trying to be clever and show that it could handle some general case mentioned in Bronstein's book, but it obviously isn't designed to work that way.

@asmeurer
Copy link
Member

SymPy Bot Summary: 🔴 Failed after merging jrioux/relational (0ef7411) into master (1b1f247).
@jrioux: Please fix the test failures.
🔴 Python 2.6.8-final-0: fail
🔴 Python 2.7.3-final-0: fail
🔴 PyPy 1.8.0-final-0; 2.7.2-final-42: fail
Python 3.2.3-final-0: pass
🔴 Sphinx 1.1.3: fail

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants