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
fix(integrals): handle degenerate case in Risch PRDE #23959
Conversation
✅ Hi, I am the SymPy bot (v167). I'm here to help you write a release notes entry. Please read the guide on how to write release notes. Your release notes are in good order. Here is what the release notes will look like:
This will be added to https://github.com/sympy/sympy/wiki/Release-Notes-for-1.12. Click here to see the pull request description that was parsed.
Update The release notes on the wiki have been updated. |
sympy/integrals/prde.py
Outdated
@@ -344,7 +344,7 @@ def prde_no_cancel_b_large(b, Q, n, DE): | |||
|
|||
if all(qi.is_zero for qi in Q): | |||
dc = -1 | |||
M = zeros(0, 2, DE.t) | |||
M = zeros(0, m, DE.t) | |||
else: | |||
dc = max([qi.degree(DE.t) for qi in Q]) | |||
M = Matrix(dc + 1, m, lambda i, j: Q[j].nth(i), DE.t) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not really sure why I ever did it like this since it isn't this way in the book, but this line can just be used for both cases. I think I just didn't have a clear understanding of how empty matrices work, or possibly there was some bug with how they behaved back then, I don't know.
This is page 234 in Bronstein's book, ParamPolyRishDENoCancel1, for anyone checking. Really these docstrings ought to reference the exact pseudocode + page number in the book to make this easier.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe you did it like this because:
(Pdb) p Q[0].degree(DE.t)
-oo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh to be clear, the dc = -1 part is there because that's how it's written in the book (and the ambiguity of degree(0) is clearly the reason). I meant I don't know why I put the matrix definition inside of the if
. In the book, it's separate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I see what you mean. I've moved the matrix definition out.
cbad9cf
to
086f3b5
Compare
This line is also probably wrong Line 388 in 88664e6
|
Also I'm surprised that |
Can you think of a test-case that would hit that line?
Nothing like this surprises me any more. |
Benchmark results from GitHub Actions Lower numbers are good, higher numbers are bad. A ratio less than 1 Significantly changed benchmark results (PR vs master) Significantly changed benchmark results (master vs previous release) before after ratio
[26f7bdbe] [5eb59bda]
<sympy-1.11^0>
- 1.35±0.05ms 833±30μs 0.62 solve.TimeSparseSystem.time_linear_eq_to_matrix(10)
- 3.90±0.2ms 1.49±0.09ms 0.38 solve.TimeSparseSystem.time_linear_eq_to_matrix(20)
- 7.38±0.2ms 2.21±0.2ms 0.30 solve.TimeSparseSystem.time_linear_eq_to_matrix(30)
Full benchmark results can be found as artifacts in GitHub Actions |
086f3b5
to
7144736
Compare
I think this is ready to be merged. |
References to other Issues or PRs
Fixes #23948
Brief description of what is fixed or changed
Ensure that while the matrix is still empty it has the right number of columns.
Other comments
Release Notes
risch
integration routine was fixed. Previously in certain conditions involving complicated integrands the routine would fail withAssertionError
.