docs(hpc/soa): P2 savant tightenings — f32-only scope + hpc::soa layering rationale + integration test ungate (orphan rescue from #156)#157
Merged
Conversation
…ring rationale + integration test ungate Three small follow-ups from the P2 codex savant review of merged PR #156 (review doc at .claude/knowledge/w3-w6-p2-savant-review.md on the prior branch, also persists the rationale). C3 + A4 (src/hpc/soa.rs module header): - Document that aos_to_soa / soa_to_aos are currently hardwired to f32 output. i8 / u8 / u16 / bf16 SoA consumers must hand-roll the extract loop today; macro itself supports any field type. - Explain WHY these helpers live in hpc::soa and not crate::simd_ops: W1a consumer contract litmus rejects 'generic-library free functions' from the SIMD-dispatch glue layer; pure-scalar layout helpers belong at hpc:: co-located with the data type. A3 (src/hpc/bulk.rs): ungate the bulk_apply x aos_to_soa integration test. Worker-isolation deferral reason is gone now that both modules co-merged in #156. Test exercises the canonical SoA-stage-then-process compose pattern. bulk test count 16 -> 17. Non-blocking from savant view; ship-with-followups verdict. Other deferred P2s (soa_struct! parity with GaussianBatch lane-padded with_capacity, bulk_scan rename, #[inline] hints on hot-path entries, realistic SIMD-staging doctests) are queued for a separate cleanup PR.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Three small follow-ups from the P2 codex savant review of the just-merged PR #156 (W3-W6 SoA/AoS helpers). The review's verdict was SHIP-WITH-FOLLOWUPS — #156 merged before this commit could land on its branch, so the follow-ups carry as their own PR.
The full review doc is included:
.claude/knowledge/w3-w6-p2-savant-review.md— covers six review categories (A. ergonomics, B. naming, C. doc-prose, D. distance-typing visibility, E. future-proofing, F. CI signal) and the net call.What's in
C3 + A4 (
src/hpc/soa.rsmodule header expansion)Two related doc gaps the P2 savant flagged:
aos_to_soa<T, N, F: Fn(&T) -> [f32; N]>andsoa_to_aosare hardwired toSoaVec<f32, N>. Downstream consumers withi8/u8/u16/bf16SoA fields (palette indices, quantized embeddings, BF16 mantissa bytes) cannot use the public helpers today; module header now says this explicitly. The macro itself supports any field type.hpc::soavssimd_opsplacement rationale. Module header explains why these helpers live at thehpc::level and not incrate::simd_ops: the W1a consumer contract litmus rejects generic-library free functions from the SIMD-dispatch glue layer. Saves a future contributor from re-deriving the rule.A3 (
src/hpc/bulk.rsintegration test ungate)The
bulk_apply×aos_to_soaintegration test was gated behind#[cfg(any())](canonical never-compile sentinel) by Worker B becausehpc::soawasn't yet on the branch base when the worker ran. Now that both modules co-merged in #156, the deferral reason is gone. Ungating exercises the canonical SoA-stage-then-process compose pattern: outerbulk_applychunk loop, inneraos_to_soaper-chunk staging, closure body processes the SoA layout.hpc::bulktest count: 16 → 17.P2 savant review doc
.claude/knowledge/w3-w6-p2-savant-review.md— the savant's full advisory review of PR #156, persisted for future sessions.What's NOT in (P2 follow-ups deferred to a separate cleanup PR)
The savant flagged additional P2s. Deferred to a future cleanup PR:
soa_struct!macro is not parity with the existingGaussianBatchinsplat3d/gaussian.rs. Missing: lane-paddedwith_capacity(thepad_to_lanes(n, PREFERRED_F32_LANES)discipline),reserve,truncate,swap_remove,iter_mut. Means the macro is the "simple SoA struct" path;GaussianBatchstays bespoke until the macro grows awith_capacity_paddedknob.#[inline]hints onaos_to_soa/soa_to_aos/bulk_apply/bulk_scan. Useful for hot-path inlining of the user closure across crate boundaries.bulk_scannaming. Rust idiomIterator::scan= fold-with-state; this fn is a chunks walker. Rename tobulk_for_each/bulk_viewbefore any external consumer adopts the name.SoaVec::chunksandaos_to_soa(current doctests are 2-element smoke tests).aos_to_soa_scalar/soa_to_aos_scalarprivate fns so the future SIMD swap is a clean arm-addition not a structural refactor.Test plan
cargo check -p ndarray --no-default-features --features stdcleancargo test -p ndarray --lib --no-default-features --features std hpc::bulk— 17 passed (was 16)cargo test -p ndarray --lib --no-default-features --features std hpc::soa— 29 passed (unchanged)cargo fmt --all -- --checkcleancargo clippy --no-default-features --features std -- -D warningsclean (PR W3-W6: SoA/AoS layout helpers — SoaVec + soa_struct! + aos_to_soa + soa_to_aos + bulk_apply (scalar; SIMD deferred) #156's clean state preserved)Generated by Claude Code