fix: avoid slow simplification during multivector display - #598
Conversation
|
Thanks, this is much cleaner than #596. The display-only boundary holds up, and the full suite, SymPy 1.13/1.14 checks, nbval, and unchanged notebooks all pass. However, the threshold catches an ordinary trig sum where fallback rendering takes 4.32s versus 0.53s with Also make the boundary test version-aware for SymPy 1.12, avoid mutable default-mode aliasing so explicit profiles remain authoritative. Changelog is also stale now, it still says it is about |
|
Addressed in
Full suites pass on SymPy 1.13.3 and 1.14: 141 passed, 3 skipped on each. LaTeX nbval remains 22/22, and notebook stored outputs are unchanged. |
|
Thanks, the SymPy version handling, profile authority, and changelog fixes look good. There is still something wrong in the predicate. It uses the whole expression’s cost but requires only one small qualifying Pow. With 16 ordinary trig pairs plus sqrt(sin(u) + sinh(v)), it routes to the fallback: 7.67s versus 0.59s for simplify on SymPy 1.14. Please calculate the cost within each candidate power/base and add this case as a real negative regression. Everything else looks promising. |
|
Addressed in The predicate now computes traversal cost inside each qualifying power base. The 16-pair counterexample has a local score of 10, so unrelated terms can no longer push it over the boundary; the prolate The exact counterexample is now a real regression test. On SymPy 1.14 it does not route, completes through the helper in 0.72 s, reduces the rational term, and is symbolically equivalent to the input. The representative prolate display still routes once and renders 434 characters in 2.64 s. Focused tests pass on SymPy 1.12, 1.13.3, and 1.14. Full suites pass on 1.13.3 and 1.14 with 142 passed and 3 skipped on each; LaTeX nbval remains 22/22. Fresh CI is running on the revised head. |
028c051 to
41e98be
Compare
|
Thanks, the previous counterexample is fixed and the test, CI, notebook, profile, changelog, and identity checks all hold. I found another counterexample: Please narrow the signal to the actual squared trig/hyperbolic shape, or scope the fallback to the candidate subexpression, and add this case as a real negative regression. Try your best to make it bounded safely. |
|
Addressed in The repeated false positives show that a numerical tree score is not a safe signal here, so I removed it rather than tuning another threshold. The fallback now matches only the observed shape: a square root or reciprocal square root whose two-term base is one squared trigonometric function plus one squared hyperbolic function. The exact new counterexample is a real negative regression. On SymPy 1.13.3 and 1.14 it does not route, completes through ordinary simplification in about 0.75 s, reduces the rational term, and remains symbolically equivalent. The representative prolate expression still routes once and renders the unchanged 434-character result in about 2.5 s. Focused tests pass on SymPy 1.12, 1.13.3, and 1.14. Full suites pass on 1.13.3 and 1.14 with 143 passed and 3 skipped on each; LaTeX nbval remains 22/22. No notebook file changed. Fresh CI is running on the revised head. |
|
That is possibly the best we can do without going back to use explicit example profile. Removing the numerical heuristic and matching only the observed squared trig/hyperbolic radical is the right boundary. It looks that the prior counterexamples stay on ordinary |
|
Final performance summary for
The five-run PR ranges were 0.069–0.072 s Cartesian, 0.451–0.479 s spherical, 0.197–0.213 s paraboloidal, and 2.448–2.492 s prolate. Across the ordinary cases, median differences were −4.1% to +3.0%, at most 11 ms; five of six timing ranges overlapped. A route inventory covered 46 coefficient simplifications across the Cartesian, spherical, and paraboloidal samples: none matched. The prolate divergence contributed one coefficient and it matched. All three review-derived false-positive families remained on ordinary The complete SymPy 1.13.3 spot checks showed the same behavior: 0.765 s for the latest benign case and 2.546 s for prolate. These figures establish bounded behavior for this representative sample, not every possible symbolic composition. CI and documentation checks are green on the exact head. |
I don't like that we have to keep this old-trigsimp profile but it seems to add ~10% time if we remove it, and what's worse, the simplified results become less desirable. I hope we can have one last round to try structural pattern matching on bottlenecks, and develop broader structural matching only alongside #597’s visual gallery and regression audit, where every new family gets explicit mathematical and visual acceptance criteria. |
|
I tried one final targeted structural-profile spike. It preserved the complete stored TeX output exactly, but it did not improve on the existing example profile. The prototype routed 67 of 208 coefficient simplifications: 17/53 spherical, 2/78 paraboloidal, and 48/77 prolate. Fresh-process comparisons found it 3.0% slower than the current profile on SymPy 1.14 and 5.0% slower on 1.13.3. The paraboloidal section was 89% and 106% slower respectively, while prolate was effectively unchanged. The paraboloidal repeated-radical denominator does have a sound narrow structural match. Reproducing all established prolate forms is the problem: it requires routing every trig/hyperbolic coefficient, including small expressions such as So I am leaving this PR unchanged. The explicit example profile remains the faster presentation policy, while this PR keeps the exact display-only fallback for the observed SymPy slowdown. Broader canonical-form matching, including the paraboloidal family, should proceed with #597's mathematical and visual regression gates. |
|
Thanks, I concur. We defer to #597 to solve this with better signals from the visual regression gate. |
Why a replacement PR
#590 fixed the curvilinear-coordinate notebook by selecting
trigsimp(method='old')throughSimp.profile. That workaround still works and remains unchanged here, but it only protects that example.#596 tried to make the workaround available throughout galgebra. Its simplification route was too broad: it could affect algebraic operations and change familiar canonical output where there was no performance problem.
The missing distinction was between calculation and presentation. The costly terminal
simplifycall only needs to be avoided while rendering the specific expression shape known to enter SymPy 1.13's expensive FU traversal.What this changes
Simp.applybehavior unchanged.trigsimp(method='old')only for that shape on SymPy 1.13 or newer.Simp.profilechanges.The earlier numerical cost heuristic was removed after benign expressions repeatedly reached the same score through unrelated terms. The existing notebook profile remains; its saved mode is restored by identity rather than through a mutable copy. The notebook note and stored equations are unchanged, preserving the established fraction, termwise-sum, and blade-coefficient forms.
Performance
On SymPy 1.13.3 and 1.14, the representative prolate-spheroidal divergence selects the fallback once and renders the same 434-character result in about 2.5 seconds. The triggering coefficient did not complete within 15 seconds through plain
simplifyin the same environment.The latest reviewed false positive now stays on ordinary simplification, completes in about 0.75 seconds on both versions, reduces its rational term, and remains symbolically equivalent. The two earlier false positives remain covered as negative regressions.
Verification
LaTeX.ipynbnbval: 22 passedgit diff --check: passedmaster: no differencesThe changelog distinguishes the original example workaround from this display-scoped fallback and corrects the old claim that the slowdown occurred during
Ga.build.Supersedes #596.