Repository: sympy/sympy
Pull request: raise value error for Eq(RootOf(), Symbol)
<!-- 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 -->


Problem statement:
Eq(RootOf(), y) evaluates as False
Creating an Eq with a `RootOf` evaluates as False.
```julia
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
```
Since `y` is Symbol there is no way to know that it isn't equal to `r` so this should do the same as
```julia
In [24]: Eq(1, y)                                                                                                                 
Out[24]: 1 = y
```

Hints from issue discussion:
This is where it becomes false:

https://github.com/sympy/sympy/blob/27f2f10b965f912ea38bb189e6b498ec00efdb08/sympy/polys/rootoftools.py#L964-L965

```
In [10]: y.has(AppliedUndef)
Out[10]: False

In [11]: y.is_number
Out[11]: False
```

As I teach digital design at the moment, I cannot help thinking that the if-clause is equivalent to:
`not other.is_number or other.has(AppliedUndef)`, so clearly it is the `other.is_number` property that makes it fail.
@oscarbenjamin  instead of `False`. What should be the output?
I think we can raise `ValueError`.
The result should be `Eq(RootOf(x**5 - x + 1, 0), y)`.

Not sure if I understand the questions, but if I do: most solutions are merged. Sometimes the solutions are too focused (like making sure that only this particular polynomial works correctly), but general solutions not breaking anything else are more or less guaranteed to be merged.

For this particular problem there is an ongoing PR which seems to progress so there may be better ones to spend effort.
Without knowing the details in your case, it is quite common that issues are fixed in a very specific way and therefore not merged. Where that line is drawn is hard to in general. On one hand one can think that it is better to have a very specific fix than none and that someone else will fix it later. However, that rarely happens.

It takes a few attempts to get to know the code base and provide a good fix, so keep going! Personally, I have spent quite some time to improve the code quality without really adding any new functionality. In that way one gets to see lots of code and know Sympy a little bit better. There are still many things to clean up and improve from a coding perspective.
@XtremeGood This issue is not the right place to discuss this (although I don't know what is the right place) but I will say this here: I think that your work in #15694 will get merged. Currently the code you wrote is in the first commit of #15992 as you can see here: [https://github.com/sympy/sympy/pull/15992/commits](https://github.com/sympy/sympy/pull/15992/commits)
I can't say exactly when that will be merged because there are other problems to solve before that PR is ready.
Actually it looks to me like #15923 has stalled. The proper fix is probably as described in [this comment](https://github.com/sympy/sympy/pull/15923#issuecomment-463236979) although it would need more tests to see that it does the right thing in different cases.

Commit message: modified code and add test

Patch to review:
diff --git a/sympy/polys/rootoftools.py b/sympy/polys/rootoftools.py
index 4a65b15822ae..749572848bec 100644
--- a/sympy/polys/rootoftools.py
+++ b/sympy/polys/rootoftools.py
@@ -3,7 +3,7 @@
 from __future__ import print_function, division
 
 from sympy.core import (S, Expr, Integer, Float, I, oo, Add, Lambda,
-    symbols, sympify, Rational, Dummy)
+    symbols, sympify, Rational, Dummy, Symbol)
 from sympy.core.cache import cacheit
 from sympy.core.function import AppliedUndef
 from sympy.functions.elementary.miscellaneous import root as _root
@@ -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:
+            # 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)):
             return S.false
         if not other.is_finite:
diff --git a/sympy/polys/tests/test_rootoftools.py b/sympy/polys/tests/test_rootoftools.py
index 138d51f77c27..afdcb84a1538 100644
--- a/sympy/polys/tests/test_rootoftools.py
+++ b/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
 )
 
 from sympy.utilities.pytest import raises
@@ -138,12 +138,11 @@ def test_CRootOf___eval_Eq__():
     r1 = rootof(eq, 1)
     assert Eq(r, r1) is S.false
     assert Eq(r, r) is S.true
-    assert Eq(r, x) is S.false
+    assert Eq(r, x).lhs is r and Eq(r, x).rhs is x
     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)
     sol = solve(eq)
     for s in sol:
         if s.is_real:
@@ -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")
+    r = rootof(x**5-x+1,0)
+    p = Integral(x, (x, 1, y))
+    assert Eq(r, p).lhs is r and Eq(r, p).rhs is p


Code review comments:
diff_hunk: @@ -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
path: sympy/polys/tests/test_rootoftools.py
text: @user1:
Are these extra imports needed?

@user2:
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.