Repository navigation
Conversation
Every test run printed
Warning: `zeros(R::NCRing, r::Int...)` is deprecated, use
`zero_matrix(R, r...)` instead.
`zero_matrix` is not a replacement at these call sites. The deprecated
method returns a plain Julia `Array` of ring elements, while
`zero_matrix` builds a matrix type; the arrays built here are
concatenated with `vcat`/`hcat` alongside ordinary coefficient matrices,
indexed elementwise and returned as vectors, so swapping in a matrix type
would change those types.
Add `zero_array`, which reproduces the old behaviour explicitly, and use
it for the 18 ring-element call sites. `zeros(Int, ...)` calls are
`Base.zeros` and are left alone.
No behaviour change: `TEST_GROUP=easy` passes with the same 202 tests, and
`TEST_GROUP=difficult` reports numbers identical to `main` (Risch: 57
succeeded, 77 failed, 40 maybe failed, 3 errored), with the deprecation
warnings gone.
Assisted-by: AI
Verdict: changes needed before merge (reviewed at Replaces 18 deprecated ring-element Risk assessment
Findings
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 |
The warning fix and the new allocator had no test, so reverting either would have gone unnoticed. `test/methods/risch/test_zero_array.jl` covers: - shape, element type and zero values of `zero_array` over `QQ`, a polynomial ring and a fraction field, in vector, matrix and empty shapes; - that the entries are distinct objects, since ring elements are mutable and a shared zero would make an in-place update to one entry visible in the others; - that `convolution` emits no warning, which is the actual regression guard; - that `convolution` still computes the same result. The deprecation check is discriminating: restoring `c = zeros(R, ...)` in `convolution` makes it fail with "`zeros(R::NCRing, r::Int...)` is deprecated" (28 passed, 1 failed), and it passes with `zero_array` (29 passed). `collect_test_logs` installs a fresh logger, which resets the `maxlog = 1` budget of `Base.depwarn`, so a warning emitted earlier in the suite cannot mask it. A positive control asserts the deprecated method still warns in the same process, so the check cannot pass vacuously; it is skipped when the process does not enable depwarn. `TEST_GROUP=easy`: 231 passed, 1 broken (pre-existing), exit 0, with zero deprecation warnings in the whole run. Also add the `return` that Runic wants in `zero_array`, and drop the trailing whitespace that `git diff --check` flagged in `parametric_problems.jl`. The remaining Runic complaints about `general.jl` are pre-existing on main and are left alone. Assisted-by: AI
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #138 +/- ##
==========================================
+ Coverage 50.98% 51.26% +0.28%
==========================================
Files 23 23
Lines 4309 4219 -90
==========================================
- Hits 2197 2163 -34
+ Misses 2112 2056 -56 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
8e93d2a to
76fa9e3
Compare
Brings in JuliaSymbolics#147, which fixes baseline entry 15, `sin(sqrt(1+x))/sqrt(1+x)`. That entry was the single RuleBased regression failing the `difficult` gate on this pull request, and it was never caused by it: it fails the same way on `main` and on every other open pull request based on the older `main`. No conflict. `TEST_GROUP=easy`: 250 passed, 1 broken (pre-existing), exit 0, with zero deprecation warnings in the whole run, which is what this pull request is about. Assisted-by: AI
4 participants
Every test run prints
Why not
zero_matrixThe deprecated method returns a plain Julia
Arrayof ring elements (Array{elem_type(R)}(undef, dims)filled with zeros), whilezero_matrixbuilds an AbstractAlgebra matrix type. That difference matters at these call sites:parametric_problems.jl, the arrays are concatenated withvcat/hcatnext toA = ConstantSystem([coeff(q, i) for i=0:dc, q in qs], ...), an ordinary JuliaMatrix, and then indexed elementwise (A[i+neq, i] = one(C));frontend.jlandgeneral.jl, the result is used as a vector (c[t+1] += ..., and splatted into a call).So substituting a matrix type would change the types flowing through those expressions.
What this does
Adds
zero_array(R, dims...), which reproduces the old behaviour explicitly and documents whyzero_matrixis not used, and switches the 18 ring-element call sites to it.zeros(Int, ...)calls areBase.zerosand are left untouched.The helper depends only on
AbstractAlgebra.elem_typeandAbstractAlgebra.NCRing, so it works across the whole supported AbstractAlgebra range rather than tracking when the deprecation lands.Validation
No behaviour change intended, and none measured:
TEST_GROUP=easy: 202 pass, 1 pre-existing broken — same asmain.TEST_GROUP=difficult: numbers identical tomain, RuleBased 92 succeeded / 50 failed / 35 maybe failed / 0 errored, Risch 57 succeeded / 77 failed / 40 maybe failed / 3 errored (the three errors are the pre-existingbuffered_operate_to!failures, unrelated to this change).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, and the reported counts are the compared results of those runs; the deprecated method's own source was read to confirm it returns a plainArrayrather than a matrix.Changes since the first review
Added
test/methods/risch/test_zero_array.jl, wired into theeasygroup. It covers theallocator (shape, element type, zero values over
QQ, a polynomial ring and a fractionfield, in vector / matrix / empty shapes; and that the entries are distinct objects, since a
shared mutable zero would make an in-place update to one entry visible in the others), that
convolutionemits no warning, and thatconvolutionstill computes the same result.The deprecation check is discriminating, as asked:
convolutionbuildscwithzeros(R, ...)(before this PR)zeros(R::NCRing, r::Int...)is deprecated"zero_array(R, ...)(this PR)Two details that make it reliable:
collect_test_logsinstalls a fresh logger, which resetsthe
maxlog = 1budget ofBase.depwarn, so a warning emitted earlier in the suite cannotmask this one; and a positive control asserts the deprecated method still warns in the same
process, so the check cannot pass vacuously. It is skipped when the process does not enable
depwarn, rather than passing for the wrong reason.
Also addressed the P3 items: the
returnRunic wants inzero_array, and the trailingwhitespace
git diff --checkflagged inparametric_problems.jl:934. The remaining Runiccomplaints about
general.jlare pre-existing onmain(verified by running Runic againstmain's copy of the file), so they are left out of this behaviour change.TEST_GROUP=easyon this head: 231 passed, 1 broken (pre-existing), exit 0, with zerodeprecation warnings in the whole run. Julia 1.13.0, AbstractAlgebra 0.50.2, Nemo 0.56.1,
SymbolicUtils 4.48.0, Symbolics 7.41.1.
Two red checks on this PR are not caused by it: the
difficultjobs fail on baseline entry15,
sin(sqrt(1+x))/sqrt(1+x), now filed as #145; and the Maximaqajobs fail to resolvebecause of the stale 0.2 bound fixed by #146.
Assisted by: AI
CI on head
6b2540a33 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.