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.

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

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

Closed
oscarbenjamin opened this issue Feb 4, 2019 · 7 comments · Fixed by #15923
Closed

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

oscarbenjamin opened this issue Feb 4, 2019 · 7 comments · Fixed by #15923

Comments

@oscarbenjamin
Copy link
Contributor

Creating an Eq with a RootOf evaluates as False.

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

In [24]: Eq(1, y)                                                                                                                 
Out[24]: 1 = y
@oscargus
Copy link
Contributor

oscargus commented Feb 4, 2019

This is where it becomes false:

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

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.

@RituRajSingh878
Copy link
Contributor

RituRajSingh878 commented Feb 4, 2019

@oscarbenjamin instead of False. What should be the output?
I think we can raise ValueError.

@oscarbenjamin
Copy link
Contributor Author

The result should be Eq(RootOf(x**5 - x + 1, 0), y).

@oscargus
Copy link
Contributor

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.

@oscargus
Copy link
Contributor

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.

@oscarbenjamin
Copy link
Contributor Author

@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
I can't say exactly when that will be merged because there are other problems to solve before that PR is ready.

@oscarbenjamin
Copy link
Contributor Author

Actually it looks to me like #15923 has stalled. The proper fix is probably as described in this comment although it would need more tests to see that it does the right thing in different cases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants