feat: SSM infrastructure - S6Scan, hybrid architectures, vision/language models (Issue #275) - #832
Conversation
…ion/language models, inference optimizations (Issue #275) Implements remaining phases of Issue #275: Phase 1: S6Scan standalone utility extracting selective scan from MambaBlock - Sequential and parallel (iterative doubling) scan algorithms - MambaBlock refactored to delegate scan ops to S6Scan Phase 3: Hybrid architectures and model assembly - HybridBlockScheduler with Jamba/Zamba/Samba/Custom patterns - Factory methods: CreateJambaSchedule, CreateZambaSchedule, CreateSambaSchedule - Zamba uses shared attention weights (single instance reused) - ScanPatterns: bidirectional, cross-scan, continuous, spatio-temporal - MambaLanguageModel: embedding + N×MambaBlocks + RMSNorm + LM head - VisionMambaModel: patch embed + scan + MambaBlocks + classifier Phase 4: Inference optimizations - SSMStateCache for autoregressive generation with O(1) per-token cost - MambaLanguageModel.GenerateStep() integrated with SSMStateCache - SSMQuantizationHelper for SSM-aware parameter quantization 352 integration tests pass on both net471 and net10.0. Closes #275 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughRefactors Mamba forecasting to use per-timestep 3D MambaBlock processing and adds a large SSM subsystem: S6Scan, Mamba/Mamba2 blocks, S4D, RWKV, GLA, RG‑LRU, ExtendedLSTM, GatedDeltaNet, HybridBlockScheduler, language/vision wrappers, SSM state cache & quantization, LayerHelper conv-kernel configurability, extensive tests, and multiple small API member-hiding edits. Some layers explicitly mark training/JIT disabled or contain simplified/backward scaffolds. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client/Input
participant Model as MambaLanguageModel / VisionModel
participant Scheduler as HybridBlockScheduler
participant Block as MambaBlock / SSM Block
participant State as SSMStateCache
participant Head as OutputProjection
Client->>Model: submit input (2D or 3D)
Model->>State: load per-layer cached states (GenerateStep path if autoreg)
Model->>Scheduler: normalize input to 3D and invoke scheduler
Scheduler->>Block: call each Block.Forward([batch,seq,dim])
Block->>State: read/update hidden states / conv buffers
Block-->>Scheduler: return block output (3D)
Scheduler-->>Model: aggregate residuals across blocks
Model->>Head: flatten to 2D, apply output projection(s)
Head-->>Client: logits / predictions
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120+ minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull request overview
This PR completes the remaining general-purpose SSM (State Space Model) infrastructure (Issue #275) by adding reusable scan utilities, state caching/quantization helpers, hybrid scheduling utilities, and reference model compositions (language + vision), along with extensive unit/integration tests. It also updates existing tabular/forecasting components to integrate the new SSM layer types and refactors Mamba-related code to reuse the shared scan implementation.
Changes:
- Added core SSM utilities:
S6Scan,ScanPatterns,SSMStateCache, andSSMQuantizationHelper, plus new SSM/linear-recurrence layers (e.g., GLA, RWKV, etc.) and model wrappers. - Refactored forecasting
Mamba<T>and default layer creation to use realMambaBlock<T>instead of prior dense “simulation”. - Added broad unit/integration test coverage for new SSM layers, models, scan patterns, and caching behavior.
Reviewed changes
Copilot reviewed 43 out of 43 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
src/NeuralNetworks/Layers/SSM/S6Scan.cs |
Adds reusable sequential + parallel selective scan utilities intended to be shared across Mamba-family layers. |
src/NeuralNetworks/Layers/SSM/ScanPatterns.cs |
Adds vision-oriented scan order utilities (bidirectional/cross/continuous/spatio-temporal) and a merge helper. |
src/NeuralNetworks/Layers/SSM/SSMStateCache.cs |
Adds autoregressive hidden-state/conv-buffer caching with optional min-max “compression”. |
src/NeuralNetworks/Layers/SSM/SSMQuantizationHelper.cs |
Adds SSM-focused parameter/state quantization helpers built on existing quantization abstractions. |
src/NeuralNetworks/Layers/SSM/GatedLinearAttentionLayer.cs |
Introduces a Gated Linear Attention (GLA) layer implementation. |
src/Helpers/LayerHelper.cs |
Updates default Mamba layer factory to emit real MambaBlock<T> stacks and adjust embedding/projection expectations. |
src/Finance/Forecasting/StateSpace/Mamba.cs |
Refactors the forecasting Mamba model to embed per-timestep, run 3D MambaBlock<T> layers, then flatten for output projection. |
src/NeuralNetworks/Tabular/*.cs (shown: TabTransformerNetwork.cs, TabRNetwork.cs, TabPFNNetwork.cs, TabNetNetwork.cs, TabMNetwork.cs, TabDPTNetwork.cs, SAINTNetwork.cs, NODENetwork.cs, MambularNetwork.cs, GANDALFNetwork.cs, FTTransformerNetwork.cs, AutoIntNetwork.cs) |
Adds new keyword to strongly-typed Options properties to explicitly hide the base Options. |
tests/AiDotNet.Tests/UnitTests/NeuralNetworks/Layers/SSM/*.cs (multiple) |
Adds comprehensive unit/integration test suites for new SSM layers and model compositions. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…gration, add SSMStateCache layer enumeration - Created SSMQuantizationHelperTests.cs with 23 tests covering: - QuantizeSSMLayer with/without D parameter protection - QuantizeStateCache with state migration verification - EstimateMemorySavings compression ratio validation - ComputeQuantizationError monotonicity and non-destructiveness - Double type support, null argument validation - Non-MambaBlock layer quantization - Fixed QuantizeStateCache to migrate existing cached states into the new compressed cache instead of returning an empty cache - Added GetSSMStateLayerIndices() and GetConvBufferLayerIndices() to SSMStateCache for proper state enumeration during migration All 375 SSM tests pass on both net471 and net10.0. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
CodeQL found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
- Add #nullable disable to 53 test/benchmark files for null-handling tests - Fix CS8604 in InterpretabilityDashboard.cs with pattern matching null check - Fix CS8602 in AttributionVisualizer.cs with pattern matching null checks - Fix CS8425 in MetaLearningTestInfrastructure.cs with EnumeratorCancellation - Fix CS8629 in ClusteringAdditionalMetricsIntegrationTests.cs with GetValueOrDefault - Fix CS0618 in DeploymentIntegrationTests.cs: BitWidth -> EffectiveBitWidth - Fix CS0618 in Phase2GateTests.cs: InitializationStrategy -> InitializationStrategies - Fix CS0618 in InterpretabilityIntegrationTests.cs with pragma warning disable Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 99 out of 99 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
GatedLinearAttentionLayer.cs: - Remove unused allStates tensor allocation and _lastStates field - Fix forward output computation: O[k] = sum_d Q[d] * S[d,k] (correct GLA equation) - Cache pre-output-projection attention output for backward pass - Implement full GLA backward: gradients for Q, K, V, gate projections with sigmoid derivative, state propagation through time SSMQuantizationHelper.cs: - Fix int overflow: use 1L << bitWidth with special-case for bitWidth >= 32 - Remove unused using AiDotNet.Tensors.Engines - Update QuantizeStateCache docs to reflect precision reduction vs memory compression SSMStateCache.cs: - Fix int overflow in CompressState: use long for levels calculation - Use Unsafe.SizeOf<T>() in GetMemoryUsageBytes instead of hardcoded sizeof(float) - Update docs to clarify compression is precision reduction, not memory reduction ScanPatterns.cs: - Clone first tensor in MergeScanOutputs to avoid mutating input Mamba.cs: - Validate seqLen == _contextLength in Forward to prevent shape mismatches - Store _lastForwardSeqLen and use it in Backward instead of _contextLength Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 99 out of 99 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
SSMStateCache.cs: - Update constructor param docs to reflect precision reduction, not memory savings - Treat bitWidth >= 32 as no-op in CompressState (clone instead of quantize) SSMQuantizationHelper.cs: - Use Unsafe.SizeOf<T>() in EstimateMemorySavings for correct T-aware byte size - Treat bitWidth >= 32 as no-op in QuantizeVector (return copy instead of quantize) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 63
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (40)
tests/AiDotNet.Tests/IntegrationTests/GaussianProcesses/GPInferenceCorrectnessTests.cs (1)
218-231:⚠️ Potential issue | 🔴 CriticalBLOCKING: Faulty assertion logic allows positive values to pass.
The assertion
logLik < 0 || double.IsNaN(logLik) == falsedoes NOT enforce negativity. WhenlogLikis positive (e.g., 5.0):
5.0 < 0→falsedouble.IsNaN(5.0) == false→true- Result:
false || true→ passesThis test will pass for any finite value, completely defeating its stated purpose. The message claims "should be negative and finite" but the logic only enforces "finite."
🐛 Proposed fix
- // Log-likelihood should be negative (probability < 1) - Assert.True(logLik < 0 || double.IsNaN(logLik) == false, - $"Log-likelihood should be negative and finite, got {logLik}"); + // Log-likelihood should be negative (probability < 1) and finite + Assert.False(double.IsNaN(logLik), $"Log-likelihood should be finite, got {logLik}"); + Assert.False(double.IsInfinity(logLik), $"Log-likelihood should be finite, got {logLik}"); + Assert.True(logLik < 0, $"Log-likelihood should be negative, got {logLik}");tests/AiDotNet.Tests/UnitTests/RAG/Embeddings/VoyageAIEmbeddingModelTests.cs (1)
311-321:⚠️ Potential issue | 🟠 MajorBLOCKING: Trivial test with only
NotNullassertions — verifies nothing about input types.This test claims to verify "different input types" but only checks that the objects were created (NotNull). The constructor's ability to create instances is already covered by
Constructor_WithValidParameters_CreatesInstance.What should this test actually verify?
- That the input type is stored and retrievable (e.g., via a property)
- That "document" vs "query" input types produce different embeddings for the same text (if that's the expected behavior)
- Or that the input type is passed correctly to the underlying API
As written, this test adds zero value and will always pass regardless of whether input types work correctly.
🔥 Proposed fix: Add meaningful assertions or remove the test
If input types affect embedding behavior:
[Fact] public void Constructor_WithDifferentInputTypes_CreatesInstances() { // Arrange & Act var documentModel = new VoyageAIEmbeddingModel<double>("test-api-key", "voyage-model-path.onnx", "document", 1024); var queryModel = new VoyageAIEmbeddingModel<double>("test-api-key", "voyage-model-path.onnx", "query", 1024); + var text = "Test input type behavior"; // Assert - Assert.NotNull(documentModel); - Assert.NotNull(queryModel); + // Verify input types are accessible + Assert.Equal("document", documentModel.InputType); + Assert.Equal("query", queryModel.InputType); + + // If input types should affect embeddings, verify that: + var docEmbedding = documentModel.Embed(text); + var queryEmbedding = queryModel.Embed(text); + // Add appropriate assertions based on expected behavior }If
InputTypeproperty doesn't exist and input types don't affect behavior, delete this test entirely — it provides no coverage.tests/AiDotNet.Tests/IntegrationTests/CrossValidators/StratifiedKFoldCrossValidatorIntegrationTests.cs (4)
64-72:⚠️ Potential issue | 🔴 CriticalBLOCKING: Trivial assertion - only checks non-null.
This test only verifies
Assert.NotNull(validator)without checking any actual constructor behavior. A production-quality test must verify that the validator is properly configured with default options (e.g., correct NumberOfFolds, ShuffleData settings).💪 Strengthen the test with meaningful assertions
[Fact] public void Constructor_WithDefaultOptions_CreatesValidator() { // Act var validator = new StratifiedKFoldCrossValidator<double, Matrix<double>, Vector<double>, double>(); // Assert Assert.NotNull(validator); + + // Verify default behavior by running a simple validation + var model = CreateMockModel(); + var optimizer = new PassthroughOptimizer<double, Matrix<double>, Vector<double>>(model); + var X = CreateTestMatrix(50, 2); + var y = CreateClassLabels([25, 25]); + + var result = validator.Validate(model, X, y, optimizer); + + // Default should be 5 folds based on standard k-fold conventions + Assert.Equal(5, result.FoldResults.Count); }As per coding guidelines: "Tests MUST be production-quality. Flag ALL of the following as blocking issues: Trivial assertions: Tests that only check
Assert.NotNullwhen they should verify actual behavior/values."
74-90:⚠️ Potential issue | 🔴 CriticalBLOCKING: Trivial assertion - doesn't verify options were applied.
This test creates a validator with custom options (NumberOfFolds=3, ShuffleData=false, RandomSeed=42) but only checks
Assert.NotNull(validator). It never verifies that these options actually affect the validator's behavior.💪 Verify that custom options are actually respected
[Fact] public void Constructor_WithCustomOptions_CreatesValidator() { // Arrange var options = new CrossValidationOptions { NumberOfFolds = 3, ShuffleData = false, RandomSeed = 42 }; // Act var validator = new StratifiedKFoldCrossValidator<double, Matrix<double>, Vector<double>, double>(options); // Assert Assert.NotNull(validator); + + // Verify the custom options are actually used + var model = CreateMockModel(); + var optimizer = new PassthroughOptimizer<double, Matrix<double>, Vector<double>>(model); + var X = CreateTestMatrix(60, 2); + var y = CreateClassLabels([30, 30]); + + var result = validator.Validate(model, X, y, optimizer); + + // Verify NumberOfFolds option was applied + Assert.Equal(3, result.FoldResults.Count); }As per coding guidelines: "Tests MUST be production-quality. Flag ALL of the following as blocking issues: Trivial assertions: Tests that only check
Assert.NotNullwhen they should verify actual behavior/values."
271-308:⚠️ Potential issue | 🔴 CriticalBLOCKING: Reproducibility test doesn't verify reproducibility.
This test claims to verify that the same seed produces reproducible results, but it only checks that
ValidationIndices.Lengthis equal—not that the actual indices are identical. The comment on lines 302-303 even admits "the actual values might differ," which defeats the purpose of a reproducibility test. With the same seed, the results should be exactly identical, not just the same length.🔧 Fix the reproducibility test to verify actual equality
[Fact] public void Validate_WithSameSeed_ProducesReproducibleResults() { // Arrange var options = new CrossValidationOptions { NumberOfFolds = 3, ShuffleData = true, RandomSeed = 42 }; var model1 = CreateMockModel(); var model2 = CreateMockModel(); var optimizer1 = new PassthroughOptimizer<double, Matrix<double>, Vector<double>>(model1); var optimizer2 = new PassthroughOptimizer<double, Matrix<double>, Vector<double>>(model2); var X = CreateTestMatrix(60, 2); var y = CreateClassLabels([30, 30]); // Act var validator1 = new StratifiedKFoldCrossValidator<double, Matrix<double>, Vector<double>, double>(options); var result1 = validator1.Validate(model1, X, y, optimizer1); var validator2 = new StratifiedKFoldCrossValidator<double, Matrix<double>, Vector<double>, double>(options); var result2 = validator2.Validate(model2, X, y, optimizer2); // Assert for (int i = 0; i < result1.FoldResults.Count; i++) { Assert.NotNull(result1.FoldResults[i].ValidationIndices); Assert.NotNull(result2.FoldResults[i].ValidationIndices); - // With same seed, validation indices should be in same order - // (though the actual values might differ due to stratification algorithm) - Assert.Equal( - result1.FoldResults[i].ValidationIndices.Length, - result2.FoldResults[i].ValidationIndices.Length); + // With same seed, validation indices must be EXACTLY the same + Assert.Equal(result1.FoldResults[i].ValidationIndices, result2.FoldResults[i].ValidationIndices); + Assert.Equal(result1.FoldResults[i].TrainingIndices, result2.FoldResults[i].TrainingIndices); } }As per coding guidelines: "Tests MUST be production-quality. Flag ALL of the following as blocking issues: Tests with conditional assertions that skip verification when things fail."
314-340:⚠️ Potential issue | 🔴 CriticalBLOCKING: Mostly trivial assertions - doesn't verify fold correctness.
This test primarily consists of
Assert.NotNullchecks (lines 329-330, 335-338) with only one meaningful assertion (TotalTime > TimeSpan.Zero on line 331). A production-quality test should verify that the fold structure is actually correct: training + validation indices cover all samples exactly once per fold, indices are within valid ranges, predictions have correct dimensions, etc.💪 Add meaningful assertions for fold result validation
[Fact] public void Validate_ReturnsValidFoldResults() { // Arrange var options = new CrossValidationOptions { NumberOfFolds = 3 }; var validator = new StratifiedKFoldCrossValidator<double, Matrix<double>, Vector<double>, double>(options); var model = CreateMockModel(); var optimizer = new PassthroughOptimizer<double, Matrix<double>, Vector<double>>(model); var X = CreateTestMatrix(60, 2); var y = CreateClassLabels([30, 30]); // Act var result = validator.Validate(model, X, y, optimizer); // Assert Assert.NotNull(result); Assert.NotNull(result.FoldResults); Assert.True(result.TotalTime > TimeSpan.Zero); foreach (var foldResult in result.FoldResults) { Assert.NotNull(foldResult.TrainingIndices); Assert.NotNull(foldResult.ValidationIndices); Assert.NotNull(foldResult.ActualValues); Assert.NotNull(foldResult.PredictedValues); + + // Verify indices are valid and non-overlapping + Assert.True(foldResult.TrainingIndices.Length > 0); + Assert.True(foldResult.ValidationIndices.Length > 0); + Assert.All(foldResult.TrainingIndices, idx => Assert.InRange(idx, 0, 59)); + Assert.All(foldResult.ValidationIndices, idx => Assert.InRange(idx, 0, 59)); + + // Verify train + validation cover all samples exactly once + var allIndices = foldResult.TrainingIndices.Concat(foldResult.ValidationIndices).OrderBy(x => x).ToArray(); + Assert.Equal(60, allIndices.Length); + + // Verify predictions match validation set size + Assert.Equal(foldResult.ValidationIndices.Length, foldResult.PredictedValues.Count); + Assert.Equal(foldResult.ValidationIndices.Length, foldResult.ActualValues.Count); } }As per coding guidelines: "Tests MUST be production-quality. Flag ALL of the following as blocking issues: Trivial assertions: Tests that only check
Assert.NotNullwhen they should verify actual behavior/values."tests/AiDotNet.Tests/IntegrationTests/DistributedTraining/DistributedTrainingIntegrationTests.cs (4)
291-311:⚠️ Potential issue | 🟠 MajorBLOCKING: Trivial
Assert.True(true)assertion does not verify behavior.This test uses
Assert.True(true)which will always pass regardless of whatBarrier()actually does. The test should verify observable side effects or state changes after the barrier completes. If there's no observable state to verify, consider whether this test adds value or if it should be removed/replaced with a multi-process test that actually exercises synchronization.🔧 Suggested fix
try { // Act - should not block for single process backend.Barrier(); - // Assert - if we reach here, it worked - Assert.True(true); + // Assert - verify backend is still in valid state after barrier + Assert.True(backend.IsInitialized); } finally
150-164:⚠️ Potential issue | 🟠 MajorBLOCKING: Test has no assertions - it will always pass.
This test calls
ClearEnvironmentbut never asserts anything about the result. The comment "no exception thrown" is not a valid test strategy—if you need to verify an operation doesn't throw, usevar exception = Record.Exception(() => ...)and assert on it. Better yet, verify the environment was actually cleared.🔧 Suggested fix
[Fact] public void InMemoryCommunicationBackend_ClearEnvironment_StaticCleanup() { // Arrange var envId = Guid.NewGuid().ToString(); var backend = new InMemoryCommunicationBackend<double>(rank: 0, worldSize: 1, environmentId: envId); backend.Initialize(); backend.Shutdown(); - // Act - static cleanup method - InMemoryCommunicationBackend<double>.ClearEnvironment(envId); - - // Assert - no exception thrown + // Act & Assert - static cleanup should not throw + var exception = Record.Exception(() => + InMemoryCommunicationBackend<double>.ClearEnvironment(envId)); + + Assert.Null(exception); + + // Verify: re-creating backend with same envId should work (environment was cleared) + var newBackend = new InMemoryCommunicationBackend<double>(rank: 0, worldSize: 1, environmentId: envId); + Assert.NotNull(newBackend); }
511-529:⚠️ Potential issue | 🟠 MajorBLOCKING: Tests only verify
Assert.NotNullbut should verify construction parameters.Both
ParameterAnalyzer_ValidConstructionandParameterAnalyzer_DefaultConstructionpass specific constructor arguments but only assert that the result is not null. Tests should verify that the analyzer was actually configured with the provided values. IfParameterAnalyzerdoesn't expose these as properties, consider whether these tests provide meaningful coverage.🔧 Suggested approach
If
ParameterAnalyzerexposesMinimumGroupSizeandWorldSizeproperties:[Fact] public void ParameterAnalyzer_ValidConstruction() { // Act var analyzer = new ParameterAnalyzer<double>(minimumGroupSize: 1024, worldSize: 4); - // Assert - no exception means success - Assert.NotNull(analyzer); + // Assert - verify construction parameters are set + Assert.NotNull(analyzer); + // TODO: Add assertions for MinimumGroupSize and WorldSize if exposed + // Assert.Equal(1024, analyzer.MinimumGroupSize); + // Assert.Equal(4, analyzer.WorldSize); }If properties aren't exposed, verify behavior indirectly by calling
AnalyzeParameterswith known input and checking the result reflects the configuredminimumGroupSize.
1262-1289:⚠️ Potential issue | 🟠 MajorBLOCKING: Trivial
Assert.True(true)assertion does not verify behavior.Same issue as the other Barrier test—this assertion will always pass. Verify the
CommunicationManagerremains in a valid initialized state after the barrier call.🔧 Suggested fix
// Act - should not throw for single process CommunicationManager.Barrier<double>(); - // Assert - if we reach here, it worked - Assert.True(true); + // Assert - verify manager is still in valid state after barrier + Assert.True(CommunicationManager.IsInitialized); }tests/AiDotNet.Tests/UnitTests/Genetics/ModelIndividualTests.cs (3)
337-348:⚠️ Potential issue | 🔴 CriticalBLOCKING: Test only checks "does not throw" without verifying behavior.
This test has no meaningful assertions—it only verifies that calling
SetActiveFeatureIndicesdoesn't throw an exception. Per the test quality guidelines, tests that skip verification of actual behavior are production blockers.The test should verify that:
- The active feature indices were actually updated in the underlying model
- Subsequent calls to
GetActiveFeatureIndices()return the updated indices- Or, if the mock intentionally does nothing, document why this is acceptable behavior
🔧 Proposed fix to add meaningful assertions
[Fact] public void SetActiveFeatureIndices_DelegatesToInnerModel() { // Arrange var model = new MockModel(); var genes = new List<ModelParameterGene> { new ModelParameterGene(0.1) }; var individual = new ModelIndividual<double, double[], double[], ModelParameterGene>(model, genes, CreateMockModel); var newIndices = new[] { 0, 2 }; - // Act & Assert - Should not throw + // Act individual.SetActiveFeatureIndices(newIndices); + + // Assert - Verify the indices were actually set + var activeIndices = individual.GetActiveFeatureIndices().ToArray(); + Assert.Equal(newIndices, activeIndices); }Note: This requires updating the
MockModel.SetActiveFeatureIndicesimplementation to actually store the indices rather than being a no-op.
471-486:⚠️ Potential issue | 🔴 CriticalBLOCKING: Test doesn't verify caching mechanism.
This test claims to verify that
ParameterCountis cached, but it only checks that calling the property twice returns the same value—which would be true even without any caching. This is a trivial assertion that doesn't validate the actual caching behavior.To properly test caching, you need to verify that:
- The underlying expensive operation (e.g.,
GetParameters().Length) is only called once- Subsequent calls return the cached value without re-invoking the expensive operation
- This typically requires either instrumentation, a spy/mock, or measuring side effects
🔧 Recommended approach
Either:
- Remove this test if caching is an internal optimization detail that doesn't affect observable behavior
- Add instrumentation to MockModel to track how many times
GetParameters()is called and assert it's only called once- Rename and simplify to
ParameterCount_ReturnsConsistentValueif you only want to verify consistencyThe current test name is misleading because it doesn't actually verify caching.
624-648:⚠️ Potential issue | 🔴 CriticalBLOCKING: Test only verifies count, not actual gene values or model update.
This test claims to verify that
SetGenesupdates both genes and the model, but it only asserts that the gene collection count changed to 3. It doesn't verify:
- The actual gene values (0.5, 0.6, 0.7) were stored
- The underlying model was reconstructed or updated with the new genes
- The model's parameters reflect the new gene configuration
Per the test quality guidelines, tests must assert specific expected values, not just collection sizes.
🛠️ Proposed fix to verify actual gene values
[Fact] public void SetGenes_UpdatesGenesAndModel() { // Arrange var initialGenes = new List<ModelParameterGene> { new ModelParameterGene(0.1), new ModelParameterGene(0.2) }; var individual = new ModelIndividual<double, double[], double[], ModelParameterGene>(initialGenes, CreateMockModel); var newGenes = new List<ModelParameterGene> { new ModelParameterGene(0.5), new ModelParameterGene(0.6), new ModelParameterGene(0.7) }; // Act individual.SetGenes(newGenes); // Assert var retrievedGenes = individual.GetGenes(); Assert.Equal(3, retrievedGenes.Count); + Assert.Equal(0.5, retrievedGenes[0].Value); + Assert.Equal(0.6, retrievedGenes[1].Value); + Assert.Equal(0.7, retrievedGenes[2].Value); + + // Verify the model was updated (parameter count should match gene count) + Assert.Equal(3, individual.ParameterCount); }tests/AiDotNet.Tests/UnitTests/FitnessCalculators/CosineSimilarityLossFitnessCalculatorTests.cs (4)
393-412:⚠️ Potential issue | 🟠 MajorBLOCKING: Trivial assertion that will always pass.
The assertion
Assert.True(result >= 0.0 && result <= 2.0)verifies nothing meaningful. By mathematical definition, cosine similarity loss is always in the range [0, 2]. Any functioning implementation will pass this test. The test name claims "ReturnsCorrectValue" but doesn't assert any specific value.Calculate the expected value and assert it explicitly:
Proposed fix with specific expected value
// Assert - // Dot product = 0.42 + 0.30 + 0.12 + 0.56 + 0.02 = 1.42 - // Should handle feature vector comparison correctly - Assert.True(result >= 0.0 && result <= 2.0); + // Dot product = 0.7*0.6 + 0.5*0.6 + 0.3*0.4 + 0.8*0.7 + 0.2*0.1 = 0.42 + 0.30 + 0.12 + 0.56 + 0.02 = 1.42 + // Norm predicted = sqrt(0.49 + 0.25 + 0.09 + 0.64 + 0.04) = sqrt(1.51) ≈ 1.2288 + // Norm actual = sqrt(0.36 + 0.36 + 0.16 + 0.49 + 0.01) = sqrt(1.38) ≈ 1.1747 + // Cosine similarity = 1.42 / (1.2288 * 1.1747) ≈ 0.9835 + // Loss = 1 - 0.9835 ≈ 0.0165 + Assert.Equal(0.016536458645083694, result, 10);
414-431:⚠️ Potential issue | 🟠 MajorBLOCKING: Trivial assertion identical to the previous test.
Same issue:
Assert.True(result >= 0.0 && result <= 2.0)is trivially true for any cosine loss implementation. The test claims to verify handling of "very small values" without numerical instability, but doesn't actually verify the computed result is correct.Proposed fix with specific expected value
// Assert // Should handle small values without numerical instability - Assert.True(result >= 0.0 && result <= 2.0); + // Dot product = 0.001*0.001 + 0.002*0.001 + 0.001*0.002 = 0.000001 + 0.000002 + 0.000002 = 0.000005 + // Norm predicted = sqrt(0.000001 + 0.000004 + 0.000001) = sqrt(0.000006) ≈ 0.002449 + // Norm actual = sqrt(0.000001 + 0.000001 + 0.000004) = sqrt(0.000006) ≈ 0.002449 + // Cosine similarity = 0.000005 / (0.002449 * 0.002449) ≈ 0.8333 + // Loss = 1 - 0.8333 ≈ 0.1667 + Assert.Equal(0.16666666666666663, result, 10);
373-391:⚠️ Potential issue | 🟡 MinorTest name mismatch: claims "ReturnsCorrectValue" but uses loose bound.
The test is named
RecommendationSystemScenario_ReturnsCorrectValuebut only assertsresult < 0.1. Either rename toReturnsLowLossor compute and assert the specific expected value to match the test name.Proposed fix with specific expected value
// Assert - // Should show high similarity (low loss) for similar preferences - Assert.True(result < 0.1); // Less than 10% loss + // Dot product = 0.9*0.8 + 0.1*0.2 + 0.5*0.6 + 0.3*0.2 = 0.72 + 0.02 + 0.30 + 0.06 = 1.10 + // Norm predicted = sqrt(0.81 + 0.01 + 0.25 + 0.09) = sqrt(1.16) ≈ 1.0770 + // Norm actual = sqrt(0.64 + 0.04 + 0.36 + 0.04) = sqrt(1.08) ≈ 1.0392 + // Cosine similarity = 1.10 / (1.0770 * 1.0392) ≈ 0.9828 + // Loss = 1 - 0.9828 ≈ 0.0172 + Assert.Equal(0.017168319946829025, result, 10);
458-476:⚠️ Potential issue | 🟡 MinorTest name mismatch: claims "ReturnsCorrectValue" but uses loose bound.
Same issue as the recommendation scenario test. Named
TextEmbeddingScenario_ReturnsCorrectValuebut only assertsresult < 0.15.Proposed fix with specific expected value
// Assert - // Should indicate high similarity for semantically similar embeddings - Assert.True(result < 0.15); // Less than 15% loss + // Dot product = 0.8*0.7 + 0.6*0.7 + (-0.2)*(-0.1) + 0.9*0.8 + (-0.1)*(-0.2) = 0.56 + 0.42 + 0.02 + 0.72 + 0.02 = 1.74 + // Norm predicted = sqrt(0.64 + 0.36 + 0.04 + 0.81 + 0.01) = sqrt(1.86) ≈ 1.3638 + // Norm actual = sqrt(0.49 + 0.49 + 0.01 + 0.64 + 0.04) = sqrt(1.67) ≈ 1.2923 + // Cosine similarity = 1.74 / (1.3638 * 1.2923) ≈ 0.9873 + // Loss = 1 - 0.9873 ≈ 0.0127 + Assert.Equal(0.012670117712306658, result, 10);tests/AiDotNet.Tests/Performance/Phase2GateTests.cs (3)
386-414: 🧹 Nitpick | 🔵 TrivialPerformance comparison test may be flaky in CI environments.
Elapsed time comparisons can fail on overloaded CI runners. Consider adding a tolerance margin or using median of multiple runs if this proves flaky. For a gate test, documenting the expected behavior difference is valuable, but strict assertions on timing can be brittle.
416-438:⚠️ Potential issue | 🟠 MajorBLOCKING: Dead code -
eagerLayeris declared but never used.The test creates
eagerLayer(line 419-420) but never references it. This looks like either:
- An incomplete test that intended to compare lazy vs eager output
- Dead code that should be removed
The test name "ProducesCorrectOutput" implies verification against expected behavior, but the test only checks
NotNulland shape dimensions without validating actual output values or comparing against the eager layer.🔧 Proposed fix: Either complete the comparison or remove dead code
Option A: Remove unused variable if comparison isn't needed:
public void DenseLayer_LazyInit_ProducesCorrectOutput() { - var eagerLayer = new DenseLayer<float>(10, 5, - initializationStrategy: InitializationStrategies<float>.Zero); var lazyLayer = new DenseLayer<float>(10, 5, initializationStrategy: InitializationStrategies<float>.Lazy);Option B: Complete the comparison test if that was the intent:
var output = lazyLayer.Forward(input); + var eagerOutput = eagerLayer.Forward(input); Assert.NotNull(output); + Assert.NotNull(eagerOutput); Assert.Equal(5, output.Shape[^1]); Assert.True(lazyLayer.IsInitialized); + // Verify outputs have same shape + Assert.Equal(eagerOutput.Shape[^1], output.Shape[^1]);
515-548: 🧹 Nitpick | 🔵 TrivialSame timing-based assertion flakiness concern applies here.
See earlier comment on
DenseLayer_LazyInit_ConstructsFaster. Consider similar adjustments for CI stability.tests/AiDotNet.Tests/UnitTests/FitnessCalculators/PoissonLossFitnessCalculatorTests.cs (3)
14-34:⚠️ Potential issue | 🟠 MajorBLOCKING: Trivial assertion that doesn't verify actual behavior.
This test is named
ReturnsMinimumLossbut only asserts the result is finite. The Poisson loss formulapredicted - actual * log(predicted)produces a known computable value for these inputs. You should assert the expected value, not just that it's a number.The same issue affects these tests:
CalculateFitnessScore_WithDifferentPredictions_ReturnsValue(lines 36-52)CalculateFitnessScore_WithCountData_HandlesCorrectly(lines 54-71)CalculateFitnessScore_WithLargeCounts_WorksCorrectly(lines 73-89)CalculateFitnessScore_WithSmallPositiveValues_HandlesCorrectly(lines 91-108)CalculateFitnessScore_WithFloatType_WorksCorrectly(lines 146-162)CalculateFitnessScore_WithMixedCounts_WorksCorrectly(lines 214-231)A completely broken implementation returning
42.0for every input would pass all these tests.Example fix for the first test
// Assert - Result should be finite and well-defined (exact value depends on implementation) -Assert.True(!double.IsNaN(result) && !double.IsInfinity(result)); +// For Poisson loss: sum(predicted - actual * log(predicted)) / n +// = (2 - 2*ln(2) + 3 - 3*ln(3) + 5 - 5*ln(5) + 7 - 7*ln(7)) / 4 +var expected = (2.0 - 2.0 * Math.Log(2.0) + 3.0 - 3.0 * Math.Log(3.0) + + 5.0 - 5.0 * Math.Log(5.0) + 7.0 - 7.0 * Math.Log(7.0)) / 4.0; +Assert.Equal(expected, result, 10);
233-263:⚠️ Potential issue | 🟠 MajorBLOCKING: Missing critical assertion - test always passes.
The test name and comment on line 259 explicitly state "result1 should be lower" but this is never asserted. The test only checks that values are finite, making it an always-passing test that provides zero verification of the claimed behavior.
Proposed fix
// Assert - Both should be finite and result2 should not be smaller (poor predictions shouldn't be better) Assert.True(!double.IsNaN(result1) && !double.IsNaN(result2)); -// Note: With Poisson loss, matched predictions minimize loss, so result1 should be lower -// However, the exact comparison depends on the Poisson loss formula behavior -// Let's just verify both values are finite -Assert.True(!double.IsInfinity(result1) && !double.IsInfinity(result2)); +Assert.True(!double.IsInfinity(result1) && !double.IsInfinity(result2)); +// With Poisson loss, matched predictions minimize loss, so result1 MUST be lower +Assert.True(result1 < result2, $"Expected better predictions ({result1}) to have lower loss than poor predictions ({result2})");
174-194:⚠️ Potential issue | 🟡 MinorConstructor tests don't verify what they claim.
Both
Constructor_WithTrainingDataSetType_SetsCorrectlyandConstructor_WithTestDataSetType_SetsCorrectlyclaim to verify the DataSetType is set correctly, but neither test actually verifies the DataSetType. They only checkNotNullandIsHigherScoreBetter.If the calculator exposes a
DataSetTypeproperty, assert it. If not, test the behavior difference (e.g., passModelEvaluationDatawith both Training and Test sets and verify the correct one is used).tests/AiDotNet.Tests/UnitTests/FitnessCalculators/ContrastiveLossFitnessCalculatorTests.cs (2)
27-36:⚠️ Potential issue | 🟡 MinorTest claims to verify custom margin but doesn't.
The test name
Constructor_WithCustomMargin_UsesSpecifiedMarginstates it verifies the margin is set to 2.0, but the only assertion isAssert.False(calculator.IsHigherScoreBetter). Either expose the margin property and assert it, or rename the test to reflect what it actually verifies.The same applies to
Constructor_WithTrainingDataSetType_UsesTraining(lines 38-47).
382-400:⚠️ Potential issue | 🟡 MinorWeak assertion: test name promises "CorrectValue" but only checks non-negativity.
The test
CalculateFitnessScore_SignatureVerificationScenario_ReturnsCorrectValueonly assertsresult >= 0.0. This is a trivial assertion that any semi-functional implementation would pass.Given the detailed comment explaining the scenario, compute the expected loss value and assert it like the other tests in this file do (e.g.,
Assert.Equal(expectedValue, result, 10)orAssert.True(result >= X && result <= Y)).tests/AiDotNet.Tests/UnitTests/RAG/Embeddings/MultiModalEmbeddingModelTests.cs (2)
77-97:⚠️ Potential issue | 🔴 CriticalWeak assertion: test doesn't verify unnormalized behavior.
The test
Embed_WithoutNormalization_ReturnsUnnormalizedVectorclaims to verify that the vector is NOT normalized (comment line 94), but only assertsmagnitude > 0(line 96). This assertion passes even for normalized vectors, making it ineffective at verifying the intended behavior.As per coding guidelines, trivial assertions that don't verify actual behavior should be flagged as blocking issues.
🐛 Strengthen assertion to verify unnormalized state
magnitude = Math.Sqrt(magnitude); - // Should not be normalized to 1.0 when normalization is false - // We can't assert exact value, but we verify it's generated - Assert.True(magnitude > 0); + // Should not be normalized to 1.0 when normalization is false + Assert.True(magnitude > 0, "Magnitude should be positive"); + Assert.NotEqual(1.0, magnitude, 5); // Verify it's NOT normalized }
14-19:⚠️ Potential issue | 🔴 CriticalImage tests use fake text file instead of real image data.
CreateTempImageFile()creates a plain text file containing "fake image content" rather than a valid image file (PNG, JPEG, etc.). All image embedding tests (lines 120-346) use this helper, meaning they're testing with invalid image data.This is non-production test quality. Tests should either:
- Use real minimal image files (e.g., 1×1 PNG), or
- Make it explicit that
MultiModalEmbeddingModelis mocked/stubbed to accept text files in test modeAs per coding guidelines, non-production-ready test infrastructure should be flagged as blocking.
Run the following to check if MultiModalEmbeddingModel has test-mode stubbing:
#!/bin/bash # Description: Check if MultiModalEmbeddingModel has internal test stubs or mocking # Search for test-related stubbing in the implementation rg -A 10 'class MultiModalEmbeddingModel' --type cs rg 'test-model-path|fake.*image' --type cs -g '!tests/**'tests/AiDotNet.Tests/UnitTests/RAG/Embeddings/ONNXSentenceTransformerTests.cs (3)
101-113:⚠️ Potential issue | 🔴 CriticalTrivial assertions: verify actual embedding behavior, not just structure.
This test only checks that a non-null vector of the correct length is returned. It does not verify that the embedding contains meaningful values. According to test quality guidelines, tests that only check
Assert.NotNullwhen they should verify actual behavior are considered trivial and must be flagged as blocking issues.Strengthen this test by asserting:
- At least one element is non-zero (embedding is not empty)
- No elements are NaN or infinite
- Values are within a reasonable range (e.g., normalized embeddings should be in [-1, 1])
- Or verify specific expected properties of the embedding algorithm
🧪 Suggested enhancement
// Assert Assert.NotNull(embedding); Assert.Equal(384, embedding.Length); + +// Verify embedding contains meaningful values +Assert.True(embedding.Any(v => Math.Abs(v) > 1e-10), "Embedding should contain non-zero values"); +Assert.All(embedding, v => Assert.False(double.IsNaN(v) || double.IsInfinity(v)));As per coding guidelines: Tests MUST be production-quality and must verify actual behavior, not just structure.
209-222:⚠️ Potential issue | 🔴 CriticalTrivial assertions: verify actual batch embedding behavior.
Similar to the single embedding test, this only verifies the matrix structure (rows and columns) without checking that meaningful embeddings were produced.
Add assertions to verify:
- Matrix contains non-zero values
- No NaN or infinite values
- Each row represents a valid embedding (could check normalization if applicable)
🧪 Suggested enhancement
// Assert Assert.NotNull(embeddings); Assert.Equal(3, embeddings.Rows); Assert.Equal(384, embeddings.Columns); + +// Verify embeddings contain meaningful values +var hasNonZero = false; +for (int i = 0; i < embeddings.Rows; i++) +{ + for (int j = 0; j < embeddings.Columns; j++) + { + var val = embeddings[i, j]; + Assert.False(double.IsNaN(val) || double.IsInfinity(val)); + if (Math.Abs(val) > 1e-10) hasNonZero = true; + } +} +Assert.True(hasNonZero, "Batch embeddings should contain non-zero values");As per coding guidelines: Tests MUST be production-quality and must verify actual behavior, not just structure.
348-409:⚠️ Potential issue | 🔴 CriticalDuplicate coverage: multiple tests verify the same trivial assertion.
These three tests (
Embed_WithTextContainingMultipleWords_ReturnsEmbedding,Embed_WithPunctuationMarks_ReturnsEmbedding,Embed_WithLongText_ReturnsEmbedding) all verify the exact same behavior asEmbed_WithValidText_ReturnsVectorOfCorrectDimension(lines 101-113): that a non-null vector of length 384 is returned. Varying the input text doesn't add meaningful coverage when you're only asserting structure.Either:
- Remove these duplicate tests (preferred if no additional behavior is being verified)
- Enhance them to verify input-specific behavior, such as:
- Long text: verify truncation/handling of text longer than maxTokens
- Punctuation: verify that punctuation is properly tokenized/handled
- Multiple words: verify sentence-level vs word-level embedding differences
As per coding guidelines: Tests must not have duplicate coverage. Multiple tests that verify the exact same thing should be consolidated.
tests/AiDotNet.Tests/UnitTests/FitnessCalculators/DiceLossFitnessCalculatorTests.cs (3)
28-35:⚠️ Potential issue | 🟡 MinorTest doesn't verify what its name implies.
Constructor_WithTrainingDataSetType_UsesTrainingonly checksIsHigherScoreBetter, which is identical to the default constructor test (line 18-25). The test name implies verification that the Training dataset type is actually used, but no such verification exists. Either rename the test to reflect what it actually checks, or add meaningful assertions that confirm the Training dataset type is selected and used in calculations.💡 Consider adding verification or renaming
Option 1: Add meaningful verification (requires testing actual dataset selection behavior):
[Fact] public void Constructor_WithTrainingDataSetType_UsesTraining() { // Arrange & Act var calculator = new DiceLossFitnessCalculator<double, Vector<double>, Vector<double>>(DataSetType.Training); - - // Assert - Assert.False(calculator.IsHigherScoreBetter); + var evaluationData = new ModelEvaluationData<double, Vector<double>, Vector<double>> + { + TrainingSet = new DataSetStats<double, Vector<double>, Vector<double>> + { + Predicted = new Vector<double>(new double[] { 1.0 }), + Actual = new Vector<double>(new double[] { 1.0 }) + }, + ValidationSet = new DataSetStats<double, Vector<double>, Vector<double>> + { + Predicted = new Vector<double>(new double[] { 0.0 }), + Actual = new Vector<double>(new double[] { 1.0 }) + } + }; + + // Act + var result = calculator.CalculateFitnessScore(evaluationData); + + // Assert - should use TrainingSet (perfect match) not ValidationSet (no overlap) + Assert.Equal(0.0, result, 10); }Option 2: Rename to reflect actual behavior:
-public void Constructor_WithTrainingDataSetType_UsesTraining() +public void Constructor_WithTrainingDataSetType_InitializesCorrectly()
260-267:⚠️ Potential issue | 🟡 MinorDuplicate test coverage - remove redundant test.
IsHigherScoreBetter_ReturnsFalseduplicates the assertion already verified inConstructor_WithDefaultDataSetType_UsesValidation(line 24) andConstructor_WithTrainingDataSetType_UsesTraining(line 34). Multiple tests checking the exact same property with no variation add no value and clutter the test suite.🗑️ Proposed fix: remove redundant test
-[Fact] -public void IsHigherScoreBetter_ReturnsFalse() -{ - // Arrange - var calculator = new DiceLossFitnessCalculator<double, Vector<double>, Vector<double>>(); - - // Assert - Assert.False(calculator.IsHigherScoreBetter); -} -
324-340:⚠️ Potential issue | 🟡 MinorTrivial assertion - verify specific expected value.
CalculateFitnessScore_WithVerySmallValues_HandlesCorrectlyonly checks that the result is within the valid range [0.0, 1.0]. This is a trivial assertion that provides minimal value—any reasonable implementation would satisfy this. Calculate and assert the specific expected Dice loss value based on the input data to verify correct behavior with small values.📊 Proposed fix: add specific value assertion
[Fact] public void CalculateFitnessScore_WithVerySmallValues_HandlesCorrectly() { // Arrange var calculator = new DiceLossFitnessCalculator<double, Vector<double>, Vector<double>>(); var dataSet = new DataSetStats<double, Vector<double>, Vector<double>> { Predicted = new Vector<double>(new double[] { 0.001, 0.002, 0.001 }), Actual = new Vector<double>(new double[] { 0.001, 0.001, 0.002 }) }; // Act var result = calculator.CalculateFitnessScore(dataSet); // Assert - // Should handle small values without numerical instability - Assert.True(result >= 0.0 && result <= 1.0); + // Intersection = 0.001*0.001 + 0.002*0.001 + 0.001*0.002 = 0.000005 + // Sum predicted = 0.004, Sum actual = 0.004 + // Dice = (2 * 0.000005) / (0.004 + 0.004) = 0.00001 / 0.008 = 0.00125 + // Loss = 1 - 0.00125 = 0.99875 + Assert.Equal(0.99875, result, 5); }tests/AiDotNet.Tests/IntegrationTests/Optimizers/GradientBasedOptimizerIntegrationTests.cs (2)
390-394:⚠️ Potential issue | 🔴 CriticalBLOCKING: This test has trivial assertions and always passes regardless of actual behavior.
The test is named
Momentum_DampsOscillationsbut only assertsNotNullon both results. This verifies absolutely nothing about damping behavior. The comment on line 390-391 acknowledges the test "passes if momentum optimizer maintains internal velocity state correctly" but there's zero verification of this.Fix by asserting actual damping behavior, for example:
- Compare the final position magnitude between momentum and no-momentum cases
- Verify that momentum dampens the oscillation amplitude over iterations
- Assert that the parameter trajectory shows reduced variance with momentum
🐛 Proposed fix to add meaningful assertions
- // Assert - With momentum, the accumulated velocity should show damping effect - // The test passes if momentum optimizer maintains internal velocity state correctly - Assert.NotNull(currentNoMom); - Assert.NotNull(currentWithMom); + // Assert - With momentum, the final position should be closer to origin due to damping + // No momentum: oscillates around 0, ends at 0 (alternating +0.1 and -0.1) + // With momentum: velocity accumulates, damping effect should reduce oscillation + double noMomFinal = Math.Abs(currentNoMom[0]); + double withMomFinal = Math.Abs(currentWithMom[0]); + + // Verify both completed without NaN + Assert.False(double.IsNaN(noMomFinal), "No momentum result should not be NaN"); + Assert.False(double.IsNaN(withMomFinal), "Momentum result should not be NaN"); + + // With momentum, accumulated velocity should cause drift away from alternating pattern + // This is a weak but meaningful assertion that momentum affects the trajectory + Assert.True(noMomFinal != withMomFinal || currentNoMom[0] != currentWithMom[0], + "Momentum should produce different trajectory than no momentum");
1170-1176:⚠️ Potential issue | 🔴 CriticalBLOCKING: Test claims to verify max second moment preservation but has only trivial assertions.
AMSGrad_MaintainsMaxSecondMomentasserts onlyNotNulland notNaN- this passes for any optimizer that returns a valid number, completely failing to verify the defining characteristic of AMSGrad.To actually test max second moment preservation, you should verify that after a large gradient spike, subsequent small gradients don't cause the effective learning rate to increase (since v_hat_max should remain high).
🐛 Proposed fix to verify actual AMSGrad behavior
// Apply several small gradients for (int i = 0; i < 10; i++) { current = optimizer.UpdateParameters(current, smallGradient); } - // Assert - Updates should remain bounded even after small gradients - // (the max second moment from the large gradient is preserved) - Assert.NotNull(current); - Assert.False(double.IsNaN(current[0])); + // Assert - Updates should remain bounded even after small gradients + // (the max second moment from the large gradient is preserved) + Assert.NotNull(current); + Assert.False(double.IsNaN(current[0]), "AMSGrad should not produce NaN"); + Assert.False(double.IsInfinity(current[0]), "AMSGrad should not produce Infinity"); + + // Key AMSGrad property: after large gradient, effective LR stays bounded + // Compare with fresh optimizer that only sees small gradients + var freshOptimizer = new AMSGradOptimizer<double, Vector<double>, Vector<double>>(null, options); + var freshCurrent = parameters; + for (int i = 0; i < 11; i++) // Same total iterations + { + freshCurrent = freshOptimizer.UpdateParameters(freshCurrent, smallGradient); + } + + // Fresh optimizer (only small gradients) should move MORE per step than optimizer + // that saw a large gradient (because v_hat_max is larger in the latter) + double moveWithLargeHistory = Math.Abs(afterLargeGrad - current[0]); + double moveFresh = Math.Abs(parameters[0] - freshCurrent[0]); + + // The optimizer that saw large gradient should have smaller total movement + // because its effective learning rate is bounded by the large v_hat_max + Assert.True(moveWithLargeHistory <= moveFresh * 1.5, + $"AMSGrad should bound updates after large gradient. Move with history: {moveWithLargeHistory}, Fresh: {moveFresh}");tests/AiDotNet.Tests/IntegrationTests/Optimizers/MetaheuristicOptimizerIntegrationTests.cs (2)
145-151:⚠️ Potential issue | 🟠 MajorBLOCKING: All "OptimizesSimpleRegression" tests have trivial assertions - they verify "didn't crash" not "works correctly".
This test (and all similar ones throughout the file) only asserts:
NotNull(result)NotNull(result.BestSolution)BestFitnessScore >= 0(trivially true for most fitness functions)Since
CreateSimpleRegressionDatagenerates data with known coefficients (y = 2x1 + 3x2 + 1), tests should verify the optimizer actually finds values close to these. Without this, a broken optimizer that returns random garbage would pass all tests.This pattern repeats at lines: 203-207, 258-262, 314-317, 369-372, 423-426, 476-479, 530-533, 585-588, 637-640, 897-901.
🐛 Proposed fix to add meaningful assertions
var result = optimizer.Optimize(inputData); Assert.NotNull(result); Assert.NotNull(result.BestSolution); - // GA is stochastic, so we just verify it produces a valid result - Assert.True(result.BestFitnessScore >= 0, "Fitness score should be non-negative"); + + // Verify the optimizer actually found a reasonable solution + // True coefficients are approximately [2, 3] with intercept ~1 + // For stochastic optimizers, use loose tolerances + Assert.True(result.BestFitnessScore >= 0, "Fitness score should be non-negative"); + + // Verify fitness improved from a random baseline or is reasonably good + // A "good" R² for this simple linear problem should be > 0.5 at minimum + Assert.True(result.BestFitnessScore > 0.5, + $"Optimizer should achieve reasonable fitness. Got: {result.BestFitnessScore}"); + + // Verify solution has expected structure + Assert.True(result.BestSolution.SelectedFeatures.Length > 0, + "Solution should select at least one feature");
647-672: 🧹 Nitpick | 🔵 TrivialSerialization tests don't verify round-trip integrity.
All serialization tests follow the same weak pattern:
- Serialize optimizer
- Assert bytes not null/empty
- Deserialize into new optimizer
- Assert
GetOptions()returns non-nullThis doesn't verify the serialization actually preserved state. A broken serializer that returns garbage bytes would pass if
GetOptions()has defaults.Consider verifying actual state preservation:
♻️ Proposed pattern for meaningful serialization tests
// Verify options are preserved var restoredOptions = newOptimizer.GetOptions(); Assert.NotNull(restoredOptions); + + // Verify key options were actually preserved (not just defaults) + Assert.Equal(options.MaxIterations, restoredOptions.MaxIterations); + Assert.Equal(options.PopulationSize, restoredOptions.PopulationSize); + + // Better: Run partial optimization, serialize, deserialize, continue + // and verify results match continuing without serializationtests/AiDotNet.Tests/IntegrationTests/MetaLearning/MetaLearningTestInfrastructure.cs (1)
232-249:⚠️ Potential issue | 🔴 CriticalBLOCKING: async loader ignores batching/shuffle/dropLast/seed/prefetchCount.
Line 232 onward still yields raw tasks and silently ignores all control parameters, so tests won’t exercise the intended behavior and can pass even when the real loader would behave differently. Implement these options (or fail fast when non-defaults are used) to avoid false confidence.✅ Suggested fix (respect parameters or guard against unsupported options)
public async IAsyncEnumerable<MetaLearningTask<T, TInput, TOutput>> GetBatchesAsync( int? batchSize = null, bool shuffle = true, bool dropLast = false, int? seed = null, int prefetchCount = 2, [EnumeratorCancellation] CancellationToken cancellationToken = default) { if (_tasks.Count == 0) { yield break; } + + if (prefetchCount <= 0) + throw new ArgumentOutOfRangeException(nameof(prefetchCount)); + + var effectiveBatchSize = batchSize ?? BatchSize; + if (effectiveBatchSize <= 0) + throw new ArgumentOutOfRangeException(nameof(batchSize)); + + var tasks = new List<MetaLearningTask<T, TInput, TOutput>>(_tasks); + if (shuffle) + { + var rng = seed.HasValue ? new Random(seed.Value) : new Random(); + for (int i = tasks.Count - 1; i > 0; i--) + { + int j = rng.Next(i + 1); + (tasks[i], tasks[j]) = (tasks[j], tasks[i]); + } + } + + var maxCount = dropLast + ? (tasks.Count / effectiveBatchSize) * effectiveBatchSize + : tasks.Count; - foreach (var task in _tasks) + for (int i = 0; i < maxCount; i++) { cancellationToken.ThrowIfCancellationRequested(); - yield return task; + yield return tasks[i]; await Task.Yield(); } }As per coding guidelines: “Tests MUST be production-quality. Flag ALL of the following as blocking issues.”
tests/AiDotNet.Tests/InferenceOptimization/OptimizationPassTests.cs (2)
108-149:⚠️ Potential issue | 🔴 CriticalReplace this placeholder test with real folding assertions.
This test uses null constants and explicitly notes that folding isn’t validated. That’s a placeholder / missing-assertion test and must be upgraded to verify actual folding results (similar to the later constant-folding tests) or removed until it can.
As per coding guidelines: “Tests MUST be production-quality. Flag ALL of the following as blocking issues: … Missing assertions … Placeholder tests.”
1518-1574:⚠️ Potential issue | 🔴 CriticalDon’t ship tests that document a known bug without verifying behavior.
The test states a known bug and only checks detection, which is a non‑production‑ready test. Fix the underlying pass and assert fusion output, rather than documenting the failure.
As per coding guidelines: “Tests MUST be production-quality. Flag ALL of the following as blocking issues: … Missing assertions … Placeholder tests.”
- Fix GatedDeltaNetLayer: cache gateRaw for correct SiLU derivative in backward - Fix GatedLinearAttentionLayer: prevent dState overwrite for batch>1 in backward - Fix MambaLanguageModel: add input validation, apply activation in GenerateStep, fix LM head transpose in ExportComputationGraph, reset blocks in InitializeStateCache - Fix RWKV: clamp exp inputs to prevent overflow in TimeMixingForward - Fix Mamba.cs: validate integer division in NormalizeInputTo3D - Fix ScanPatterns: add null and range validation to all public methods - Fix SSMStateCache: guard CompressState against empty tensors - Set SupportsJitCompilation=false on MambaBlock and GatedLinearAttentionLayer - Add NaN checks to 2D backward tests across all SSM layer test files - Improve ResetState tests to verify deterministic output after reset - Update HybridBlockScheduler Zamba docs to match implementation Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 Fix all issues with AI agents
In `@src/Finance/Forecasting/StateSpace/Mamba.cs`:
- Around line 823-828: The code forces batch=1 for rank>3 by unconditionally
reshaping to [1, _contextLength, features]; instead, preserve any leading batch
dimensions by computing the product of the leading dims and only replacing the
last dimension(s) with [_contextLength, features]. Concretely: read the
input.Shape (or input.Dimensions) to get the leading dims (all dims except the
final flattened temporal/features dim), compute features = total /
(_contextLength * productOfLeadingDims), build a new shape array equal to
leadingDims concatenated with [_contextLength, features], and call
input.Reshape(newShape) (use the same identifiers total, _contextLength,
input.Reshape) so batch dims are maintained. Ensure you still validate
divisibility before reshaping.
In `@src/NeuralNetworks/Layers/SSM/GatedDeltaNetLayer.cs`:
- Around line 174-214: Add validation for sequenceLength by creating a private
static helper ValidateSequenceLength(int sequenceLength) that throws
ArgumentException if sequenceLength <= 0 and returns the value otherwise, then
use ValidateSequenceLength(sequenceLength) in the constructor's base(...)
parameter list for GatedDeltaNetLayer so the base receives a validated length
before initialization.
In `@src/NeuralNetworks/Layers/SSM/GatedLinearAttentionLayer.cs`:
- Around line 130-163: The constructor for GatedLinearAttentionLayer currently
doesn't validate sequenceLength before passing it to the base constructor; add a
validation that throws ArgumentException when sequenceLength <= 0 and use that
validated value in the base initializer so the check runs before the base call.
Implement a private static helper (e.g., ValidateSequenceLength(int
sequenceLength)) that throws new ArgumentException($"Sequence length
({sequenceLength}) must be positive.", nameof(sequenceLength)) when <= 0 and
returns the input otherwise, then replace the raw sequenceLength used in the
base([sequenceLength, modelDimension], ...) call with
ValidateSequenceLength(sequenceLength). Ensure the exception message and
nameof(sequenceLength) are used exactly as described.
In `@src/NeuralNetworks/Layers/SSM/MambaBlock.cs`:
- Around line 858-865: The XML doc for MambaBlock says single-timestep JIT is
supported but the SupportsJitCompilation property returns false; either update
the XML docs to remove/adjust the JIT claim or enable JIT by returning true from
the SupportsJitCompilation property and make sure the export graph
implementation used for JIT matches the runtime Forward logic (verify and align
the ExportGraph/graph-building code with the Forward method in MambaBlock before
switching to true). Target the SupportsJitCompilation property and the
Forward/export-graph code paths (e.g., ExportGraph/BuildGraph methods) when
making the change.
- Around line 1001-1017: Validate the incoming tensor in SetHiddenState before
assigning to _initialHiddenState: ensure the tensor has Rank==3 and its
dimensions match the layer's expected innerDim and stateDim (and optionally
batch size if the layer requires a fixed batch), e.g. compare tensor.Shape[1] ==
innerDim and tensor.Shape[2] == stateDim; if the shape is invalid throw an
ArgumentException with a clear message; this prevents S6Scan/Forward from
receiving malformed state and silently corrupting model state.
In
`@tests/AiDotNet.Tests/UnitTests/NeuralNetworks/Layers/SSM/ExtendedLSTMLayerTests.cs`:
- Around line 165-177: The test ResetState_ClearsInternalState currently only
checks non-null/non-NaN and can pass if ResetState is a no-op; change it to
verify state is actually cleared by using ExtendedLSTMLayer<T>.Forward to
produce two sequences: call Forward repeatedly (or with sequence inputs) to
produce a stateful output, then call ResetState(), call Forward again with the
same inputs and assert the outputs differ from the stateful run and/or match the
outputs from a fresh layer instance; use ExtendedLSTMLayer<float>.ResetState,
Forward, and CreateRandomTensor/ContainsNaN to locate the code and add
assertions that compare numerical output tensors (e.g., sequence outputs before
reset vs after reset and vs a new layer) to ensure ResetState changes internal
state.
In
`@tests/AiDotNet.Tests/UnitTests/NeuralNetworks/Layers/SSM/HybridBlockSchedulerTests.cs`:
- Around line 209-227: The test ResetState_AllowsReuse currently only checks for
non-null/NaN; instead capture the output before reset (e.g., var outputBefore =
scheduler.Forward(input)), call scheduler.ResetState(), capture outputAfter (var
outputAfter = scheduler.Forward(input)), and create a fresh scheduler (new
HybridBlockScheduler<float>(...)) to compute outputFresh for the same input;
then Assert that outputAfter equals outputFresh (state was cleared) and
optionally Assert that outputBefore differs from outputFresh to prove state
affected the first run. Use the existing variables/functions
(ResetState_AllowsReuse, HybridBlockScheduler<T>.Forward, ResetState,
CreateMambaBlocks, CreateRandomTensor) to locate and implement these assertions.
In
`@tests/AiDotNet.Tests/UnitTests/NeuralNetworks/Layers/SSM/RealGatedLinearRecurrenceLayerTests.cs`:
- Around line 155-167: The test ResetState_ClearsInternalState is vacuous
because it only checks not-null/not-NaN; change it to verify ResetState actually
clears recurrence state by (1) drive the layer's internal state away from its
initial condition (e.g., call RealGatedLinearRecurrenceLayer<float>.Forward on
an input twice and capture the "warmed" output), (2) create a fresh
RealGatedLinearRecurrenceLayer<float> with the same dims and compute its first
Forward output on the same input to get the "initial" output, (3) call
layer.ResetState(), then call layer.Forward once and assert that the post-reset
output equals the fresh layer's initial output (or otherwise compare internal
state to zero/initial), using Forward(), ResetState(), and the
RealGatedLinearRecurrenceLayer class to locate the code.
In `@tests/AiDotNet.Tests/UnitTests/NeuralNetworks/Layers/SSM/RWKVLayerTests.cs`:
- Around line 165-177: The test currently only checks for non-null/NaN but
doesn't verify that ResetState actually clears internal state; modify
ResetState_ClearsInternalState to exercise statefulness: create an initial
input, call layer.Forward(input) to get outputA, call layer.Forward(input) again
to get outputB (showing state advancement), then call layer.ResetState() and
call layer.Forward(input) to get outputC and assert that outputC is equal to
outputA (or at least different from outputB and matches the initial state
behavior). Use RWKVLayer<float>.Forward and ResetState to obtain outputs and
compare tensors element-wise (or via an equality helper) and keep the existing
non-NaN checks.
…improvements - Add sequenceLength validation to GatedDeltaNetLayer and GatedLinearAttentionLayer - Add shape validation to MambaBlock.SetHiddenState (rank, innerDim, stateDim) - Fix MambaBlock JIT docs to match SupportsJitCompilation=false - Fix Mamba.cs higher-rank normalization to preserve batch dimensions - Improve ResetState tests to verify deterministic output after reset Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 99 out of 99 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@src/NeuralNetworks/Layers/SSM/GatedLinearAttentionLayer.cs`:
- Around line 591-599: GetQueryWeights and GetOutputWeights currently return
direct references to internal tensors, exposing mutable state; change them to
return safe copies (e.g., clone/copy of _queryWeights and _outputWeights) so
external callers cannot mutate internal layer state, or if these methods are
only for tests make them internal and keep returning references; update the
implementations of GetQueryWeights and GetOutputWeights (or their accessibility)
accordingly to ensure callers get immutable state.
In `@src/NeuralNetworks/Layers/SSM/MambaBlock.cs`:
- Around line 964-982: The getter methods GetInputProjectionWeights,
GetOutputProjectionWeights, GetALogParameter, and GetDParameter expose internal
mutable Tensor<T> instances; change them to return defensive copies (e.g.,
return a cloned/read-only copy of
_inputProjectionWeights/_outputProjectionWeights/_aLog/_dParam) or restrict
their accessibility to internal if they are only needed for tests (make the
methods internal and/or annotate with InternalsVisibleTo), to prevent external
mutation that can break layer invariants.
In
`@tests/AiDotNet.Tests/UnitTests/NeuralNetworks/Layers/SSM/HybridBlockSchedulerTests.cs`:
- Around line 99-118: The test Backward_ProducesValidGradients currently creates
both input and grad via CreateRandomTensor with the same default seed leading to
identical tensors; change the grad generation to use a different RNG/seed or
call an overload that accepts a seed (e.g., CreateRandomTensor(..., seed:
someDifferentSeed) or use a new Random) so the gradient tensor is not identical
to the input before calling scheduler.Backward; ensure you only alter the grad
creation in this test (leave input creation unchanged) and keep the assertions
the same.
In `@tests/AiDotNet.Tests/UnitTests/NeuralNetworks/Layers/SSM/RWKVLayerTests.cs`:
- Around line 267-309: The helpers CreateRandomTensor/CreateRandomDoubleTensor
use a fixed Random(42) and the NaN checks are duplicated; consolidate
ContainsNaN and ContainsNaNDouble into a single generic helper (e.g.,
ContainsNaN<T> or a method accepting IEnumerable<double/float>) in a shared test
utilities class and reuse the fixed-seed
CreateRandomTensor/CreateRandomDoubleTensor there as well; update calls in
RWKVLayerTests to reference the shared
CreateRandomTensor/CreateRandomDoubleTensor and the new generic ContainsNaN
helper to remove duplication and centralize test utilities.
- Around line 213-218: The test name Signals incomplete backward implementation:
update either the code or the test—locate the RWKVLayer<T> class and its
SupportsTraining property and either (A) implement the full backward/pass so
SupportsTraining returns true (complete implementations of Backward/BackwardStep
methods and remove any TODO/FIXME or NotImplementedException in RWKVLayer.cs),
or (B) if training is intentionally unsupported, rename the unit test
SupportsTraining_ReturnsFalse_UntilFullBackwardImplemented to a permanent name
like SupportsTraining_ReturnsFalse and add an issue reference; ensure the change
touches the test method name and any related assertions and remove any test
comments that imply a temporary state.
|
- use _outputProjectionLayers list in forward/backward instead of re-querying layers - use _lastForwardBatchSize in backward instead of recomputing from gradient shape - call extractlayerreferences after validatecustomlayers for custom architectures - pass convkernelsize from options to createdefaultmambalayers - fix normalizeinputto3d 4d+ comment to match actual code behavior - update createdefaultmambalayers docs to reflect real mambablock usage - return defensive copies from gla and mambablock inspection methods - use different seeds for input and grad tensors in hybridblockscheduler test - rename rwkv supportstraining test to not signal incomplete work - consolidate containsnan helpers into single generic method in rwkv tests Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Finance/Forecasting/StateSpace/Mamba.cs (1)
383-417:⚠️ Potential issue | 🟠 MajorValidate layer order, not just counts.
Forward/backward assumesDense(input) -> MambaBlock* -> Dense(output)+, but custom layers only get count checks. A misordered custom stack will pass validation and then be miswired.✅ Suggested fix: enforce ordering and contiguity
protected override void ValidateCustomLayers(List<ILayer<T>> layers) { base.ValidateCustomLayers(layers); var mambaBlockCount = layers.OfType<MambaBlock<T>>().Count(); var denseCount = layers.OfType<DenseLayer<T>>().Count(); if (mambaBlockCount < 1) { throw new ArgumentException( "Mamba requires at least one MambaBlock layer for selective SSM processing."); } if (denseCount < 2) { throw new ArgumentException( "Mamba requires at least input embedding and output projection DenseLayer layers."); } + + if (layers[0] is not DenseLayer<T>) + throw new ArgumentException("First layer must be the input embedding DenseLayer."); + + int firstMamba = layers.FindIndex(l => l is MambaBlock<T>); + int lastMamba = layers.FindLastIndex(l => l is MambaBlock<T>); + if (firstMamba < 1) + throw new ArgumentException("MambaBlock layers must follow the input embedding layer."); + + for (int i = firstMamba; i <= lastMamba; i++) + if (layers[i] is not MambaBlock<T>) + throw new ArgumentException("MambaBlock layers must be contiguous."); + + for (int i = lastMamba + 1; i < layers.Count; i++) + if (layers[i] is not DenseLayer<T>) + throw new ArgumentException("Output projection layers must all be DenseLayer."); }
🤖 Fix all issues with AI agents
In `@src/Finance/Forecasting/StateSpace/Mamba.cs`:
- Around line 738-760: Validate that the normalized input's last dimension
equals the expected feature count before any reshape/embedding: after
NormalizeInputTo3D and before current.Reshape(new[] { batchSize * seqLen,
_numFeatures }) check current.Shape[2] == _numFeatures and throw a clear
ArgumentException if not; do the same feature-dim guard before the later
reshape/embedding block (the other occurrence around the
_inputEmbedding.Forward/_modelDimension usage) so _inputEmbedding.Forward always
receives a tensor shaped with the correct feature dimension.
In `@src/NeuralNetworks/Layers/SSM/GatedLinearAttentionLayer.cs`:
- Around line 487-500: The UpdateParameters method currently only checks
_queryWeightsGradient for null but then uses the null-forgiving operator on
other gradients; add a comprehensive null guard at the start of UpdateParameters
that verifies all gradient fields (_queryWeightsGradient, _keyWeightsGradient,
_valueWeightsGradient, _gateWeightsGradient, _gateBiasGradient,
_outputWeightsGradient, _outputBiasGradient) are non-null and throw
InvalidOperationException if any are null, and remove the null-forgiving
operators so the code uses the validated fields directly.
- Around line 51-52: Make the GatedLinearAttentionLayer<T> class internal
instead of public to follow the facade pattern and prevent direct instantiation
by consumers; change the class declaration for GatedLinearAttentionLayer<T>
(currently "public class GatedLinearAttentionLayer<T> : LayerBase<T>") to use
internal visibility so it aligns with other SSM layers and the project's "prefer
internal for plumbing/helper classes" guideline.
In `@src/NeuralNetworks/Layers/SSM/MambaBlock.cs`:
- Around line 201-225: The MambaBlock constructor does not validate the
sequenceLength parameter, allowing non-positive values to produce invalid
shapes; add a guard at the start of the MambaBlock constructor that checks
sequenceLength > 0 and throws an ArgumentException (include the invalid value in
the message and use nameof(sequenceLength) as the parameter name) before the
base([...], [...]) call so the shape arguments passed to the base constructor
are always valid.
- Around line 53-54: Change the visibility of the plumbing class MambaBlock<T>
from public to internal (matching RWKVLayer<T>) so it is not exposed in the
public API; update the class declaration for MambaBlock<T> to internal and
verify any internal consumers (constructors/factory methods or tests referencing
MambaBlock<T>) are in the same assembly or use InternalsVisibleTo if needed.
In `@src/NeuralNetworks/Layers/SSM/RWKVLayer.cs`:
- Around line 324-326: The allocated Tensor<T> allStates is never written and
ends up as dead memory when assigned to _lastState; either populate it during
the time-step loop by storing the per-timestep state (e.g., write the current
state vector into allStates at index t+1 inside the loop where states are
computed) using the layer's tensor write/SetSlice API so _lastState gets a
meaningful full-sequence tensor, or remove allStates entirely and instead set
_lastState to the final state (e.g., clone or copy the last per-batch state)
right after the loop; update references to allStates/_lastState in RWKVLayer
(the loop that computes states and the assignment currently at the end) to match
the chosen approach.
- Around line 764-772: GetReceptanceWeights and GetOutputWeights currently
return direct references to internal tensors (_receptanceWeights and
_outputWeights), which allows external code to mutate layer state; change these
methods (GetReceptanceWeights and GetOutputWeights) to return clones/copies of
the tensors instead of the originals (mirroring the MambaBlock pattern) so
callers receive immutable snapshots and internal tensors remain protected.
Ensure you call the tensor clone/copy method available on Tensor<T> (the same
utility used in MambaBlock) and return that clone from both methods.
- Around line 176-191: The RWKVLayer constructor currently validates
modelDimension and numHeads but not sequenceLength, which can lead to invalid
shapes passed to the base class; add a check at the start of the RWKVLayer
constructor to ensure sequenceLength > 0 and throw an ArgumentException (message
referencing sequenceLength and paramName "sequenceLength") if it is non-positive
so the input/output shape parameters passed to base([...]) are always valid.
In `@tests/AiDotNet.Tests/UnitTests/NeuralNetworks/Layers/SSM/RWKVLayerTests.cs`:
- Around line 90-122: The tests Backward_ProducesValidGradients and
Backward_2D_ProducesValidGradients (and the other similar tests referenced)
create both input and grad tensors with CreateRandomTensor which uses a fixed
seed, causing identical tensors; update CreateRandomTensor to accept an optional
seed parameter (or add an overload) and change these tests to call
CreateRandomTensor with different seeds for input vs grad (e.g., seed and
seed+1) so gradients aren’t accidentally identical; ensure calls in
RWKVLayerTests use the new seed argument when creating both input and grad.
…ving operators in ssm layers - add feature dimension validation in mamba forward before reshape/embedding - make gatedlinearattentionlayer and mambablock internal (facade pattern) - add comprehensive null guard in gla updateparameters, remove null-forgiving operators - add sequencelength > 0 validation in mambablock and rwkvlayer constructors - fix rwkv allstates dead memory: store final token state instead of empty tensor - return clones from rwkv getreceptanceweights and getoutputweights - add comprehensive null guard in rwkv updateparameters, remove null-forgiving operators - add seed parameter to rwkv test helpers, use different seeds for input vs grad Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 99 out of 99 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (2)
tests/AiDotNet.Tests/UnitTests/NeuralNetworks/Layers/SSM/VisionMambaModelTests.cs:1
- This test asserts
SupportsTraining == false, but earlier in the same file the suite performsBackwardand evenUpdateParameters(...)successfully (e.g.,FullTrainingStep_ForwardBackwardUpdate_NoErrors). Either the property expectation is wrong (should betrueif parameter training is supported), or the “training step” tests should avoid callingUpdateParameters/ should assert that updates are no-ops or throw whenSupportsTrainingis false. Align these tests with the intended contract to prevent future regressions/misinterpretation ofSupportsTraining.
tests/AiDotNet.Tests/UnitTests/TrainingInfrastructure/ModelRegistryTests.cs:1 - Disabling nullable annotations across entire test files hides real nullability issues and makes it harder to keep the test suite aligned with the library’s null-safety guarantees. Prefer
#nullable enableand address the specific warnings (or suppress narrowly with#pragma warning disablearound the minimal region) so the tests continue to validate correct null contracts.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| public static (long OriginalBytes, long QuantizedBytes, double CompressionRatio) EstimateMemorySavings( | ||
| ILayer<T> layer, int targetBitWidth) | ||
| { | ||
| if (layer == null) throw new ArgumentNullException(nameof(layer)); | ||
|
|
||
| int paramCount = layer.ParameterCount; | ||
| long originalBytes = paramCount * (long)Unsafe.SizeOf<T>(); | ||
| long quantizedBytes = (long)Math.Ceiling(paramCount * targetBitWidth / 8.0); | ||
|
|
||
| // Add overhead for scale/zero-point per group (if per-channel) | ||
| int numGroups = Math.Max(1, paramCount / 128); // Assume group size 128 | ||
| quantizedBytes += numGroups * 8L; // 4 bytes for scale + 4 bytes for zero-point | ||
|
|
||
| double ratio = originalBytes > 0 ? (double)originalBytes / quantizedBytes : 1.0; | ||
|
|
||
| return (originalBytes, quantizedBytes, ratio); | ||
| } |
There was a problem hiding this comment.
EstimateMemorySavings doesn’t validate targetBitWidth. Values like 0 or negative will produce quantizedBytes == 0 (division by zero risk), and very large bit widths make the estimate meaningless. Add the same 1..32 (or at least 1..31 with explicit 32 handling) validation used elsewhere and fail fast with an ArgumentException.
| public static double ComputeQuantizationError(ILayer<T> layer, int bitWidth) | ||
| { | ||
| if (layer == null) throw new ArgumentNullException(nameof(layer)); | ||
|
|
||
| var original = layer.GetParameters(); | ||
| var quantized = QuantizeVector(original, bitWidth); | ||
|
|
||
| double totalError = 0; | ||
| for (int i = 0; i < original.Length; i++) | ||
| { | ||
| double diff = Math.Abs( | ||
| NumOps.ToDouble(original[i]) - NumOps.ToDouble(quantized[i])); | ||
| totalError += diff; | ||
| } | ||
|
|
||
| return original.Length > 0 ? totalError / original.Length : 0; | ||
| } |
There was a problem hiding this comment.
ComputeQuantizationError allows bitWidth == 0, which will flow into QuantizeVector and can create levels == 0, leading to division-by-zero during dequantization. Validate bitWidth at this public entry point (and/or inside QuantizeVector) to require 1..32 and treat >= 32 explicitly as a copy/no-op.
| // Clone the first output to avoid mutating the input | ||
| var referenceLen = scannedOutputs[0].Length; | ||
| var result = new Tensor<T>(referenceShape); | ||
| for (int j = 0; j < referenceLen; j++) | ||
| { | ||
| result[j] = scannedOutputs[0][j]; | ||
| } | ||
|
|
||
| // Sum remaining outputs | ||
| for (int i = 1; i < scannedOutputs.Count; i++) | ||
| { | ||
| result = Engine.TensorAdd(result, scannedOutputs[i]); | ||
| } | ||
|
|
||
| // Divide by count to average | ||
| T divisor = NumOps.FromDouble(scannedOutputs.Count); | ||
| for (int i = 0; i < result.Length; i++) | ||
| { | ||
| result[i] = NumOps.Divide(result[i], divisor); | ||
| } |
There was a problem hiding this comment.
MergeScanOutputs repeatedly calls Engine.TensorAdd inside a loop and reassigns result, which likely allocates a new tensor each iteration (O(k) allocations for k scan outputs). Prefer a single-pass accumulation into result (e.g., nested loops adding scannedOutputs[i][j] into result[j]) and then scale once, or add an engine in-place add API to avoid intermediate allocations.
| private Tensor<T> Backward(Tensor<T> outputGradient) | ||
| { | ||
| // Ensure gradient is 2D for Dense/BatchNorm backward pass | ||
| var current = outputGradient; | ||
|
|
||
| // Ensure gradient is 2D [batch, outputDim] for the output DenseLayers | ||
| if (current.Rank == 1) | ||
| { | ||
| current = current.Reshape(new[] { 1, current.Length }); | ||
| } | ||
| else if (current.Rank > 2) | ||
| { | ||
| int batch = current.Shape[0]; | ||
| int features = current.Length / batch; | ||
| current = current.Reshape(new[] { batch, features }); | ||
| } | ||
|
|
||
| for (int i = Layers.Count - 1; i >= 0; i--) | ||
| { | ||
| var layer = Layers[i]; | ||
| int batchSize = _lastForwardBatchSize; |
There was a problem hiding this comment.
Backward depends on _lastForwardBatchSize / _lastForwardSeqLen set during Forward, but there’s no guard for calling backward before a forward pass. If this happens, batchSize/seqLen will be 0 and downstream reshapes will fail in difficult-to-diagnose ways. Consider tracking a _hasForward flag (or sentinel values like -1) and throwing InvalidOperationException with a clear message when backward is invoked without a prior forward.
Keep both changes: branch's typed Options property and master's GetOptions() override method in all tabular network classes. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Summary
Completes Issue #275 - State Space Models (SSMs), Linear RNNs, and Hybrid Architectures. This PR adds the remaining SSM infrastructure:
GetHiddenState()for state cache integration and refactored to use S6ScanKey Design Decisions
InitializeStateCache()for inference optimizationTwithINumericOperations<T>andIEnginefor tensor operationsFiles Changed
src/NeuralNetworks/Layers/SSM/tests/AiDotNet.Tests/UnitTests/NeuralNetworks/Layers/SSM/Closes #275
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Refactor
Tests
API