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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 7 commits into from
Mar 4, 2019
Merged

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
018e005
returning same equation for Eq(root, Symbol) instead of False.
RituRajSingh878 Feb 11, 2019
f392871
modified code
RituRajSingh878 Feb 11, 2019
b33e6da
modified code and add test
RituRajSingh878 Feb 28, 2019
905ac79
Update test_rootoftools.py
smichr Feb 28, 2019
51b7865
remove unnecessary imports
RituRajSingh878 Mar 1, 2019
54c8ec3
Merge branch 'equation' of https://github.com/RituRajSingh878/sympy i…
RituRajSingh878 Mar 1, 2019
2568b6c
modify code
RituRajSingh878 Mar 3, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 3 additions & 1 deletion sympy/polys/rootoftools.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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):
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

return
if not (other.is_number and not other.has(AppliedUndef)):
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

right: return None if other.is_constant else False

return S.false
if not other.is_finite:
Expand Down
2 changes: 1 addition & 1 deletion sympy/polys/tests/test_rootoftools.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,7 @@ 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
Expand Down