Fix: view-free G, zero-pivot info, analysis-time pair semantics, overflow guards, docstrings (#84)

[michel2323]

Closes #84

Refs #86, #65, #69, #72, #76, #80, #97

Task: post-T21 clean-up PR 1 (STATE.md §3, "Correctness or semantics for MadNLP" and "Tests and docs"), out of the task chain.

What was built:

1. Structure "G" refuses views 'L'/'U' (cuDSS accepts them) #84 — "G" ignores the view, as cuDSS (_entry_filter, src/symbolic/pattern.jl). The ported cudss_solver test gives "G" the full matrix with 'L'/'U'/'F' and checks analysis → factorization → solve give bitwise equal solutions; test_symbolic_etree checks equal SymmetricPattern/full_pattern_map for the three views; the old rejection tests in test_api/test_numeric_lu now solve with 'L'/'U'. view docstrings updated (DirectSolver, csr_of_transpose, SymmetricPattern).
2. Pivot pairs decided from analysis-time values: documented in the options table, setparam!, analysis_pairs and compute_ordering (Pairs: 2×2 pivot candidate pairs in the analysis (#64) #69).
3. pivot_threshold also sets the partner threshold for pivot_pairs = "default"; a value set after "analysis" doesn't change the pairs (#66: 2×2 pivot pairs only for structurally zero pivots by default; "all" keeps the #64 pairing #72).
4. getparam table: "npivots"/"inertia"/"pivot_stats" are valid only when info == 0; members outside the last ubatch_index/ubatch_mask keep their last counts (T15: device LDLᵀ/LDLᴴ in regimes A/B/C, diagonal solve, pivot statistics #76, T17: report #80). Also documents that pivot_sign is ignored for "G".
5. pivot_epsilon = 0 (T14: CPU reference LDLᵀ/LDLᴴ (in-front Bunch–Kaufman, perturbation, pivot_sign, inertia) #65): an exactly zero 1×1 pivot is now always tiny. With effective ε = 0 it stays zero, and the front records the failed assembled local column in stats[STAT_INFO]. This applies to ref_ldlt!, ref_lu!, the device LDLᵀ kernels (regimes A/B and the blocked regime-C path) and the LU kernels. reduce_stats! now also reduces the smallest failed factor column into info[ns+1]. factorize_ldlt!/factorize_lu! return the original column, and member_info!/"info" report it for every structure. For ε > 0 nothing changes. Tests in test_numeric_ldlt/test_numeric_lu use singular_block_matrix with pivot_epsilon = 0, stored_zero ∈ {false, true}, the default schedule and a regime-C root, for all ELTYPES. They check info == j on the reference, on the device and through getparam(solver, "info"), with piv/pivot_kind/D/the per-front info equal to the reference.
6. csr_of_transpose refuses rowval/nzval buffers longer than nnz(A) with InvalidValueError (T02: CSR container, backend adapters, matrix descriptors #31); tested.
7. Overflow guards: packed offsets in src/numeric/assembly.jl are computed in Int (T11: regime A fused subtree kernels, packed contribution blocks #59); allocate_numeric checks nbatch · work_len < typemax(INT) (perf: regime-C LDLᵀ through blocked pivot steps and GEMMs (#75) #89).
8. memory_estimates with matching adds the scaled values, weights, both scale vectors and, for "G", cperm to slots 1, 2 and 5 (T21: matching and scaling (MC64 jobs, perm_matching, scale_row/col, matching-based 2×2 pairs) #97); test_api covers "G" and "S".
9. Docstrings: factorize_ldlt! (signature, blocked/vendor-GEMM path, return value), factorize_lu!, factorize!; meaning of W = 0 (narrow regime-B in the kernel vs. group_width = 0 for the blocked path); sweep headers now show transpose = false; one docstring per LDLT_BLOCKED_MIN_*; perm_row/perm_col written as Dr[perm_row] A[perm_row, perm_col] Dc[perm_col], with the CSC remark. Also renamed the local sin in refinement.jl and gave FactorPreconditioner a concrete SC parameter for scaling.
10. size(solver, d), size(op, d), size(P, d) throw BoundsError for d < 1, as Base does for arrays.
11. TASKS.md: the Device LDLᵀ differs from ref_ldlt! on the K2 dumps (pivot sequence, nperturbed; inertia equal) #86 line under the T21 owner note.

Tests: SDS_TEST_GPU=0 julia --project=. -e 'using Pkg; Pkg.test()' on the KA CPU backend: 71014 pass, 0 fail, 1 broken (the existing @test_broken r1 <= r0/100 in test_refinement.jl, #79). CUDA: pending CI on the PR.

Deviations from PLAN.md / TASKS.md:

• pivot_pairs = "all" is not a workaround for analysis-time values. It reads the values too: candidates are rows with |aᵢᵢ| ≤ τ max|aᵢⱼ|, and partners must be non-candidates with aᵢⱼ ≠ 0. So an all-zero nzval gives no pairs under "all" either. The docs say to run "analysis" after the first assembly, for "default" and "all" alike. The suggestion "or use pivot_pairs = "all"" was not written.
• factorize_ldlt!/factorize_lu! now synchronize with the host once (reading nbatch Int32) when the factorization can fail: pivot_epsilon == 0, or pivot_epsilon_alg = "algo1", whose scaled ε can underflow. With the defaults they stay sync-free.
• After a zero pivot with ε = 0, the following divisions are left to IEEE arithmetic (the factor is unusable), as Cholesky leaves garbage above a failed front.
• PLAN §1.1 views row still says "until that fix lands "G" requires 'F'"; that clause is now stale (owner edit).
• LinearAlgebra _check_info still says "not positive definite" in its message; ldlt/lu cannot set ε = 0, so the message cannot be reached from them.

Left out of STATE.md §3 "Tests and docs" (§5 item 2), still open for the owner:

• growth_tol instead of panel_tol in test_numeric_ldlt.jl (STATE.md §3, Device LDLᵀ differs from ref_ldlt! on the K2 dumps (pivot sequence, nperturbed; inertia equal) #86 fragility).
• The missing per-testset Random.seed!(666) in the LDLᵀ/LU testsets (T15: device LDLᵀ/LDLᴴ in regimes A/B/C, diagonal solve, pivot statistics #76, T19: general LU ("G"): reference, device kernels in regimes A/B/C, LU solves with A/Aᵀ/Aᴴ, lu/lu!, report #85).
• Coverage of the blocked regime-B branch of ldlt_blocked_path by a default-analysis test (perf: regime-C LDLᵀ through blocked pivot steps and GEMMs (#75) #89).
• Tests for DirectSolver(::SparseMatrixCSC) and update!(::SparseMatrixCSC) (T13: public API v0.1 (DirectSolver, execute! phases, generic Cholesky, ported tests) #63).
• The @test_broken r1 <= r0/100 that can flip to an unexpected pass on another backend (T16: iterative refinement, solve_mode, interrupt, logging #79).
• The cholesky(::CuSparseMatrixCSR) overlap with CUDSS.jl (T13: public API v0.1 (DirectSolver, execute! phases, generic Cholesky, ported tests) #63) and the refine! capacity-column waste (T16: iterative refinement, solve_mode, interrupt, logging #79) are not in this PR either.

Follow-up issues opened: none.

🤖 Generated with Claude Code

https://claude.ai/code/session_01AvwMxoGmJm3eSmpzxAy72A

---

Generated by Claude Code

[claude]

Blocking: bitwise comparison of three independent solves is not reproducible on CUDA for real T.

The default forward sweep (deterministic_mode = 0) accumulates ancestor rows with Atomix.@atomic on backends that support it (src/solve/sweeps.jl header: "atomic (default) … regime B, and rows of a regime-A subtree that lie above its root"). The order of the floating-point additions then varies between launches, so xs[1] == xs[3] / xs[2] == xs[3] can fail intermittently in the cuda CI job for Float32/Float64 even though all three solvers see the same matrix. The existing solve tests guard bitwise checks with ws.atomic || for exactly this reason (test/test_solve.jl:131, issue #36). The implementer could only run the CPU backend, where the atomic variant is serial.

Fix (keeps the bitwise check meaningful, which is the point of the test): set the deterministic solve on the three solvers before the phases, e.g.

solver = DirectSolver(api_matrix(backend, A_cpu, INT), "G", view)
setparam!(solver, "deterministic_mode", 1)

Alternatively compare with norm(xs[1] - xs[3]) <= tol(T) * norm(xs[3]), but the deterministic mode is the better test of "the view is ignored".

[michel2323]

Fixed in the next push (commit not yet pushed): the three "G" solvers set deterministic_mode = 1 before the phases, so the bitwise comparison no longer depends on the atomic forward sweep's addition order on CUDA.

---

Generated by Claude Code

[claude]

nit (non-blocking): this is the "T21 note for #86" that STATE.md §5 asks for, but it is glued to the end of the #71 owner note without a blank line and without the **Owner note (issue #86)**: prefix the neighbouring notes use, and the line wrap does not follow the file's width. Suggest a separate paragraph in the same style:

**Owner note (issue #86)**: compare the device and the reference on the K2
dumps after scaling (inertia, `nperturbed`, `piv`); close #86 if scaling
removed the divergence, else close it as documented.

[michel2323]

Fixed in the same push: separate paragraph **Owner note (issue #86)**: in the style and wrap of the neighbouring notes.

---

Generated by Claude Code

[claude]

VERDICT: CHANGES_REQUESTED (head be091df)

Checked: scope against STATE.md §3/§5 (out-of-chain clean-up PR, Closes #84, label claude:pr, milestone set; PLAN.md and Manifest.toml untouched; the only TASKS.md change is the #86 note under T21 that STATE.md asks for); the "G" view change in _entry_filter and the absence of any other "G"/'F' check in src/ and ext/; the zero-pivot status path end to end: _lt_select!/_lu_pivot! record the assembled local column (piv[c0+c-1] - c0 + 1, resp. k), _lt_reset! clears _ST_STATUS per front in the regime-A, regime-B, LU and blocked-panel kernels (_lt_front_setup!, _ltc_setup!), _lt_stats!/_ltc_block_end! write STAT_INFO (first failed column across blocks), _reduce_stats_kernel! reduces super_ptr[s] + info - 1 into info[ns+1] with a Val-sized @localmem, _numeric_info!/_host_info map it through perm identically on device and reference, member_info! and the solver report it; the sync-only-when-it-can-fail condition (eps == 0 || scaled); the Int widening of the packed offsets, the work_len guard, the csr_of_transpose length check; _memory_estimates against the buffers _matching_state allocates (values, weights, rscale, cscale, cperm for "G" only); the pivot_threshold partner-threshold claim (zero_pivot_pairs!); the new tests (loops over BACKENDS/ELTYPES, Random.seed!(666), shared helpers, exact piv/pivot_kind comparison, singular_block_matrix gives an isolated 1×1 front so the "IEEE garbage after the failure" case is not exercised, which is fine). CI: CPU and CUDA test jobs still pending, build green; the failed fix job is the implementer workflow, not a test.

Blocking

1. test/ported/cudss_solver.jl:206-207 — the new "G" ignores the view test compares three independent solves bitwise, but the default forward sweep uses atomics on CUDA for real T (deterministic_mode = 0), so the check can fail intermittently in the cuda job. Set deterministic_mode = 1 on the three solvers (or compare with a tolerance). See the inline comment.

Non-blocking

• TASKS.md:2721 — format the #86 note as a separate **Owner note (issue #86)** paragraph (inline nit).
• The PR body names STATE.md §3 "Tests and docs" as its scope but does not say which items of that group were left out (growth_tol in test_numeric_ldlt.jl, the missing per-testset seeds of #76/#85, the blocked regime-B coverage of #89, the DirectSolver(::SparseMatrixCSC)/update! tests of #63, the @test_broken flip of #79). Please list them as left out, or do them, so the owner can track what remains of §5 item 2.
• src/numeric/assembly.jl:27,58-60 use Int(...) inside a kernel. This is the overflow guard the owner asked for (#59) and Int is 64-bit on every backend, so it is acceptable; noting it only because AGENTS.md otherwise forbids Int in kernels.

[claude]

nit (non-blocking): the blocked path records the failed column in stats[base + STAT_INFO] only, while _lt_stats! (regimes A and B) also writes info[s]. For a front on the blocked path numeric.info[s] therefore stays 0 after a zero pivot with ε = 0, so host_numeric(...).info[1:ns] disagrees with stats[6:6:end]. Nothing reads the per-front info for LDLᵀ today (reduce_stats! derives info[ns + 1] from stats), so this is cosmetic; writing info[Int(ctl[_ST_NODE])] = Int32(stats[base + STAT_INFO]) here (last block, or on every block end) would keep the two views consistent.

[michel2323]

Agreed, cosmetic today (only stats feeds info[ns + 1]). Writing info[s] at the block end rides this PR's next code push if one comes; otherwise it stays as is.

---

Generated by Claude Code

[claude]

VERDICT: APPROVE (head 2ef4a26)

Post-T21 clean-up PR out of the task chain (STATE.md §3 / §5 item 2, issue #84), reviewed against AGENTS.md, PLAN.md and the referenced issues.

Checked

• Scope: PLAN.md and Manifest.toml untouched; TASKS.md changed only by the T21 owner note for #86 that STATE.md §5 asks for; public names, parameter and phase strings unchanged. All eleven items of the PR body are in STATE.md §3 "Correctness or semantics for MadNLP" / "Tests and docs".
• "G" ignores the view: _entry_filter is the only place that rejected it; no view check remains in ext/; test_symbolic_etree, test_api, test_numeric_lu and the ported cudss_solver test cover L/U/F (bitwise-equal solutions with deterministic_mode = 1, consistent with PLAN determinism).
• pivot_epsilon = 0: the device status (piv[c0 + c - 1] - c0 + 1) is the front-local assembled column, matching the reference lp[k] read after the swap; c0 = super_ptr[s] also on the blocked path (_ltc_setup!), and _ltc_block_end! keeps the first failure over the blocks; _lt_reset! clears _ST_STATUS per front in all five kernels; _reduce_stats_kernel! is a deterministic @localmem min-reduction into info[ns + 1] (interleaved layout matches _numeric_info!); _host_info computes the same perm[super_ptr[s] + fi - 1]. The host sync is only taken when eps == 0 or scaled, as documented. For ε > 0 the pivot test is unchanged.
• Overflow guards (Int offsets in assembly, work_len check), csr_of_transpose capacity check, memory_estimates matching buffers (test reproduces the formula), size(·, d < 1) BoundsError, FactorPreconditioner type parameter, docstring fixes: all match the diff.
• Tests: new testsets loop over BACKENDS × ELTYPES, seed with Random.seed!(666), use singular_block_matrix, reference_ldlt, d_error, panel_tol, tol, relres from the shared helpers; no test weakened; no new @test_broken.
• Report: claims match the diff; the left-out STATE.md items (growth_tol, missing seeds, #89/#63/#79) are listed explicitly and all have existing issues. CPU count (71014 pass / 1 broken, #79) is plausible; CI (test-github-cpuonly, test-gpu cuda) is still pending at review time and does not contradict it. Julia could not be run in this session, so the local counts were not reproduced here.

Blocking findings: none.

Non-blocking: one inline nit on src/numeric/ldlt_c.jl (blocked path writes stats[STAT_INFO] but not info[s]). For the owner: STATE.md §5 item 2 also asked for growth_tol in test_numeric_ldlt.jl and the missing per-testset seeds; the PR leaves them out and says so.