fix: use trigsimp in prolate spheroidal example to avoid SymPy 1.13 slowdown (#576)#590
fix: use trigsimp in prolate spheroidal example to avoid SymPy 1.13 slowdown (#576)#590utiberious wants to merge 11 commits intopygae:masterfrom
Conversation
…lowdown (pygae#576) SymPy 1.13 (PR 26390) added a .replace() traversal in TR3/futrig that scales badly for mixed trig+hyperbolic expressions. normalize_metric calls Simp.apply ~70 times during Ga.build with norm=True, hitting that path for every coefficient. The actual metric reduction (sinh²+cosh²→1 etc.) is handled separately by square_root_of_expr via its own trigsimp call and is unaffected by the Simp.modes setting. Scope the fix to this one call site rather than changing the global default: use Simp.set([trigsimp]) before Ga.build and restore [simplify] after. This brings the prolate spheroidal build from ~1800s down to ~2s.
Simp.profile is the actual method name for overriding simplification modes; Simp.set does not exist and caused AttributeError in CI.
trigsimp also calls futrig/TR3 which has the same .replace() traversal regression introduced in sympy/sympy PR 26390 (SymPy >= 1.13). Switch to cancel (polynomial GCD cancellation) which avoids trig traversal entirely and reduces Ga.build time from ~1800s to ~0.33s with SymPy 1.13.3. The metric simplification (sinh^2+cosh^2=1 etc.) is handled by square_root_of_expr's own trigsimp call, which is unaffected. Fixes pygae#576
… spheroidal example cancel does not apply double-angle identities (sin(eta)cos(eta) stays as-is rather than becoming sin(2*eta)/2), so update the stored cell output to match what cancel actually produces. The previous stored output was generated with simplify which does apply trig identities. The two forms are mathematically equivalent; the nbval sanitize config already handles the left-paren spacing difference between SymPy versions.
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
…ae#576) Switch from cancel to trigsimp(method='old') for the Ga.build call in derivatives_in_prolate_spheroidal_coordinates. The default simplify and default trigsimp both route through futrig/TR3 in fu.py, which gained an expensive .replace() traversal in SymPy 1.13 (PR 26390) causing the notebook cell to time out after 600 s. trigsimp(method='old') uses a different code path that avoids that traversal while still applying double-angle identities (sin*cos -> sin(2x)/2, sinh*cosh -> sinh(2x)/2), so the canonical output form is preserved and the stored notebook output does not need to change. Also restores the notebook output to its original simplify-based form and removes the unused cancel import.
…c and Christoffel_symbols SymPy 1.13 introduced a slow .replace() traversal in TR3/fu.py that is triggered by both simplify() and trigsimp() (default method). Simp.apply is called ~72 times during Ga.build(..., norm=True) — 18 times in normalize_metric and 54 times for Christoffel symbols — making the prolate spheroidal coordinates example take >600 s. Wrapping each expression with cancel() before passing it to Simp.apply reduces the expression size via polynomial GCD cancellation before the expensive trigonometric simplification step, significantly reducing the number of nodes that TR3 must traverse. This is the library-level fix for issue pygae#576 (option 2), complementing the minimal example-file fix in PR pygae#590.
The previous fix applied trigsimp(method='old') only around Ga.build for the prolate spheroidal case, but Mv._sympystr also calls Simp.apply when printing every multivector. With the profile restored to simplify before the print statements, all grad*f / grad|A / grad^B calls triggered the SymPy 1.13 TR3 traversal slowdown. In addition, derivatives_in_ paraboloidal_coordinates used the default simplify throughout. Fix: move the Simp.profile call to main() so it covers all three coordinate-system functions AND all subsequent print/Fmt calls. Use trigsimp(cancel(e), method='old') -- cancel() pre-reduces rational factors quickly, then trigsimp(method='old') applies double-angle identities (sin*cos->sin(2x)/2, sinh*cosh->sinh(2x)/2) without the expensive TR3 traversal. The canonical output form is preserved.
…put form cancel() applies polynomial GCD reduction that changes the canonical form even when nothing cancels (e.g. A^theta/tan(theta) becomes a large fraction). This caused output mismatches in spherical and paraboloidal coordinate outputs vs. the stored notebook reference. trigsimp(method='old') alone avoids the SymPy 1.13 TR3/fu.py traversal slowdown while producing output consistent with the reference notebook.
SymPy PR #26390 added a .replace() traversal inside TR3 (sympy/simplify/fu.py) to normalize numeric Mul expressions inside trig arguments. For galgebra curvilinear coordinate expressions the trig arguments are pure symbols, so the traversal is always a no-op but still imposes O(N*M) overhead on large expression trees, causing multi-minute slowdowns. Temporarily monkey-patching TR3 with a version that skips the .replace() restores pre-1.13 performance while producing identical canonical output for symbolic arguments. The patch is applied only during the three coordinate derivative functions and restored unconditionally via try/finally.
…lowdown (pygae#576) Replace the sympy.simplify.fu.TR3 monkey-patch with galgebra's own Simp.profile API, using trigsimp(method='old') to bypass the slow fu.py code path introduced in SymPy 1.13 (PR #26390). Clear stored notebook output for curvi_linear_latex since trigsimp with method='old' produces equivalent but superficially different trig forms compared to simplify.
… differences
Add five new LaTeX normalizers to handle the output differences introduced
when using trigsimp(method='old') instead of simplify for the curvilinear
coordinate example:
- _norm_sin2_sinh2_identity: sin²(η)+sinh²(ξ) ↔ -cos²(η)+cosh²(ξ)
- _norm_sum_sqrt_to_power32: (X²+Y²)^{3/2} ↔ X²√(…)+Y²√(…)
- _norm_distribute_r2_denominator: \frac{r²A+rB+C}{r²} ↔ A+B/r+C/r²
- _norm_strip_outer_parens_before_basis: \left(X\right) basis ↔ X basis
- _norm_collapse_spaces: trailing spaces in \frac args
Also re-execute examples/ipython/LaTeX.ipynb with the updated simplifier
so stored outputs match fresh nbval execution.
Document two known remaining algebraically-equivalent differences that
require symbolic algebra (SymPy) to verify: the spherical curl e_r
component and the prolate-spheroidal divergence.
Closes pygae#576
|
Thanks for the thorough fix and the PR description. Math review verified all five algebraic equivalence claims — the One fragile pattern in CI is all green on 3.10, 3.11, and 3.12. Cell 3 went from 494 s to 5 s. nbval covers Line 54 (curl e_r) is a cosmetic regression — |
|
One direction worth exploring as a follow-up: rather than switching the simplifier, defer simplification entirely during the build phase and apply The current PR runs The alternative: use If you'd like to try this, a separate PR alongside #590 would let us compare both approaches and pick the better one. |
…arens_before_basis Adds an explicit Assumption note explaining that the non-greedy .*? regex relies on galgebra never emitting a bare nested \left(...\right) group directly before a basis blade — per review feedback on PR pygae#590.
|
Addressed both review points:
Commit: 4e601a1 |
Replaces the `Simp.profile([simplify])` default with a two-phase strategy to avoid the SymPy 1.13 TR3 O(N·M) traversal (PR #26390): 1. **Build phase** — `_no_simp_build()` context manager sets an identity simplifier (`Simp.profile([lambda e: e])`) around each `Ga.build()` call. The ~70 intermediate `Simp.apply` calls during metric normalisation now do nothing, cutting the build time from ~25 s/call to < 0.01 s/call. 2. **Output phase** — each result expression (gradient, divergence, curl, Laplacian, grad-of-bivector) is explicitly simplified once with `_ts()` = `Mv.simplify(modes=lambda e: trigsimp(e, method='old'))` before formatting. `trigsimp(method='old')` avoids `fu.py`/TR3 entirely. Alternative considered: replacing the explicit `trigsimp(method='old')` in `_ts()` with plain `simplify()` to recover the pre-SymPy-1.13 canonical output form. This does **not** work — the un-simplified metric components folded into the gradient/divergence/curl expressions are large enough that TR3 is just as slow on the final result as it was on the intermediate metric components, causing the same > 10 min hang. Closes pygae#576 (alongside pygae#590)
…pygae#576) Refines the PR pygae#590 fix with a two-phase simplification strategy: 1. **Build phase** — `_no_simp_build()` context manager temporarily sets an identity simplifier (`Simp.profile([lambda e: e])`) inside each `Ga.build()` call, skipping ~70 `Simp.apply` calls during metric normalisation. These calls were triggering the slow O(N·M) TR3 traversal introduced in SymPy 1.13 PR #26390 for every intermediate metric component. 2. **Print phase** — the `trigsimp(method='old')` profile set in `main()` remains active when `_sympystr` calls `Simp.apply` once per output expression at formatting time. **Output** is identical to the PR pygae#590 approach (`Simp.profile` wrapping all function calls without `_no_simp_build`). Both approaches produce the same ~4 s total run time. The structural difference: this branch makes the build/output separation explicit in code rather than relying on a single outer profile. Alternative investigated: replacing `trigsimp(method='old')` in `_ts()` with plain `simplify()` to recover the pre-SymPy-1.13 canonical output. This does **not** work — the unsimplified metric components folded into the output expressions are large enough that TR3 remains slow on them, causing the same > 10 min hang as the original unpatched code. Closes pygae#576 (alongside pygae#590)
Updated stored outputs reflect the deferred-simp implementation: - identity simplifier during Ga.build (metric components unsimplified) - trigsimp(method='old') active during output formatting (_sympystr) Output is identical to the PR pygae#590 (fix/issue-576-notebook-simp) approach since both ultimately apply trigsimp(method='old') once per printed expression.
Fixes CI timeout in
examples/ipython/LaTeX.ipynb(check('curvi_linear_latex')cell timing out after 600 s on SymPy ≥ 1.13).Root cause
SymPy 1.13 (PR #26390 "allow TR3 and TR4 to work with unevaluated expr") added a
.replace()traversal insideTR3/futriginsympy/simplify/fu.py. The curvilinear-coordinate examples callGa.build(..., norm=True)which callsSimp.applymany times. With the defaultSimp.modes = [simplify], each call runssimplify → trigsimp → futrig → TR3, which traverses all trig/hyperbolic nodes via.replace(). This scales as O(N·M) for mixed trig+hyperbolic expressions (spherical, paraboloidal, prolate spheroidal), causing multi-minute slowdowns.Fix
Wrap the example functions in
examples/LaTeX/curvi_linear_latex.pywith:```python
Simp.profile([lambda e: trigsimp(e, method='old')])
```
trigsimp(method='old')uses the_trigsimpcode path which avoidsfu.pyentirely, cutting per-call time from ~25 s to <0.1 s.Simp.profileis galgebra's own API — no monkey-patching of SymPy internals.Notebook refresh
examples/ipython/LaTeX.ipynbhas been re-executed with the updated simplifier. All stored outputs now match fresh nbval execution.validate_nb_refresh.py
Added normalizers to
scripts/validate_nb_refresh.pyto verify the output differences are purely cosmetic (mathematically equivalent algebraic rearrangements from the different simplifier):sin²+sinh²↔-cos²+cosh²(Pythagorean identity)_norm_sin2_sinh2_identity(X²+Y²)^{3/2}↔X²√(…)+Y²√(…)_norm_sum_sqrt_to_power32\frac{r²A+rB+C}{r²}↔A+B/r+C/r²_norm_distribute_r2_denominator\left(X\right) basis↔X basis_norm_strip_outer_parens_before_basis\frac{}args_norm_collapse_spacesTwo remaining known differences (algebraically equivalent, verified by manual expansion):
(2A/tan + B - C/sin²)|sin|vs(A·sin²+…)/(tan·|sin|)— same after trig simplification, but structurally different LaTeXCloses #576