Skip to content

T17: report - #80

Merged
github-actions[bot] merged 4 commits into
mainfrom
task/T17-uniform-batch
Oct 5, 2026
Merged

github-actions[bot] merged 4 commits into
mainfrom
task/T17-uniform-batch

Conversation

@github-actions

@github-actions github-actions Bot commented Oct 2, 2026

Copy link
Copy Markdown
Contributor

Closes #17

Task: T17 — opened by the workflow after the implementer run https://github.com/exanauts/SparseDirectSolver.jl/actions/runs/37063836067 ended without a pull request (the App token had expired). The Report block in TASKS.md and the commit message are the implementer's; the body of the PR template is not filled in.

Co-Authored-By: Claude Opus 5.5 noreply@anthropic.com

@github-actions github-actions Bot added the claude:pr PR opened by the Claude implementer; handled by the Claude pipeline label Oct 2, 2026
claude[bot] and others added 4 commits October 5, 2026 07:57
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@michel2323 michel2323 added claude:pr PR opened by the Claude implementer; handled by the Claude pipeline and removed claude:pr PR opened by the Claude implementer; handled by the Claude pipeline labels Oct 5, 2026
@michel2323
michel2323 force-pushed the task/T17-uniform-batch branch from f7cbd8e to a7d2107 Compare October 5, 2026 13:10
Comment thread src/numeric/factorize.jl
view(N.info, (ioff + 1):(ioff + count)), count)
ka_chol_check_info!(N.info, ioff, F11)
else
_potrf_batched_impl!(p.bpotrf, F11, N.info, ioff)

ghost Oct 5, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit (non-blocking): the :ka batched potrf goes through ka_potrf! on a 3-D array, which launches with a 2-D ndrange (WG, nb) (src/dense/fallback/potrf.jl:79). On the KA CPU backend that allocates per launch (the same reason the Report gives for flattening the batch index into 1-D ndranges everywhere else), so the batched regime-C path with impl = :auto/:ka on the CPU backend is not allocation-free, unlike the single-member path. GPUs are unaffected. Suggest a follow-up: a 1-D _ka_potrf_kernel! variant (group index decoded as (WG, member)) so the CPU budget tests can cover batched regime C too.

Comment thread src/solver.jl
Comment on lines +800 to +801
_set_members!(solver)
reduce_stats!(N, S)

ghost Oct 5, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit (non-blocking): for Cholesky, getparam("inertia"/"pivot_stats"/"npivots") on a batch re-reduces stats only for the members selected by the current ubatch_index/ubatch_mask, but the returned vector covers all nbatch members. The inactive members' entries come from whatever totals held after the last reduction (zeros if they were never active). That matches "inactive members are left untouched", but it is worth a sentence in the getparam docstring's uniform-batch paragraph so a caller reading one member's inertia after a masked refactorization knows the others may be stale.

ghost left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VERDICT: APPROVE (head a7d2107)

Reviewed T17 (uniform batch) against the task text in TASKS.md, AGENTS.md and PLAN.md §1.3, §1.4, §1.6, §3.5.

Scope. Only the task's deliverables: batch stride in Numeric (panels interleaved per front, info interleaved per entry, the rest member-major), a BatchMap argument replacing first in every numeric and solve kernel with the member index flattened into the 1-D ndrange, regime C over runs of consecutive members with strided-batched potrf/trsm/gemm and vendor pointer arrays built once at allocation, ubatch_size/ubatch_index/ubatch_mask, per-member info/inertia/npivots/pivot_stats, batched RHS layouts and MatrixDescriptor(T, n[, nrhs]; nbatch), generic auto-detection. PLAN.md and Manifest.toml untouched; TASKS.md changed only in the T17 section (marker [!], Report). Public names, parameter strings and phase strings unchanged. Backend code stays in ext/; the three new vendor bindings are declared in src/dense/vendor.jl with NotSupportedError defaults.

Tests. test/test_ubatch.jl covers every listed case: nb ∈ {1,2,3,16,64} × nrhs ∈ {1,2,4} for SPD and "S"; ubatch_index/ubatch_mask with a bitwise comparison of the untouched members' factor, D, piv, kinds, stats and info; strided / matrix / 3-D / descriptor layouts giving identical solutions; one non-SPD member with info[k] == column, 0 elsewhere, across regimes A+B, B, C; LinearAlgebra layer and error paths. test/ported/cudss_uniform_batch.jl ports the LDLᴴ and Cholesky parts of test_uniform_batch_cudss.jl (LU skipped, T19, as the task allows). Shared helpers were added in test/matrices.jl, test/utils.jl, test/backends.jl; loops over BACKENDS, ELTYPES (and INTTYPES in the port); tol(T); Random.seed!(666) from the driver. The three test_api.jl assertions that encoded "not until T17" now assert the implemented behaviour and are listed in the Report. No @test_broken added.

Conventions. Generic in T and INT; the batch overflow check (nb · factor_len < typemax(INT), nb · (ns+1) < typemax(Int32)) runs on the host before the device arrays are built. 1-D workgroups, @localmem sized by Val, no atomics in assembly/extend-add/pack_add! (each packed entry of each member is written by exactly one work item), dense ops through the interface, no @allowscalar, errors from src/errors.jl, docstrings on new public functions.

Correctness (read closely). MemberPanels is only ever indexed at s, never used as P[s+1] - P[s]; _zero_front! was changed to use f·w accordingly and layout.jl:186 confirms panels are exactly f·w and contiguous. amap offsets stay in the single-matrix layout and are shifted by _member_shift; the regime-A _subtree_scatter! keeps the single-layout front_ptr (local buffer offsets) while _subtree_write!/_lt_subtree_write! get member panels. info reads/writes (_iview, (s-1)·nb + k, ns·nb + k) are consistent across regimes A/B/C, cholesky_stats! and the host read. Solve: compact columns map to user columns through _bm_ucol, the per-member piv/d/pivot_kind views and the member panels are selected by _bm_cmember; the two kernels that still receive the single-matrix front_ptr (_sv_pull_kernel!, _sv_piv_kernel!) never read the factor. Refinement norms are indexed 2r-1, 2r on compact columns and read with ncols. trsm side/trans flags and gemm with transB = 'C' (real diagonal forced in the batched pack_add!) match the single path's herk. d_len = 2n matches the hard-coded stride in _factor_diag. ubatch_mask is a host Vector{Int} option, so active_members does no scalar device indexing.

Report. Claims match the diff; deviations (vendor trsm per member in the solve, transposed batched RHS and logabsdet of a batch unsupported, pivot pairs from member 1, the test_api.jl assertion changes) are listed. Test counts are plausible and not contradicted by CI: build is green, the CPU and CUDA test jobs are still pending at review time (handled by the pipeline). I could not run Julia in this session, so the counts are taken from the Report pending CI.

Blocking findings: none.

Non-blocking nits (inline): the :ka batched potrf uses ka_potrf!'s 2-D ndrange, which allocates on the KA CPU backend only; getparam of per-member pivot statistics after a masked Cholesky refactorization returns stale totals for inactive members (worth a docstring sentence).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

claude:pr PR opened by the Claude implementer; handled by the Claude pipeline

Projects

None yet

Development

Successfully merging this pull request may close these issues.

T17 — Uniform batch (v1 cut line)

1 participant