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

Misc fixes #1115

Merged
merged 3 commits into from Feb 7, 2021
Merged

Misc fixes #1115

merged 3 commits into from Feb 7, 2021

Conversation

skirpichev
Copy link
Collaborator

No description provided.

@skirpichev skirpichev added this to the 0.13 milestone Jan 27, 2021
@skirpichev skirpichev marked this pull request as ready for review January 29, 2021 09:52
@skirpichev
Copy link
Collaborator Author

@AaronStiff, I'm glad you are using part of this work, related to sympy/sympy#20902, but please remember that such sorting (either in the solve_poly_inequality() or Poly.real_roots()) is just a workaround.

I think that the correct solution - is fixing is_disjoint() method of RealInterval by using strict inequalities.

@skirpichev
Copy link
Collaborator Author

@AaronStiff, please don't try to ping me in the sympy repo. I will be not notified about, in general.

This time I've seen your question, so I'll try to addess this. The problem is that solve_poly_inequalities() assume that (distinct) real roots are sorted by value. And such sorting should be expected after RooOf._reals_sorted(). Unfortunately, it doesn't work if compared intervals intersect by boundaries, e.g. corresponding left and right ends are equal. IIRC, in sympy/sympy#20902 the problem polynomial is something like (x + 1)**3*(3*x + 1). In this case, for distinct roots, we have intervals like r1=[-1, -1] (this root is from the ground domain, i.e. ZZ) and something like r2=[-1, 0] for x=-1/3. Second root is not going to be improved by sorting helper, since r1.b==r2.a.

I think it's perfectly fine to keep improving roots in such cases, using strict inequalities in the RealInterval.is_disjoint(), as it was suggested. I would appreciate @jksuom feedback (see above commit), for better safety.

@AaronStiff
Copy link

Oh sorry about that. Thanks for explaining!

@skirpichev skirpichev added the wrong answer if mathematically wrong result was obtained label Feb 6, 2021
@skirpichev
Copy link
Collaborator Author

Ok, rebased fix for RealInterval goes to #1117, merged.

@skirpichev skirpichev added bug an unexpected problem or unintended behavior core interactive testing and removed wrong answer if mathematically wrong result was obtained documentation labels Feb 6, 2021
@skirpichev skirpichev merged commit 7f24859 into diofant:master Feb 7, 2021
@skirpichev skirpichev deleted the misc branch February 7, 2021 00:45
@diofant diofant deleted a comment from github-actions bot Nov 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug an unexpected problem or unintended behavior core interactive maintainability polys testing
Development

Successfully merging this pull request may close these issues.

None yet

2 participants