-
-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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 centering of expressions with over-strike marks #25673
Conversation
✅ 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. |
wdelta = wtarget - wstring | ||
assert wdelta >= 0 | ||
|
||
wleft = wdelta // 2 |
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.
Although this follows the convention that was used above, the string behavior or 'ab'.center(3)
places the ab
to the right. To preserve the existing master behavior we could do wleft = sum(divmod(wdelta, 2))
, i.e. ceiling(wdelta/2
. Does anyone have a preference?
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 [8059df73] <sympy-1.12^0> | After [908215f0] | Ratio | Benchmark (Parameter) |
|----------|------------------------------------|---------------------|---------|----------------------------------------------------------------------|
| - | 88.1±1ms | 58.2±0.3ms | 0.66 | integrate.TimeIntegrationRisch02.time_doit(10) |
| - | 86.6±0.8ms | 57.1±0.6ms | 0.66 | integrate.TimeIntegrationRisch02.time_doit_risch(10) |
| + | 22.1±0.08μs | 38.9±0.07μs | 1.76 | integrate.TimeIntegrationRisch03.time_doit(1) |
| - | 7.03±0.01ms | 3.81±0.01ms | 0.54 | logic.LogicSuite.time_load_file |
| - | 2.11±0ms | 658±3μs | 0.31 | polys.TimePREM_LinearDenseQuadraticGCD.time_op(3, 'sparse') |
| - | 10.4±0.02ms | 1.96±0.01ms | 0.19 | polys.TimePREM_LinearDenseQuadraticGCD.time_op(5, 'sparse') |
| - | 367±0.9μs | 82.4±0.5μs | 0.22 | polys.TimePREM_QuadraticNonMonicGCD.time_op(1, 'sparse') |
| - | 4.85±0.01ms | 364±0.7μs | 0.08 | polys.TimePREM_QuadraticNonMonicGCD.time_op(3, 'sparse') |
| - | 10.6±0.04ms | 1.10±0ms | 0.1 | polys.TimePREM_QuadraticNonMonicGCD.time_op(5, 'sparse') |
| - | 6.28±0.01ms | 3.97±0.01ms | 0.63 | polys.TimeSUBRESULTANTS_LinearDenseQuadraticGCD.time_op(2, 'sparse') |
| - | 27.3±0.05ms | 12.1±0.03ms | 0.44 | polys.TimeSUBRESULTANTS_LinearDenseQuadraticGCD.time_op(3, 'sparse') |
| - | 6.96±0.02ms | 1.17±0ms | 0.17 | polys.TimeSUBRESULTANTS_QuadraticNonMonicGCD.time_op(1, 'sparse') |
| - | 16.4±0.03ms | 9.25±0.02ms | 0.56 | polys.TimeSUBRESULTANTS_QuadraticNonMonicGCD.time_op(2, 'sparse') |
| - | 211±0.2ms | 70.8±0.06ms | 0.33 | polys.TimeSUBRESULTANTS_QuadraticNonMonicGCD.time_op(3, 'sparse') |
| - | 6.42±0.03ms | 537±1μs | 0.08 | polys.TimeSUBRESULTANTS_SparseGCDHighDegree.time_op(3, 'sparse') |
| - | 27.8±0.03ms | 858±1μs | 0.03 | polys.TimeSUBRESULTANTS_SparseGCDHighDegree.time_op(5, 'sparse') |
| - | 614±2μs | 200±2μs | 0.33 | polys.TimeSUBRESULTANTS_SparseNonMonicQuadratic.time_op(1, 'sparse') |
| - | 6.49±0.02ms | 203±0.4μs | 0.03 | polys.TimeSUBRESULTANTS_SparseNonMonicQuadratic.time_op(3, 'sparse') |
| - | 17.0±0.05ms | 207±1μs | 0.01 | polys.TimeSUBRESULTANTS_SparseNonMonicQuadratic.time_op(5, 'sparse') |
| - | 162±0.5μs | 90.2±0.3μs | 0.56 | solve.TimeMatrixOperations.time_rref(3, 0) |
| - | 311±0.5μs | 109±0.4μs | 0.35 | solve.TimeMatrixOperations.time_rref(4, 0) |
| - | 32.1±0.09ms | 13.6±0.02ms | 0.43 | solve.TimeSolveLinSys189x49.time_solve_lin_sys |
Full benchmark results can be found as artifacts in GitHub Actions |
assert pretty(expr) in [ascii_str_1, ascii_str_2] | ||
assert upretty(expr) in [ucode_str_1, ucode_str_2] | ||
ucode_str_3 = \ | ||
"""\ |
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.
All lines are the same length (in terms of characters); variable width characters make it look otherwise.
typsetting might need adjustment to select top right and bottom left forms (if preferred) # -1 -1 # -- -- # 3 3 # # 2 2 # x x # -- -- # y y
I'm not sure about this change to line wrapping in the pretty printer: In [9]: 10**200
Out[9]:
100000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000 ↪
↪ 0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000 ↪
↪ 00000 This is the previous output: In [1]: 10**200
Out[1]:
100000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000
000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000
000 The release notes are also incomprehensible:
|
+1. When I saw this in the Sage CI output, it took me some time to understand it. Also, the way it comes out in the GitHub Actions log doesn't help:
The |
The nice thing about the continuation character is that it lets you accurately reconstruct the output with the proper whitespace. You can't do this if the whitespace is stripped from the lines after wrapping. |
I don't know what this means. In the long integer case the previous output could be copy-pasted but the current output cannot without editing because it has extra unicode characters and whitespace inserted. |
I'm -1 to this change too. Having pretty printing line wrapping mechanisms should probably be optional. |
I just discovered this behavior, and I have to say I don't like it either. Even if we agree some sort of continuation character is needed, the implementation here isn't particularly helpful. Consider the case where it most helps to indicate continuation, when the pretty printed output is several lines tall. In this case, it isn't clear where the line wrapping occurs. This is an example
The continuation characters aren't especially helpful here, because there are so many of them. The important line here is the gap where there isn't a continuation character. And for something like Oscar's |
I opened #26221 to discuss this. |
References to other Issues or PRs
continues #23449
closes #25674
Brief description of what is fixed or changed
Other comments
Release Notes