raise value error for Eq(RootOf(), Symbol) #15923

RituRajSingh878 · 2019-02-04T15:14:55Z

References to other Issues or PRs

Brief description of what is fixed or changed

Other comments

In [20]: x = Symbol('x')                                                                                                          

In [21]: y = Symbol('y')                                                                                                          

In [22]: r = RootOf(x**5 - x + 1, 0)                                                                                              

In [23]: Eq(r, y)                                                                                                                 
Out[23]: False

As y is symbol so we do not know the exact answer of Eq(r, y) so we can't say it's answer will be false .
SO I am returning same equation as it is.

Release Notes

polys
- returning same equation for Eq(root, Symbol) instead of False.

sympy-bot · 2019-02-04T15:15:04Z

✅

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

polys
- returning same equation for Eq(root, Symbol) instead of False. (#15923 by @RituRajSingh878 and @smichr)

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
<!-- 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
Fixes #15920

#### Other comments
```
In [20]: x = Symbol('x')                                                                                                          

In [21]: y = Symbol('y')                                                                                                          

In [22]: r = RootOf(x**5 - x + 1, 0)                                                                                              

In [23]: Eq(r, y)                                                                                                                 
Out[23]: False
```
As y is symbol so we do not know the exact answer of Eq(r, y) so we can't say it's answer will be false .
SO I am returning same equation as it is.
#### 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 -->
- polys
  - returning  same equation for Eq(root, Symbol) instead of False.
<!-- END RELEASE NOTES -->

Update

The release notes on the wiki have been updated.

oscarbenjamin · 2019-02-04T18:07:39Z

An Eq should return an equation and not raise ValueError.

oscarbenjamin · 2019-02-05T10:37:04Z

Your test

    assert Eq(r, x) is not S.false

is not very informative. Virtually any object created will pass this test.

What is actually the result here? You should test for that. Probably a better test would check the lhs and rhs attributes of the Eq.

oscarbenjamin · 2019-02-06T20:31:29Z

Okay this patch looks good.

Can you combine it into one commit? I would do that with:

$ git reset HEAD~4  # Assuming 4 is the number of commits to "undo"
$ git commit
$ git push -f

Make sure to save your work before doing this in case it goes wrong.

Also can you write a more informative release note. The release note should make sense to end users so describe the effect of the change not the code that was changed. You can see previous release notes here: https://github.com/sympy/sympy/wiki/Release-Notes-for-1.3

RituRajSingh878 · 2019-02-11T12:04:41Z

@asmeurer can you check it?

oscarbenjamin · 2019-02-11T19:48:11Z

Actually I'm not sure about this. It seems like you're only checking for the case when one side of the equation is a pure symbol. It could be more complicated like Eq(RootOf(...), x*y) and

In [1]: (x*y).is_Symbol                                                                                                                                       
Out[1]: False

Rather than inserting this check can you work out which case is causing the Eq to become False? There must be some incorrect logic in there somewhere and we need to know what it is.

smichr · 2019-02-11T22:22:58Z

sympy/polys/rootoftools.py

@@ -970,6 +970,8 @@ def _eval_Eq(self, other):
        # is_real value of the CRootOf instance.
        if type(self) == type(other):
            return sympify(self == other)
+        if other.has(Symbol):


The case below would return False for Eq(root, Integral(x, (x, 1, 10))) but yours would return None. As was suggested, which if is sending back the False? It is the one making the mistake and should be fixed.

The case below would return False for Eq(root, Integral(x, (x, 1, 10)))
but yours would return None

In my case also it is returning false -

In [1]: r = RootOf(x**5 - x + 1, 0) In [2]: Eq(r, Integral(x, (x, 1, 10))) Out[2]: False

We need to figure out the problem before deciding how to fix it

In my case also it is returning false

Then it is not doing so from this function because the first check is false and your check is true and would return None.

RituRajSingh878 · 2019-02-12T15:56:13Z

Rather than inserting this check can you work out which case is causing the Eq to become False? There must be some incorrect logic in there somewhere and we need to know what it is.

I am returning the same equation whenever rhs has Symbol So I think this logic is fine.

smichr · 2019-02-13T11:54:57Z

So I think this logic is fine.

But we are burying a problem with a patch. This needs more investigation to see from where the wrong value is coming from.

oscarbenjamin · 2019-02-13T13:34:03Z

This check is incorrectly returning False in this case:

sympy/sympy/polys/rootoftools.py

Line 964 in 946be88

if not (other.is_number and not other.has(AppliedUndef)):

I checked the blame but it's just been there since the method was added so not informative.

smichr · 2019-02-13T15:15:59Z

I checked the blame but it's just been there since the method was added

So that's saying 'if it's not a number or if it's a number with undefined functions, return False'. That should probably just return None if there are undefined functions because the expression would have to be simplified to see if the unknown values disappeared. So the logic should be

if other.is_number and other.has(AppliedUndef) or not other.is_number:
    # it's a number that can't be evaluated or it's not a number in its current form
    # though simplification might identify it as such (but that is the user's choice)
    return None

But in the mods that were made, he said that Eq(RootOf(x**5 - x + 1, 0), Integral(x, (x, 1, 10))) returned False and I don't see how that is happening because the integral has a symbol.

oscarbenjamin · 2019-02-13T17:09:54Z

I can confirm that with the PR as it stands (hasn't changed since) Eq(RootOf(x**5 - x + 1, 0), Integral(x, (x, 1, 10))) results in False. What happens is that CRootOf._eval_eq returns None but then further on Eq.new evaluates the two sides numerically and discovers that they are unequal. The integral only has a dummy symbol but we can change it to actually depend on a symbol:

In [2]: r = rootof(x**5-x+1,0)                                                                                                                                

In [3]: Eq(r, Integral(x, (x, 1, y)))                                                                                                                         
Out[3]: 
                         y     
       ⎛ 5           ⎞   ⌠     
CRootOf⎝x  - x + 1, 0⎠ = ⎮ x dx
                         ⌡     
                         1

@RituRajSingh878 these examples are good candidates for tests that you could add. Also can you change the PR to be as suggested by @smichr above?

oscarbenjamin

Looks good. I left one comment.

oscarbenjamin · 2019-02-28T18:39:22Z

sympy/polys/tests/test_rootoftools.py

@@ -12,7 +12,7 @@

 from sympy import (
    S, sqrt, I, Rational, Float, Lambda, log, exp, tan, Function, Eq,
-    solve, legendre_poly
+    solve, legendre_poly, Symbol, Integral


Are these extra imports needed?

Yes and No. They are both used in test 15920, but x is already imported at the top from abc so needs not to be recreated.

smichr · 2019-02-28T23:45:43Z

sympy/polys/tests/test_rootoftools.py

@@ -566,3 +565,9 @@ def test_eval_approx_relative():
    assert [str(i) for i in a] == [
        '-0.10', '0.05 - 3.2*I', '0.05 + 3.2*I']
    assert all(abs(((a[i] - t[i])/t[i]).n()) < 1e-2 for i in range(len(a)))
+
+def test_issue_15920():
+    x = Symbol("x")


not needed

please put space around -+ operators (below) when fixing

I patched it up. This should be ready to go.

…nto equation

smichr · 2019-03-02T13:14:24Z

sympy/polys/rootoftools.py

@@ -970,6 +970,10 @@ def _eval_Eq(self, other):
        # is_real value of the CRootOf instance.
        if type(self) == type(other):
            return sympify(self == other)
+        if other.is_number and other.has(AppliedUndef) or not other.is_number:


other will never have AppliedUndef if other.is_number is True:

>>> f(1).is_number False

So I think this should be

if not other.is_number: return # simplification by user might make it a number

smichr · 2019-03-02T13:21:01Z

sympy/polys/rootoftools.py

+        if other.is_number and other.has(AppliedUndef) or not other.is_number:
+            # it's a number that can't be evaluated or it's not a number in its current form
+            # though simplification might identify it as such (but that is the user's choice)
+            return None
        if not (other.is_number and not other.has(AppliedUndef)):


These are mutually exclusive so if it is a number it doesn't have AppliedUndef so this simplifies to if not other.is_number...but that is what the previous if-block is testing so this is redundant.

It might make more sense to do

if not other.is_number: # it might be a number after simplification but it isn't right now # but if it is known that it is not constant, then it can't be a # RootOf value (hence False) return other.is_constant

But I would tend to just return the None and let the user decide if they want to attempt simplification; I know equals is expensive, but I forget how expensive is_constant is.

I didn't change this online because you might want to test it locally before making this change.

I'm not sure I understand this. Surely we don't want to return True just because other.is_constant is True? That doesn't imply that these two are equal.

right: return None if other.is_constant else False

oscarbenjamin · 2019-03-03T15:16:54Z

sympy/polys/rootoftools.py

-        if not (other.is_number and not other.has(AppliedUndef)):
-            return S.false
+        if not other.is_number:
+            return None


Change this to return None if other.is_constant else False

Actually sorry I see that this might work okay. Have you tested it on your computer? Is it better than the other suggestions?

Have you tested it on your computer? Is it better than the other suggestions?

I have tested it and I think it is a better suggestion.

Those were not really yes/no questions. Can you explain what the difference is? Why is this version better? What examples are handled differently?

return None is a fast, conservative (safe) result; using is_constant() method by default will inolve simplification but simplify=False can be passed. In that case, two random numerical evaluations will be attempted to see if the results are the different (hence not constant) or not (return None since it could be a fluke). We could think of Eq as a "low hanging fruit" method which tests for really obvious cases and equals/simplification/expression manipulation as being the much more expensive 'help me out, here' methods.

I am happy with this as it is. It would be interesting to see if foo.is_constant(simplify=False) can be used with any profit here or in other +eval_Eq methods.

I'm also happy with this as it is. The current version makes sense to me even if further optimisations might be possible.

oscarbenjamin · 2019-03-04T10:44:56Z

sympy/polys/tests/test_rootoftools.py

    assert Eq(r, 0) is S.false
    assert Eq(r, S.Infinity) is S.false
    assert Eq(r, I) is S.false
-    assert Eq(r, f(0)) is S.false
-    assert Eq(r, f(0)) is S.false
+    assert Eq(r, f(0)).lhs is r and Eq(r, f(0)).rhs is f(0)


Is it guaranteed that is will do the right thing here? Is f(0) guaranteed to be a singleton?

This is the type of test that I wrote this function for; it would be used like:

unchanged(Eq, r, f(0))

That would be useful but wasn't include...

Maybe this test for now should just be:

eq = Eq(r, f(0)) assert isinstance(eq, Eq) and eq.args == (r, f(0))

I think using is is ok here since is tells if the two operands are the same object...which they are. In this case they are not something like S.One and Python 1. If this is an issue it can be fixed (and should maybe be part of code quality testing).

@smichr this only works because of caching otherwise f(0) is not f(0):

$ cat t.py from sympy import * f = Function('f') print(f(0) is f(0)) $ python t.py True $ SYMPY_USE_CACHE=no python t.py False

oscarbenjamin added the core label Feb 4, 2019

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

returning same equation for Eq(root, Symbol) instead of False.

018e005

modified code

f392871

smichr reviewed Feb 11, 2019

View reviewed changes

oscarbenjamin mentioned this pull request Feb 18, 2019

Eq(RootOf(), y) evaluates as False #15920

Closed

modified code and add test

b33e6da

oscarbenjamin reviewed Feb 28, 2019

View reviewed changes

smichr reviewed Feb 28, 2019

View reviewed changes

smichr and others added 3 commits February 28, 2019 17:46

Update test_rootoftools.py

905ac79

remove unnecessary imports

51b7865

Merge branch 'equation' of https://github.com/RituRajSingh878/sympy i…

54c8ec3

…nto equation

smichr reviewed Mar 2, 2019

View reviewed changes

modify code

2568b6c

oscarbenjamin reviewed Mar 3, 2019

View reviewed changes

oscarbenjamin reviewed Mar 4, 2019

View reviewed changes

smichr merged commit 2f7882e into sympy:master Mar 4, 2019

oscarbenjamin mentioned this pull request Mar 6, 2019

add function to test for unchanged expression #16177

Closed

raise value error for Eq(RootOf(), Symbol) #15923

raise value error for Eq(RootOf(), Symbol) #15923

Conversation

RituRajSingh878 commented Feb 4, 2019 • edited

References to other Issues or PRs

Brief description of what is fixed or changed

Other comments

Release Notes

sympy-bot commented Feb 4, 2019 • edited

oscarbenjamin commented Feb 4, 2019

oscarbenjamin commented Feb 5, 2019

oscarbenjamin commented Feb 6, 2019

RituRajSingh878 commented Feb 11, 2019

oscarbenjamin commented Feb 11, 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

RituRajSingh878 commented Feb 12, 2019

smichr commented Feb 13, 2019

oscarbenjamin commented Feb 13, 2019

smichr commented Feb 13, 2019

oscarbenjamin commented Feb 13, 2019

oscarbenjamin left a 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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RituRajSingh878 commented Feb 4, 2019 •

edited

sympy-bot commented Feb 4, 2019 •

edited