Conversation
The consciously deferred PARTIAL findings from the #366 CodeRabbit triage (quick-wins landed in PR #367; these are the rest): - RowBroadcast: identity-orderOfDimensions gate on mat/vec/out -- the kernels index flat row-major, a permuted operand was silently misindexed (no OOB, wrong values). Same gate family as PointwiseFused (#339). - ppcaReplayCreate: NaN-robust sigma2Floor > 0 guard (adamWInit betas idiom) -- a NaN/non-positive floor silently collapses replay samples toward the class mean (research-poisoning, no crash). cfg-NULL check deliberately NOT added (module convention). - validateOptimizerGradStorage: NULL grad in a collected trainable slot now fails fast at create instead of being silently skipped -- no freeze mechanism exists and step/zeroGrad dereference grads unconditionally (decision recorded from the triage: fix at the validator, #261 context). - PpcaReplaySerialize round-trip: pin file-carried qConfig metadata (SYM basis scale, ASYM mean scale+zeroPoint) so packed payload bytes can never silently decode against a drifted grid. Mutation-checked (scale mis-serialization goes RED here while payload asserts stay green). - RNG: 0x1p24f instead of (float)(1 << 24) -- value-identical, removes the repo's sole untyped wide shift (UB only on 16-bit int). - CONTINUAL_LEARNING.md: half-sentence on the exact uint32 count-wrap (degenerate weights, not a crash; ~256x beyond the 2^24 float horizon). All guards TDD'd (death tests RED first); full ctest 81/81, examples build, pytest 29/29. Co-Authored-By: Claude Fable 5 <[email protected]>
Adopt the sweep-ratified policy: optimizer param/state write-backs requant with seeded SR_HALF_AWAY by default, escaping the fixed-scale dead-zone (sub-ULP updates survive in expectation; deterministic HALF_AWAY froze sym4 HAR training at random-guessing from step 0). Mechanics: rounding for training write-backs is operation-owned (#282 direction) -- sgdStepM/adamWStep route every param/state OUT_WRITE through the new funnel entry executeOpWithEpilogueRounding(spec, target, mode), which transiently swaps the target's storage roundingMode and restores it before returning. The tensor's own qConfig stays authoritative for storage/inference encodes and serialization (roundingMode is file-carried). API: optimizer_t.writeBackRounding, factory default SR_HALF_AWAY in sgdMCreateOptim/adamWCreateOptim; optimizerSetWriteBackRounding(optim, HALF_AWAY) is the explicit deterministic opt-out. Hand-assembled optimizers (test-tier) set the field explicitly -- it is uninitialized stack otherwise (caught by the UBSan CI job on the first push). Examples: train_c_har_classifier_sym maps SYM_ROUNDING=det onto the opt-out setter (param storage configs now stay HALF_AWAY -> deterministic init encode); mixed_width_mlp deliberately trains under the new default (loss gate re-verified: 2.29 -> 1.55). Smoke: 1-epoch HAR sym4 SR 0.353 test_acc vs det 0.176 (~1/6 random). Tests: override beats param qConfig in both directions (SGD fast path, momentum param path, quantized momentum state; AdamW K3 + m/v moments), restore pinned via storage-mode asserts, factory defaults + setter pinned. Mutation-checked: override-skip, restore-skip, m-only, v-only all RED. Co-Authored-By: Claude Fable 5 <[email protected]>
…ool1d Funnel-routed integer-exact kernels, forward AND backward (dx via OUT_WRITE, Conv1d-dx precedent); FLOAT32 arms untouched. - MaxPool1d: mantissa select + scale copy (ReLU idiom); quantized-domain argmax tie-break (strict >, first wins) pinned by an argmax-gold tie fixture - AvgPool1d: int32 window sum + exact scale fold s_out = s_in/K (Dropout idiom) -- zero kernel rounding, count_include_pad=true falls out of the fold - AdaptiveAvgPool1d: rounded integer division of the mantissa sum (half-away, roundByMode-consistent) at unchanged scale, <= 0.5 LSB per element - value-sum overflow guards (review finding): poolValidateSymValueSum per summing arm -- qMaxBits in [1,16] + worst-case terms < 2^(32-qMaxBits) (Reduce.c/LayerNorm validator precedent; unreachable at the int12 default, closes the silent int32 overflow for legal wider wires, e.g. a 16-bit global-average pool over L >= 65536); death-tested per call site - goldgen: bit-exact mantissa emulation per layer; shared int12 helpers + windowSlice1dAt emulation lifted additively into test/unit/goldgen/sym_gold.py - 16 new Unity tests (10 gold + 6 death) across the three suites; gold tests mutation-verified (9 mutations: tie-break, scale copy/fold x4, dilation x2, scatter-overwrite x2, trunc-div -- each caught by the expected test), guard call sites verified by the death tests' RED-first runs - docs: pooling-SYM section incl. guard contract in docs/conventions/arithmetic-sym.md; FEATURES.md matrix rows + known-gaps entry updated; stale 'Only a FLOAT32 arm exists' adapter comments fixed Closes #205 Co-Authored-By: Claude Fable 5 <[email protected]>
…xture Fake-quant arms (the MSE idiom — dequant, float core, requant): - forward: crossEntropyForwardFakeQuant dequantizes softmax-output + label into FLOAT32 stack scratch and reuses crossEntropyForwardFloat (clamp, logf, microbatch-MEAN divisor all shared); SYM_INT32 dispatch case added - backward: the ASYM helper was already dtype-generic (convertTensor-only) -- renamed to crossEntropySoftmaxBackwardFakeQuant, SYM_INT32 case routes to it; (p-y) requantizes into the seed wire's own int12 config (fresh absmax scale). The CalculateGradsSequential combined-gradient shortcut needs no change: the seed inherits the model output's dtype (initGradTensor NULL) - large-logit (~90) softmax regression fixture, FLOAT32 + SYM twin (#201 closure residual): pins the funnel kernel's max-subtraction -- previously mutation-unprotected (fixtures topped out at logit 5) - goldgen: generate_expected_cross_entropy_sym.py (sym_gold int12 helpers, round-trip-stable fixtures, f64 self-checks) - 5 new tests, mutation-verified (5 mutations: stabilization removal caught by exactly the two new fixtures, both SYM dispatch cases, sign flip, dequant skip -- each caught) - cleanup: dead commented-out crossEntropyForwardFloat block removed - docs: FEATURES.md loss row + known-gaps entry updated Part of #206 -- the empirical acceptance half (full-SYM HAR 10-seed run vs FLOAT32 twin, measured degradation into the example README) follows as its own PR; the issue stays open until that lands. Co-Authored-By: Claude Fable 5 <[email protected]>
…rics argmax Acceptance-run plumbing for #206: - TrainingLoopApi: argmaxByTensor dispatches on the output wire's dtype -- SYM_INT32 mantissa order IS value order (scale > 0), while the previous unconditional float* cast reinterpreted negative mantissas as NaN bit patterns and froze the argmax at index 0 (accuracy garbage on any SYM output wire). Fail-fast on other dtypes. TDD: identity-SYM-linear fixture with negative features REDs the old cast, greens the dispatch. - train_c_sym.c: SYM_WIRES=1 env knob -- switches activation + dx wires to SYM_INT32 (int12) and dx compute to the native SYM arms across all twelve layers (conv/linear propLossMath SYM; ReLU/pools/Softmax on a uniform SYM layerQuant). Param grads stay FLOAT32 (#261). Smoke-verified: gates pass, initial loss ~ln(6) through SYM softmax + fake-quant CE, epoch-1 val_acc 0.13 -> 0.38 (the full SYM wire chain trains). - run_matrix.py: sym8w config arm (SYM_BITS=8 + SYM_WIRES=1). Part of #206 -- the 10-seed sym8w-vs-float sweep result lands in the example README next. Co-Authored-By: Claude Fable 5 <[email protected]>
…ontract The constant is routinely misread as a backward-compute width or a memory knob. Spell out the actual contract at the definition: it caps the declared width of SYM grad ACCUMULATION TARGETS (enforced in accumulateOut), while backward kernels always compute at operand width, and SYM_INT32 grads save no memory at any width (#261). Also fix the stale gradInitSymInt32 docstring (missing roundingMode param, no mention of the fixed 16-bit width or its test/research-helper role). Co-Authored-By: Claude Fable 5 <[email protected]>
|
Warning Review limit reached
Next review available in: 49 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe PR adds SYM_INT32 support for pooling and CrossEntropy, introduces optimizer-owned write-back rounding with seeded stochastic defaults, enables configurable full-SYM HAR wires, updates dtype-aware evaluation, and expands validation, gold-generation, and unit-test coverage. ChangesSYM arithmetic and training integration
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (1 warning, 2 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
examples/har_classifier/train_c_sym.c (1)
540-540: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse
sq.floatQinstead of allocating a fresh FLOAT32 quantization.
sq.floatQis already aquantizationInitFloat()handle serving exactly this purpose (seesymLayerQuant, Line 254, which reusessq->floatQ). Allocating a second float quant here is redundant and the handle is never freed. Minor and example-only, but it also keeps the two wire paths symmetric.- layerQuantInitUniform(&lqNontrain, g_symWires ? sq.symWireQ : quantizationInitFloat()); + layerQuantInitUniform(&lqNontrain, g_symWires ? sq.symWireQ : sq.floatQ);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@examples/har_classifier/train_c_sym.c` at line 540, Update the layerQuantInitUniform call for lqNontrain to reuse sq.floatQ when g_symWires is false, matching the existing symLayerQuant reuse pattern and avoiding a new quantizationInitFloat allocation; leave the g_symWires path unchanged.test/unit/loss_functions/generate_expected_cross_entropy_sym.py (1)
82-82: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUnused unpacked variable
pq.Ruff flags this as unused; prefix with underscore for clarity.
🧹 Proposed fix
- pq, _, p_deq = stable_dequant_i12(p) + _pq, _, p_deq = stable_dequant_i12(p)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/unit/loss_functions/generate_expected_cross_entropy_sym.py` at line 82, Rename the unused first unpacked result in the stable_dequant_i12 call to use an underscore-prefixed name, while preserving the existing _, p_deq assignments and behavior.Source: Linters/SAST tools
test/unit/layer/generate_expected_adaptive_avg_pool_1d.py (1)
159-193: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMinor lint cleanups flagged by Ruff.
- Line 159:
_adaptive_window(L, O, o)— ambiguous single-char nameO(E741).- Line 193:
int(round(s / k))—round()withoutndigitsalready returnsint(RUF046).♻️ Suggested tweaks
-def _adaptive_window(L, O, o): - start = (o * L) // O - end = ((o + 1) * L + O - 1) // O +def _adaptive_window(L, out_size, o): + start = (o * L) // out_size + end = ((o + 1) * L + out_size - 1) // out_size return start, end - start- lambda s, k: int(round(s / k))) # Python banker's rounding = half-even + lambda s, k: round(s / k)) # Python banker's rounding = half-evenNote
_adaptive_windowis called at several sites, so update the callers when renaming the parameter.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/unit/layer/generate_expected_adaptive_avg_pool_1d.py` around lines 159 - 193, Apply the Ruff cleanups in _adaptive_window and its callers: rename the ambiguous parameter O to a descriptive name such as output_size, updating every positional or keyword reference consistently, and remove the redundant int() wrapper around round(s / k) in the y_even calculation while preserving its half-even behavior.Source: Linters/SAST tools
test/unit/layer/generate_expected_avg_pool_1d.py (1)
257-257: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
randnfixtures are seeded fromhash(name), which is not stable across Python processes. With the default saltedPYTHONHASHSEED,hash(name)varies per invocation, so regenerating produces differentlossGrad/gold. It is self-consistent within a single generation run, but it diverges from the deterministic contract ingenerate_expected_adaptive_avg_pool_1d.py(explicitloss_seed, raises if missing). Prefer a fixed integer seed per fixture.
test/unit/layer/generate_expected_avg_pool_1d.py#L257-L257: replacetorch.manual_seed(hash(name) & 0xFFFF)with a fixed per-fixture seed (mirror the adaptive generator's explicit-seed pattern).test/unit/layer/generate_expected_max_pool_1d.py#L262-L262: same fix — use an explicit fixed seed instead ofhash(name).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/unit/layer/generate_expected_avg_pool_1d.py` at line 257, The fixture generators use process-dependent hash values for random seeds, so replace them with explicit fixed per-fixture seeds following the pattern in generate_expected_adaptive_avg_pool_1d.py. Update test/unit/layer/generate_expected_avg_pool_1d.py at lines 257-257 and test/unit/layer/generate_expected_max_pool_1d.py at lines 262-262, ensuring each fixture has a defined seed and missing seeds are handled consistently with the adaptive generator.src/layer/AdaptiveAvgPool1d.c (1)
71-92: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoff
poolValidateSymValueSumand itsPOOL_SYM_VALUESUM_QMAXBITSbound are duplicated verbatim across three pooling TUs. The guard, its constant, and the doc comment are byte-identical; a future fix (e.g. bound tuning or a dtype assertion) must be applied in three places. Consider hoisting into a shared internal header (e.g.PoolSymCommon.h) included by all three.
src/layer/AdaptiveAvgPool1d.c#L71-L92: replace the local define/function with the shared include.src/layer/AvgPool1d.c#L74-L95: replace the local define/function with the shared include.src/layer/MaxPool1d.c#L236-L257: replace the local define/function with the shared include.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/layer/AdaptiveAvgPool1d.c` around lines 71 - 92, Centralize the duplicated POOL_SYM_VALUESUM_QMAXBITS definition, documentation, and poolValidateSymValueSum function in a shared internal PoolSymCommon.h, then include it from src/layer/AdaptiveAvgPool1d.c lines 71-92, src/layer/AvgPool1d.c lines 74-95, and src/layer/MaxPool1d.c lines 236-257 while removing each local copy.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@examples/har_classifier/README.md`:
- Line 179: Reflow the paragraph continuation in README.md so the line
containing “#137–#142 ladder), not this run.” no longer starts with a hash;
preserve the intended issue references and sentence meaning.
---
Nitpick comments:
In `@examples/har_classifier/train_c_sym.c`:
- Line 540: Update the layerQuantInitUniform call for lqNontrain to reuse
sq.floatQ when g_symWires is false, matching the existing symLayerQuant reuse
pattern and avoiding a new quantizationInitFloat allocation; leave the
g_symWires path unchanged.
In `@src/layer/AdaptiveAvgPool1d.c`:
- Around line 71-92: Centralize the duplicated POOL_SYM_VALUESUM_QMAXBITS
definition, documentation, and poolValidateSymValueSum function in a shared
internal PoolSymCommon.h, then include it from src/layer/AdaptiveAvgPool1d.c
lines 71-92, src/layer/AvgPool1d.c lines 74-95, and src/layer/MaxPool1d.c lines
236-257 while removing each local copy.
In `@test/unit/layer/generate_expected_adaptive_avg_pool_1d.py`:
- Around line 159-193: Apply the Ruff cleanups in _adaptive_window and its
callers: rename the ambiguous parameter O to a descriptive name such as
output_size, updating every positional or keyword reference consistently, and
remove the redundant int() wrapper around round(s / k) in the y_even calculation
while preserving its half-even behavior.
In `@test/unit/layer/generate_expected_avg_pool_1d.py`:
- Line 257: The fixture generators use process-dependent hash values for random
seeds, so replace them with explicit fixed per-fixture seeds following the
pattern in generate_expected_adaptive_avg_pool_1d.py. Update
test/unit/layer/generate_expected_avg_pool_1d.py at lines 257-257 and
test/unit/layer/generate_expected_max_pool_1d.py at lines 262-262, ensuring each
fixture has a defined seed and missing seeds are handled consistently with the
adaptive generator.
In `@test/unit/loss_functions/generate_expected_cross_entropy_sym.py`:
- Line 82: Rename the unused first unpacked result in the stable_dequant_i12
call to use an underscore-prefixed name, while preserving the existing _, p_deq
assignments and behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1bf52f46-fd5c-40eb-aff7-18bb45886de5
📒 Files selected for processing (42)
docs/CONTINUAL_LEARNING.mddocs/FEATURES.mddocs/conventions/arithmetic-sym.mdexamples/har_classifier/README.mdexamples/har_classifier/run_matrix.pyexamples/har_classifier/train_c_sym.cexamples/mixed_width_mlp/train_c.csrc/arithmetic/ExecuteOp.csrc/arithmetic/RowBroadcast.csrc/arithmetic/include/ExecuteOp.hsrc/layer/AdaptiveAvgPool1d.csrc/layer/AvgPool1d.csrc/layer/MaxPool1d.csrc/loss_functions/CrossEntropy.csrc/optimizer/AdamW.csrc/optimizer/Optimizer.csrc/optimizer/Sgd.csrc/optimizer/include/Optimizer.hsrc/rng/RNG.csrc/userApi/continual_learning/PpcaReplayApi.csrc/userApi/optimizer/AdamWApi.csrc/userApi/optimizer/OptimizerApi.csrc/userApi/optimizer/SgdApi.csrc/userApi/training_loop/TrainingLoopApi.ctest/unit/arithmetic/UnitTestRowBroadcast.ctest/unit/continual_learning/UnitTestPpcaReplay.ctest/unit/continual_learning/UnitTestPpcaReplaySerialize.ctest/unit/goldgen/sym_gold.pytest/unit/layer/CMakeLists.txttest/unit/layer/UnitTestAdaptiveAvgPool1d.ctest/unit/layer/UnitTestAvgPool1d.ctest/unit/layer/UnitTestMaxPool1d.ctest/unit/layer/UnitTestSoftmax.ctest/unit/layer/generate_expected_adaptive_avg_pool_1d.pytest/unit/layer/generate_expected_avg_pool_1d.pytest/unit/layer/generate_expected_max_pool_1d.pytest/unit/loss_functions/CMakeLists.txttest/unit/loss_functions/UnitTestCrossEntropy.ctest/unit/loss_functions/generate_expected_cross_entropy_sym.pytest/unit/optimizer/UnitTestAdamW.ctest/unit/optimizer/UnitTestSgd.ctest/unit/userAPI/UnitTestTrainingLoopApi.c
| **gradient** range, and this configuration deliberately keeps gradients | ||
| FLOAT32, so parity here is consistent with the paper rather than contradicting | ||
| it. Quantizing the gradient path is the open research axis (#218, Jan's | ||
| #137–#142 ladder), not this run. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Avoid a line starting with # (markdownlint MD018).
The paragraph continuation #137–#142 ladder), not this run. starts with a hash, which markdownlint flags as a malformed ATX heading and which some renderers can misinterpret. Reflow so the line does not begin with # (e.g., move a word up, or write issues #137–#142).
🧰 Tools
🪛 markdownlint-cli2 (0.23.0)
[warning] 179-179: No space after hash on atx style heading
(MD018, no-missing-space-atx)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@examples/har_classifier/README.md` at line 179, Reflow the paragraph
continuation in README.md so the line containing “#137–#142 ladder), not this
run.” no longer starts with a hash; preserve the intended issue references and
sentence meaning.
Source: Linters/SAST tools
Merged
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
1 participant
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.
No description provided.