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

Improved code in Expr.__lt__ and similar functions in Expr #16956

Merged
merged 7 commits into from Jun 19, 2019

Conversation

Projects
None yet
6 participants
@ShubhamKJha
Copy link
Member

commented Jun 3, 2019

References to other Issues or PRs

Fixes #16915 and #16582

Brief description of what is fixed or changed

With this, the Relational expressions evaluate in the certain conditions.

>>> from sympy import *
>>> from sympy.abc import x,y,z
>>> a = symbols('a', extended_real =True)
>>> a > oo # since a can be real or oo both results in False
False
>>> a < oo
a < oo
>>> b = symbols('b', extended_positive=False)
>>> b > 0
b > 0
>>>

Other comments

Release Notes

  • core
    • improved code in Expr._lt_ and other functions.
@sympy-bot

This comment has been minimized.

Copy link

commented Jun 3, 2019

Hi, I am the SymPy bot (v147). I'm here to help you write a release notes entry. Please read the guide on how to write release notes.

Your release notes are in good order.

Here is what the release notes will look like:

This will be added to https://github.com/sympy/sympy/wiki/Release-Notes-for-1.5.

Note: This comment will be updated with the latest check if you edit the pull request. You need to reload the page to see it.

Click here to see the pull request description that was parsed.

<!-- Your title above should be a short description of what
was changed. Do not include the issue number in the title. -->

#### References to other Issues or PRs
<!-- If this pull request fixes an issue, write "Fixes #NNNN" in that exact
format, e.g. "Fixes #1234". See
https://github.com/blog/1506-closing-issues-via-pull-requests . Please also
write a comment on that issue linking back to this pull request once it is
open. -->
Fixes #16915 and #16582


#### Brief description of what is fixed or changed
With this, the Relational expressions evaluate in the certain conditions.
```py
>>> from sympy import *
>>> from sympy.abc import x,y,z
>>> a = symbols('a', extended_real =True)
>>> a > oo # since a can be real or oo both results in False
False
>>> a < oo
a < oo
>>> b = symbols('b', extended_positive=False)
>>> b > 0
b > 0
>>>
```


#### Other comments


#### Release Notes

<!-- Write the release notes for this release below. See
https://github.com/sympy/sympy/wiki/Writing-Release-Notes for more information
on how to write release notes. The bot will check your release notes
automatically to see if they are formatted correctly. -->

<!-- BEGIN RELEASE NOTES -->
* core
  * improved code in Expr.\__lt\__ and other functions. 
<!-- END RELEASE NOTES -->

Update

The release notes on the wiki have been updated.

@asmeurer

This comment has been minimized.

Copy link
Member

commented Jun 3, 2019

I guess this doesn't affect much (assuming the Travis tests pass).

There have been discussions elsewhere about making invalid inequalities like I > 0 gives False. This would make Implies(Not(x > y), y <= x) rather than Equivalent(Not(x > y), y <= x). This could make certain simplifications on inequalities more difficult, but it would solve issues like this, because for instance x.extended_positive = False would be equivalent to (x > 0) == False.

@asmeurer asmeurer self-requested a review Jun 3, 2019

@codecov

This comment has been minimized.

Copy link

commented Jun 4, 2019

Codecov Report

Merging #16956 into master will increase coverage by 0.477%.
The diff coverage is 100%.

@@              Coverage Diff              @@
##            master    #16956       +/-   ##
=============================================
+ Coverage   73.921%   74.398%   +0.477%     
=============================================
  Files          620       622        +2     
  Lines       160369    160867      +498     
  Branches     37632     37762      +130     
=============================================
+ Hits        118547    119683     +1136     
+ Misses       36335     35864      -471     
+ Partials      5487      5320      -167
Show resolved Hide resolved sympy/core/expr.py Outdated
Show resolved Hide resolved sympy/core/expr.py Outdated
@@ -389,7 +389,7 @@ def test_issue_10198():
def test_issue_10047():
# this must remain an inequality, not True, since if x
# is not real the inequality is invalid
assert solve(sin(x) < 2) == (x <= oo)
assert solve(sin(x) < 2) == True

This comment has been minimized.

Copy link
@ShubhamKJha

ShubhamKJha Jun 6, 2019

Author Member

I changed this test because, somewhere in the execution of solve, reduce_inequalities is called on sin(x) < 2 which adds the assumptions extended_real = True on x. The result therefore changes, since with this PR x <= oo i s True for extended_real.

This comment has been minimized.

Copy link
@oscarbenjamin

oscarbenjamin Jun 8, 2019

Contributor

Is this fixable? It could be punted to a new issue if it is difficult...

This comment has been minimized.

Copy link
@ShubhamKJha

ShubhamKJha Jun 14, 2019

Author Member

I can't argue what solve(sin(x) < 2) should return. Apparently what solveset does in the same scenario is that it rejects such queries where the arguments are not real. I believe this should be the case. The whole idea of enforcing extended_real assumptions into arguments does not seem valid to me. But if pushing assumptions is required, the results I am getting is valid. From solve's perspective S.Reals should be a better result. But I don't completely understand solve's API.

This comment has been minimized.

Copy link
@oscarbenjamin

oscarbenjamin Jun 14, 2019

Contributor

I think that solve has a poorly designed API. Too many different possibilities are crammed into one function. The inequality solving in particular is misleading and should really be in a separate function.

If the inequality solver is based on the assumption that unknowns are real then the result here should clearly be True.

This comment has been minimized.

Copy link
@oscarbenjamin

oscarbenjamin Jun 15, 2019

Contributor

The comment should be updated if this is changed. The comment should describe what happened before and what is changed here

@ShubhamKJha

This comment has been minimized.

Copy link
Member Author

commented Jun 6, 2019

@oscarbenjamin I modified the logic as suggested. Is it good now?

Show resolved Hide resolved sympy/core/expr.py Outdated
@oscarbenjamin

This comment has been minimized.

Copy link
Contributor

commented Jun 13, 2019

It's worth checking if this fixes #17025

@ShubhamKJha

This comment has been minimized.

Copy link
Member Author

commented Jun 14, 2019

Apparently, the code in Add needs to be fixed first. Currently,

In [24]: x = symbols('x', negative=True)

In [25]: y = symbols('y', positive=True)

In [26]: x < y
Out[26]: True

In [27]: 2*x < y-1
Out[27]: 2*x < y - 1

In [28]: ((y-1)-2*x).is_extended_nonnegative # returns None

The return shouldn't be None since (y-1) is non-negative and 2*x is negative, the result should be True.

@ShubhamKJha

This comment has been minimized.

Copy link
Member Author

commented Jun 15, 2019

@oscarbenjamin is it good to merge?

Show resolved Hide resolved sympy/core/expr.py Outdated
@oscarbenjamin

This comment has been minimized.

Copy link
Contributor

commented Jun 15, 2019

I had a quick look and I think it looks good but don't have time to check it fully right now.

Also I think @asmeurer wanted to review this.

@oscarbenjamin

This comment has been minimized.

Copy link
Contributor

commented Jun 15, 2019

I don't think this should evaluate:

In [4]: x = Symbol('x', extended_positive=True)                                                                                   

In [5]: x < oo                                                                                                                    
Out[5]: True

The problem seems to be this:

In [12]: (x-oo).is_extended_negative                                                                                              
Out[12]: True
Show resolved Hide resolved sympy/core/expr.py Outdated
@asmeurer

This comment has been minimized.

Copy link
Member

commented Jun 17, 2019

I left a couple of line comments. I'm not sure the logic here is correct for oo > oo. oo > x should require x to be finite to evaluate. Although x > oo should always be False.

@asmeurer

This comment has been minimized.

Copy link
Member

commented Jun 17, 2019

We should also double check that all the logic does the right thing for None values. I think it does, but we should make sure. If in doubt we can replace and and or with fuzzy_and and fuzzy_or and if ... with if ... is True.

@oscarbenjamin

This comment has been minimized.

Copy link
Contributor

commented Jun 17, 2019

I think the PR is good but there's a bit of finishing to do.

  1. There is a case that isn't right: #16956 (comment) It would be good to fix that but the root of the problem is not due to the changes here so it can also be punted to a new issue.

  2. There are some comments that should be updated since they don't match the surrounding code after the changes here. I think it would be good to make sure that the new comments reference both the new and old behaviour with links to relevant PRs/issues.

  3. The is_infinite and is_extended_nonnegative etc can be made clearer by using positive/negative instead of nonnegative/nonpositive.

  4. There are one or two other unresolved comments above.

@ShubhamKJha

This comment has been minimized.

Copy link
Member Author

commented Jun 19, 2019

This can be merged now. Ping @oscarbenjamin @asmeurer.

@oscarbenjamin

This comment has been minimized.

Copy link
Contributor

commented Jun 19, 2019

Looks good to me

@asmeurer

This comment has been minimized.

Copy link
Member

commented Jun 19, 2019

Me as well. Let's make sure to open an issue for the solve thing.

@asmeurer asmeurer merged commit f4d40fb into sympy:master Jun 19, 2019

3 checks passed

codecov/project 74.398% (target 0%)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
sympy-bot/release-notes The release notes look OK
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.