Added use of real_roots in _solve_as_poly #24898
Conversation
…mial are rational and domain is R or a subset of R
|
✅ Hi, I am the SymPy bot. I'm here to help you write a release notes entry. Please read the guide on how to write release notes.
Click here to see the pull request description that was parsed. |
|
This has caused some test failures that need to be investigated. It is possible that some other places in the code need to be updated to handle RootOf. It is also likely that there are some tests that would need to be changed because they assert that the output of solve is something other than a RootOf. |
| if f.is_polynomial(symbol): | ||
| solns = roots(f, symbol, cubics=True, quartics=True, | ||
| if (domain.is_subset(S.Reals) or domain==S.Reals) and (f.atoms(Symbol)==f.free_symbols) and (FiniteSet(*Poly(f,symbol).coeffs()).is_subset(S.Rationals)): | ||
| solns=FiniteSet(*real_roots(Poly(f,symbol))).evalf() | ||
| return solns | ||
| else: | ||
| solns = roots(f, symbol, cubics=True, quartics=True, |
There was a problem hiding this comment.
I would write this like:
if f.is_polynomial(symbol):
f_poly = f.as_poly(Symbol)
if f_poly.domain.is_QQ:
if domain.is_subset(S.Reals):
result = FiniteSet(*f_poly.real_roots())
else:
result = FiniteSet(*f_poly.all_roots())
else:
solns = roots(f_poly, ...)
...All branches will ultimately end up converting to Poly so we might as well do that at the start and then the poly domain tells us if the coefficients are rational more easily.
It would be better in general though if this was added as an option to the roots function so you can just do something like:
solns = roots(f, symbol, rootof=True, ...)This is not the only place that would benefit from being able to call roots and have it return RootOf when possible.
There was a problem hiding this comment.
I'll look into the failed tests and try to solve the issue. Thanks a lot for the advice on improving the code and all the help.
There was a problem hiding this comment.
I believe this PR may be closed now as #24904 supersedes this in functionality. Would love to contribute if more changes can be made (if any are left out by your PR).
There was a problem hiding this comment.
No gh-24904 does not supersede the changes here. The intention there is just to make the roots function more maintainable which would make it easier to add something like the rootof parameter that I mentioned above. For now though that will not solve the problems that this PR aims to solve.
|
@HeisenberG2575 It is your first PR you need to add your name and email in .mailmap file, read here for instructions |
Added use of real_roots in _solve_as_poly when coefficients are rational numbers and the domain is a subset of Reals.
Fixes #22793
Previously roots were being calculated by roots() in all cases, however a much simpler approach of using real_roots can be used if the domain is R and the coefficients are rational numbers.
NO ENTRY