fix: HTMNetwork eval mode in Predict + CapsuleNetwork auxloss optimization - #1087
Conversation
…oss when disabled 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
|
WalkthroughAuxiliary (reconstruction) loss in CapsuleNetwork.Train is computed only when Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Makes inference/training behavior more predictable and efficient by ensuring Predict doesn’t leave training-mode side effects and by avoiding unnecessary auxiliary-loss computation during Capsule training.
Changes:
HTMNetwork.Predict: Capture each layer’s prior training-mode state and restore it after inference.CapsuleNetwork.Train: Only compute auxiliary loss whenUseAuxiliaryLossis enabled.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/NeuralNetworks/HTMNetwork.cs | Restores per-layer training-mode state after Predict to keep inference side-effect free. |
| src/NeuralNetworks/CapsuleNetwork.cs | Skips auxiliary-loss forward pass when auxiliary loss is disabled. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/NeuralNetworks/CapsuleNetwork.cs (1)
375-395:⚠️ Potential issue | 🔴 CriticalBLOCKING: Simplified placeholder implementation with explicit "for now" comments in production code.
The private
ComputeReconstructionLossmethod (lines 375-395) contains a simplified implementation that violates production-readiness requirements:
- Comments explicitly state
"Simplified reconstruction loss computation"and"For now, compute a simple L2 loss"— equivalent to TODO comments- Implementation shortcuts proper CapsNet reconstruction by computing naive MSE between raw capsule outputs and input (with incorrect
minlength logic)- Does not implement the decoder network (3 FC layers) documented in the architecture
- Does not apply capsule masking required by the Sabour et al. (2017) paper
Production-ready code must:
- Implement proper reconstruction loss with a trained decoder network
- Apply masking to reconstruct from the target class capsule vector
- Compute MSE between reconstructed and original input
- Remove all "simplified" and "for now" language
This is non-negotiable — replace with a complete, production-ready implementation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/NeuralNetworks/CapsuleNetwork.cs` around lines 375 - 395, The ComputeReconstructionLoss method currently contains a placeholder MSE loop and "for now" comments; replace it with a production-ready implementation that (1) removes placeholder comments and TODO language, (2) applies capsule masking to select the target class capsule vector (use the same mask logic/data used by the forward pass), (3) passes the masked capsule vector through a trained decoder network of three fully-connected layers (implement or call existing Decoder class/method) to produce a reconstructed input, and (4) computes the mean squared error between that decoder output and originalInput (using NumOps operations for subtraction, square, sum and divide over the full input length). Update ComputeReconstructionLoss, capsuleOutputs, and originalInput usages to reference the decoder/Masking utilities and ensure correct tensor shapes and numeric conversions instead of the current minLength loop.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/NeuralNetworks/CapsuleNetwork.cs`:
- Around line 375-395: The ComputeReconstructionLoss method currently contains a
placeholder MSE loop and "for now" comments; replace it with a production-ready
implementation that (1) removes placeholder comments and TODO language, (2)
applies capsule masking to select the target class capsule vector (use the same
mask logic/data used by the forward pass), (3) passes the masked capsule vector
through a trained decoder network of three fully-connected layers (implement or
call existing Decoder class/method) to produce a reconstructed input, and (4)
computes the mean squared error between that decoder output and originalInput
(using NumOps operations for subtraction, square, sum and divide over the full
input length). Update ComputeReconstructionLoss, capsuleOutputs, and
originalInput usages to reference the decoder/Masking utilities and ensure
correct tensor shapes and numeric conversions instead of the current minLength
loop.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b9332272-1c41-4098-b81c-58ff65063ab6
📒 Files selected for processing (2)
src/NeuralNetworks/CapsuleNetwork.cssrc/NeuralNetworks/HTMNetwork.cs
…e changes Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
….com/ooples/AiDotNet into fix/review-comments-and-build-errors
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
ILayer<T> doesn't expose IsTrainingMode, so capture/restore pattern isn't possible. Simplified to just set eval mode before forward pass. Train() explicitly sets training mode when needed. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/HTMNetwork.cs`:
- Around line 495-502: HTMNetwork.Predict currently forces all layers into eval
by calling SetTrainingMode(false) without capturing prior states or restoring
them; update this overload (the Predict method that takes Tensor<T> input) to
mirror the other Predict(Vector<T>) implementation: first capture each layer's
current training mode (e.g., iterate Layers and read layer.IsTraining or
appropriate getter into a local list), then set training mode to false for each
layer, perform the forward pass, and in a finally block restore each layer's
training mode using the captured states so the method is side-effect free and
exceptions do not leave layers in eval mode; reference the methods/properties
Layers, SetTrainingMode, Forward and the Predict(Tensor<T> input) method name
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: b5b287e1-dfa9-4b72-8ef8-217db9f87788
📒 Files selected for processing (1)
src/NeuralNetworks/HTMNetwork.cs
LayerBase.IsTrainingMode is protected — not accessible from network-level code. Since Train() always explicitly sets training mode and Predict is always eval, save/restore is unnecessary. Simplified to just set eval mode before forward. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
ooples
changed the title
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/NeuralNetworks/HTMNetwork.cs (1)
495-503:⚠️ Potential issue | 🔴 CriticalBLOCKING:
Predict(Tensor<T>)now mutates training mode and doesn’t restore it.At Line 497, all layers are forced into eval mode, but there is no restoration path after Line 502 (including exceptions), so this method is stateful and can affect subsequent calls. Please restore prior mode in
finallyto keep prediction side-effect free.Proposed fix
public override Tensor<T> Predict(Tensor<T> input) { if (TryForwardGpuOptimized(input, out var gpuResult)) return gpuResult; - // Ensure eval mode for deterministic inference. Train() explicitly sets - // training mode when needed, so no restore is required here. - foreach (var layer in Layers) - layer.SetTrainingMode(false); - - Tensor<T> current = input; - foreach (var layer in Layers) - current = layer.Forward(current); - return current; + bool originalTrainingMode = IsTrainingMode; + SetTrainingMode(false); + try + { + Tensor<T> current = input; + foreach (var layer in Layers) + current = layer.Forward(current); + return current; + } + finally + { + SetTrainingMode(originalTrainingMode); + } }As per coding guidelines, “Simplified implementations… take shortcuts… are BLOCKING issues requiring immediate fix.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/NeuralNetworks/HTMNetwork.cs` around lines 495 - 503, Predict(Tensor<T>) currently forces all Layers into eval mode via Layer.SetTrainingMode(false) but never restores their previous training state, which makes the method stateful and exception-unsafe; modify Predict(Tensor<T>) to first capture each layer's current training mode (e.g., read a bool from each Layer or call a getter such as IsTrainingMode on the Layers collection), then wrap the forward pass in a try/finally and in the finally iterate the same Layers to restore their saved training flags by calling SetTrainingMode(originalValue) so the original per-layer training/eval states are restored even if Forward throws.
🤖 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/HTMNetwork.cs`:
- Around line 495-503: Predict(Tensor<T>) currently forces all Layers into eval
mode via Layer.SetTrainingMode(false) but never restores their previous training
state, which makes the method stateful and exception-unsafe; modify
Predict(Tensor<T>) to first capture each layer's current training mode (e.g.,
read a bool from each Layer or call a getter such as IsTrainingMode on the
Layers collection), then wrap the forward pass in a try/finally and in the
finally iterate the same Layers to restore their saved training flags by calling
SetTrainingMode(originalValue) so the original per-layer training/eval states
are restored even if Forward throws.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 94ad3e27-a411-4ec6-95cc-909ff5d633c9
📒 Files selected for processing (1)
src/NeuralNetworks/HTMNetwork.cs
Brings AiDotNet.Tensors #1085 and #1087: MathHelper.Tanh and the engine's strided Tanh, Mish, GELU and LSTM-cell copies returned NaN once e^2x overflowed (x > ~44 in float, ~5.5 in Half). CQL and IQL squashed an untrained policy mean through it and emitted NaN actions from the first step. OfflineAgentContinuousActionTests passes against this release. The native packages move in lockstep, as the file's notes require. Closes #2216 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Summary
ComputeAuxiliaryLoss()inside theUseAuxiliaryLosscheck to avoid unnecessary reconstruction forward pass when auxiliary loss is disabledTest plan
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Refactor