fix(transformer): vaswani recipe + working schedule + deterministic init - #1270
Merged
Merged
Conversation
…nit — closes 3 bugs Three coupled bugs surfaced during V=256 batched flake investigation on PR #1265 follow-up. All three are required to make Transformer training deterministic AND paper-faithful AND robust on small-budget tasks. BUG 1: Vaswani β₂=0.98 / ε=1e-9 require a paired warmup+inverse-sqrt LR schedule (Vaswani 2017 §5.3). PR #1265 added the β₂/ε then had to revert them because the framework didn't apply the schedule. This commit ships the schedule alongside the hyperparameters so they travel together as the "Vaswani recipe": - New `NoamSchedule` class implementing lr(t) = factor · d_model^(-0.5) · min(t^(-0.5), t · warmup^(-1.5)) - Transformer<T> default optimizer now constructs Adam with β₂=0.98, ε=1e-9, and a NoamSchedule(d_model, warmupSteps) attached. - `TransformerArchitecture.WarmupSteps` (default 4000, paper-faithful) exposed as a constructor parameter so consumers with shorter training budgets can pass a smaller value. BUG 2: `OnBatchEnd` (the public entry point that advances the optimizer's LR scheduler) was never called from any training path. `grep -rn "OnBatchEnd\b" src/` returned only the definition, no callers. Schedulers attached to optimizers via either the new default-recipe path OR a user-supplied `LearningRateScheduler =` config were silently inert: the LR stayed pinned at its initial value forever and `StepScheduler()` was never invoked. Fix: NeuralNetworkBase.TrainWithTape now calls GradientBasedOptimizerBase.OnBatchEnd() at the end of each training step, immediately after `opt.Step(context)` and the network-level extras update. This is the canonical per-batch boundary that the OnBatchEnd contract documents ("called at the end of each training batch"). Optimizers that don't derive from GradientBasedOptimizerBase fall through unchanged. BUG 3: V=256 batched 100-step memorization test showed 12-28% top-1 accuracy spread across runs (init-seed flake). Root cause: `InitializationStrategyBase` sources Xavier draws from `RandomHelper.ThreadSafeRandom` which is non-deterministic across process instances; every `new Transformer(arch, ...)` got different initial weights and small-budget convergence varied accordingly. Fix: `TransformerArchitecture.RandomSeed` (default null, preserves pre-fix behaviour) lets consumers request reproducible weight init. When set, `LayerHelper.CreateDefaultTransformerLayers` wires every weight-bearing layer's `InitializationStrategy` to a shared `EagerInitializationStrategy(new Random(seed))`. Same seed -> same weights every run, so unit tests, multi-seed experiment harnesses, and CI all see deterministic accuracy. `EagerInitializationStrategy` and `InitializationStrategyBase` gained a `Random?` constructor parameter to thread the seeded RNG through (default behaviour unchanged). Test results — all 8 transformer integration tests pass deterministically across 5 consecutive runs: Constructor_DefaultOptimizer_IsAdamNotGradientDescent ✓ Train_SingleSample_V4_MemorisesAfter5000Steps ✓ Train_SingleSample_V16_MemorisesAfter1000Steps ✓ TrainBatched_V256_LearnsBatchAfter100Steps ✓ (was flaky 12-28%) Train_LossDecreasesByAtLeastHalfOnMemorizationTask ✓ ExplicitAdamMatchesDefaultBehavior ✓ SetBaseTrainOptimizer_OverridesCtorDefault_OnTrainCall ✓ Facade_Predict_MatchesDirectModelPredict_AfterBuildAsync ✓ The SetBaseTrainOptimizer test was updated to use explicit non-default optimizers on both sides — without that change the Vaswani+Noam default would converge in 50 steps just like the "high-LR" override and the test couldn't differentiate them. The test's ACTUAL semantic (verify SetBaseTrainOptimizer takes effect) is now isolated from the default-optimizer recipe. The integration-test helper `MakeArch` opts in to deterministic init (`randomSeed: 42`) and a small warmup (`warmupSteps: 10`) because tests run on tiny budgets where the 4000-step paper warmup would never exit; production users still get the paper-faithful 4000 default. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Deployment failed with the following error: Learn More: https://vercel.com/docs/concepts/projects/project-configuration |
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds optional deterministic per-layer initialization driven by TransformerArchitecture.RandomSeed, a Noam learning-rate scheduler and Vaswani-style default Adam optimizer wired into Transformer, advances optimizer schedulers per batch during training, exposes RNG plumbing for initialization strategies, and updates embedding initialization and related tests for determinism. ChangesDeterministic per-layer init + Transformer defaults
Noam scheduler and optimizer integration
Tests
Sequence Diagram(s)sequenceDiagram
participant Trainer as Trainer
participant Transformer as Transformer
participant Optimizer as AdamOptimizer
participant Scheduler as NoamSchedule
Trainer->>Transformer: Train(batch)
Transformer->>Transformer: Forward(batch) / ComputeLoss()
Transformer->>Transformer: Backward() / Accumulate grads
Transformer->>Optimizer: Step(grads)
Optimizer->>Scheduler: Request LR (current step)
Scheduler-->>Optimizer: Computed LR (Noam)
Optimizer->>Optimizer: Update parameters
Trainer->>Optimizer: (end of batch) OnBatchEnd
Optimizer->>Scheduler: Advance step (if StepPerBatch)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly related PRs
BLOCKING NOTES (production-readiness checks)
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/NeuralNetworks/TransformerArchitecture.cs (1)
393-414:⚠️ Potential issue | 🟠 Major | ⚡ Quick winBlocking: validate
warmupStepsas a positive value at construction.
warmupStepsis externally supplied configuration, but there is no guard on Line 393/Line 413.<= 0can produce invalid LR schedule behavior downstream.Suggested fix
public TransformerArchitecture( InputType inputType, NeuralNetworkTaskType taskType, @@ List<ILayer<T>>? layers = null, int warmupSteps = 4000, int? randomSeed = null) : base( inputType: inputType, taskType: taskType, complexity: complexity, inputSize: inputSize, outputSize: outputSize, layers: layers) { + if (warmupSteps <= 0) + throw new ArgumentOutOfRangeException(nameof(warmupSteps), "Warmup steps must be greater than 0."); + NumEncoderLayers = numEncoderLayers; NumDecoderLayers = numDecoderLayers; @@ Temperature = temperature; WarmupSteps = warmupSteps; RandomSeed = randomSeed;As per coding guidelines: “Production Readiness (CRITICAL - Flag as BLOCKING) … missing validation of external inputs”.
🤖 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/NeuralNetworks/TransformerArchitecture.cs` around lines 393 - 414, The constructor of TransformerArchitecture currently assigns the warmupSteps parameter directly to the WarmupSteps property; add validation to ensure warmupSteps > 0 and throw an ArgumentOutOfRangeException (or ArgumentException) if not, before assigning to WarmupSteps so invalid values cannot propagate into the LR schedule; update the constructor parameter handling (the warmupSteps parameter in the TransformerArchitecture constructor) to perform this check and include a clear message referencing the parameter name.tests/AiDotNet.Tests/IntegrationTests/NeuralNetworks/TransformerEndToEndIntegrationTests.cs (1)
413-436:⚠️ Potential issue | 🟠 Major | ⚡ Quick winBlocking: this helper now invalidates
ExplicitAdamMatchesDefaultBehavior().
MakeArch()now forceswarmupSteps: 10, so every default-constructed transformer runs the Noam warmup schedule during the first 200 steps.ExplicitAdamMatchesDefaultBehavior()still builds a flat Adam with no scheduler, so that test is no longer comparing equivalent optimizers and can fail for the wrong reason. Either parameterizeMakeArch()so that test can opt out, or attach the sameNoamSchedule+StepPerBatchto the explicit optimizer there.Suggested direction
-private static TransformerArchitecture<float> MakeArch(int vocab, int ctxLen, int dModel, int dFf, int layers, int heads) +private static TransformerArchitecture<float> MakeArch( + int vocab, + int ctxLen, + int dModel, + int dFf, + int layers, + int heads, + int warmupSteps = 10, + int? randomSeed = 42) => new TransformerArchitecture<float>( inputType: InputType.TwoDimensional, taskType: NeuralNetworkTaskType.SequenceClassification, numEncoderLayers: layers, numDecoderLayers: 0, numHeads: heads, modelDimension: dModel, feedForwardDimension: dFf, inputSize: ctxLen, outputSize: vocab, maxSequenceLength: ctxLen, vocabularySize: vocab, - warmupSteps: 10, - randomSeed: 42); + warmupSteps: warmupSteps, + randomSeed: randomSeed);Then make the parity test either pass the same scheduler into
adamOpts, or choose helper parameters that keep both paths equivalent.As per coding guidelines, "Tests MUST be production-quality" and "Good tests should ... assert specific expected values" against the behavior they actually intend to compare.
🤖 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 `@tests/AiDotNet.Tests/IntegrationTests/NeuralNetworks/TransformerEndToEndIntegrationTests.cs` around lines 413 - 436, The test helper MakeArch currently hardcodes warmupSteps: 10 which causes default transformers to use a Noam warmup and breaks the parity test ExplicitAdamMatchesDefaultBehavior; either (A) change MakeArch to accept a warmupSteps parameter (default 10) and have ExplicitAdamMatchesDefaultBehavior call MakeArch(..., warmupSteps: 0) so both paths use no scheduler, or (B) keep MakeArch as-is but modify ExplicitAdamMatchesDefaultBehavior to attach the same NoamSchedule wrapped with StepPerBatch to its explicit adamOpts so both optimizers see identical scheduling (refer to MakeArch, ExplicitAdamMatchesDefaultBehavior, NoamSchedule, StepPerBatch, and adamOpts when making the change).
🤖 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 `@src/NeuralNetworks/NeuralNetworkBase.cs`:
- Around line 3909-3923: TrainWithCustomLoss currently calls opt.Step(context)
but doesn't advance optimizers' schedulers; mirror the TrainWithTape fix by
detecting if opt is an Optimizers.GradientBasedOptimizerBase<T, Tensor<T>,
Tensor<T>> (e.g., use "if (opt is Optimizers.GradientBasedOptimizerBase<T,
Tensor<T>, Tensor<T>> stepped)") immediately after opt.Step(context) and invoke
stepped.OnBatchEnd(); this ensures schedulers configured on the optimizer are
advanced consistently across both TrainWithTape and TrainWithCustomLoss paths.
In `@src/NeuralNetworks/Transformer.cs`:
- Around line 787-806: The deserialization fallback creates a local _optimizer
that can diverge from the actual base training optimizer used later via
SetBaseTrainOptimizer, causing wrong optimizer/scheduler to be
reported/serialized; fix by making the base-train-optimizer the single source of
truth: when you construct the fallback optimizer after DeserializeInterface,
call SetBaseTrainOptimizer(fallbackOptimizer) instead of assigning only
_optimizer, and/or update SetBaseTrainOptimizer to set _optimizer =
BaseTrainOptimizer (or have GetModelMetadata/SerializeNetworkSpecificData read
BaseTrainOptimizer directly) so the serialized metadata always reflects the
active base optimizer/scheduler.
---
Outside diff comments:
In `@src/NeuralNetworks/TransformerArchitecture.cs`:
- Around line 393-414: The constructor of TransformerArchitecture currently
assigns the warmupSteps parameter directly to the WarmupSteps property; add
validation to ensure warmupSteps > 0 and throw an ArgumentOutOfRangeException
(or ArgumentException) if not, before assigning to WarmupSteps so invalid values
cannot propagate into the LR schedule; update the constructor parameter handling
(the warmupSteps parameter in the TransformerArchitecture constructor) to
perform this check and include a clear message referencing the parameter name.
In
`@tests/AiDotNet.Tests/IntegrationTests/NeuralNetworks/TransformerEndToEndIntegrationTests.cs`:
- Around line 413-436: The test helper MakeArch currently hardcodes warmupSteps:
10 which causes default transformers to use a Noam warmup and breaks the parity
test ExplicitAdamMatchesDefaultBehavior; either (A) change MakeArch to accept a
warmupSteps parameter (default 10) and have ExplicitAdamMatchesDefaultBehavior
call MakeArch(..., warmupSteps: 0) so both paths use no scheduler, or (B) keep
MakeArch as-is but modify ExplicitAdamMatchesDefaultBehavior to attach the same
NoamSchedule wrapped with StepPerBatch to its explicit adamOpts so both
optimizers see identical scheduling (refer to MakeArch,
ExplicitAdamMatchesDefaultBehavior, NoamSchedule, StepPerBatch, and adamOpts
when making the change).
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a6ea2a1f-e829-4f86-a1b0-6a62fa232722
📒 Files selected for processing (8)
src/Helpers/LayerHelper.cssrc/Initialization/EagerInitializationStrategy.cssrc/Initialization/InitializationStrategyBase.cssrc/LearningRateSchedulers/NoamSchedule.cssrc/NeuralNetworks/NeuralNetworkBase.cssrc/NeuralNetworks/Transformer.cssrc/NeuralNetworks/TransformerArchitecture.cstests/AiDotNet.Tests/IntegrationTests/NeuralNetworks/TransformerEndToEndIntegrationTests.cs
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes three coupled issues affecting Transformer training: it aligns the default optimizer with the Vaswani (2017) Adam + Noam LR schedule recipe, ensures LR schedulers actually advance during training, and adds an opt-in deterministic initialization path to eliminate flaky small-budget Transformer tests.
Changes:
- Add
NoamScheduleand wire it intoTransformer<T>’s default Adam optimizer (β₂=0.98, ε=1e-9) usingTransformerArchitecture.WarmupSteps. - Call
GradientBasedOptimizerBase.OnBatchEnd()fromNeuralNetworkBase.TrainWithTapeso per-batch schedulers step. - Add
TransformerArchitecture.RandomSeedand propagate seeded initialization through default Transformer layer construction; update integration tests to use small warmup + deterministic seed.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/AiDotNet.Tests/IntegrationTests/NeuralNetworks/TransformerEndToEndIntegrationTests.cs | Adjusts tests to avoid relying on the default Vaswani+Noam optimizer and sets warmup/seed for deterministic behavior. |
| src/NeuralNetworks/TransformerArchitecture.cs | Adds WarmupSteps and RandomSeed to control default Noam schedule and deterministic init for default layers. |
| src/NeuralNetworks/Transformer.cs | Updates default optimizer to Vaswani Adam hyperparams + Noam schedule; mirrors behavior on deserialization fallback. |
| src/NeuralNetworks/NeuralNetworkBase.cs | Steps optimizer LR schedulers by calling OnBatchEnd() at the end of tape-based training steps. |
| src/LearningRateSchedulers/NoamSchedule.cs | Introduces Noam (warmup + inverse-sqrt) learning-rate scheduler implementation. |
| src/Initialization/InitializationStrategyBase.cs | Adds RNG injection support to initialization strategies to enable deterministic seeding. |
| src/Initialization/EagerInitializationStrategy.cs | Adds constructors for default vs seeded RNG usage. |
| src/Helpers/LayerHelper.cs | Wires seeded initialization strategy into default Transformer layers when RandomSeed is provided. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
addresses 8 unresolved review threads on pr #1270: noamschedule (o_xq + o_yU + o_yf): - map t = step + 1 internally so the library's "step at end of batch" convention (currentstep is 0-based batches-completed) lines up with the vaswani 2017 paper's 1-based t. without the mapping the first two batches both used lr(t=1) and every subsequent batch lagged by one step. matches pytorch / huggingface scheduler convention - override reset() to restore the warmup-start lr (t=1) instead of the base ctor's _baselearningrate which we use as a peak-lr sentinel — before this, reset would skip warmup on resume - add 6 unit tests pinning the behavior: initial lr is warmup-start not peak; first two steps give distinct lrs; warmup boundary hits peak; post-warmup decays as inverse sqrt; reset restores warmup-start; zero/negative warmup throws neuralnetworkbase (o5vH): - mirror the onbatchend advance from trainwithtape into trainwithcustomloss so schedulers configured on the optimizer (noam, linearwarmup, cosineannealing, ...) tick when training goes through the custom-loss path. without this, lr was pinned at its initial value forever for any caller using trainwithcustomloss neuralnetworkbase + transformer (o5vV): - make setbasetrainoptimizer virtual so subclasses with a private optimizer field can override and keep that field in sync - transformer overrides to also assign _optimizer when the base slot is set, so getmodelmetadata + serializenetworkspecificdata always read the live training optimizer instead of the stale ctor instance after aimodelbuilder.configureoptimizer paths layerhelper (o_zA + o_z2): - replace the single shared eagerinitializationstrategy across all layers with per-layer seeded strategies: build a seed-rng from the architecture seed, derive a fresh int per layer, construct that layer's strategy backed by its own private system.random. preserves determinism (same architecture seed → same layer seeds → same weights) and eliminates the data race between concurrent lazy-init paths sharing one non-thread-safe rng transformerarchitecture (o_0g): - validate warmupsteps > 0 in the architecture ctor so failures are attributed to this parameter immediately instead of bubbling up later as an exception from noamschedule during transformer construction Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ooples
added a commit
that referenced
this pull request
May 5, 2026
… rng addresses 8 unresolved review threads on pr #1270: noamschedule (o_xq + o_yU + o_yf): - map t = step + 1 internally so the library's "step at end of batch" convention (currentstep is 0-based batches-completed) lines up with the vaswani 2017 paper's 1-based t. without the mapping the first two batches both used lr(t=1) and every subsequent batch lagged by one step. matches pytorch / huggingface scheduler convention - override reset() to restore the warmup-start lr (t=1) instead of the base ctor's _baselearningrate which we use as a peak-lr sentinel — before this, reset would skip warmup on resume - add 6 unit tests pinning the behavior: initial lr is warmup-start not peak; first two steps give distinct lrs; warmup boundary hits peak; post-warmup decays as inverse sqrt; reset restores warmup-start; zero/negative warmup throws neuralnetworkbase (o5vH): - mirror the onbatchend advance from trainwithtape into trainwithcustomloss so schedulers configured on the optimizer (noam, linearwarmup, cosineannealing, ...) tick when training goes through the custom-loss path. without this, lr was pinned at its initial value forever for any caller using trainwithcustomloss neuralnetworkbase + transformer (o5vV): - make setbasetrainoptimizer virtual so subclasses with a private optimizer field can override and keep that field in sync - transformer overrides to also assign _optimizer when the base slot is set, so getmodelmetadata + serializenetworkspecificdata always read the live training optimizer instead of the stale ctor instance after aimodelbuilder.configureoptimizer paths layerhelper (o_zA + o_z2): - replace the single shared eagerinitializationstrategy across all layers with per-layer seeded strategies: build a seed-rng from the architecture seed, derive a fresh int per layer, construct that layer's strategy backed by its own private system.random. preserves determinism (same architecture seed → same layer seeds → same weights) and eliminates the data race between concurrent lazy-init paths sharing one non-thread-safe rng transformerarchitecture (o_0g): - validate warmupsteps > 0 in the architecture ctor so failures are attributed to this parameter immediately instead of bubbling up later as an exception from noamschedule during transformer construction Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/NeuralNetworks/TransformerArchitecture.cs`:
- Around line 392-394: Add XML <param> documentation entries for the two newly
added constructor parameters on the TransformerArchitecture constructor:
document "warmupSteps" (int warmupSteps) and "randomSeed" (int? randomSeed) in
the existing XML comment block above the public TransformerArchitecture(...)
constructor so they appear in generated API docs and satisfy XML-doc checks;
briefly describe what each parameter controls (e.g., number of warmup steps for
learning rate scheduling and optional RNG seed) and ensure the parameter names
exactly match "warmupSteps" and "randomSeed".
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7ee6dcc7-a20e-459d-bd5b-afee46575af2
📒 Files selected for processing (6)
src/Helpers/LayerHelper.cssrc/LearningRateSchedulers/NoamSchedule.cssrc/NeuralNetworks/NeuralNetworkBase.cssrc/NeuralNetworks/Transformer.cssrc/NeuralNetworks/TransformerArchitecture.cstests/AiDotNet.Tests/UnitTests/LearningRateSchedulers/LearningRateSchedulerTests.cs
…seed closes review-comment #1270.p2zq: the constructor signature includes sequencepooling, warmupsteps, and randomseed but the <param> doc list hadn't been updated when those params were added. each param now has an xml doc explaining its semantic, defaults, and the production guidance the reviewer expected (warmup is paper-canonical 4000, randomseed is null = production-secure rng, sequencepooling defers to a per-tasktype default). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ooples
added a commit
that referenced
this pull request
May 5, 2026
…domseed closes review-comment #1270.p2zq: the constructor signature includes sequencepooling, warmupsteps, and randomseed but the <param> doc list hadn't been updated when those params were added. each param now has an xml doc explaining its semantic, defaults, and the production guidance the reviewer expected (warmup is paper-canonical 4000, randomseed is null = production-secure rng, sequencepooling defers to a per-tasktype default). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/NeuralNetworks/TransformerArchitecture.cs`:
- Around line 351-381: The XML docs include a stale <param name="rbmLayers">
entry that no longer matches the TransformerArchitecture constructor signature;
remove the entire <param name="rbmLayers">...</param> XML block from the
class/constructor XML comment (the comment above the
TransformerArchitecture(...) constructor in TransformerArchitecture.cs) so the
documented parameters match the actual constructor parameters and XML-doc
validation will pass.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: f412450b-28ff-4967-a665-4972798f0518
📒 Files selected for processing (1)
src/NeuralNetworks/TransformerArchitecture.cs
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
ooples
added a commit
that referenced
this pull request
May 5, 2026
addresses 3 unresolved review comments on pr #1269: transformerarchitecture.cs (pqdj): - xml param docs added for sequencepooling, warmupsteps, randomseed. the constructor signature was already including these params but the <param> doc list hadn't been updated when they were introduced. doc copy mirrors the equivalent fix shipped on pr #1270 for the same ctor's vaswani-side doc gap onnxsymbolicaxisruntimetests.cs (pqdo): - replace naive utf-8 substring scan with protobuf length-prefixed-string byte-pattern search. the previous Assert.Contains("H") / Assert.Contains("W") could match any single 'H' or 'W' byte in the protobuf binary by chance (e.g., as part of a protobuf field tag, or random data in a non-shape-related field). new helper assertlengthprefixedstringinbytes searches for the exact `<varint-length-byte> <utf-8-bytes>` sequence that protobuf's length-delimited encoding produces for a dim_param — random binary is essentially impossible to produce that pattern by chance, especially for the 30+ char sentinels the first test now uses (DYNAMIC_BATCH_AXIS_SENTINEL_TEST etc.). the second test (which uses framework defaults "batch"/"H"/"W") gets the same byte-pattern strengthening so even single-letter axis names are matched only when they appear with the protobuf length prefix in front of them. tests still pass layerhelper.cs (pqdz): - replace the single shared eagerinitializationstrategy across all transformer layers with per-layer seeded strategies: build a seed-rng from the architecture seed, derive a fresh int per layer, construct that layer's strategy backed by its own private system.random. preserves determinism (same architecture seed -> same layer seeds -> same weights) and eliminates the data race between concurrent lazy-init paths sharing one non-thread-safe rng. mirrors the same fix on the vaswani branch for the same code path builds cleanly, onnx tests pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
addresses 5 unresolved review comments on pr #1270: transformerarchitecture.cs (vgnt): - delete stale <param name="rbmlayers"> doc — the constructor on lines 404-423 has no rbmlayers parameter; the doc was a leftover from a removed overload and tripped xml-doc validation in stricter builds transformer.cs setbasetrainoptimizer (vhme): - null-clearing path now reconstructs the vaswani 2017 default optimizer (adam β₁=0.9, β₂=0.98, ε=1e-9 + noamschedule on _transformerarchitecture modeldimension/warmupsteps) instead of leaving _optimizer pointing at the stale instance. mirrors the deserialization-fallback pattern at line 794-806 for consistency. without this, getmodelmetadata / serializenetworkspecificdata would report the OLD optimizer after a caller cleared the base slot — the exact staleness this override is meant to prevent layerhelper.cs Wire (vhmn): - only override layer.initializationstrategy when the layer doesn't already have one. previous version unconditionally replaced any existing strategy with the eagerinitializationstrategy's hardcoded xavier-normal, silently overriding per-layer init policies (denselayer with relu uses he init; multiheadattention uses simdrandom-based xavier-uniform). preserves layer-specific tuning. notes the remaining seed-determinism gap for layers with their own strategies as follow-up work embeddinglayer.cs (vhmx): - initializeparameters now derives its simdrandom seed from the installed initializationstrategy's randomgenerator when one is present. previously embeddinglayer always used a process-wide-counter- seeded simdrandom regardless of the architecture's randomseed, breaking the reproducibility contract for embedding weights specifically. falls back to default simdrandom() when no strategy is wired in, preserving existing non-seeded behaviour initializationstrategybase.cs: - new `public Random RandomGenerator` accessor that exposes the protected Random field as a public read-only property so layers driving their own SIMD RNG (embeddinglayer, multiheadattentionlayer, ...) can seed deterministically from the same generator the strategy itself uses, without the framework needing to make every layer go through the strategy's xavier/he init helpers transformer.cs comment (vhm-): - replaced misleading "lr=base · noamschedule(d_model)" with the actual contract: gradientbasedoptimizerbase uses the scheduler's currentlearningrate as the absolute lr; initiallearningrate is just the positive-lr-guard sentinel for the base ctor and is bypassed once a scheduler is present. effective lr per batch = noamschedule(t) directly, not the multiplicative product previously implied builds cleanly net10.0. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ooples
added a commit
that referenced
this pull request
May 5, 2026
addresses 5 unresolved review comments on pr #1270: transformerarchitecture.cs (vgnt): - delete stale <param name="rbmlayers"> doc — the constructor on lines 404-423 has no rbmlayers parameter; the doc was a leftover from a removed overload and tripped xml-doc validation in stricter builds transformer.cs setbasetrainoptimizer (vhme): - null-clearing path now reconstructs the vaswani 2017 default optimizer (adam β₁=0.9, β₂=0.98, ε=1e-9 + noamschedule on _transformerarchitecture modeldimension/warmupsteps) instead of leaving _optimizer pointing at the stale instance. mirrors the deserialization-fallback pattern at line 794-806 for consistency. without this, getmodelmetadata / serializenetworkspecificdata would report the OLD optimizer after a caller cleared the base slot — the exact staleness this override is meant to prevent layerhelper.cs Wire (vhmn): - only override layer.initializationstrategy when the layer doesn't already have one. previous version unconditionally replaced any existing strategy with the eagerinitializationstrategy's hardcoded xavier-normal, silently overriding per-layer init policies (denselayer with relu uses he init; multiheadattention uses simdrandom-based xavier-uniform). preserves layer-specific tuning. notes the remaining seed-determinism gap for layers with their own strategies as follow-up work embeddinglayer.cs (vhmx): - initializeparameters now derives its simdrandom seed from the installed initializationstrategy's randomgenerator when one is present. previously embeddinglayer always used a process-wide-counter- seeded simdrandom regardless of the architecture's randomseed, breaking the reproducibility contract for embedding weights specifically. falls back to default simdrandom() when no strategy is wired in, preserving existing non-seeded behaviour initializationstrategybase.cs: - new `public Random RandomGenerator` accessor that exposes the protected Random field as a public read-only property so layers driving their own SIMD RNG (embeddinglayer, multiheadattentionlayer, ...) can seed deterministically from the same generator the strategy itself uses, without the framework needing to make every layer go through the strategy's xavier/he init helpers transformer.cs comment (vhm-): - replaced misleading "lr=base · noamschedule(d_model)" with the actual contract: gradientbasedoptimizerbase uses the scheduler's currentlearningrate as the absolute lr; initiallearningrate is just the positive-lr-guard sentinel for the base ctor and is bypassed once a scheduler is present. effective lr per batch = noamschedule(t) directly, not the multiplicative product previously implied builds cleanly net10.0. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/NeuralNetworks/TransformerArchitecture.cs (1)
331-350:⚠️ Potential issue | 🟠 Major | ⚡ Quick winConstructor XML docs still contain non-existent parameters (
inputHeight,inputWidth,inputDepth).Line 342–344 documents parameters that are not in the constructor signature at Line 403–422. In strict XML-doc builds this can fail with CS1572.
Suggested fix
- /// <param name="inputHeight">The height of the input for 2D inputs like images. Defaults to 0.</param> - /// <param name="inputWidth">The width of the input for 2D inputs like images. Defaults to 0.</param> - /// <param name="inputDepth">The depth of the input for multi-channel inputs. Defaults to 1.</param>🤖 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/NeuralNetworks/TransformerArchitecture.cs` around lines 331 - 350, The XML summary for the TransformerArchitecture<T> constructor documents parameters inputHeight, inputWidth and inputDepth that are not present in the actual constructor signature; update the docs to match the signature in the TransformerArchitecture<T> constructor by either removing the stale <param> entries for inputHeight/inputWidth/inputDepth or adding corresponding constructor parameters (with appropriate default values and behavior) to the TransformerArchitecture<T> constructor so the XML <param> tags and the method signature are consistent and the CS1572 warning is resolved.
🤖 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 `@src/Initialization/InitializationStrategyBase.cs`:
- Around line 33-43: The public RandomGenerator property is exposing mutable RNG
state; change its visibility from public to internal (i.e., make "public Random
RandomGenerator => Random;" into "internal Random RandomGenerator => Random;")
so only assembly-level callers like EmbeddingLayer<T> can access the live
Random; keep the existing getter semantics and XML summary but update
accessibility and any related unit tests or callers to use the internal
accessor.
In `@src/NeuralNetworks/Layers/EmbeddingLayer.cs`:
- Around line 410-429: The projection-weight initialization still uses
RandomHelper.CreateSecureRandom(), bypassing the seeded path you added for
_embeddingTensor; in both the CPU and GPU projection-init blocks (the code that
currently calls RandomHelper.CreateSecureRandom()) replace those calls to
instead use the same seeded SimdRandom instance logic you set up for rng: when
InitializationStrategy is an Initialization.InitializationStrategyBase<T> derive
a seed via baseStrategy.RandomGenerator.Next() and construct a SimdRandom from
it (or reuse the local rng variable), otherwise fall back to new SimdRandom();
apply this change to both CPU and GPU projection-init code paths so continuous
projection weights honor InitializationStrategy and remain reproducible.
In `@src/NeuralNetworks/Transformer.cs`:
- Around line 285-295: Extract the duplicated Vaswani-2017 Adam+Noam
construction into a private helper called e.g. CreateDefaultVaswaniOptimizer
that returns AdamOptimizer<T, Tensor<T>, Tensor<T>>; replace the inline
constructions in the Transformer constructor, SetBaseTrainOptimizer null-path,
and the deserialization fallback with calls to CreateDefaultVaswaniOptimizer;
ensure the helper uses the same architecture fields (architecture or
_transformerArchitecture) for modelDimension and warmupSteps and preserves
InitialLearningRate=1e-3, Beta2=0.98, Epsilon=1e-9 and
SchedulerStepMode.StepPerBatch so all three sites reference the single
implementation.
- Around line 192-212: You create a default AdamOptimizer into _optimizer when
optimizer==null but do not populate the base-optimizer slot that
GetOrCreateBaseOptimizer() reads, causing divergence; after constructing
_optimizer in the null path, also assign that same instance into the base
optimizer storage used by GetOrCreateBaseOptimizer() (e.g., set the backing
field the method reads such as _baseOptimizer or call the class's
SetBaseOptimizer/AssignBaseOptimizer method) so Train(), GetModelMetadata and
SerializeNetworkSpecificData all reference the same optimizer instance.
---
Outside diff comments:
In `@src/NeuralNetworks/TransformerArchitecture.cs`:
- Around line 331-350: The XML summary for the TransformerArchitecture<T>
constructor documents parameters inputHeight, inputWidth and inputDepth that are
not present in the actual constructor signature; update the docs to match the
signature in the TransformerArchitecture<T> constructor by either removing the
stale <param> entries for inputHeight/inputWidth/inputDepth or adding
corresponding constructor parameters (with appropriate default values and
behavior) to the TransformerArchitecture<T> constructor so the XML <param> tags
and the method signature are consistent and the CS1572 warning is resolved.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 04845a44-1ef8-4d45-9538-e3f22885728b
📒 Files selected for processing (5)
src/Helpers/LayerHelper.cssrc/Initialization/InitializationStrategyBase.cssrc/NeuralNetworks/Layers/EmbeddingLayer.cssrc/NeuralNetworks/Transformer.cssrc/NeuralNetworks/TransformerArchitecture.cs
ooples
added a commit
that referenced
this pull request
May 5, 2026
addresses 3 unresolved review comments on pr #1269 + an underlying onnx exporter limitation surfaced by the test rewrite: transformerarchitecture.cs (vur5/vzgk): - validate warmupsteps > 0 in the ctor with argumentoutofrangeexception. previously a 0/negative value bubbled up later as an exception from the noamschedule ctor during transformer construction or training, obscuring the root cause. mirrors the equivalent fix on pr #1270's branch for the same parameter onnxsymbolicaxisruntimetests.cs (vzge): - assertlengthprefixedstringinbytes helper now also checks the protobuf field-tag byte (0x12 = field 2, wire type LEN) preceding the length+name. previously only matched <length><name>, so a stray length+name byte sequence elsewhere in the protobuf could falsely match. closes review-comment #1269.vzge onnxsymbolicaxisruntimetests.cs (vzgt): - tinyvisionnet stub now uses dense + activation (relu) instead of conv + maxpool. the onnxexporter only handles dense / linear / fullyconnected / activations / dropout / flatten — using conv/pool meant the exporter was silently skipping those layers, making the test brittle on (a) the silent-skip behaviour being load-bearing and (b) conv/pool's 4d shape contract not matching the rank-3 dynamic- spatial architecture the test sets up onnxexporter.cs (underlying): - exportdenselayer reflection now falls back from "weights" / "bias" property accessors to getweights() / getbiases() method accessors, the layerbase<t> standard surface. without this fallback the reflection probe couldn't reach denselayer's weights at all - converttofloatmatrix and converttofloatarray gain tensor<t> support via reflection (shape-walk for rank-2 weights, length+indexer for rank-1 biases). cross-tfm safe via tensor<t>.shape's int[] / TensorShape duck-typed extraction - both fallbacks unblock any test or production path that puts a layerbase<t>-derived dense layer through the exporter — what the reviewer's vzgt comment was indirectly flagging builds cleanly, both onnx tests pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
3 tasks done
ooples
added a commit
that referenced
this pull request
May 5, 2026
… override addresses 2 unresolved review comments on pr #1269: onnxsymbolicaxisruntimetests.cs (xzd-): - tinyvisionnet stub now uses two shape-preserving relu activation layers and warms up at the SAME [1, 3, 32, 32] shape that's passed to onnxexporter.exporttobytes. previous version warmed dense at [1, 16] but exported with [1, 3, 32, 32] — produced an onnx graph where matmul's input rank/inner-dim didn't align with the declared input shape, so the exported model wasn't runnable even though the symbolic-axis byte assertions passed. activation-only layers are rank-agnostic so the same 4d tensor flows from warmup through both layers and into export without any matmul/conv weight-shape concerns noamschedule.cs (xzeb): - override Reset() to restore the warmup-start lr (t=1) instead of the peak _baseLearningRate. _baseLearningRate is set to the peak lr via ComputePeakLr to satisfy the base ctor's positive-lr guard, so the default base.Reset() would set _currentLearningRate to the peak — skipping warmup and jumping straight to the peak lr on any subsequent training run. mirrors the equivalent fix on pr #1270's branch for the same scheduler builds cleanly net10.0, both onnx tests pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ooples
added a commit
that referenced
this pull request
May 5, 2026
Mirror the PR #1270 fix here so PR #1269 stays in lock-step: MultiHeadAttentionLayer.InitializeParameters now reads LayerBase<T>.RandomSeed and seeds its SimdRandom from it. Without this hook, TransformerArchitecture.RandomSeed flowed through LayerHelper.Wire to MHA's RandomSeed property but MHA's init was still pulling unseeded SimdRandom values, so attention weights stayed non-deterministic across runs even with a fixed architecture seed. Industry-standard layer-level seed pattern (matches PyTorch's nn.Module + torch.manual_seed cascade): each layer reads its own RandomSeed and constructs its own RNG, so concurrent lazy-init paths get independent RNG instances and never share mutable state.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 15 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- NoamSchedule.ComputeLearningRate XML doc: corrected the negative-step description from "clamped to 0" to "clamped to t = 1 (the warmup-start value)", matching the actual `step < 0 ? 1 : step + 1` clamp. Closes #1270.zKib. - NeuralNetworkBase: extracted StepSchedulerIfSupported(opt) helper. All three training entry points (legacy Train fallback, TrainWithTape, TrainWithCustomLoss) now route through this single OnBatchEnd contract point so scheduler-step semantics can't drift across paths. Closes #1270.zKjB. - EmbeddingLayer: moved InitializeProjectionWeights below InitializeParameters so the embedding-tensor init XML doc attaches to the right method. Previously the helper was inserted between the doc block and InitializeParameters, leaving the doc orphaned onto the wrong member with two consecutive <summary> tags. Closes #1270.zKji.
ooples
added a commit
that referenced
this pull request
May 5, 2026
- NoamSchedule.ComputeLearningRate XML doc: corrected the negative-step description from "clamped to 0" to "clamped to t = 1 (the warmup-start value)", matching the actual `step < 0 ? 1 : step + 1` clamp. Closes #1270.zKib. - NeuralNetworkBase: extracted StepSchedulerIfSupported(opt) helper. All three training entry points (legacy Train fallback, TrainWithTape, TrainWithCustomLoss) now route through this single OnBatchEnd contract point so scheduler-step semantics can't drift across paths. Closes #1270.zKjB. - EmbeddingLayer: moved InitializeProjectionWeights below InitializeParameters so the embedding-tensor init XML doc attaches to the right method. Previously the helper was inserted between the doc block and InitializeParameters, leaving the doc orphaned onto the wrong member with two consecutive <summary> tags. Closes #1270.zKji.
…e-and-flake # Conflicts: # src/Helpers/LayerHelper.cs # src/LearningRateSchedulers/NoamSchedule.cs # src/NeuralNetworks/Layers/EmbeddingLayer.cs # src/NeuralNetworks/NeuralNetworkBase.cs # src/NeuralNetworks/Transformer.cs # src/NeuralNetworks/TransformerArchitecture.cs
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
ooples
enabled auto-merge (squash)
May 6, 2026 00:33
ooples
disabled auto-merge
May 6, 2026 00:33
ooples
enabled auto-merge
May 6, 2026 00:34
ooples
disabled auto-merge
May 6, 2026 00:34
ooples
force-pushed
the
fix/vaswani-schedule-and-flake
branch
from
May 6, 2026 00:37
324bf6c to
ede8ff2
Compare
- NoamSchedule: pre-compute the step-invariant factors factor·d_model^(-0.5) and factor·d_model^(-0.5)·warmup^(-1.5) in the ctor; ComputeLearningRate's per-step cost drops from three Math.Pow calls to one Math.Sqrt + a couple of multiplies. Math.Pow with non-integer exponents is materially slower than Math.Sqrt on every modern .NET runtime (Pow goes through exp(y·ln(x)); Sqrt has a dedicated SSE/AVX intrinsic). The schedule fires once per batch so this saves real time on long training runs. Closes #1270.zzwx. - LearningRateSchedulerTests: converted the 6 NoamSchedule tests from `async Task` + `await Task.CompletedTask` (which did no real async work and tripped analyzer noise) to plain synchronous `void` test methods. Dropped the `Timeout = 60000` attributes too — xunit's [Fact(Timeout=...)] only works on async, and these sync tests complete in milliseconds. Closes #1270.zzxP / #1270.zzxi.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+397
to
+403
| [Fact] | ||
| public void NoamSchedule_InitialLR_IsWarmupStart_NotPeak() | ||
| { | ||
| // d_model=512, warmup=4000, factor=1 — paper-canonical Vaswani recipe. | ||
| var scheduler = new NoamSchedule(modelDimension: 512, warmupSteps: 4000); | ||
|
|
||
| // Peak LR (at t=warmup) — what `_baseLearningRate` is set to. |
Comment on lines
1727
to
+1733
| Random? seedRng = architecture.RandomSeed.HasValue | ||
| ? RandomHelper.CreateSeededRandom(architecture.RandomSeed.Value) | ||
| : null; | ||
|
|
||
| // Apply a freshly-seeded init strategy to a layer if reproducibility | ||
| // was requested. No-op when randomSeed wasn't set (preserves | ||
| // backward-compatible behaviour for users who don't request | ||
| // reproducibility). | ||
| // Apply the per-layer seed when reproducibility was requested. | ||
| // No-op when randomSeed wasn't set (preserves backward-compatible | ||
| // behaviour for users who don't request reproducibility). |
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 coupled bugs surfaced during the V=256 batched flake investigation on the PR #1265 follow-up. All three are required for Transformer training to be deterministic AND paper-faithful AND robust on small-budget tasks.
Bug 1 — Vaswani β₂/ε without paired LR schedule
PR #1265 added Vaswani 2017 hyperparameters (β₂=0.98, ε=1e-9) then had to revert them because the framework wasn't applying the paper's warmup+inverse-sqrt schedule. This PR ships the schedule alongside the hyperparameters so they travel together as the "Vaswani recipe":
NoamScheduleclass:lr(t) = factor · d_model^(-0.5) · min(t^(-0.5), t · warmup^(-1.5))Transformer<T>default optimizer constructs Adam with β₂=0.98, ε=1e-9, andNoamSchedule(d_model, warmupSteps)attached.TransformerArchitecture.WarmupSteps(default 4000) configurable for shorter training budgets.Bug 2 —
OnBatchEndnever calledOnBatchEndis the entry point that advances the optimizer's LR scheduler.grep -rn "OnBatchEnd\b" src/returned only the definition — no callers. Schedulers attached to optimizers were silently inert.Fix:
NeuralNetworkBase.TrainWithTapenow callsGradientBasedOptimizerBase.OnBatchEnd()at the end of each training step.Bug 3 — V=256 batched flake (12-28% accuracy spread)
InitializationStrategyBasesources Xavier draws fromRandomHelper.ThreadSafeRandomwhich is non-deterministic across process instances; everynew Transformer(arch, ...)got different initial weights.Fix:
TransformerArchitecture.RandomSeed(default null) lets consumers request reproducible weight init.LayerHelper.CreateDefaultTransformerLayerswires every weight-bearing layer'sInitializationStrategyto a sharedEagerInitializationStrategy(new Random(seed)).Test plan
All 8 transformer integration tests pass deterministically across 5 consecutive runs:
Constructor_DefaultOptimizer_IsAdamNotGradientDescentTrain_SingleSample_V4_MemorisesAfter5000StepsTrain_SingleSample_V16_MemorisesAfter1000StepsTrainBatched_V256_LearnsBatchAfter100Steps(was flaky 12-28% across runs)Train_LossDecreasesByAtLeastHalfOnMemorizationTaskExplicitAdamMatchesDefaultBehaviorSetBaseTrainOptimizer_OverridesCtorDefault_OnTrainCallFacade_Predict_MatchesDirectModelPredict_AfterBuildAsync🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Chores
Tests