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 1 commit
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
6 changes: 4 additions & 2 deletions sympy/polys/rootoftools.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -970,8 +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.has(Symbol):
return
if other.is_number and other.has(AppliedUndef) or not other.is_number:
Copy link
Member

Choose a reason for hiding this comment

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

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

# 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
Copy link
Contributor

Choose a reason for hiding this comment

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

Change this to return None if other.is_constant else False

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

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 also happy with this as it is. The current version makes sense to me even if further optimisations might be possible.

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
11 changes: 8 additions & 3 deletions sympy/polys/tests/test_rootoftools.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these extra imports needed?

Copy link
Member

Choose a reason for hiding this comment

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

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.

)

from sympy.utilities.pytest import raises
Expand Down Expand Up @@ -142,8 +142,7 @@ def test_CRootOf___eval_Eq__():
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)
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

unchanged(Eq, r, f(0))

Copy link
Contributor

Choose a reason for hiding this comment

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

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))

Copy link
Member

Choose a reason for hiding this comment

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

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).

Copy link
Contributor

Choose a reason for hiding this comment

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

@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

sol = solve(eq)
for s in sol:
if s.is_real:
Expand Down Expand Up @@ -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")
Copy link
Member

Choose a reason for hiding this comment

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

not needed

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks

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