fix: scoped ParameterBuffer view replacement — restore originals after training (#1084) - #1102
Merged
Merged
Conversation
…r training (#1084) GetOrCreateParameterBuffer now saves original tensor references before replacing with buffer-backed views. TrainWithTape wraps the training step in try/finally and calls RestoreOriginalParameters which: 1. Copies updated data from views back to original tensors 2. Restores original tensor references on each layer 3. Clears the buffer so it's rebuilt on the next training step This ensures Clone, serialization, and shape assumptions see the real tensors outside of the training step scope. Matches PyTorch's approach where parameter buffers are contiguous storage but nn.Parameter objects maintain their identity. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
Contributor
WalkthroughParameter-buffer handling is scoped: original trainable tensor references are saved, buffer-backed view tensors are used during a tape-based training step, and originals are restored in a try/finally. Gradient computation now computes all gradients then filters for trainable params. DenseLayer training path uses a single FusedLinear call then applies activation. IdentityActivation gains a Tensor-specific no-op override. Several integration tests validate reference restoration, value updates, and activation behavior. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client as Client
participant Network as NeuralNetwork
participant Buffer as ParameterBufferManager
participant Tape as GradientTape
participant Optimizer as Optimizer
Client->>Network: Train(input, target)
Network->>Buffer: GetOrCreateParameterBuffer(layers)
Buffer-->>Network: viewTensors (saves originals internally)
Network->>Tape: BeginTapeScope()
Tape-->>Network: tape-active
Network->>Tape: Forward(input) [uses viewTensors]
Tape-->>Network: outputs
Network->>Tape: ComputeLoss(outputs, target)
Tape-->>Network: loss
Network->>Tape: ComputeGradients(loss) -> allGrads
Tape-->>Network: all gradients
Network->>Network: filter allGrads -> grads for trainableParams
Network->>Optimizer: Step(grads, viewTensors)
Optimizer-->>Network: updated view tensors
Network->>Buffer: RestoreOriginalParameters() (copy back & restore refs)
Network-->>Client: return loss/result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Production-readiness / Blocking concerns
Poem
🚥 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 docstrings
🧪 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: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/NeuralNetworks/NeuralNetworkBase.cs`:
- Around line 2750-2775: RestoreOriginalParameters clears the buffer
(_parameterBuffer = null) but doesn't change _layerStructureVersion, so
subsequent CollectParameters(Layers, _layerStructureVersion) may return a cached
list of stale view tensors; to fix, invalidate the parameter cache by updating
_layerStructureVersion after dropping the buffer (e.g., increment or assign a
new unique value) so CollectParameters cannot reuse old cached views; update
RestoreOriginalParameters (after setting _parameterBuffer = null and
_savedOriginalParameters = null) to modify _layerStructureVersion accordingly.
- Around line 2503-2547: The buffer/view swap can throw before the current try
block starts, leaving layer fields pointing at buffer views; ensure
RestoreOriginalParameters() always runs by expanding the try/finally to begin
before any buffer setup (e.g., before GetOrCreateParameterBuffer(...) /
re-collect / LossFunctionBase<T> cast) or alternatively wrap the buffer
creation/recollection and cast in their own try/finally that calls
RestoreOriginalParameters() on exception; update the code around
GetOrCreateParameterBuffer, any re-collection logic, and the LossFunctionBase<T>
cast so RestoreOriginalParameters() is invoked on all failure paths.
In
`@tests/AiDotNet.Tests/IntegrationTests/NeuralNetworks/ParameterBufferScopeTests.cs`:
- Around line 45-61: The test currently only checks a prefix of parameters via
the idx guard; fix by materializing the post-train parameter list from
network.Layers (call GetTrainableParameters() on each ITrainableLayer<double>
and collect into a List), then Assert.Equal(originalParams.Count,
newParams.Count) to ensure no parameters were added/dropped, and finally loop
index-wise and use Assert.Same(originalParams[i], newParams[i]) (or Assert.Same
for each element) to validate identity for every parameter instead of the
partial ReferenceEquals check.
- Around line 64-65: Remove the Skip attribute from the
Train_UpdatesParameterValues [Fact] and convert the test into a deterministic,
production-quality test by supplying fixed, small input and target tensors,
running the training step in
ParameterBufferScopeTests.Train_UpdatesParameterValues, and asserting that
parameter tensors (the originals referenced by the test) are updated after Train
is called; ensure the test constructs explicit initial parameter values, copies
them into the model/buffer, performs a single known training/update iteration,
copies the updated values back (verifying the restore/copy-back path), and uses
precise assertions on the expected changed elements rather than relying on
non-deterministic data so it can run reliably in CI.
🪄 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: 8c83da2e-a532-4b5d-947c-45afde2a6bfe
📒 Files selected for processing (2)
src/NeuralNetworks/NeuralNetworkBase.cstests/AiDotNet.Tests/IntegrationTests/NeuralNetworks/ParameterBufferScopeTests.cs
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes a critical training-time bug where NeuralNetworkBase.GetOrCreateParameterBuffer() permanently replaced layer parameter tensor fields with buffer-backed view tensors, breaking post-training Clone/serialization and some shape assumptions. The fix scopes the replacement to a single training step by saving original tensor references before replacement and restoring them in a finally block after the optimizer step.
Changes:
- Wrap
TrainWithTapeforward/backward/optimizer step intry/finallyand restore original tensor references after each training step. - Save original per-layer trainable parameter tensor references prior to applying buffer-backed views; copy updated values back to originals on restore.
- Add an integration test verifying training does not permanently replace layer tensor references.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
src/NeuralNetworks/NeuralNetworkBase.cs |
Implements scoped ParameterBuffer view replacement and restores original layer parameter tensors after each tape-based training step. |
tests/AiDotNet.Tests/IntegrationTests/NeuralNetworks/ParameterBufferScopeTests.cs |
Adds an integration test to ensure training does not permanently replace layer tensor references. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…test Changed AsSpan/AsWritableSpan copy to GetFlat/SetFlat for correct view-to- original data transfer. Added diagnostic test that confirms the zero-gradient issue is pre-existing (parameters don't change even WITHOUT the restore), not caused by the scoped replacement fix. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
tests/AiDotNet.Tests/IntegrationTests/NeuralNetworks/ParameterBufferScopeTests.cs (2)
64-65:⚠️ Potential issue | 🔴 CriticalBLOCKING: Remove or enable the skipped test before merging.
Shipping a
[Fact(Skip = "...")]placeholder is a production-quality violation. If the zero-gradient issue is a separate bug, create a tracking issue and either:
- Remove this test entirely, or
- Make it deterministic with controlled inputs and enable it
Per coding guidelines: "Placeholder tests: Tests with
// TODO: add assertionsor empty test bodies" are blocking issues.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/AiDotNet.Tests/IntegrationTests/NeuralNetworks/ParameterBufferScopeTests.cs` around lines 64 - 65, The test method Train_UpdatesParameterValues is marked with [Fact(Skip = "...")], which is blocking; either remove the skipped test or make it deterministic and enable it: if you want to enable, remove the Skip attribute from Train_UpdatesParameterValues, replace any non-deterministic inputs with controlled fixtures/mocked data so gradients are reproducible and add assertions validating parameter changes; if you prefer to remove it, delete the Train_UpdatesParameterValues test and create a tracking issue referencing this behavior (zero-gradient during tape training) and link that issue in test removal comment so the regression is tracked.
45-61:⚠️ Potential issue | 🟠 MajorTest assertion is incomplete—parameter count changes would go undetected.
The
if (idx < originalParams.Count)guard (line 53) silently skips verification when parameters are added or removed during restore. IfRestoreOriginalParameterscorrupts the parameter list, this test passes for the matching prefix and ignores the rest.Materialize the post-train parameter list, assert counts match, then compare every element:
Suggested fix
- // Tensor references should be the same objects (not buffer views) - int idx = 0; - foreach (var layer in network.Layers) - { - if (layer is ITrainableLayer<double> trainable) - { - foreach (var p in trainable.GetTrainableParameters()) - { - if (idx < originalParams.Count) - { - Assert.True(ReferenceEquals(p, originalParams[idx]), - $"Parameter {idx} was permanently replaced with a buffer view"); - } - idx++; - } - } - } + // Tensor references should be the same objects (not buffer views) + var currentParams = new List<Tensor<double>>(); + foreach (var layer in network.Layers) + { + if (layer is ITrainableLayer<double> trainable) + currentParams.AddRange(trainable.GetTrainableParameters()); + } + + Assert.Equal(originalParams.Count, currentParams.Count); + for (int i = 0; i < currentParams.Count; i++) + { + Assert.Same(originalParams[i], currentParams[i]); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/AiDotNet.Tests/IntegrationTests/NeuralNetworks/ParameterBufferScopeTests.cs` around lines 45 - 61, The test currently only compares a prefix of parameters via the idx < originalParams.Count guard, letting added/removed parameters go undetected; update the test to first materialize the post-restore parameter list (e.g., collect all p from network.Layers where layer is ITrainableLayer<double> and call GetTrainableParameters()), assert that the resulting list.Count equals originalParams.Count, then iterate by index and use ReferenceEquals to compare each element (so each parameter is checked and any count mismatch fails the test). Ensure you reference the existing symbols network.Layers, ITrainableLayer<double>.GetTrainableParameters(), and originalParams when implementing these assertions.src/NeuralNetworks/NeuralNetworkBase.cs (2)
2750-2778:⚠️ Potential issue | 🔴 CriticalCache not invalidated after dropping the buffer—stale view references will be reused.
RestoreOriginalParameters()sets_parameterBuffer = null(line 2777) but leaves_layerStructureVersionunchanged. On the next training step,CollectParameters(Layers, _layerStructureVersion)can return cached view tensors from the previous step instead of the freshly restored originals, routing gradients and optimizer updates to stale objects.Invalidate the cache after clearing the buffer:
Suggested fix
_savedOriginalParameters = null; _parameterBuffer = null; + Training.TapeTrainingStep<T>.InvalidateCache();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/NeuralNetworks/NeuralNetworkBase.cs` around lines 2750 - 2778, RestoreOriginalParameters currently clears _parameterBuffer but does not update the cache version, so subsequent CollectParameters(Layers, _layerStructureVersion) may return stale view tensors; update/invalidate the layer-structure cache after restoring originals by bumping or resetting _layerStructureVersion (or calling whatever cache-invalidator CollectParameters expects) inside RestoreOriginalParameters after setting _parameterBuffer = null so future CollectParameters calls will rebuild views from the restored originals (refer to RestoreOriginalParameters, _parameterBuffer, _layerStructureVersion, CollectParameters and Layers).
2494-2547:⚠️ Potential issue | 🔴 CriticalBuffer view replacement still occurs outside the try/finally scope.
The buffer/view swap happens at
GetOrCreateParameterBuffer(initialParams)(line 2495) before the try block starts (line 2503). If theLossFunctionBase<T>cast throws (line 2500-2501), layer fields remain pointed at buffer views andRestoreOriginalParameters()never runs—leaving the model in the broken post-training state this PR aims to fix.Move the buffer setup and cast inside the try block:
Suggested fix
- var initialParams = Training.TapeTrainingStep<T>.CollectParameters(Layers, _parameterBuffer is null ? -1 : _layerStructureVersion); - var paramBuffer = GetOrCreateParameterBuffer(initialParams); - - // Re-collect after buffer initialization — parameter tensor references may have changed - var trainableParams = Training.TapeTrainingStep<T>.CollectParameters(Layers, _layerStructureVersion); - - var loss = LossFunction as LossFunctions.LossFunctionBase<T> - ?? throw new InvalidOperationException("LossFunction must derive from LossFunctionBase<T> for tape-based training."); - try { + var initialParams = Training.TapeTrainingStep<T>.CollectParameters(Layers, _parameterBuffer is null ? -1 : _layerStructureVersion); + var paramBuffer = GetOrCreateParameterBuffer(initialParams); + + // Re-collect after buffer initialization — parameter tensor references may have changed + var trainableParams = Training.TapeTrainingStep<T>.CollectParameters(Layers, _layerStructureVersion); + + var loss = LossFunction as LossFunctions.LossFunctionBase<T> + ?? throw new InvalidOperationException("LossFunction must derive from LossFunctionBase<T> for tape-based training."); + // Forward + loss under tape — uses the buffer-backed view tensors🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/NeuralNetworks/NeuralNetworkBase.cs` around lines 2494 - 2547, The buffer/view swap via GetOrCreateParameterBuffer(initialParams) and the LossFunction cast to LossFunctions.LossFunctionBase<T> must be moved inside the try so RestoreOriginalParameters() always runs on exceptions; specifically, compute initialParams using Training.TapeTrainingStep<T>.CollectParameters(Layers, ...), but defer calling GetOrCreateParameterBuffer(initialParams) and assigning paramBuffer, and defer the cast of LossFunction to LossFunctions.LossFunctionBase<T>, until immediately inside the try block (before creating the GradientTape and using trainableParams), ensuring RestoreOriginalParameters() in the finally will always revert any buffer/view replacements if an exception occurs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@tests/AiDotNet.Tests/IntegrationTests/NeuralNetworks/ParameterBufferScopeTests.cs`:
- Around line 120-146: The debug snapshot and exception block (variables
tensorIdx, debugBefore, debugAfter, beforeSnapshot, anyChanged and the loop over
network.Layers / ITrainableLayer<double>.GetTrainableParameters()) is dead code
because the test is skipped—either remove this entire debug-output block and the
conditional throw, or re-enable the test if you want these diagnostics to run;
if you need to keep it, wrap the debug construction and exception in an explicit
conditional (e.g., a local boolean like enableDebugOutput) and document why it
remains so it isn’t left as unreachable code.
---
Duplicate comments:
In `@src/NeuralNetworks/NeuralNetworkBase.cs`:
- Around line 2750-2778: RestoreOriginalParameters currently clears
_parameterBuffer but does not update the cache version, so subsequent
CollectParameters(Layers, _layerStructureVersion) may return stale view tensors;
update/invalidate the layer-structure cache after restoring originals by bumping
or resetting _layerStructureVersion (or calling whatever cache-invalidator
CollectParameters expects) inside RestoreOriginalParameters after setting
_parameterBuffer = null so future CollectParameters calls will rebuild views
from the restored originals (refer to RestoreOriginalParameters,
_parameterBuffer, _layerStructureVersion, CollectParameters and Layers).
- Around line 2494-2547: The buffer/view swap via
GetOrCreateParameterBuffer(initialParams) and the LossFunction cast to
LossFunctions.LossFunctionBase<T> must be moved inside the try so
RestoreOriginalParameters() always runs on exceptions; specifically, compute
initialParams using Training.TapeTrainingStep<T>.CollectParameters(Layers, ...),
but defer calling GetOrCreateParameterBuffer(initialParams) and assigning
paramBuffer, and defer the cast of LossFunction to
LossFunctions.LossFunctionBase<T>, until immediately inside the try block
(before creating the GradientTape and using trainableParams), ensuring
RestoreOriginalParameters() in the finally will always revert any buffer/view
replacements if an exception occurs.
In
`@tests/AiDotNet.Tests/IntegrationTests/NeuralNetworks/ParameterBufferScopeTests.cs`:
- Around line 64-65: The test method Train_UpdatesParameterValues is marked with
[Fact(Skip = "...")], which is blocking; either remove the skipped test or make
it deterministic and enable it: if you want to enable, remove the Skip attribute
from Train_UpdatesParameterValues, replace any non-deterministic inputs with
controlled fixtures/mocked data so gradients are reproducible and add assertions
validating parameter changes; if you prefer to remove it, delete the
Train_UpdatesParameterValues test and create a tracking issue referencing this
behavior (zero-gradient during tape training) and link that issue in test
removal comment so the regression is tracked.
- Around line 45-61: The test currently only compares a prefix of parameters via
the idx < originalParams.Count guard, letting added/removed parameters go
undetected; update the test to first materialize the post-restore parameter list
(e.g., collect all p from network.Layers where layer is ITrainableLayer<double>
and call GetTrainableParameters()), assert that the resulting list.Count equals
originalParams.Count, then iterate by index and use ReferenceEquals to compare
each element (so each parameter is checked and any count mismatch fails the
test). Ensure you reference the existing symbols network.Layers,
ITrainableLayer<double>.GetTrainableParameters(), and originalParams when
implementing these assertions.
🪄 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: 12ab34fc-3388-4ce9-9bc7-729498d24280
📒 Files selected for processing (2)
src/NeuralNetworks/NeuralNetworkBase.cstests/AiDotNet.Tests/IntegrationTests/NeuralNetworks/ParameterBufferScopeTests.cs
Root cause found: DenseLayer.Forward called Engine.FusedLinear twice during training — once with activation (ReLU), once without (to capture pre-activation for _lastOutput). The second call's RemoveLastNTapeEntries corrupted tape entries from the first call, resulting in 0 recorded operations and zero gradients. Fix: during training, use single FusedLinear(None) + separate ApplyActivation. This records one clean tape entry per DenseLayer forward. Tape now records 5 entries (from 0). Fused activation path kept for inference only. Gradient flow from loss to parameters still returns 0 — investigating the backward graph reachability analysis in ComputeGradients next. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
tests/AiDotNet.Tests/IntegrationTests/NeuralNetworks/ParameterBufferScopeTests.cs (1)
46-62:⚠️ Potential issue | 🔴 CriticalAssert the full post-train parameter list, not just a prefix.
The
idx < originalParams.Countguard makes this pass when parameters are added or dropped, as long as the common prefix keeps the same identities. Materialize the current parameter list, assert the counts match, then compare every element withAssert.Same.As per coding guidelines, "Tests MUST be production-quality. Flag ALL of the following as blocking issues: ... Missing assertions".Suggested change
- int idx = 0; - foreach (var layer in network.Layers) - { - if (layer is ITrainableLayer<double> trainable) - { - foreach (var p in trainable.GetTrainableParameters()) - { - if (idx < originalParams.Count) - { - Assert.True(ReferenceEquals(p, originalParams[idx]), - $"Parameter {idx} was permanently replaced with a buffer view"); - } - idx++; - } - } - } + var currentParams = new List<Tensor<double>>(); + foreach (var layer in network.Layers) + { + if (layer is ITrainableLayer<double> trainable) + currentParams.AddRange(trainable.GetTrainableParameters()); + } + + Assert.Equal(originalParams.Count, currentParams.Count); + for (int i = 0; i < currentParams.Count; i++) + Assert.Same(originalParams[i], currentParams[i]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/AiDotNet.Tests/IntegrationTests/NeuralNetworks/ParameterBufferScopeTests.cs` around lines 46 - 62, The test currently only checks a common prefix due to the "idx < originalParams.Count" guard; instead, materialize the full current parameter list from network.Layers by iterating trainable layers and calling ITrainableLayer<double>.GetTrainableParameters() into a list, assert the counts match originalParams with Assert.Equal(originalParams.Count, currentParams.Count), then iterate by index and use Assert.Same(originalParams[i], currentParams[i]) for every element (remove the prefix guard and the idx-based null-check logic).src/NeuralNetworks/NeuralNetworkBase.cs (2)
2806-2807:⚠️ Potential issue | 🔴 CriticalInvalidate cached parameter lists when clearing the buffer.
RestoreOriginalParameters()drops_parameterBuffer, but it does not bump_layerStructureVersionor clearTapeTrainingStep<T>'s cache. The nextCollectParameters(Layers, _layerStructureVersion)can therefore reuse the previous step's cached view list instead of the freshly created views.Suggested change
_savedOriginalParameters = null; _parameterBuffer = null; + Training.TapeTrainingStep<T>.InvalidateCache();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/NeuralNetworks/NeuralNetworkBase.cs` around lines 2806 - 2807, RestoreOriginalParameters() currently sets _savedOriginalParameters and _parameterBuffer to null but doesn't invalidate cached parameter views; update it to increment or update _layerStructureVersion and clear the TapeTrainingStep<T> cache so subsequent CollectParameters(Layers, _layerStructureVersion) cannot reuse stale cached views. Locate RestoreOriginalParameters(), the fields _savedOriginalParameters, _parameterBuffer and _layerStructureVersion, and add logic to bump _layerStructureVersion (or assign a new unique marker) and call whatever clear/Invalidate method or static cache reset exists on TapeTrainingStep<T> (or add one) so the next CollectParameters call rebuilds fresh views.
2498-2508:⚠️ Potential issue | 🔴 CriticalStart the restore scope before any parameter-view swap can happen.
GetOrCreateParameterBuffer(...), the secondCollectParameters(...), and theLossFunctionBase<T>cast all execute after layers may already be rebound to buffer views but beforeRestoreOriginalParameters()is guaranteed. If any of those throws, the network is left in the same broken post-training state this PR is fixing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/NeuralNetworks/NeuralNetworkBase.cs`:
- Around line 2514-2523: The optimizer re-evaluation callbacks (the
ComputeForward call and the loss delegate passed into TapeStepContext<T>) are
not applying the same output/expected rank-normalization used before
ComputeTapeLoss, causing shape mismatches during re-evaluation; modify the
re-evaluation paths so they run the same alignment logic that checks for a
leading batch dim (if output.Rank > expected.Rank && output.Shape[0] == 1 &&
output.Length == expected.Length then reshape output to expected._shape, and the
symmetric case for expected) before invoking the loss delegate or
ComputeForward, ensuring the same reshaped tensors are used for any optimizer
re-evaluations (also apply the same change to the other occurrence noted in the
diff).
- Around line 2791-2803: The restoration loop must fail fast and mark restored
tensors dirty: before copying, verify trainable.GetTrainableParameters().Count
equals originals.Count and for each pair ensure view.Length == orig.Length (do
not use Math.Min to silently truncate); if any mismatch throw/abort so resized
parameters are rejected (this addresses changes from
DenseLayer<T>.Forward()/EnsureWeightShapeForInput). After copying values with
orig.SetFlat(...), immediately invalidate the original tensor's
engine/persistent state (call the tensor's cache-invalidator—e.g.,
Invalidate/MarkDirty/ResetEngineState on orig; rename to the actual method if
different) and then call trainable.SetTrainableParameters(originals) only after
all checks and invalidations succeed.
In
`@tests/AiDotNet.Tests/IntegrationTests/NeuralNetworks/ParameterBufferScopeTests.cs`:
- Around line 15-23: The tests only exercise a flat FeedForwardNeuralNetwork and
re-enumerate network.Layers, so they miss recursive GetSubLayers() and
post-train persistence (Clone/serialization) regressions; update the tests by
creating a deterministic nested-layer network (e.g., build a parent composite
layer that contains at least one child layer and one nested composite child)
instead of only CreateSimpleNetwork(), ensure deterministic initialization
(fixed RNG seed or fixed weights), then assert that enumeration via
network.GetSubLayers() returns all nested layers and that after training or a
simulated parameter update, calling network.Clone() and
serializing/deserializing the network preserves parameter values and structure
(use the same assertions to compare original vs cloned/deserialized parameters).
Include references to the CreateSimpleNetwork(), GetSubLayers(), Layers, and
Clone() usages in the new test additions.
---
Duplicate comments:
In `@src/NeuralNetworks/NeuralNetworkBase.cs`:
- Around line 2806-2807: RestoreOriginalParameters() currently sets
_savedOriginalParameters and _parameterBuffer to null but doesn't invalidate
cached parameter views; update it to increment or update _layerStructureVersion
and clear the TapeTrainingStep<T> cache so subsequent CollectParameters(Layers,
_layerStructureVersion) cannot reuse stale cached views. Locate
RestoreOriginalParameters(), the fields _savedOriginalParameters,
_parameterBuffer and _layerStructureVersion, and add logic to bump
_layerStructureVersion (or assign a new unique marker) and call whatever
clear/Invalidate method or static cache reset exists on TapeTrainingStep<T> (or
add one) so the next CollectParameters call rebuilds fresh views.
In
`@tests/AiDotNet.Tests/IntegrationTests/NeuralNetworks/ParameterBufferScopeTests.cs`:
- Around line 46-62: The test currently only checks a common prefix due to the
"idx < originalParams.Count" guard; instead, materialize the full current
parameter list from network.Layers by iterating trainable layers and calling
ITrainableLayer<double>.GetTrainableParameters() into a list, assert the counts
match originalParams with Assert.Equal(originalParams.Count,
currentParams.Count), then iterate by index and use
Assert.Same(originalParams[i], currentParams[i]) for every element (remove the
prefix guard and the idx-based null-check logic).
🪄 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: 9b35b2bb-7079-45b6-aa2a-93f781bae809
📒 Files selected for processing (3)
src/NeuralNetworks/Layers/DenseLayer.cssrc/NeuralNetworks/NeuralNetworkBase.cstests/AiDotNet.Tests/IntegrationTests/NeuralNetworks/ParameterBufferScopeTests.cs
Tape now records 5 entries during forward (was 0 before DenseLayer fix). Loss computation adds 3 more (total 8). But ComputeGradients still returns 0 gradients — the graph-based backward walk (via GradFn chain) doesn't find the parameter view tensors in the sources list. Investigating ParameterBuffer view identity vs GradFn chain tensor references. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (6)
src/NeuralNetworks/NeuralNetworkBase.cs (4)
2823-2826:⚠️ Potential issue | 🔴 CriticalBLOCKING: Invalidate tape parameter cache when dropping the buffer.
After setting
_parameterBuffer = null, the_layerStructureVersionremains unchanged. The nextCollectParameters(Layers, _layerStructureVersion)call can return a cached list containing the now-disposed view tensors, sending gradients to stale objects.Proposed fix
_savedOriginalParameters = null; _parameterBuffer = null; + Training.TapeTrainingStep<T>.InvalidateCache(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/NeuralNetworks/NeuralNetworkBase.cs` around lines 2823 - 2826, When nulling out the parameter buffer, also invalidate the layer-structure version so cached parameter lists aren’t reused; specifically, after setting _parameterBuffer = null (and _savedOriginalParameters = null) update _layerStructureVersion (e.g., bump the version token or assign a new unique value) so CollectParameters(Layers, _layerStructureVersion) can’t return stale/disposed view tensors.
2806-2821:⚠️ Potential issue | 🔴 CriticalBLOCKING: Fail fast on parameter count/shape mismatches instead of silently truncating.
The
Math.Min()loops at lines 2810 and 2815 silently discard data if a layer resizes its parameters during the buffered step (e.g.,DenseLayer<T>.EnsureWeightShapeForInput()). This could drop newly-sized weights and restore incorrect tensor shapes.Assert equality of counts/lengths and throw if mismatched.
Proposed fix
if (layer is ITrainableLayer<T> trainable) { var currentViews = trainable.GetTrainableParameters(); - for (int i = 0; i < Math.Min(originals.Count, currentViews.Count); i++) + if (currentViews.Count != originals.Count) + throw new InvalidOperationException( + $"Parameter count changed during training for {layer.GetType().Name}: " + + $"expected {originals.Count}, got {currentViews.Count}"); + + for (int i = 0; i < originals.Count; i++) { - // Copy updated weights from view back to original tensor var view = currentViews[i]; var orig = originals[i]; - for (int j = 0; j < Math.Min(view.Length, orig.Length); j++) + if (view.Length != orig.Length) + throw new InvalidOperationException( + $"Parameter {i} length changed during training: " + + $"expected {orig.Length}, got {view.Length}"); + + for (int j = 0; j < orig.Length; j++) orig.SetFlat(j, view.GetFlat(j)); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/NeuralNetworks/NeuralNetworkBase.cs` around lines 2806 - 2821, The current loop in the block handling ITrainableLayer<T> silently truncates when counts or tensor lengths differ; instead validate and fail fast: after obtaining trainable.GetTrainableParameters() and before copying, check that originals.Count == currentViews.Count and throw an InvalidOperationException (or a specific exception type) if they differ; likewise, inside the per-index loop assert or throw if view.Length != orig.Length rather than using Math.Min so mismatched tensor shapes (e.g., due to DenseLayer<T>.EnsureWeightShapeForInput) are detected; keep using trainable.SetTrainableParameters(originals) only after validations pass.
2493-2507:⚠️ Potential issue | 🔴 CriticalBLOCKING: Try/finally scope must cover buffer-setup failures.
The
tryblock starts at line 2508, butGetOrCreateParameterBuffer()at line 2500 callsSaveOriginalParameters()which mutates_savedOriginalParameters. IfGetOrCreateParameterBuffer(), the re-collect, or theLossFunctionBase<T>cast throws after parameters are saved but before entering thetry, layer fields remain pointed at buffer views andRestoreOriginalParameters()never runs.Move all buffer/parameter setup inside the
tryblock.Proposed fix
protected void TrainWithTape(Tensor<T> input, Tensor<T> expected, IGradientBasedOptimizer<T, Tensor<T>, Tensor<T>>? optimizer = null) { var engine = AiDotNetEngine.Current; - // Initialize parameter buffer BEFORE collecting params and running the forward pass. - var initialParams = Training.TapeTrainingStep<T>.CollectParameters(Layers, _parameterBuffer is null ? -1 : _layerStructureVersion); - var paramBuffer = GetOrCreateParameterBuffer(initialParams); - - // Re-collect after buffer initialization — parameter tensor references may have changed - var trainableParams = Training.TapeTrainingStep<T>.CollectParameters(Layers, _layerStructureVersion); - - var loss = LossFunction as LossFunctions.LossFunctionBase<T> - ?? throw new InvalidOperationException("LossFunction must derive from LossFunctionBase<T> for tape-based training."); - try { + // Initialize parameter buffer BEFORE collecting params and running the forward pass. + var initialParams = Training.TapeTrainingStep<T>.CollectParameters(Layers, _parameterBuffer is null ? -1 : _layerStructureVersion); + var paramBuffer = GetOrCreateParameterBuffer(initialParams); + + // Re-collect after buffer initialization — parameter tensor references may have changed + var trainableParams = Training.TapeTrainingStep<T>.CollectParameters(Layers, _layerStructureVersion); + + var loss = LossFunction as LossFunctions.LossFunctionBase<T> + ?? throw new InvalidOperationException("LossFunction must derive from LossFunctionBase<T> for tape-based training."); + // Forward + loss under tape — uses the buffer-backed view tensors🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/NeuralNetworks/NeuralNetworkBase.cs` around lines 2493 - 2507, Move the parameter-buffer and related setup into the try/finally so failures during buffer creation or subsequent steps always trigger RestoreOriginalParameters(); specifically, in TrainWithTape wrap the calls to Training.TapeTrainingStep<T>.CollectParameters(Layers, _layerStructureVersion), GetOrCreateParameterBuffer(initialParams) (which calls SaveOriginalParameters and mutates _savedOriginalParameters), the re-collect of trainableParams, and the LossFunction as LossFunctions.LossFunctionBase<T> cast inside the try block, leaving only RestoreOriginalParameters() in the finally; ensure references to _parameterBuffer and _layerStructureVersion are used unchanged and that RestoreOriginalParameters() is invoked if any of these steps throw.
2567-2573:⚠️ Potential issue | 🟠 MajorRe-evaluation callbacks skip shape normalization applied to initial loss computation.
Lines 2516-2523 reshape
output/expectedto handle batch dimension mismatches beforeComputeTapeLoss. However, theComputeForwarddelegate (line 2567) and loss delegate (line 2572) passed toTapeStepContext<T>bypass this alignment. Optimizers that re-evaluate the model (e.g., line search) will see mismatched shapes.Extract the alignment logic into a helper and apply it in both paths.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/NeuralNetworks/NeuralNetworkBase.cs` around lines 2567 - 2573, The ComputeForward delegate and the loss delegate passed into TapeStepContext<T> bypass the batch-dimension alignment applied earlier, so re-evaluations (e.g., line-search) can receive mismatched shapes; extract the reshape/align logic used around lines 2516-2523 into a helper method (e.g., AlignBatchShapes(Tensor<T> output, Tensor<T> expected) or AlignInputAndTarget) and use that helper inside ForwardForTraining wrappers: replace Tensor<T> ComputeForward(Tensor<T> inp, Tensor<T> _) => ForwardForTraining(inp); with a delegate that calls ForwardForTraining(inp) then applies the alignment helper to the returned output, and similarly wrap the loss delegate (pred, tgt) => loss.ComputeTapeLoss(pred, tgt) so it first aligns pred/tgt via the same helper before calling ComputeTapeLoss; ensure TapeStepContext<T> receives these wrapped delegates so any re-evaluations use normalized shapes.tests/AiDotNet.Tests/IntegrationTests/NeuralNetworks/ParameterBufferScopeTests.cs (2)
46-62:⚠️ Potential issue | 🟠 MajorBLOCKING: Conditional assertion can hide parameter count mismatches.
The
if (idx < originalParams.Count)guard at line 54 allows this test to pass even if parameters were added or removed during restore. This is exactly the "conditional assertions that skip verification when things fail" anti-pattern flagged in coding guidelines.Materialize the post-train parameter list, assert count equality, then compare every element unconditionally.
Proposed fix
- // Tensor references should be the same objects (not buffer views) - int idx = 0; - foreach (var layer in network.Layers) - { - if (layer is ITrainableLayer<double> trainable) - { - foreach (var p in trainable.GetTrainableParameters()) - { - if (idx < originalParams.Count) - { - Assert.True(ReferenceEquals(p, originalParams[idx]), - $"Parameter {idx} was permanently replaced with a buffer view"); - } - idx++; - } - } - } + // Collect post-train parameters + var postTrainParams = new List<Tensor<double>>(); + foreach (var layer in network.Layers) + { + if (layer is ITrainableLayer<double> trainable) + { + foreach (var p in trainable.GetTrainableParameters()) + postTrainParams.Add(p); + } + } + + // Assert count equality first + Assert.Equal(originalParams.Count, postTrainParams.Count); + + // Then verify each reference + for (int i = 0; i < originalParams.Count; i++) + { + Assert.Same(originalParams[i], postTrainParams[i]); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/AiDotNet.Tests/IntegrationTests/NeuralNetworks/ParameterBufferScopeTests.cs` around lines 46 - 62, The test currently skips comparisons when idx >= originalParams.Count; instead, collect the post-restore parameter list into a concrete list (e.g., iterate network.Layers and for each ITrainableLayer<double> call GetTrainableParameters() and add to a new list), assert that postParams.Count equals originalParams.Count, then iterate by index and unconditionally Assert.True(ReferenceEquals(postParams[i], originalParams[i]), ...) to ensure no parameters were added/removed or replaced with buffer views; update uses of network.Layers, ITrainableLayer<double>.GetTrainableParameters(), originalParams and idx to locate and replace the guarded loop.
15-23:⚠️ Potential issue | 🟠 MajorMissing test coverage for nested layers and post-train Clone/serialization.
Both tests use a flat
FeedForwardNeuralNetwork<double>and only iteratenetwork.Layers. The PR objectives cite failures in:
- GraphSAGE: Clone constructor failure (nested layers via
GetSubLayers())- Serialization: Incorrect serialization of view tensors post-training
Add tests that:
- Use a network with nested layers to exercise recursive
SaveOriginalParameters/RestoreOriginalParameters- Call
network.Clone()andnetwork.Serialize()/Deserialize()after training and assert correctness[Fact] public void Train_Clone_PreservesOriginalTensorReferences() { var network = CreateSimpleNetwork(); var input = Tensor<double>.CreateRandom([1, 4]); var target = Tensor<double>.CreateRandom([1, 2]); network.Train(input, target); // Clone should succeed and produce functional copy var cloned = network.Clone(); Assert.NotNull(cloned); // Both should produce same prediction var originalOutput = network.Predict(input); var clonedOutput = ((NeuralNetworkBase<double>)cloned).Predict(input); for (int i = 0; i < originalOutput.Length; i++) Assert.Equal(originalOutput.GetFlat(i), clonedOutput.GetFlat(i), precision: 8); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/AiDotNet.Tests/IntegrationTests/NeuralNetworks/ParameterBufferScopeTests.cs` around lines 15 - 23, Add tests that construct a network containing nested layers (so layer instances that return sub-layers from GetSubLayers(), not just a flat FeedForwardNeuralNetwork), train it, call SaveOriginalParameters/RestoreOriginalParameters via the same flow that ParameterBufferScopeTests exercise, then verify Clone() and Serialize()/Deserialize() work post-train and preserve tensor/view references and behavior: create Train_Clone_PreservesOriginalTensorReferences_NestedLayers and Train_Serialize_Deserialize_PreservesViews_NestedLayers tests that (1) build a nested-layer network, (2) call Train(input,target), (3) call network.Clone() and assert the clone is non-null and predictions from network.Predict(input) and cloned.Predict(input) match within tolerance, and (4) call network.Serialize() then Deserialize() into a new instance and assert predictions match and that SaveOriginalParameters/RestoreOriginalParameters logic exercised via GetSubLayers() handles view tensors correctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/NeuralNetworks/NeuralNetworkBase.cs`:
- Around line 2664-2675: These 11 internal debug fields on NeuralNetworkBase
(_lastGradientCount, _lastParameterCount, _lastTapeEntryCount,
_lastTapeWasActive, _lastLossLength, _lastLossValue, _forwardTapeActive,
_forwardTapeEntriesAfter, _lastNonZeroGradCount, _lastLossHasGradFn) are
diagnostic-only; wrap their declarations in a conditional compilation block (`#if`
DEBUG ... `#endif`) or remove them, and add a short comment describing their
purpose and that ParameterBufferScopeTests relies on them via reflection so
tests must run in Debug or be updated if you remove them. Ensure the conditional
block preserves existing accessibility (internal) and add a note in the class
comment about why/when to keep or remove these fields.
In
`@tests/AiDotNet.Tests/IntegrationTests/NeuralNetworks/ParameterBufferScopeTests.cs`:
- Around line 149-166: Replace the throw new Exception(...) block in
ParameterBufferScopeTests (the conditional guarded by if (!anyChanged)) with a
test failure assertion by calling Assert.Fail(...) using the exact same
interpolated diagnostic message (include beforeSnapshot.Count,
network._lastGradientCount, network._lastParameterCount,
network._lastTapeEntryCount, network._lastTapeWasActive,
network._lastLossLength, network._lastLossValue, network._forwardTapeActive,
network._forwardTapeEntriesAfter, network._lastNonZeroGradCount,
network._lastLossHasGradFn, string.Join(" | ", debugBefore) and string.Join(" |
", debugAfter)); ensure the test framework Assert type is imported/available so
the Replace occurs in the same scope as the original throw.
---
Duplicate comments:
In `@src/NeuralNetworks/NeuralNetworkBase.cs`:
- Around line 2823-2826: When nulling out the parameter buffer, also invalidate
the layer-structure version so cached parameter lists aren’t reused;
specifically, after setting _parameterBuffer = null (and
_savedOriginalParameters = null) update _layerStructureVersion (e.g., bump the
version token or assign a new unique value) so CollectParameters(Layers,
_layerStructureVersion) can’t return stale/disposed view tensors.
- Around line 2806-2821: The current loop in the block handling
ITrainableLayer<T> silently truncates when counts or tensor lengths differ;
instead validate and fail fast: after obtaining
trainable.GetTrainableParameters() and before copying, check that
originals.Count == currentViews.Count and throw an InvalidOperationException (or
a specific exception type) if they differ; likewise, inside the per-index loop
assert or throw if view.Length != orig.Length rather than using Math.Min so
mismatched tensor shapes (e.g., due to DenseLayer<T>.EnsureWeightShapeForInput)
are detected; keep using trainable.SetTrainableParameters(originals) only after
validations pass.
- Around line 2493-2507: Move the parameter-buffer and related setup into the
try/finally so failures during buffer creation or subsequent steps always
trigger RestoreOriginalParameters(); specifically, in TrainWithTape wrap the
calls to Training.TapeTrainingStep<T>.CollectParameters(Layers,
_layerStructureVersion), GetOrCreateParameterBuffer(initialParams) (which calls
SaveOriginalParameters and mutates _savedOriginalParameters), the re-collect of
trainableParams, and the LossFunction as LossFunctions.LossFunctionBase<T> cast
inside the try block, leaving only RestoreOriginalParameters() in the finally;
ensure references to _parameterBuffer and _layerStructureVersion are used
unchanged and that RestoreOriginalParameters() is invoked if any of these steps
throw.
- Around line 2567-2573: The ComputeForward delegate and the loss delegate
passed into TapeStepContext<T> bypass the batch-dimension alignment applied
earlier, so re-evaluations (e.g., line-search) can receive mismatched shapes;
extract the reshape/align logic used around lines 2516-2523 into a helper method
(e.g., AlignBatchShapes(Tensor<T> output, Tensor<T> expected) or
AlignInputAndTarget) and use that helper inside ForwardForTraining wrappers:
replace Tensor<T> ComputeForward(Tensor<T> inp, Tensor<T> _) =>
ForwardForTraining(inp); with a delegate that calls ForwardForTraining(inp) then
applies the alignment helper to the returned output, and similarly wrap the loss
delegate (pred, tgt) => loss.ComputeTapeLoss(pred, tgt) so it first aligns
pred/tgt via the same helper before calling ComputeTapeLoss; ensure
TapeStepContext<T> receives these wrapped delegates so any re-evaluations use
normalized shapes.
In
`@tests/AiDotNet.Tests/IntegrationTests/NeuralNetworks/ParameterBufferScopeTests.cs`:
- Around line 46-62: The test currently skips comparisons when idx >=
originalParams.Count; instead, collect the post-restore parameter list into a
concrete list (e.g., iterate network.Layers and for each ITrainableLayer<double>
call GetTrainableParameters() and add to a new list), assert that
postParams.Count equals originalParams.Count, then iterate by index and
unconditionally Assert.True(ReferenceEquals(postParams[i], originalParams[i]),
...) to ensure no parameters were added/removed or replaced with buffer views;
update uses of network.Layers, ITrainableLayer<double>.GetTrainableParameters(),
originalParams and idx to locate and replace the guarded loop.
- Around line 15-23: Add tests that construct a network containing nested layers
(so layer instances that return sub-layers from GetSubLayers(), not just a flat
FeedForwardNeuralNetwork), train it, call
SaveOriginalParameters/RestoreOriginalParameters via the same flow that
ParameterBufferScopeTests exercise, then verify Clone() and
Serialize()/Deserialize() work post-train and preserve tensor/view references
and behavior: create Train_Clone_PreservesOriginalTensorReferences_NestedLayers
and Train_Serialize_Deserialize_PreservesViews_NestedLayers tests that (1) build
a nested-layer network, (2) call Train(input,target), (3) call network.Clone()
and assert the clone is non-null and predictions from network.Predict(input) and
cloned.Predict(input) match within tolerance, and (4) call network.Serialize()
then Deserialize() into a new instance and assert predictions match and that
SaveOriginalParameters/RestoreOriginalParameters logic exercised via
GetSubLayers() handles view tensors correctly.
🪄 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: 776431de-f0be-4b21-bceb-421c36bcea97
📒 Files selected for processing (2)
src/NeuralNetworks/NeuralNetworkBase.cstests/AiDotNet.Tests/IntegrationTests/NeuralNetworks/ParameterBufferScopeTests.cs
…1084) Root cause 2: ActivationFunctionBase.Activate(Tensor<T>) creates a NEW tensor via TensorAllocator.RentUninitialized even for Identity, breaking the tape dependency chain. The loss operates on this new tensor which has no GradFn or tape entry connecting it back to the FusedLinear output. Fix: override Activate(Tensor<T>) on IdentityActivation to return input directly — zero allocation, preserves tape chain. Combined with the DenseLayer single-FusedLinear fix, gradients now flow from loss through all layers to parameters. Both tests pass: parameters are updated AND original tensor references are restored after training. Summary of 3 bugs fixed in this PR: 1. ParameterBuffer permanently replaces layer tensors → scoped replacement 2. DenseLayer double FusedLinear corrupts tape → single call + separate activation 3. IdentityActivation breaks tape chain → passthrough override Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (6)
src/NeuralNetworks/NeuralNetworkBase.cs (4)
2855-2856:⚠️ Potential issue | 🟠 MajorInvalidate the tape parameter cache when the buffer is dropped.
You clear
_parameterBuffer, but the tape cache still sees the same_layerStructureVersion. The nextCollectParameters(Layers, _layerStructureVersion)can therefore reuse the previous step's cached view list instead of the freshly rebuilt views.Suggested change
_savedOriginalParameters = null; _parameterBuffer = null; + Training.TapeTrainingStep<T>.InvalidateCache();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/NeuralNetworks/NeuralNetworkBase.cs` around lines 2855 - 2856, When you drop the buffer by setting _parameterBuffer and _savedOriginalParameters to null, also invalidate the tape's cached parameter views so future CollectParameters(Layers, _layerStructureVersion) cannot reuse stale views: update the cache key by bumping or changing _layerStructureVersion (or explicitly clear the tape cache entry) immediately after nulling _parameterBuffer/_savedOriginalParameters so CollectParameters will rebuild fresh views for Layers.
2841-2851:⚠️ Potential issue | 🟠 MajorReject parameter-count or shape drift during restore.
The
Math.Min(...)loops silently truncate if a layer changes its parameter count or tensor length during the buffered step. That drops updated values and then restores the old tensor objects anyway. Fail fast on count/length mismatch instead of partially copying.Suggested change
- for (int i = 0; i < Math.Min(originals.Count, currentViews.Count); i++) + if (originals.Count != currentViews.Count) + throw new InvalidOperationException("Trainable parameter count changed during buffered training."); + + for (int i = 0; i < originals.Count; i++) { // Copy updated weights from view back to original tensor var view = currentViews[i]; var orig = originals[i]; - for (int j = 0; j < Math.Min(view.Length, orig.Length); j++) + if (view.Length != orig.Length) + throw new InvalidOperationException("Trainable parameter shape changed during buffered training."); + + for (int j = 0; j < view.Length; j++) orig.SetFlat(j, view.GetFlat(j)); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/NeuralNetworks/NeuralNetworkBase.cs` around lines 2841 - 2851, The code silently truncates parameter copying by using Math.Min on originals vs currentViews and on tensor lengths (view.Length vs orig.Length), which can hide layer parameter-count/shape drift; update the restore logic in the block that iterates currentViews and originals to first verify that originals.Count == currentViews.Count and that for each i orig.Length == view.Length, and if any mismatch occurs throw an informative exception (or return an error) instead of performing a partial copy, then perform the full-element copy and call trainable.SetTrainableParameters(originals) only after these validations succeed; refer to currentViews, originals, view.Length, orig.Length and trainable.SetTrainableParameters to locate the code to change.
2508-2512:⚠️ Potential issue | 🔴 CriticalStart the restore scope before any buffer/view mutation.
The current
try/finallybegins afterGetOrCreateParameterBuffer(...), the re-collect, and the loss cast. If any of those throws after the layer fields have been swapped to buffer views,RestoreOriginalParameters()never runs and the model stays permanently mutated.Suggested change
- // Initialize parameter buffer BEFORE collecting params and running the forward pass. - var initialParams = Training.TapeTrainingStep<T>.CollectParameters(Layers, _parameterBuffer is null ? -1 : _layerStructureVersion); - var paramBuffer = GetOrCreateParameterBuffer(initialParams); - - // Re-collect after buffer initialization — parameter tensor references may have changed - var trainableParams = Training.TapeTrainingStep<T>.CollectParameters(Layers, _layerStructureVersion); - - var loss = LossFunction as LossFunctions.LossFunctionBase<T> - ?? throw new InvalidOperationException("LossFunction must derive from LossFunctionBase<T> for tape-based training."); - try { + // Initialize parameter buffer BEFORE collecting params and running the forward pass. + var initialParams = Training.TapeTrainingStep<T>.CollectParameters( + Layers, + _parameterBuffer is null ? -1 : _layerStructureVersion); + var paramBuffer = GetOrCreateParameterBuffer(initialParams); + + // Re-collect after buffer initialization — parameter tensor references may have changed + var trainableParams = Training.TapeTrainingStep<T>.CollectParameters(Layers, _layerStructureVersion); + + var loss = LossFunction as LossFunctions.LossFunctionBase<T> + ?? throw new InvalidOperationException("LossFunction must derive from LossFunctionBase<T> for tape-based training.");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/NeuralNetworks/NeuralNetworkBase.cs` around lines 2508 - 2512, The try/finally that ensures RestoreOriginalParameters() must begin before any operation that mutates layer fields to buffer/view tensors; move the start of the restore scope so the try is placed before calling GetOrCreateParameterBuffer(...) (and before the subsequent re-collect and loss cast), then perform parameter buffer creation, view swaps and ForwardForTraining(input) inside that try, and keep RestoreOriginalParameters() in the finally block so any exception during GetOrCreateParameterBuffer, Recollect, loss casting, or ForwardForTraining will still restore the original parameters (refer to GetOrCreateParameterBuffer, RestoreOriginalParameters, ForwardForTraining, and GradientTape<T> to locate the code).
2595-2602:⚠️ Potential issue | 🟠 MajorReuse the same shape normalization inside the
TapeStepContext<T>callbacks.The initial loss path aligns
outputandexpected, but theComputeForwardcallback and the loss delegate stored inTapeStepContext<T>still operate on raw shapes. Any optimizer that re-evaluates the step can hit the same rank mismatch again.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/NeuralNetworks/NeuralNetworkBase.cs` around lines 2595 - 2602, The callbacks passed into TapeStepContext<T> (ComputeForward and the loss delegate that calls loss.ComputeTapeLoss) still use raw tensor shapes and can produce rank mismatches on re-evaluation; update these callbacks to reuse the same shape-normalization/alignment logic applied to the initial loss path so they return/compare tensors with the normalized output shape (i.e., wrap ForwardForTraining in a small adapter used as ComputeForward that applies the same output→alignedOutput transformation, and likewise adapt the loss delegate to align its predicted and target tensors before calling loss.ComputeTapeLoss), keeping changes localized around TapeStepContext<T>, ComputeForward, ForwardForTraining and the loss delegate.tests/AiDotNet.Tests/IntegrationTests/NeuralNetworks/ParameterBufferScopeTests.cs (2)
47-59:⚠️ Potential issue | 🔴 CriticalMake the identity regression fail when parameter counts drift.
The
if (idx < originalParams.Count)guard reduces this to a prefix check, so added or missing parameters won't fail the test. Materialize the full post-train parameter list, assert the counts match, then compare every element by reference.As per coding guidelines, "Tests MUST be production-quality. Flag ALL of the following as blocking issues: Always-passing tests".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/AiDotNet.Tests/IntegrationTests/NeuralNetworks/ParameterBufferScopeTests.cs` around lines 47 - 59, The test currently only checks a prefix due to the guard and can miss added/removed parameters; fix ParameterBufferScopeTests by materializing the full post-train parameter list (call GetTrainableParameters() for all trainable layers and collect into a List into e.g. postParams), assert postParams.Count == originalParams.Count, then iterate every index i and Assert.True(ReferenceEquals(postParams[i], originalParams[i])) to ensure one-to-one identity; use the existing network.Layers and ITrainableLayer<double>.GetTrainableParameters() to locate where to change the logic.
15-23:⚠️ Potential issue | 🟠 MajorAdd the post-train persistence regression this PR is still claiming to fix.
Both tests stay on a flat
FeedForwardNeuralNetwork<double>and never callClone()or perform a serialize/deserialize round-trip after training, so the clone/serialization failures cited in#1084remain unverified. Please add at least one deterministic post-train persistence check, ideally with a nested trainable layer so the recursiveGetSubLayers()restore path is exercised too.As per coding guidelines, "Good tests should: ... Test edge cases and error conditions".
Also applies to: 25-169
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/AiDotNet.Tests/IntegrationTests/NeuralNetworks/ParameterBufferScopeTests.cs` around lines 15 - 23, The test suite's CreateSimpleNetwork() and its related tests never exercise post-training persistence or cloning, so the clone/serialize-deserialize regression cited in `#1084` isn't validated; modify or add a test that trains a network containing at least one nested trainable layer, then perform a deterministic post-train persistence check by calling Clone() and/or serializing and deserializing the FeedForwardNeuralNetwork<double>, then assert that critical state (weights/biases, architecture, and sub-layer instances returned by GetSubLayers()) are equal/consistent between the original and restored instances; ensure the training is deterministic (fixed seed, synthetic data) so assertions are reliable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/NeuralNetworks/NeuralNetworkBase.cs`:
- Around line 2855-2856: When you drop the buffer by setting _parameterBuffer
and _savedOriginalParameters to null, also invalidate the tape's cached
parameter views so future CollectParameters(Layers, _layerStructureVersion)
cannot reuse stale views: update the cache key by bumping or changing
_layerStructureVersion (or explicitly clear the tape cache entry) immediately
after nulling _parameterBuffer/_savedOriginalParameters so CollectParameters
will rebuild fresh views for Layers.
- Around line 2841-2851: The code silently truncates parameter copying by using
Math.Min on originals vs currentViews and on tensor lengths (view.Length vs
orig.Length), which can hide layer parameter-count/shape drift; update the
restore logic in the block that iterates currentViews and originals to first
verify that originals.Count == currentViews.Count and that for each i
orig.Length == view.Length, and if any mismatch occurs throw an informative
exception (or return an error) instead of performing a partial copy, then
perform the full-element copy and call
trainable.SetTrainableParameters(originals) only after these validations
succeed; refer to currentViews, originals, view.Length, orig.Length and
trainable.SetTrainableParameters to locate the code to change.
- Around line 2508-2512: The try/finally that ensures
RestoreOriginalParameters() must begin before any operation that mutates layer
fields to buffer/view tensors; move the start of the restore scope so the try is
placed before calling GetOrCreateParameterBuffer(...) (and before the subsequent
re-collect and loss cast), then perform parameter buffer creation, view swaps
and ForwardForTraining(input) inside that try, and keep
RestoreOriginalParameters() in the finally block so any exception during
GetOrCreateParameterBuffer, Recollect, loss casting, or ForwardForTraining will
still restore the original parameters (refer to GetOrCreateParameterBuffer,
RestoreOriginalParameters, ForwardForTraining, and GradientTape<T> to locate the
code).
- Around line 2595-2602: The callbacks passed into TapeStepContext<T>
(ComputeForward and the loss delegate that calls loss.ComputeTapeLoss) still use
raw tensor shapes and can produce rank mismatches on re-evaluation; update these
callbacks to reuse the same shape-normalization/alignment logic applied to the
initial loss path so they return/compare tensors with the normalized output
shape (i.e., wrap ForwardForTraining in a small adapter used as ComputeForward
that applies the same output→alignedOutput transformation, and likewise adapt
the loss delegate to align its predicted and target tensors before calling
loss.ComputeTapeLoss), keeping changes localized around TapeStepContext<T>,
ComputeForward, ForwardForTraining and the loss delegate.
In
`@tests/AiDotNet.Tests/IntegrationTests/NeuralNetworks/ParameterBufferScopeTests.cs`:
- Around line 47-59: The test currently only checks a prefix due to the guard
and can miss added/removed parameters; fix ParameterBufferScopeTests by
materializing the full post-train parameter list (call GetTrainableParameters()
for all trainable layers and collect into a List into e.g. postParams), assert
postParams.Count == originalParams.Count, then iterate every index i and
Assert.True(ReferenceEquals(postParams[i], originalParams[i])) to ensure
one-to-one identity; use the existing network.Layers and
ITrainableLayer<double>.GetTrainableParameters() to locate where to change the
logic.
- Around line 15-23: The test suite's CreateSimpleNetwork() and its related
tests never exercise post-training persistence or cloning, so the
clone/serialize-deserialize regression cited in `#1084` isn't validated; modify or
add a test that trains a network containing at least one nested trainable layer,
then perform a deterministic post-train persistence check by calling Clone()
and/or serializing and deserializing the FeedForwardNeuralNetwork<double>, then
assert that critical state (weights/biases, architecture, and sub-layer
instances returned by GetSubLayers()) are equal/consistent between the original
and restored instances; ensure the training is deterministic (fixed seed,
synthetic data) so assertions are reliable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: d5f07389-6bb3-4e35-860b-e3e485d77c79
📒 Files selected for processing (3)
src/ActivationFunctions/IdentityActivation.cssrc/NeuralNetworks/NeuralNetworkBase.cstests/AiDotNet.Tests/IntegrationTests/NeuralNetworks/ParameterBufferScopeTests.cs
…alidate - Removed all 11 debug instrumentation fields from NeuralNetworkBase - Widened try/finally scope to cover buffer setup (GetOrCreateParameterBuffer) - RestoreOriginalParameters: validate count/size equality (throw, don't truncate) - Clear _parameterBuffer on restore so next step creates fresh views - Invalidate tape cache (_layerStructureVersion++) on restore - ComputeForward callback applies same shape alignment as initial forward - ForwardForTraining: removed debug instrumentation - Tests: assert exact parameter count, removed debug field references, use deterministic input/target values Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
|
Note Unit test generation is a public access feature. Expect some limitations and changes as we gather feedback and continue to improve it. Generating unit tests... This may take up to 20 minutes. |
2 similar comments
Contributor
|
Note Unit test generation is a public access feature. Expect some limitations and changes as we gather feedback and continue to improve it. Generating unit tests... This may take up to 20 minutes. |
Contributor
|
Note Unit test generation is a public access feature. Expect some limitations and changes as we gather feedback and continue to improve it. Generating unit tests... This may take up to 20 minutes. |
Contributor
|
✅ Created PR with unit tests: #1105 |
Contributor
|
✅ Unit tests committed locally. Commit: |
Contributor
|
✅ Unit tests committed locally. Commit: |
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (3)
src/NeuralNetworks/NeuralNetworkBase.cs (2)
2491-2496:⚠️ Potential issue | 🔴 CriticalMove buffer setup inside the
try/finally.
GetOrCreateParameterBuffer()swaps layer fields to buffer-backed views before thetrystarts. If setup throws in that window,RestoreOriginalParameters()never runs and the model is left in the same view-backed state this fix is trying to eliminate.Suggested change
- var initialParams = Training.TapeTrainingStep<T>.CollectParameters(Layers, _parameterBuffer is null ? -1 : _layerStructureVersion); - var paramBuffer = GetOrCreateParameterBuffer(initialParams); - try { + var initialParams = Training.TapeTrainingStep<T>.CollectParameters( + Layers, + _parameterBuffer is null ? -1 : _layerStructureVersion); + var paramBuffer = GetOrCreateParameterBuffer(initialParams); + // Re-collect after buffer initialization — references are now views var trainableParams = Training.TapeTrainingStep<T>.CollectParameters(Layers, _layerStructureVersion);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/NeuralNetworks/NeuralNetworkBase.cs` around lines 2491 - 2496, The call to GetOrCreateParameterBuffer swaps layer fields to buffer-backed views before the try/finally that calls RestoreOriginalParameters, so if setup throws the original views are never restored; move the GetOrCreateParameterBuffer(initialParams) invocation into the try block (immediately before any setup that may throw) so that the try begins before layers are swapped and the finally block with RestoreOriginalParameters() always runs to revert the views; keep CollectParameters(...) outside (or before) the try so you still compute initialParams, but ensure paramBuffer creation/swap happens inside the try that has the corresponding finally calling RestoreOriginalParameters().
2765-2801:⚠️ Potential issue | 🟠 MajorMake restore validation two-phase so failure paths still unwind cleanly.
The new count/length guards throw before Lines 2796-2800 clear
_savedOriginalParametersand_parameterBuffer. If a later layer fails validation after earlier layers were already copied/restored, the network is left half-restored with stale buffer state. Validate every layer first, then do the copy-back/SetTrainableParameterspass, and keep the state cleanup in afinally.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/NeuralNetworks/NeuralNetworkBase.cs` around lines 2765 - 2801, RestoreOriginalParameters currently mutates original tensors as it validates, which can leave the network half-restored when a later validation fails; change it to a two-phase approach in RestoreOriginalParameters: first iterate over _savedOriginalParameters and for each (layer, originals) where layer is ITrainableLayer<T> call GetTrainableParameters() and validate counts and lengths for every parameter without modifying any tensors, collecting the currentViews lists; then in a second pass perform the copy-back (orig.SetFlat from view.GetFlat) and call SetTrainableParameters(originals) using the previously collected currentViews; ensure the state cleanup (_savedOriginalParameters = null, _parameterBuffer = null, and incrementing _layerStructureVersion) happens in a finally block so it always runs even if an exception is thrown.tests/AiDotNet.Tests/IntegrationTests/NeuralNetworks/ParameterBufferScopeTests.cs (1)
15-23:⚠️ Potential issue | 🟠 MajorAdd the recursive/persistence regressions this fix is supposed to cover.
Every test here uses a flat
FeedForwardNeuralNetwork<double>and only re-enumeratesnetwork.Layers, so the suite still never exercisesGetSubLayers()restoration or the post-trainClone()/serialization failures called out in#1084. Please add at least one deterministic nested-layer case and one post-train clone/serialize round-trip before merging.As per coding guidelines, "Good tests should: ... Test edge cases and error conditions".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/AiDotNet.Tests/IntegrationTests/NeuralNetworks/ParameterBufferScopeTests.cs` around lines 15 - 23, The tests currently only use CreateSimpleNetwork() which builds a flat FeedForwardNeuralNetwork<double> and re-enumerates network.Layers, so add two cases: (1) construct a deterministic nested-layer network (e.g., a parent layer containing child layers that expose GetSubLayers()) and include assertions that parameter restoration uses GetSubLayers() traversal to validate recursive/persistence behavior; and (2) after training a network instance (use the same FeedForwardNeuralNetwork<double> or the nested variant), perform a Clone() and a full serialize/deserialize round-trip and assert that parameters and GetSubLayers() structure are identical post-round-trip to catch post-train Clone()/serialization regressions referenced in `#1084`. Ensure tests reference CreateSimpleNetwork(), GetSubLayers(), Clone(), and the serialization routine used by the codebase so they deterministically fail if recursive restoration or post-train cloning is broken.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/NeuralNetworks/NeuralNetworkBase.cs`:
- Around line 2491-2496: The call to GetOrCreateParameterBuffer swaps layer
fields to buffer-backed views before the try/finally that calls
RestoreOriginalParameters, so if setup throws the original views are never
restored; move the GetOrCreateParameterBuffer(initialParams) invocation into the
try block (immediately before any setup that may throw) so that the try begins
before layers are swapped and the finally block with RestoreOriginalParameters()
always runs to revert the views; keep CollectParameters(...) outside (or before)
the try so you still compute initialParams, but ensure paramBuffer creation/swap
happens inside the try that has the corresponding finally calling
RestoreOriginalParameters().
- Around line 2765-2801: RestoreOriginalParameters currently mutates original
tensors as it validates, which can leave the network half-restored when a later
validation fails; change it to a two-phase approach in
RestoreOriginalParameters: first iterate over _savedOriginalParameters and for
each (layer, originals) where layer is ITrainableLayer<T> call
GetTrainableParameters() and validate counts and lengths for every parameter
without modifying any tensors, collecting the currentViews lists; then in a
second pass perform the copy-back (orig.SetFlat from view.GetFlat) and call
SetTrainableParameters(originals) using the previously collected currentViews;
ensure the state cleanup (_savedOriginalParameters = null, _parameterBuffer =
null, and incrementing _layerStructureVersion) happens in a finally block so it
always runs even if an exception is thrown.
In
`@tests/AiDotNet.Tests/IntegrationTests/NeuralNetworks/ParameterBufferScopeTests.cs`:
- Around line 15-23: The tests currently only use CreateSimpleNetwork() which
builds a flat FeedForwardNeuralNetwork<double> and re-enumerates network.Layers,
so add two cases: (1) construct a deterministic nested-layer network (e.g., a
parent layer containing child layers that expose GetSubLayers()) and include
assertions that parameter restoration uses GetSubLayers() traversal to validate
recursive/persistence behavior; and (2) after training a network instance (use
the same FeedForwardNeuralNetwork<double> or the nested variant), perform a
Clone() and a full serialize/deserialize round-trip and assert that parameters
and GetSubLayers() structure are identical post-round-trip to catch post-train
Clone()/serialization regressions referenced in `#1084`. Ensure tests reference
CreateSimpleNetwork(), GetSubLayers(), Clone(), and the serialization routine
used by the codebase so they deterministically fail if recursive restoration or
post-train cloning is broken.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3f7d6dbd-872d-4975-8eab-c9d14a0cd89d
📒 Files selected for processing (6)
src/ActivationFunctions/IdentityActivation.cssrc/NeuralNetworks/NeuralNetworkBase.cstests/AiDotNet.Tests/ActivationFunctions/ActivationFunctionBehaviorTests.cstests/AiDotNet.Tests/IntegrationTests/ActivationFunctions/ActivationFunctionsIntegrationTests.cstests/AiDotNet.Tests/IntegrationTests/NeuralNetworks/CoreLayersIntegrationTests.cstests/AiDotNet.Tests/IntegrationTests/NeuralNetworks/ParameterBufferScopeTests.cs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
tests/AiDotNet.Tests/IntegrationTests/NeuralNetworks/ParameterBufferScopeTests.cs (2)
149-167:⚠️ Potential issue | 🟡 MinorAssert parameter-count equality on each step before the identity checks.
This loop still only validates a prefix of the parameter list. If a later restore drops a trailing parameter,
idxnever reachesoriginalParams.Countand the test still passes. Materialize the current parameter list per step, or at least assert the finalidx, before the reference comparisons.Suggested fix
for (int step = 0; step < 5; step++) { network.Train(input, target); - int idx = 0; - foreach (var layer in network.Layers) - { - if (layer is ITrainableLayer<double> trainable) - { - foreach (var p in trainable.GetTrainableParameters()) - { - Assert.True(ReferenceEquals(p, originalParams[idx]), - $"Step {step}: parameter {idx} was permanently replaced with a buffer view."); - idx++; - } - } - } + var currentParams = new List<Tensor<double>>(); + foreach (var layer in network.Layers) + { + if (layer is ITrainableLayer<double> trainable) + currentParams.AddRange(trainable.GetTrainableParameters()); + } + + Assert.Equal(originalParams.Count, currentParams.Count); + for (int idx = 0; idx < currentParams.Count; idx++) + { + Assert.True(ReferenceEquals(currentParams[idx], originalParams[idx]), + $"Step {step}: parameter {idx} was permanently replaced with a buffer view."); + } }As per coding guidelines, "Good tests should: ... Use unconditional assertions (the test should FAIL if behavior is wrong)".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/AiDotNet.Tests/IntegrationTests/NeuralNetworks/ParameterBufferScopeTests.cs` around lines 149 - 167, The test loop in ParameterBufferScopeTests.cs only checks a prefix of parameters each step and can miss dropped trailing parameters; before doing the ReferenceEquals checks inside the per-step loop, assert that the number of collected parameters equals originalParams.Count (e.g., materialize trainable.GetTrainableParameters() into a list and verify its Count matches originalParams.Count) so idx will iterate over the full list each step; apply this check for every iteration of the for (int step...) loop around the code that inspects network.Layers and ITrainableLayer<double>.GetTrainableParameters() against originalParams.
15-23:⚠️ Potential issue | 🟠 MajorAdd a post-train persistence regression here.
Every test built from
CreateSimpleNetwork()stays on a flatFeedForwardNeuralNetwork<double>, so this suite still never exercises the recursiveGetSubLayers()restore path or the post-trainClone()/serialization failures called out in#1084. Please add at least one deterministic post-trainClone()/serialize round-trip here, ideally on a model with nested sublayers.As per coding guidelines, "Good tests should: ... Test edge cases and error conditions".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/AiDotNet.Tests/IntegrationTests/NeuralNetworks/ParameterBufferScopeTests.cs` around lines 15 - 23, Add a deterministic post-training persistence regression test in ParameterBufferScopeTests that builds a network with nested sublayers (instead of the flat CreateSimpleNetwork), performs a repeatable small training run (use a fixed RNG/seed and small dataset), then exercise the post-train Clone()/serialization round-trip and restore via GetSubLayers()/deserialization and assert that parameters and outputs match the original model; specifically, create or reuse a factory that constructs a network with at least one nested sublayer, train it deterministically, call model.Clone() and serialize/deserialize the clone, then compare parameter buffers and a few inference results to the trained original to catch the recursive restore/serialization failures referenced in `#1084`.src/NeuralNetworks/NeuralNetworkBase.cs (1)
2491-2496:⚠️ Potential issue | 🔴 CriticalMove buffer setup inside the
try/finally.Line 2494 can still throw after
GetOrCreateParameterBuffer()has saved originals and started swapping layer fields to buffer views, but before thefinallyis armed. That leaves the model stuck in the same broken post-training state this PR is trying to eliminate.Suggested fix
- var initialParams = Training.TapeTrainingStep<T>.CollectParameters(Layers, _parameterBuffer is null ? -1 : _layerStructureVersion); - var paramBuffer = GetOrCreateParameterBuffer(initialParams); - - try + ParameterBuffer<T>? paramBuffer = null; + try { + var initialParams = Training.TapeTrainingStep<T>.CollectParameters( + Layers, + _parameterBuffer is null ? -1 : _layerStructureVersion); + paramBuffer = GetOrCreateParameterBuffer(initialParams); + // Re-collect after buffer initialization — references are now views var trainableParams = Training.TapeTrainingStep<T>.CollectParameters(Layers, _layerStructureVersion);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/NeuralNetworks/NeuralNetworkBase.cs` around lines 2491 - 2496, The buffer creation and layer-field swapping done by GetOrCreateParameterBuffer should happen inside the try so the corresponding finally will always run; move the call to GetOrCreateParameterBuffer(initialParams) (and any code that begins swapping Layers fields to buffer-backed views or mutating _parameterBuffer/_layerStructureVersion) into the try block immediately after collecting initialParams (which uses Training.TapeTrainingStep<T>.CollectParameters(Layers, _parameterBuffer is null ? -1 : _layerStructureVersion)) and ensure the finally restores original layer fields; this guarantees that if swapping throws, the finally in the surrounding method (the one that currently wraps the try/finally) will still execute and restore state.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@tests/AiDotNet.Tests/IntegrationTests/NeuralNetworks/ParameterBufferScopeTests.cs`:
- Around line 210-232: The test Train_LossDecreasesOverExtendedTraining is flaky
because CreateSimpleNetwork() and optimizer state are randomized; make the test
deterministic by initializing the network and optimizer to fixed, known values
before training (e.g., call a deterministic factory or set a fixed RNG seed
and/or explicitly set layer weights/biases and optimizer hyperparameters/state
on the object returned by CreateSimpleNetwork()), so that network.Train(input,
target) and subsequent network.GetLastLoss() produce reproducible firstLoss and
finalLoss; update the test to use that deterministic network (or explicit
parameter assignment) so the Assert.True(finalLoss < firstLoss) reliably
validates the scoped-restore regression.
---
Duplicate comments:
In `@src/NeuralNetworks/NeuralNetworkBase.cs`:
- Around line 2491-2496: The buffer creation and layer-field swapping done by
GetOrCreateParameterBuffer should happen inside the try so the corresponding
finally will always run; move the call to
GetOrCreateParameterBuffer(initialParams) (and any code that begins swapping
Layers fields to buffer-backed views or mutating
_parameterBuffer/_layerStructureVersion) into the try block immediately after
collecting initialParams (which uses
Training.TapeTrainingStep<T>.CollectParameters(Layers, _parameterBuffer is null
? -1 : _layerStructureVersion)) and ensure the finally restores original layer
fields; this guarantees that if swapping throws, the finally in the surrounding
method (the one that currently wraps the try/finally) will still execute and
restore state.
In
`@tests/AiDotNet.Tests/IntegrationTests/NeuralNetworks/ParameterBufferScopeTests.cs`:
- Around line 149-167: The test loop in ParameterBufferScopeTests.cs only checks
a prefix of parameters each step and can miss dropped trailing parameters;
before doing the ReferenceEquals checks inside the per-step loop, assert that
the number of collected parameters equals originalParams.Count (e.g.,
materialize trainable.GetTrainableParameters() into a list and verify its Count
matches originalParams.Count) so idx will iterate over the full list each step;
apply this check for every iteration of the for (int step...) loop around the
code that inspects network.Layers and
ITrainableLayer<double>.GetTrainableParameters() against originalParams.
- Around line 15-23: Add a deterministic post-training persistence regression
test in ParameterBufferScopeTests that builds a network with nested sublayers
(instead of the flat CreateSimpleNetwork), performs a repeatable small training
run (use a fixed RNG/seed and small dataset), then exercise the post-train
Clone()/serialization round-trip and restore via GetSubLayers()/deserialization
and assert that parameters and outputs match the original model; specifically,
create or reuse a factory that constructs a network with at least one nested
sublayer, train it deterministically, call model.Clone() and
serialize/deserialize the clone, then compare parameter buffers and a few
inference results to the trained original to catch the recursive
restore/serialization failures referenced in `#1084`.
🪄 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: ea777417-ff99-4ca1-b24a-ca4c1570fa20
📒 Files selected for processing (6)
src/ActivationFunctions/IdentityActivation.cssrc/NeuralNetworks/NeuralNetworkBase.cstests/AiDotNet.Tests/ActivationFunctions/ActivationFunctionBehaviorTests.cstests/AiDotNet.Tests/IntegrationTests/ActivationFunctions/ActivationFunctionsIntegrationTests.cstests/AiDotNet.Tests/IntegrationTests/NeuralNetworks/CoreLayersIntegrationTests.cstests/AiDotNet.Tests/IntegrationTests/NeuralNetworks/ParameterBufferScopeTests.cs
This was referenced Apr 27, 2026
Merged
This was referenced May 5, 2026
This was referenced May 12, 2026
Merged
This was referenced May 20, 2026
This was referenced Jun 3, 2026
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
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
Critical bug fix:
NeuralNetworkBase.GetOrCreateParameterBuffer()permanently replaced layer tensor fields (_weights,_biases, etc.) with buffer-backed view tensors viaSetTrainableParameters(). This broke Clone, serialization, and shape assumptions after training.Fix: Scoped replacement — saves original tensor references before replacement, restores them after the training step in a
finallyblock.How it works
GetOrCreateParameterBuffersaves original tensor refs viaSaveOriginalParametersbefore replacing with viewsTrainWithTapewraps the entire forward/backward/optimizer step intry/finallyRestoreOriginalParametersinfinally:Affected models fixed
Test plan
Train_DoesNotPermanentlyReplaceLayerTensors— verifies tensor refs are originals after trainingCloses #1084
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
New Features
Tests