Repository navigation
Conversation
`RischMethod(use_algebraic_closure=true)` threw
DomainError: comparing nonreal numbers
as soon as the roots were not real, which is most integrands of interest:
1/(x^2 + 1), 1/(x^2 + 2), 1/(x^3 - 1) and 1/(x^2 + x + 1) all failed, and
`catch_errors` does not intercept a `DomainError`.
`positive_constant_coefficient` compared a constant coefficient against
zero to decide whether to negate the polynomial. `QQBarFieldElem` only
orders real numbers and throws for the rest, so the comparison itself was
the failure. Dispatch the sign test instead: rational coefficients keep
the plain comparison, and a nonreal algebraic number is reported as not
negative, since it has no sign to speak of.
This is a partial fix for JuliaSymbolics#13. 1/(x^4 + 1) still raises, from a different
nonreal comparison: there `integrate` succeeds and only rendering the
resulting degree-4 `Root` placeholder throws. That case is recorded as
`@test_broken` rather than left silent.
`TEST_GROUP=easy` passes; `TEST_GROUP=difficult` reports numbers identical
to `main` (Risch: 57 succeeded, 77 failed, 40 maybe failed, 3 errored).
Assisted-by: AI
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #140 +/- ##
==========================================
+ Coverage 50.98% 51.24% +0.25%
==========================================
Files 23 23
Lines 4309 4215 -94
==========================================
- Hits 2197 2160 -37
+ Misses 2112 2055 -57 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Verdict: changes needed before merge (reviewed at Guards the polynomial sign normalization against nonreal algebraic coefficients, allowing six tested Risch antiderivatives to integrate and render. The fix passes local regression and derivative checks, but the new catch-all broken test and unformatted test file need changes. Risk assessment
Findings
VerificationCommands ran from the supplied scratch directory with The base test used an unmodified Runic command in a scratch environment containing Runic 1.11.1: using Runic
exit(Runic.main(["--check", "--diff", "repo/src/methods/risch/rational_functions.jl", "repo/test/methods/risch/test_algebraic_closure.jl", "repo/test/runtests.jl"]))Head CI easy, QA, documentation, spelling, and SymbolicIntegrationMaxima checks passed. Every failing head check below failed with the same three
A separate investigation locally reproduced that CI error using a standalone SymbolicUtils expression with historical transitive dependencies: SymbolicUtils 4.46.6 gave 1 error, while changing only SymbolicUtils to 4.46.7 gave 1 pass. The existing upstream bisect identifies introducing commit Read the PR description, all comments/reviews, linked issue, and complete diff. No human review or maintainer request is present on this PR; the only PR comment is Codecov. No GitHub writes were made. Push a fix and the PR is reviewed again automatically at the new head. 🤖 Posted by an AI agent — harness: Claude Code · model: claude-opus-5-5[1m] (fleet master); review by Codex CLI 0.157.1 / gpt-6-astra |
…re-nonreal-comparisons
…ception The "Known remaining limitation" testset wrapped `string(result)` in `@test_broken try ... catch ... false end`, which accepts every exception and would therefore hide an unrelated regression in rendering. Rendering the degree-4 `Root` placeholder throws a `DomainError` carrying the offending nonreal root, verified on Julia 1.13.0 with SymbolicUtils 4.48.0 and Nemo 0.56.1, so assert exactly that with `@test_throws`, and assert that `integrate` itself still succeeds. The testset now has no broken tests, and if rendering is ever fixed the assertion fails and says so. The error carries no message text beyond the offending root, unlike the `comparing nonreal numbers` path of issue JuliaSymbolics#13, so only the type is asserted. Also format the new test file with Runic. Assisted-by: AI
432d006 to
1e568da
Compare
…re-nonreal-comparisons
…risons Brings in JuliaSymbolics#147, which fixes baseline entry 15, `sin(sqrt(1+x))/sqrt(1+x)` — the single RuleBased regression that was failing the `difficult` gate here, and which this pull request never caused: it fails identically on `main` and on every pull request based on the older `main`. No conflict. `TEST_GROUP=easy`: 244 passed, 1 broken (pre-existing), exit 0, and the `[Risch] Algebraic closure with nonreal roots` testset keeps its 23 passing assertions with no broken test. Assisted-by: AI
4 participants
Partial fix for #13.
Problem
RischMethod(use_algebraic_closure=true)threw as soon as the roots were not real, which is most integrands where the option would be interesting:Same for
1/(x^2 + 2),1/(x^3 - 1),1/(x^2 + x + 1)and(x + 1)/(x^2 + 4).catch_errors=truedoes not help, since it only interceptsNotImplementedErrorandAlgorithmFailedError.Cause
positive_constant_coefficientdecided whether to negate a polynomial withconstant_coefficient(f) < 0.QQBarFieldElemonly orders real numbers and throwsDomainErrorotherwise, so the comparison itself was the failure — not the value being compared.Fix
Dispatch the sign test rather than comparing unconditionally:
Rational coefficients keep the plain comparison; a nonreal algebraic number is reported as not negative, since it has no sign. Note that the naive form of this fix — calling
imagon the coefficient directly — breaks the ordinary path, becauseimag(::QQFieldElem)does not exist; hence the dispatch.The five integrands listed above now integrate and render. With #136 merged, they come back with exact radicals rather than floats, but the two changes are independent.
Remaining limitation
1/(x^4 + 1)still raises, from a different nonreal comparison. Thereintegrateitself succeeds and only rendering the result throws:That is reached when the degree-4
Rootplaceholder in the result is displayed. It is recorded as@test_brokenrather than left silent, so #13 should stay open until it is addressed too.Tests
New
test/methods/risch/test_algebraic_closure.jl, registered in theeasygroup: unit tests forhas_negative_signover rational, real-algebraic and nonreal-algebraic coefficients, and integration tests for the six integrands, each rendered inside the test becauseshowreaches the same comparison. Without the fix, that file reports 11 errors and 0 passes; with it, 21 passes and the 1 documented@test_broken.TEST_GROUP=easypasses (223 pass, 2 broken — one pre-existing, one added here).TEST_GROUP=difficultreports numbers identical tomain: RuleBased 92/50/35/0, Risch 57/77/40/3.Disclosure: this change was written with AI assistance, and tested locally before opening the PR.
TEST_GROUP=easyandTEST_GROUP=difficultwere run on this branch and against unmodifiedmain. The new test file was also run with the fix reverted, to confirm it fails without it (11 errors, 0 passes), and the remaining1/(x^4 + 1)limitation was narrowed down to rendering rather than integration by running the two steps separately.Changes since the first review
@test_broken try ... catch ... false endin "Known remaining limitation"with
@test_throws DomainError string(result), plus an assertion thatintegrateitselfsucceeds. The old form accepted any exception and would have hidden an unrelated rendering
regression. Verified that rendering the degree-4
Rootplaceholder throws exactly aDomainError(Julia 1.13.0, SymbolicUtils 4.48.0, Nemo 0.56.1); the error carries nomessage text beyond the offending root, so only the type is asserted. The testset now has
no broken tests: 23 pass, 0 broken.
--checknow exits 0).TEST_GROUP=easyon this head: 225 passed, 1 broken (pre-existing), exit 0.Two red checks on this PR are not caused by it:
difficultjobs fail on baseline entry 15,sin(sqrt(1+x))/sqrt(1+x), which reproduces on cleanmainand is now filed as [RuleBased] change of variables loses the domain of the substituted variable, breaking ∫sin(sqrt(1+x))/sqrt(1+x)dx #145;qajobs fail to resolve becausetest/qa/Project.tomlstill bounds thepackage at 0.2 after the 0.3.0 release, which has broken
mainsince then and is fixed byAllow SymbolicIntegrationMaxima 0.3 in the qa test environment #146.
Assisted by: AI
CI on head
a370e6a33 pass, 2 fail — and the
difficultgroup is green.Before current main was merged in, this pull request failed nine
difficultjobs. Allnine were the same single RuleBased regression, baseline entry 15,
sin(sqrt(1+x))/sqrt(1+x), which fails identically onmainand which #147 fixes. Mergingmain cleared them without touching a line of this change.
The two remaining failures are the
SymbolicIntegrationMaximaqajobs dying atresolution on the stale 0.2 bound that #146 fixes; that has failed on
mainsince the0.3.0 release and nothing here touches it.