es-ude / es-ude/OnDeviceTraining
Test/guard follow-ups from the #420 stack review
- 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
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