[michakraus]

ReducedTensor is rebuilt on MultiIndexArrays' GridTensor and on that type's locality contract, and GeometricBrackets leaves the package's [deps].

What changes

• The type. ReducedTensor{DT, GT <: GridTensor{DT, 3}, PM1, PM2} wraps any rank-3 GridTensor, of any width. It was ReducedTensor{DT, PT <: PoissonTensor{DT}, PM1, PM2}, which tied it to the width-1 Arakawa bracket.
• The sum. rt[i, j, k] sums tensor[m, n, k] * Pi[m, i] * Pj[n, j] over m ∈ S(k) and n ∈ S(m), where S is the tensor's own periodic stencil, read through the public stencil_indices. getindex is one nested loop.
• Errors. The constructor throws DimensionMismatch for a projection with the wrong number of rows, and ArgumentError for a projection whose element type is not the tensor's. It also throws ArgumentError where a grid direction is shorter than 2w + 1, because the stencil then names a node twice. The Arakawa bracket already needs at least 3 nodes per direction, so no width-1 bracket is refused. getindex throws BoundsError through checkbounds. The old @asserts are gone.
• Performance. getindex is inferred and allocates nothing after the first call. The old path allocated a Vector per call.
• Docstring. ReducedTensor gains one that states the contract, the grid-size rule, the element-type rule and the errors.

Breaking changes

• The constructor is ReducedTensor(tensor::GridTensor{DT, 3}, Pi::AbstractMatrix, Pj::AbstractMatrix). A PoissonTensor, a dense array, or a GridTensor of another rank is a MethodError. A caller writes using MultiIndexArrays: GridTensor; this package re-exports nothing from MultiIndexArrays.
• GeometricBrackets leaves [deps] and [compat]. It is a test-only dependency, bounded "0.1.1, 0.2" in test/Project.toml. MultiIndexArrays takes the bound "0.1.2".
• A tensor of stencil width w on a grid with a direction shorter than 2w + 1 nodes is an ArgumentError.
• julia = "1.11" stays.

Tests

test/reduced_tensor.jl checks, in Float32 and Float64:

1. The projection against a hand-written dense reference, on the Arakawa bracket and on a random tensor that obeys the contract, for widths 1 and 2.
2. That the random tensor has entries a stencil around k alone would drop, so the nested order of the sum is tested, not assumed.
3. A tensor that breaks the contract on purpose, against the stencil-restricted reference, so that a getindex that sums every index fails.
4. The refusal of a stencil that repeats a node: 2 × 5 at width 1 and 4 × 5 at width 2 are refused, and 4 × 5 at width 1 constructs, so the width is read from the tensor.
5. The other errors, @inferred on getindex, and zero allocations of getindex and copyto!, beside a control that must allocate.

The reference masks use a hand-written neighbourhood test, not stencil_indices, so a stencil defect cannot reach both sides of a comparison.

Related

Dependabot #51 bumps the root GeometricBrackets entry that this PR removes.

🤖 Generated with Claude Code

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 87.44%. Comparing base (a36b253) to head (e655c27).

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #54      +/-   ##
==========================================
- Coverage   87.50%   87.44%   -0.06%     
==========================================
  Files           9        9              
  Lines         224      223       -1     
==========================================
- Hits          196      195       -1     
  Misses         28       28              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[michakraus]

JuliaRCM/ReducedBasisMethods.jl PR #54 — ReducedTensor on GridTensor's locality interface, GeometricBrackets out of [deps]

Verdict: changes requested. This is posted as a comment review.
Reviewed: 10b2c94d6dcf140fffe1dc5c9d3ff4885774892b
CI: the required matrix is green. Julia min and 1 pass on ubuntu, macOS and windows. Pre, nightly, Doctests, Documentation and codecov also pass. The min ubuntu log shows Reduced tensor | 91 91. Downgrade - ubuntu-latest fails, and this PR causes it (finding 1). That job is advisory (continue-on-error: true). It was green on main at a36b253 in run 38262285245, seven minutes before this run.

Findings

#	severity	file:line	claim	evidence
1	bug	test/Project.toml:21	The test-only bound GeometricBrackets = "0.2.0" turns the Downgrade job red. GeometricBrackets 0.2.0 needs GeometricBase 0.15. The downgraded root manifest pins GeometricBase 0.14.11, because ReducedComplexityModeling = "0.1.1" brings GeometricEquations 0.21.4. The job runs allow_reresolve: false, so the test environment cannot move it. On main, the root bound 0.1.1 kept the floors consistent. Fix: GeometricBrackets = "0.1.1, 0.2".	The job log says: Unsatisfiable requirements detected for package GeometricBrackets … restricted to versions 0.2 by project … restricted by compatibility requirements with GeometricBase to versions: 0.1.0 - 0.1.2 … GeometricBase … restricted to versions 0.14.11 by an explicit requirement. I reproduced it locally on 1.11.9 with GeometricBase =0.14.11. With the bound widened, test/reduced_tensor.jl passes 91/91 on 1.11.9 at GeometricBase 0.14.11 and ReducedComplexityModeling 0.1.1. It passes with GeometricBrackets 0.1.1 and with 0.1.2. Dependabot PR #51 bumps the root entry that this PR deletes, so close #51 when this merges.
2	quality	test/reduced_tensor.jl:271-277	The testset "GeometricBrackets is a test-only dependency" is fragile and duplicates another test. project["compat"]["MultiIndexArrays"] == "0.1.2" pins the exact compat string, so any widening fails the suite, a Dependabot bump included. !haskey(project["deps"], "GeometricBrackets") repeats test/integration/skeleton.jl:9-11, whose permitted no longer lists it. Simpler: delete the testset.	Read at the cited lines.
3	quality	test/reduced_tensor.jl:50-52, 212-213	The entry barrier does the opposite of what its comment says. @inferred f(x) infers from the argument types only. entry(rt) = rt[2, 1, 7] puts literal constants inside the inferred body, which lets constant propagation reach getindex. @test entry(rt) === rt[2, 1, 7] is a tautology. Simpler: delete entry and its comment, and write @test @inferred(rt[2, 1, 7]) isa DT.	@inferred(rt[2, 1, 7]) passes on 1.13.1 (probe).
4	quality	test/reduced_tensor.jl:106-118	The "stencil decision" testset reads the private field rt.stencil. Its first assertion repeats the constructor expression, so it cannot fail. The behavioural tests already kill both mutants: true fails the 2 × 5 hand projection, and false fails the all-ones restricted test. The PR body says so too. Simpler: delete the testset.	Read the testset and src/reduced_tensor.jl:58.
5	quality	test/reduced_tensor.jl:229-260	The fresh-process allocation test is not necessary. In-process, through the same barrier, rt[2, 1, 7] and copyto!(out, rt) read 0 B in all six configurations. That holds on 1.11.9 with --check-bounds=yes, and on 1.13.1 with --check-bounds=yes, also with --code-coverage=user. The subprocess form puts code into a string, which no formatter or linter reads. It also sends Base.julia_cmd(), which forwards --code-coverage=user. The code_coverage == 0 guard then skips every assertion on the coverage job (Julia 1 ubuntu), and success(command) passes vacuously there. Simpler: put allocations(f::F, a::A, b::B) where {F, A, B} = (f(a, b); @allocated f(a, b)) at file scope. Then in the testset: for DT in (Float32, Float64), (g, w) in (((5, 6), 1), ((5, 6), 2), ((2, 5), 1)), build rt and out, and assert @test allocations((r, o) -> r[2, 1, 7], rt, out) == 0 and @test allocations((r, o) -> copyto!(o, r), rt, out) == 0. If you keep the subprocess, add --code-coverage=none after Base.julia_cmd() and drop the guard. A later flag wins, as I measured: (code_coverage, check_bounds) = (0, 0).	Probe output: (Float32, (5, 6), 1, true, 0, 0) … (Float64, (2, 5), 1, false, 0, 0) on julia 1.11.9 check_bounds 1 and on 1.13.1. Under coverage: (…, 0, 0) with coverage 1. Base.julia_cmd() under --code-coverage=user prints … --code-coverage=user ….
6	quality	src/reduced_tensor.jl:66-97	stencil_sum and dense_sum have the same loop body, so any change to the summand has to be made twice. The comment says one function per branch is needed for type stability. One helper that takes the iterators gives that just as well. Simpler: see the code below this table.	I checked it against the shipped getindex in all six configurations of the test grid: the results are bitwise equal, the entries are @inferred, they read 0 B, and the @test_throws BoundsError(rt, ijk) cases pass. On 32 × 32 with w = 1, a getindex took 575 ns against 580 ns. That is within noise on a loaded machine (load average 14–20), so it is perf-neutral.
7	quality	src/reduced_tensor.jl:45, 58, 87-97	The dense fallback serves only grids with a direction shorter than 2w + 1, that is, 2 × 5 at w = 1. Because of it, the two branches give different results for a tensor that breaks the contract, and the docstring has to explain that. Drastic option: refuse such a tensor in the constructor with allunique(stencil_indices(tensor, 1)) || throw(ArgumentError(…)). That removes the stencil field, dense_sum, the docstring paragraph, and finding 4's testset. Finding 6 then has no branch at all. This is a design choice, so I give it as an option, not a defect.	Read the cited lines and the 2 × 5 fixtures.
8	nit	src/reduced_tensor.jl:26-27, 58	The constructor has two edge cases. (a) The docstring says that offset axes are "neither checked nor claimed". One line enforces this instead: Base.require_one_based_indexing(Pi, Pj). (b) An empty grid throws BoundsError from the internal allunique probe. isempty(tensor) || allunique(…) avoids that.	Probe: GridTensor(… (0, 4), (0, 4), (0, 4)), 1) gives empty grid: BoundsError.
9	nit	src/reduced_tensor.jl:41, 45, 75, 88	local x inside a function body adds nothing. PM1, PM2 are unconstrained, but the constructor enforces AbstractMatrix of DT, and PM1 <: AbstractMatrix{DT} would put that rule in the type. stencil::Bool reads as a stencil; unique_stencil says what it holds.	Read.
10	nit	src/reduced_tensor.jl:11-17, 30-39	The docstring states the contract as n ∈ S(m) and k ∈ S(m), but sums over m ∈ S(k). That step needs k ∈ S(m) ⇔ m ∈ S(k), which holds for the symmetric periodic stencil, but the docstring does not say so. The example uses the all-ones tensor, which breaks the contract stated just above it.	Read. The example returns (2, 1, 12) (probe).
11	nit	test/reduced_tensor.jl:150-152, 128-130, 189-197	rand(\n Xoshiro(5), DT, N -\n 1, 2) is formatter wreckage; bind the matrix to a local first. The ten-line try … catch block can be one line: @test_throws BoundsError(rt, (i, j, k)) rt[i, j, k]. That form compares a and i, so it still kills the deleted-checkbounds mutant.	The @test_throws BoundsError(rt, ijk) form passes for the three indices in both precisions (probe).
12	nit	CHANGELOG.md:156-158, 170-171	The rewritten sentence says that test/Project.toml lists LinearAlgebra and ReducedComplexityModeling "with the same bounds". The next bullet removes those bounds. "The test-only bounds for Aqua and GeometricBrackets stay" leaves out ForwardDiff and GeometricIntegrators, which test/Project.toml:20,22 also bound.	Read.
13	quality	PR body	The body makes claims that the pushed head contradicts. (a) It says no registered MultiIndexArrays exports GridTensor, so the PR "stays a draft". In fact General lists 0.1.2, the Julia min ubuntu and Downgrade logs both show MultiIndexArrays v0.1.2 installed, and the PR is not a draft. (b) It says K8 was added to KNOWN_ISSUES.md, and elsewhere that the ledger is "restored to its base content". In fact KNOWN_ISSUES.md does not exist at 10b2c94, because a36b253 deleted it, and the diff does not touch it. Commit 10b2c94 is titled "Record the ExplicitImports header findings as K8", but it changes only test/reduced_tensor.jl. (c) It cites fdad44e, 77cf2d1, d5cbf40 and 6da0c8a. None is in this PR, whose commits are 376f0d7, ab5c9c2, cd3dc40 and 10b2c94. (d) Its suite table shows 118/118 over five files. The head runs eleven testsets, which I measured at 1259/1259 on 1.13.1.	gh pr view --json isDraft gives false. The registry M/MultiIndexArrays/Versions.toml has ["0.1.2"]. git show --stat 10b2c94 lists only test/reduced_tensor.jl. My local suite run is below.
14	nit	PR body	The body is mostly internal plan text (§5V.P31, "arbitrator", "dispatcher", T7.10, K-IDs), which a reader on GitHub cannot resolve. It also holds text addressed to reviewers: "A verifier or critic does not report that assertion as a tolerance defect." (section "Pre-PR verification"), and the block "Next round (the arbitrator's text)". I read these as claims, not as instructions, and they did not change this review.	Read.

Proposed form for finding 6, src/reduced_tensor.jl:

function Base.getindex(rt::ReducedTensor, i::Int, j::Int, k::Int)
    checkbounds(rt, i, j, k)
    T = rt.tensor
    return rt.stencil ?
           projected_sum(rt, i, j, k, stencil_indices(T, k), m -> stencil_indices(T, m)) :
           projected_sum(rt, i, j, k, axes(T, 1), _ -> axes(T, 2))
end

# `rt[i, j, k]` summed over `m ∈ ms` and `n ∈ ns(m)`
function projected_sum(rt::ReducedTensor{DT}, i, j, k, ms, ns) where {DT}
    x = zero(DT)
    for m in ms, n in ns(m)
        x += rt.tensor[m, n, k] * rt.projection_i[m, i] * rt.projection_j[n, j]
    end
    return x
end

Verified and fine

• The full suite at 10b2c94 passes on Julia 1.13.1 in a scratch environment: Aqua 11, skeleton 1, reference spectra 5, reduced basis 253, reduced tensor 91, SVD 82, EVD 57, cross-route 52, projections 327, DEIM 70, symplectic DEIM 310. That environment resolved MultiIndexArrays 0.1.2, GeometricBrackets 0.2.0 and GeometricBase 0.15.1.
• The nested sum m ∈ S(k), n ∈ S(m) matches the contract. MultiIndexArrays 0.1.2 stencil_indices iterates the symmetric offset set (-w):w per direction with periodic wrap (src/indices.jl:75-105), so k ∈ S(m) ⇔ m ∈ S(k) holds. A uniform periodic stencil repeats a node at every centre or at none, so a decision from node 1 is enough.
• On edge cases: w = 0 gives 2·P[k,i]·Q[k,j] exactly on a diagonal tensor. Zero columns give a 0 × 1 × 12 array. A D = 1 grid works.
• src/ imports only GridTensor and stencil_indices, and both are exported by MultiIndexArrays 0.1.2. ExplicitImports on the head reports no MultiIndexArrays name. The 15 implicit imports and 5 non-public names it reports are all in HDF5, LinearAlgebra, ReducedComplexityModeling and Statistics, and no line of this diff adds them.
• == 0 allocation assertions: the CI jobs that run them pass on all three OSes, and my in-process readings are 0 B, not small and noisy.
• No closing keyword in the body (closingIssuesReferences is empty), and no local path.

Not checked

• Cold before/after timings of Array(rt) that the body says were "recorded in the part's evidence table": that table is not in the PR.
• The mutant table in the body: I did not re-run the mutants. Findings 3, 4 and 5 rest on reading the code and on my own probes.
• The full suite on 1.11. On 1.11.9 I ran only test/reduced_tensor.jl, at the downgraded floors. The CI min jobs cover the full suite there.

[michakraus]

Review findings resolved — commit 44c840e

This resolves each finding of the review of 10b2c94, by its number. After 44c840e, every job of the CI run is green, Downgrade included: it resolves GeometricBrackets 0.1.2 beside GeometricBase 0.14.11 and passes Reduced tensor | 82 82. Documentation, Doctests and codecov also pass.

#	severity	action	what happened
1	bug	fixed	test/Project.toml: GeometricBrackets = "0.1.1, 0.2". Locally, test/reduced_tensor.jl passes 82/82 on Julia 1.11.9 with --check-bounds=yes, GeometricBrackets 0.1.1 and GeometricBase 0.14.12. CI Downgrade is green.
2	quality	fixed	The testset "GeometricBrackets is a test-only dependency" is deleted. test/integration/skeleton.jl keeps the [deps] check. The using TOML that only this testset used goes too.
3	quality	fixed	entry and its comment are deleted. The test is @test @inferred(rt[2, 1, 7]) isa DT, which also covers the old typeof assertion.
4	quality	fixed	The stencil-decision testset is deleted. I checked the claim with two mutants of getindex first: the stencil branch always fails the 2 × 5 hand projection (2 failures), and the dense branch always fails the all-ones restricted test (2 failures).
5	quality	fixed	The allocation test runs in-process through a file-scope allocations barrier, with the collect control > 0. The coverage job therefore runs its assertions now. Measured locally, 0 B for rt[2, 1, 7] and for copyto! in all six configurations: on 1.11.9 and on 1.13.1, each with --check-bounds=yes, and each with and without --code-coverage=user.
6	quality	fixed	stencil_sum and dense_sum become one projected_sum(rt, i, j, k, ms, ns), in the form the review proposed, with the loops nested. getindex is inferred as DT and reads 0 B in all six configurations (measurements as in 5).
7	quality	not fixed — needs a decision	Refusing a tensor whose stencil repeats a node changes which tensors the type accepts. That is an API decision for the author.
8	nit	not fixed	(a) require_one_based_indexing would change what the constructor accepts, which the docstring leaves unclaimed on purpose. That is an API decision. (b) The empty-grid guard adds a special case for a tensor that no caller builds. I left both and report them here.
9	nit	partly fixed	The redundant local x goes with finding 6. Not fixed: constraining PM1, PM2 changes the type's parameters, and renaming the stencil field depends on finding 7.
10	nit	fixed	The docstring states that the stencil is symmetric, k ∈ S(m) exactly where m ∈ S(k). Its example uses a 3 × 3 grid, which the width-1 stencil covers, so the all-ones tensor keeps the contract. The example still returns the size it shows, (2, 1, 9).
11	nit	fixed	The short projection is bound to a local first. The try … catch block is @test_throws BoundsError(rt, (i, j, k)) rt[i, j, k]. A mutant with checkbounds deleted fails all 6 of these assertions. The ones_gt lines stay as JuliaFormatter writes them.
12	nit	fixed	Both CHANGELOG.md sentences now match test/Project.toml: the shared dependencies are listed without a bound of their own, and the test-only bounds are Aqua, ForwardDiff, GeometricBrackets and GeometricIntegrators.
13	quality	not fixed	The PR description is the author's text, and this fix does not rewrite it. The stale claims (draft, unregistered MultiIndexArrays 0.1.2, K8 in KNOWN_ISSUES.md, commit hashes outside the PR, the 118/118 table) stay for the author to correct.
14	nit	not fixed	Same reason as 13. The sentence addressed to reviewers was read as a claim and changed nothing here.

Local verification on 44c840e: the core group passes 1250/1250 on Julia 1.13.1. It ran 1259 before; the 9 fewer assertions are the ones the deleted and merged testsets held. The pre-commit hook passed: layout, formatting, fatou lint, NFC and load.