Fix simplification of Eq(pi/L, 0) to None from False with L positive #16027

divyanshu132 · 2019-02-19T19:34:30Z

References to other Issues or PRs

Brief description of what is fixed or changed

previously:

>>> x=symbols('x', positive=True)
>>> Eq(pi/x, 0)
False

After the fix:

>>> x=symbols('x', positive=True)
>>> Eq(pi/x, 0)
Eq(pi/x, 0) 

>>> Eq(pi/x, 0).subs(x, oo)
True

Other comments

Release Notes

core
- Improve finiteness simplification of equation

sympy-bot · 2019-02-19T19:34:42Z

✅

Hi, I am the SymPy bot (v137). 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:

core
- Improve finiteness simplification of equation (#16027 by @divyanshu132)

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

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
Fixes #12690
<!-- 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. -->


#### Brief description of what is fixed or changed

previously:

```
>>> x=symbols('x', positive=True)
>>> Eq(pi/x, 0)
False

```
After the fix:

```
>>> x=symbols('x', positive=True)
>>> Eq(pi/x, 0)
Eq(pi/x, 0) 

>>> Eq(pi/x, 0).subs(x, oo)
True
```

#### 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
   *  Improve finiteness simplification of equation 
<!-- END RELEASE NOTES -->

divyanshu132 · 2019-02-19T19:36:22Z

@asmeurer please review.

divyanshu132 · 2019-02-19T19:45:05Z

Some of the tests will fail because I was not sure whether we should return None or Eq(pi/x, 0) for the test case Eq(pi/x, 0) where x is finite but as mentioned in the issue I have returned None, previously for Eq(pi/x, 1/x) as I've mentioned above it was returning Eq(pi/x, 1/x) which was of no use and the tests will fail because some of them are using .subs to check the equality of the above type equations which will not be possible now to perform .subs on None. I have not changed those tests for now, waiting for your suggestion @asmeurer.

asmeurer · 2019-02-19T21:24:40Z

Eq should not return None. It should either reduce to True or False, or remain unevaluated.

divyanshu132 · 2019-02-20T02:25:49Z

@asmeurer please review.

oscargus · 2019-02-20T10:55:10Z

sympy/core/tests/test_relational.py

@@ -17,7 +17,7 @@ def test_rel_ne():

    # issue 6116
    p = Symbol('p', positive=True)
-    assert Ne(p, 0) is S.true


These tests should be kept. We still want that behavior.

For the current master:

>>> from sympy import * >>> from sympy.core.relational import Ne >>> p=symbols('p') >>> assert Ne(p, 0) is S.true Traceback (most recent call last): File "<stdin>", line 1, in <module> AssertionError >>>

But, I don't know how its clearing the tests. Can you please check it once.

p must be positive.

from sympy import Ne, Symbol, S p = Symbol('p', positive=True)

gives:

In [8]: Ne(p, 0) == S.true Out[8]: True In [9]: Ne(p, 0) is S.true Out[9]: True

The thing is that if you change the tests you must be sure that the new behavior is the behavior you want.

oscargus · 2019-02-20T10:55:33Z

sympy/core/tests/test_relational.py

@@ -92,7 +92,7 @@ def test_Eq():

    # issue 6116
    p = Symbol('p', positive=True)
-    assert Eq(p, 0) is S.false


This too. The new test doesn't really test the same thing.

similar behaviour here as well.

In my current master:

In [1]: from sympy import Eq, Symbol, S In [2]: p = Symbol('p', positive=True) In [3]: Eq(p, 0) is S.false Out[3]: True

ohh thanks, I was missing the positive.

oscargus · 2019-02-20T10:58:06Z

You must keep all the old tests. I only commented explicitly on a few. The whole idea is to be able to simplify in certain cases. While you are correct in that the simplification you point out should not be simplified, it cannot lead to that other which should are not simplified (unless there are some really good arguments for that).

divyanshu132 · 2019-02-21T22:41:58Z

@oscargus please review. I've modified the tests only for fin = symbols('inf', finite=True) which involves finite=True.

oscargus · 2019-02-21T23:08:22Z

sympy/core/tests/test_relational.py

@@ -693,19 +694,19 @@ def test_issue_10401():

    assert Eq(1/(1/x + 1), 1).func is Eq
    assert Eq(1/(1/x + 1), 1).subs(x, S.ComplexInfinity) is S.true
-    assert Eq(1/(1/fin + 1), 1) is S.false
+    assert Eq(1/(1/fin + 1), 1).subs(fin, 1) is S.false


The problem with using subs here is that you change the test from testing if the method can determine that this is false just based on the properties of fin to testing if 1/2 is equal to 1. Which is not the same thing.

Same holds for all other tests where subs is added.

so, should we use func??

I don't understand why you've changed these tests at all.

You should try to avoid changing tests unless you know for sure that it is wanted, not only to make the test pass, but because the test was wrong/the behavior changed. So just leave all tests as is and possibly add new tests for #12690

fix-equation

9771e56

fix-equation

89a9047

divyanshu132 closed this Feb 20, 2019

divyanshu132 reopened this Feb 20, 2019

sylee957 added the core label Feb 20, 2019

oscargus reviewed Feb 20, 2019

View reviewed changes

Abdullahjavednesar added the PR: author's turn The PR has been reviewed and the author needs to submit more changes. label Feb 21, 2019

fix-relational

57ee3ed

oscargus reviewed Feb 21, 2019

View reviewed changes

Merge branch 'master' into fix-Equation

0476e01

divyanshu132 closed this May 29, 2019

divyanshu132 deleted the fix-Equation branch May 29, 2019 16:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix simplification of Eq(pi/L, 0) to None from False with L positive #16027

Fix simplification of Eq(pi/L, 0) to None from False with L positive #16027

divyanshu132 commented Feb 19, 2019 •

edited

Loading

sympy-bot commented Feb 19, 2019 •

edited

Loading

divyanshu132 commented Feb 19, 2019

divyanshu132 commented Feb 19, 2019 •

edited

Loading

asmeurer commented Feb 19, 2019

divyanshu132 commented Feb 20, 2019

oscargus Feb 20, 2019

divyanshu132 Feb 21, 2019

oscargus Feb 21, 2019

oscargus Feb 20, 2019

divyanshu132 Feb 21, 2019

oscargus Feb 21, 2019

divyanshu132 Feb 21, 2019

oscargus commented Feb 20, 2019

divyanshu132 commented Feb 21, 2019

oscargus Feb 21, 2019

divyanshu132 Feb 21, 2019

oscarbenjamin Feb 23, 2019

oscargus Feb 24, 2019

Fix simplification of Eq(pi/L, 0) to None from False with L positive #16027

Fix simplification of Eq(pi/L, 0) to None from False with L positive #16027

Conversation

divyanshu132 commented Feb 19, 2019 • edited Loading

References to other Issues or PRs

Brief description of what is fixed or changed

Other comments

Release Notes

sympy-bot commented Feb 19, 2019 • edited Loading

divyanshu132 commented Feb 19, 2019

divyanshu132 commented Feb 19, 2019 • edited Loading

asmeurer commented Feb 19, 2019

divyanshu132 commented Feb 20, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oscargus commented Feb 20, 2019

divyanshu132 commented Feb 21, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

divyanshu132 commented Feb 19, 2019 •

edited

Loading

sympy-bot commented Feb 19, 2019 •

edited

Loading

divyanshu132 commented Feb 19, 2019 •

edited

Loading