Fix for recursion error in latex() function #20264
Conversation
|
✅ Hi, I am the SymPy bot (v161). 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.9. Click here to see the pull request description that was parsed.
Update The release notes on the wiki have been updated. |
|
Travis fail looks wrong somehow. You may trigger it again by making an empty commit. Edit: Seems that it is happening on all PRs. Issue #20265 |
|
Can you please add a test for the problematic code as well? Should go in https://github.com/sympy/sympy/blob/master/sympy/printing/tests/test_latex.py |
|
Ok will do it immediately. |
|
Please help me with this. I am not able to understand what is the problem with my commit. |
|
It seems some other unknown failure, it is not because of your commits! |
Codecov Report
@@ Coverage Diff @@
## master #20264 +/- ##
=============================================
+ Coverage 75.729% 75.770% +0.041%
=============================================
Files 671 674 +3
Lines 174187 174222 +35
Branches 41109 41119 +10
=============================================
+ Hits 131911 132009 +98
+ Misses 36526 36495 -31
+ Partials 5750 5718 -32 |
|
Would it make sense to add brackets to this? The test example now looks like: which I guess is OK, but looks a bit weird. However, looking at related examples, such as Hence, I believe that it should be printed as Edit: removed incorrect comment about what was tested. (The images looks a bit weird for some reason, but hopefully one can see the point anyway...) |
|
I see the problem. Either I can parenthesize the 1/3 or I can print in the format as suggested by @oscargus. Kindly suggest Which method should I proceed with? |
|
I believe that it should be consistent if nothing else. So either print it similar to |
|
Kindly review this PR and let me know if any further corrections are needed. |
|
Looks good to me! (But I will not merge as I haven't been actively involved in a while.) (I added a release note comment.) |
|
Can you please add a test for #19950 as well? I believe it is the same issue. (And add to the description that it fixes that as well.) |
|
Added the test for the aforementioned issue. |
|
@oscargus Is it ready to be merged? |
|
Closing and reopening to activate the new tests. |
|
I was hoping that someone else would merge this, but as that has not happened I will merge it once all the tests have passed. Thanks for your contribution! |
|
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
[ed9a550f] [f57d98d8]
<sympy-1.8^0>
- 1.15±0.05s 156±5ms 0.14 dsolve.TimeDsolve01.time_dsolve
- 81.6±3μs 37.8±3μs 0.46 matrices.TimeDiagonalEigenvals.time_eigenvals
- 107±3μs 62.9±1μs 0.59 matrices.TimeMatrixGetItem.time_ImmutableDenseMatrix_getitem
- 103±2μs 68.1±1μs 0.66 matrices.TimeMatrixGetItem.time_MutableSparseMatrix_getitem
+ 94.3±4μs 151±7μs 1.60 solve.TimeMatrixArithmetic.time_dense_add(10, 5)
+ 12.0±0.8μs 21.7±3μs 1.80 solve.TimeMatrixArithmetic.time_dense_add(3, 0)
+ 14.8±0.9μs 34.3±1μs 2.32 solve.TimeMatrixArithmetic.time_dense_add(3, 5)
+ 23.9±1μs 46.3±2μs 1.94 solve.TimeMatrixArithmetic.time_dense_add(4, 5)
+ 39.8±3μs 67.8±6μs 1.70 solve.TimeMatrixArithmetic.time_dense_add(6, 5)
- 1.36±0.07ms 232±6μs 0.17 solve.TimeMatrixArithmetic.time_dense_multiply(10, 0)
- 54.3±3μs 28.1±2μs 0.52 solve.TimeMatrixArithmetic.time_dense_multiply(3, 0)
+ 84.9±2μs 161±3μs 1.89 solve.TimeMatrixArithmetic.time_dense_multiply(3, 5)
- 104±4μs 37.3±0.9μs 0.36 solve.TimeMatrixArithmetic.time_dense_multiply(4, 0)
- 316±9μs 73.5±2μs 0.23 solve.TimeMatrixArithmetic.time_dense_multiply(6, 0)
- 105±3μs 52.5±4μs 0.50 solve.TimeMatrixOperations.time_det(3, 0)
- 197±10μs 118±8μs 0.60 solve.TimeMatrixOperations.time_det(3, 2)
- 192±10μs 108±2μs 0.56 solve.TimeMatrixOperations.time_det(3, 5)
- 98.9±2μs 48.1±2μs 0.49 solve.TimeMatrixOperations.time_det_bareiss(3, 0)
- 187±6μs 115±3μs 0.62 solve.TimeMatrixOperations.time_det_bareiss(3, 2)
- 208±20μs 110±5μs 0.53 solve.TimeMatrixOperations.time_det_bareiss(3, 5)
+ 577±40μs 877±20μs 1.52 solve.TimeMatrixOperations.time_det_bareiss(4, 0)
- 105±6μs 49.8±0.7μs 0.47 solve.TimeMatrixOperations.time_det_berkowitz(3, 0)
- 196±6μs 112±3μs 0.57 solve.TimeMatrixOperations.time_det_berkowitz(3, 2)
- 194±6μs 103±1μs 0.53 solve.TimeMatrixOperations.time_det_berkowitz(3, 5)
+ 1.06±0.04ms 1.83±0.07ms 1.73 solve.TimeMatrixOperations.time_det_berkowitz(4, 2)
+ 1.01±0.05ms 2.09±0.1ms 2.08 solve.TimeMatrixOperations.time_det_berkowitz(4, 5)
+ 315±20μs 531±9μs 1.68 solve.TimeMatrixOperations.time_rank(3, 0)
+ 459±9μs 748±9μs 1.63 solve.TimeMatrixOperations.time_rank(4, 0)
+ 138±4μs 211±4μs 1.53 solve.TimeMatrixOperations.time_rref(3, 0)
- 5.55±0.1ms 3.07±0.08ms 0.55 solve.TimeRationalSystem.time_linsolve(10)
+ 3.42±0.08ms 5.28±0.3ms 1.54 solve.TimeSparseSystem.time_linear_eq_to_matrix(20)
- 2.69±0.07ms 1.62±0.06ms 0.60 solve.TimeSparseSystem.time_linsolve_eqs(20)
- 3.84±0.1ms 2.35±0.07ms 0.61 solve.TimeSparseSystem.time_linsolve_eqs(30)
Full benchmark results can be found as artifacts in GitHub Actions |
References to other Issues or PRs
Fixes #20252 and #19950
Brief description of what is fixed or changed
Fixed the issue by adding a special check for the above mentioned issue. Before the changes the latex() function gave recursion error whenever there was a base of the form 1/x where x is any positive integer and power was negative integer. Now after the special check is added it gives a suitable output.
Other comments
Release Notes