es-ude / es-ude/OnDeviceTraining

Test/guard follow-ups from the #420 stack review

Open
#423 1 comment 0 reactions 0 assignees View on GitHub
Dominant language
C
Stars
1
Forks
3
Avg merge
1d 1h
Merged PRs (30d)
8

Description

Collector for the non-blocking follow-ups from the #420 test-debt stack review (stacked on PR #422; review APPROVED, these were explicitly classified "file, don't fix there"). Ordered by expected value.

### 1. Tighten the BFP deserialize exponent bound to `min(maxStored, bias + 127)`

#420 G5 rejects every stored exponent whose scale is non-finite (`stored > bias + 127`, written against the record's own bias). The residual gap: a finite-but-out-of-width byte (e.g. `exponentBits 4`, `stored 100` → scale `2^93`) is still admitted. Not a safety hole — no OOB, no UB, no guard bypass (review verified) — but `deriveBfpStoredExponent` caps at `min(maxStored, bias + 127)`, so no in-contract writer can produce such a byte and the deserializer should reject it as corrupt. One-token change in `Deserialize.c`'s BFP arm + one death-test variant.

### 2. `findMaxFloat` / `findMinFloat` / `findMaxInt32` / `findMinInt32` still read `values[0]` at `n == 0`

Same OOB-read class #420 G4 closed for `findAbsMaxFloat` (#160 family). Define `n == 0` per function (natural zero / identity choice needs a decision for min/max — `findAbsMaxFloat`'s `0.f` was the easy case), same poisoned-in-bounds-buffer mutant discipline. `UnitTestMinMax` (new in the #420 stack) is the home.

### 3. ConvT1d padded weightGrad coverage — blocked on Phase 2

`Conv1dTransposed.c` rejects every `paddingType` but VALID (Phase-1 contract), so the ConvT weightGrad kernel's `outIdx >= outputLength` clip and its unvisited-contributor `0.f` branch are structurally dead code today (review re-derived: `max outIdx = outLen − outputPadding − 1`). The #420 C1 fixture uses `outputPadding` as the reachable gy-rebinding analogue and positively pins the all-cells-visited property. If Phase 2 ever lifts the VALID-only gate: give the ConvT fixture the Conv1d twin's tap-membership-skip and exact-`0.0`-cell coverage, and add mutants (iii)/(iv) from the #420 C1 spec — those branches stop being dead the moment EXPLICIT padding becomes constructible.

### 4. Native `ARITH_BFP` conv backward end-to-end

The #420 C2 capstone pins the conv grad-STORAGE accumulate route (FLOAT32 math, BFP grad buffers) — deliberately, since Flatten rejects BFP wires until epic PR4. A conv capstone with `ARITH_BFP` math needs BFP wires through the model. **Likely closed by PR4's planned uniform-BFP training capstone** (conv→relu→maxpool→flatten→linear→softmax+CE); verify at PR4 merge and tick this off then, or add a conv-only model with a 3-D loss shape if wanted sooner.

### 5. SYM accumulate engines: same unclamped-round / non-finite-increment gap as the BFP engines

#421 scopes the clamp-before-round idiom and the non-finite-increment fail-fast for the **BFP** walkers only. The SYM accumulate engines (`accumulateIntoSymFixedGridEngine` / `accumulateIntoSymRescaleEngine`, and the ASYM Rescale sibling) still round a possibly non-finite quotient the same way — a NaN increment or a cap-class overflow walks into the float→int round UB there too. Same fix shape as #421 once its unified-walker pattern exists; deliberately kept out of #421 to hold its behavior contract tight.

### 6. BFP geometry aborts don't name their entry point

`validateBfpQConfigShape` aborts identify the invariant but not which public route tripped it (the #421 stack's U7 labeled the `packChunkGuarded` family; the geometry validator was out of its scope). Same `what`-label plumbing, applied to the validator's call sites (engines, pack sites, serial). Small, mechanical, best done when nothing else is in flight in `TensorConversion.c`.

### 7. Relocate the chunk-alignment `_Static_assert` into `TensorConversion.h`

`_Static_assert(ODT_CONVERSION_CHUNK_ELEMS % 8 == 0, …)` sits in `TensorConversion.c` while the macro it constrains lives in `TensorConversion.h` and other TUs (`Comparison.c`, `Relu.c`, `Dropout.c` since epic PR4) depend on the invariant for their `off * mantissaBits / 8` packed-buffer indexing. The invariant IS enforced today (the .c always compiles); this is locality hardening. Deferred out of both in-flight branches deliberately — the #421 branch and epic PR4 both touch that area and a header edit would merge-conflict; do it after both merge.

### 8. OPEN DESIGN DECISION (needs @LeoBuron): BFP value-domain saturation floor

Moved here from #420's body on its close (the stack merged; this is the one item there that was a decision, not work). Shipped behavior saturates the negative side to **−2^(m−1)** (qMin, the two's-complement floor) at every value-domain emit site — including the Rescale engine — while spec D6's text says **±qMax** (symmetric). Evidence and reachability notes: #420's "Residuals" section. Since the #421 walker unification this is a **one-site change** (`bfpRescaleWalk`'s qMin constant plus the shared clamp bounds). Decide: keep −2^(m−1) (one extra code of negative range, asymmetric) and amend D6, or align to ±qMax and adjust the golds that pin qMin saturation.

### 9. Loss operand guards: element count vs full shape contract

The PR4 adversarial-fix branch guards every loss entry (MSE + CrossEntropy, all dtype arms) on **element-count equality** between model output, label, and produced wire — the memory-safety fix. The codex delta review points out the public contract is stricter (same SHAPE), and a same-count/different-shape label (e.g. `[2,3]` vs `[3,2]` with differing `orderOfDimensions`) silently pairs the wrong logical elements while passing the count guard. Promoting to a full shape comparison is a SEMANTIC tightening with a real design question: a flat `[6]` label against a `[2,3]` output works correctly today (flat elementwise) and would start aborting. Decide the contract first (exact shape? count + flat-order equivalence?), then tighten in one place (the hoisted dispatcher guard).

Refs: #420 (stack), #416 / PR #422 (base branch), #421 (the two conversion-path validation gaps found in the same review are scoped THERE, not here).

🤖 Generated with [Claude Code](https://claude.com/claude-code)

https://claude.ai/code/session_01UzyrYp4e8J2tRjAbkhtnwo

Contributor guide

Open the contributing guide

Research direction

Begin with the referenced #420/#421 stacks and the named locations: Deserialize.c, UnitTestMinMax, Conv1dTransposed.c, TensorConversion.c/.h, and the loss dispatcher. Check the existing tests and PR4 status first; completion spans independent guards, coverage, validator labels, an assertion relocation, and unresolved BFP saturation and shape-contract decisions.

Written by the indexing model from the issue text.

Assessment

Tech stack
c
Domain
backend, machine-learning, testing
Issue type
Refactor
Difficulty
5/5
Estimated time
Over a week
Activity status
Active
Clarity
Needs clarification
Newbie friendliness
20/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.