-
-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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(polys): fix apart(full=True) with floats #26649
Conversation
Don't assume that the factors divide the original polynomial exactly because if the domain is inexact then they might not. Instead compute the quotient and discard the remainder. Fixes sympygh-26648
✅ 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. 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.13. Click here to see the pull request description that was parsed.
Update The release notes on the wiki have been updated. |
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) | Change | Before [a36a8b23] <sympy-1.12.1^0> | After [71005aa0] | Ratio | Benchmark (Parameter) |
|----------|--------------------------------------|---------------------|---------|----------------------------------------------------------------------|
| - | 67.8±1ms | 43.7±0.1ms | 0.64 | integrate.TimeIntegrationRisch02.time_doit(10) |
| - | 67.0±0.3ms | 42.6±0.2ms | 0.64 | integrate.TimeIntegrationRisch02.time_doit_risch(10) |
| + | 18.6±0.4μs | 30.4±0.3μs | 1.63 | integrate.TimeIntegrationRisch03.time_doit(1) |
| - | 5.42±0.01ms | 2.90±0.02ms | 0.53 | logic.LogicSuite.time_load_file |
| - | 73.4±0.6ms | 28.5±0.1ms | 0.39 | polys.TimeGCD_GaussInt.time_op(1, 'dense') |
| - | 25.9±0.06ms | 16.9±0.06ms | 0.65 | polys.TimeGCD_GaussInt.time_op(1, 'expr') |
| - | 74.0±0.2ms | 28.9±0.1ms | 0.39 | polys.TimeGCD_GaussInt.time_op(1, 'sparse') |
| - | 256±2ms | 124±0.2ms | 0.49 | polys.TimeGCD_GaussInt.time_op(2, 'dense') |
| - | 256±2ms | 125±1ms | 0.49 | polys.TimeGCD_GaussInt.time_op(2, 'sparse') |
| - | 656±4ms | 375±2ms | 0.57 | polys.TimeGCD_GaussInt.time_op(3, 'dense') |
| - | 658±2ms | 372±1ms | 0.57 | polys.TimeGCD_GaussInt.time_op(3, 'sparse') |
| - | 499±3μs | 290±2μs | 0.58 | polys.TimeGCD_LinearDenseQuadraticGCD.time_op(1, 'dense') |
| - | 1.78±0.02ms | 1.04±0.01ms | 0.58 | polys.TimeGCD_LinearDenseQuadraticGCD.time_op(2, 'dense') |
| - | 5.85±0.02ms | 3.08±0.01ms | 0.53 | polys.TimeGCD_LinearDenseQuadraticGCD.time_op(3, 'dense') |
| - | 446±2μs | 229±1μs | 0.51 | polys.TimeGCD_QuadraticNonMonicGCD.time_op(1, 'dense') |
| - | 1.47±0.01ms | 683±10μs | 0.46 | polys.TimeGCD_QuadraticNonMonicGCD.time_op(2, 'dense') |
| - | 4.93±0.02ms | 1.67±0.01ms | 0.34 | polys.TimeGCD_QuadraticNonMonicGCD.time_op(3, 'dense') |
| - | 375±1μs | 206±1μs | 0.55 | polys.TimeGCD_SparseGCDHighDegree.time_op(1, 'dense') |
| - | 2.43±0.02ms | 1.24±0ms | 0.51 | polys.TimeGCD_SparseGCDHighDegree.time_op(3, 'dense') |
| - | 10.1±0.1ms | 4.33±0.03ms | 0.43 | polys.TimeGCD_SparseGCDHighDegree.time_op(5, 'dense') |
| - | 361±4μs | 169±0.9μs | 0.47 | polys.TimeGCD_SparseNonMonicQuadratic.time_op(1, 'dense') |
| - | 2.51±0.07ms | 893±8μs | 0.36 | polys.TimeGCD_SparseNonMonicQuadratic.time_op(3, 'dense') |
| - | 9.73±0.1ms | 2.64±0.03ms | 0.27 | polys.TimeGCD_SparseNonMonicQuadratic.time_op(5, 'dense') |
| - | 1.04±0.01ms | 429±4μs | 0.41 | polys.TimePREM_LinearDenseQuadraticGCD.time_op(3, 'dense') |
| - | 1.74±0ms | 506±0.5μs | 0.29 | polys.TimePREM_LinearDenseQuadraticGCD.time_op(3, 'sparse') |
| - | 5.96±0.03ms | 1.78±0.02ms | 0.3 | polys.TimePREM_LinearDenseQuadraticGCD.time_op(5, 'dense') |
| - | 8.56±0.04ms | 1.48±0.01ms | 0.17 | polys.TimePREM_LinearDenseQuadraticGCD.time_op(5, 'sparse') |
| - | 289±2μs | 64.7±0.3μs | 0.22 | polys.TimePREM_QuadraticNonMonicGCD.time_op(1, 'sparse') |
| - | 3.45±0.03ms | 390±4μs | 0.11 | polys.TimePREM_QuadraticNonMonicGCD.time_op(3, 'dense') |
| - | 4.02±0.02ms | 282±2μs | 0.07 | polys.TimePREM_QuadraticNonMonicGCD.time_op(3, 'sparse') |
| - | 7.08±0.09ms | 1.26±0.01ms | 0.18 | polys.TimePREM_QuadraticNonMonicGCD.time_op(5, 'dense') |
| - | 8.78±0.04ms | 831±4μs | 0.09 | polys.TimePREM_QuadraticNonMonicGCD.time_op(5, 'sparse') |
| - | 5.11±0.04ms | 2.98±0.01ms | 0.58 | polys.TimeSUBRESULTANTS_LinearDenseQuadraticGCD.time_op(2, 'sparse') |
| - | 12.1±0.05ms | 6.67±0.02ms | 0.55 | polys.TimeSUBRESULTANTS_LinearDenseQuadraticGCD.time_op(3, 'dense') |
| - | 22.5±0.05ms | 9.01±0.02ms | 0.4 | polys.TimeSUBRESULTANTS_LinearDenseQuadraticGCD.time_op(3, 'sparse') |
| - | 5.22±0.01ms | 868±3μs | 0.17 | polys.TimeSUBRESULTANTS_QuadraticNonMonicGCD.time_op(1, 'sparse') |
| - | 12.7±0.04ms | 6.97±0.02ms | 0.55 | polys.TimeSUBRESULTANTS_QuadraticNonMonicGCD.time_op(2, 'sparse') |
| - | 103±0.6ms | 25.7±0.1ms | 0.25 | polys.TimeSUBRESULTANTS_QuadraticNonMonicGCD.time_op(3, 'dense') |
| - | 166±0.4ms | 53.3±0.2ms | 0.32 | polys.TimeSUBRESULTANTS_QuadraticNonMonicGCD.time_op(3, 'sparse') |
| - | 177±0.6μs | 113±2μs | 0.64 | polys.TimeSUBRESULTANTS_SparseGCDHighDegree.time_op(1, 'dense') |
| - | 360±2μs | 218±2μs | 0.6 | polys.TimeSUBRESULTANTS_SparseGCDHighDegree.time_op(1, 'sparse') |
| - | 4.30±0.02ms | 859±6μs | 0.2 | polys.TimeSUBRESULTANTS_SparseGCDHighDegree.time_op(3, 'dense') |
| - | 5.27±0.03ms | 382±2μs | 0.07 | polys.TimeSUBRESULTANTS_SparseGCDHighDegree.time_op(3, 'sparse') |
| - | 20.0±0.4ms | 2.80±0.01ms | 0.14 | polys.TimeSUBRESULTANTS_SparseGCDHighDegree.time_op(5, 'dense') |
| - | 22.7±0.1ms | 626±5μs | 0.03 | polys.TimeSUBRESULTANTS_SparseGCDHighDegree.time_op(5, 'sparse') |
| - | 482±1μs | 135±0.6μs | 0.28 | polys.TimeSUBRESULTANTS_SparseNonMonicQuadratic.time_op(1, 'sparse') |
| - | 4.83±0.01ms | 617±3μs | 0.13 | polys.TimeSUBRESULTANTS_SparseNonMonicQuadratic.time_op(3, 'dense') |
| - | 5.31±0.04ms | 141±1μs | 0.03 | polys.TimeSUBRESULTANTS_SparseNonMonicQuadratic.time_op(3, 'sparse') |
| - | 13.3±0.1ms | 1.30±0.01ms | 0.1 | polys.TimeSUBRESULTANTS_SparseNonMonicQuadratic.time_op(5, 'dense') |
| - | 14.1±2ms | 144±1μs | 0.01 | polys.TimeSUBRESULTANTS_SparseNonMonicQuadratic.time_op(5, 'sparse') |
| - | 136±0.7μs | 74.9±0.6μs | 0.55 | solve.TimeMatrixOperations.time_rref(3, 0) |
| - | 256±0.5μs | 89.7±0.9μs | 0.35 | solve.TimeMatrixOperations.time_rref(4, 0) |
| - | 24.5±0.5ms | 10.2±0.02ms | 0.41 | solve.TimeSolveLinSys189x49.time_solve_lin_sys |
| - | 29.2±0.2ms | 15.4±0.1ms | 0.53 | solve.TimeSparseSystem.time_linsolve_Aaug(20) |
| - | 56.6±0.2ms | 24.7±0.08ms | 0.44 | solve.TimeSparseSystem.time_linsolve_Aaug(30) |
| - | 29.1±0.1ms | 15.1±0.02ms | 0.52 | solve.TimeSparseSystem.time_linsolve_Ab(20) |
| - | 56.7±0.2ms | 24.5±0.08ms | 0.43 | solve.TimeSparseSystem.time_linsolve_Ab(30) |
Full benchmark results can be found as artifacts in GitHub Actions |
This looks fine. I bet there's a lot of code that does this sort of thing. |
sympy/polys/tests/test_partfrac.py
Outdated
assert len(expected_terms) == len(found_terms) | ||
|
||
for e, f in zip(expected_terms, found_terms): | ||
assert all_close(e, f, rtol=1e-3, atol=1e-5) |
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.
We should make all_close smart enough that this sort of thing isn't needed. I guess that might not be straightforward to do.
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.
It is not hard to do but it would necessarily be horribly inefficient.
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.
Yeah, I meant to do efficiently. Your implementation should work. Hopefully in most cases you either get a fast exit or the expressions are sorted closely enough to each other that it isn't too quadratic.
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.
Note that "too quadratic" means "doubly exponential" for deep expression trees.
Don't assume that the factors divide the original polynomial exactly because if the domain is inexact then they might not. Instead compute the quotient and discard the remainder.
Fixes gh-26648
References to other Issues or PRs
Brief description of what is fixed or changed
Other comments
Release Notes
apart(full=True)
was fixed. Previously incorrect results might be returned if the expression contained floats.