fix: model family test bug fixes — systematic model validation

[ooples]

Summary

Systematic run of all model family tests to identify and fix bugs across 100+ models. Working through test categories in chunks, tracking passing/failing models, and fixing production code bugs discovered by the tests.

Approach

• Run tests in small batches by category (Regression, Classification, Clustering, etc.)
• Track results locally to avoid re-running passing tests
• Fix model code bugs (not test bugs) — these are real production issues
• Watch for performance issues that could be optimized

Progress

• Build fix: Enum.GetValues net471 compat
• Category-by-category test runs and fixes (in progress)

🤖 Generated with Claude Code

Summary by CodeRabbit

• New Features

  • Added VideoCLIP model support and a strongly-typed temporal aggregation setting; patch-embedding now exposes patch-size metadata and deserialization handles patch layers more robustly.

• Bug Fixes

  • Improved predictive variance calibration, diffusion time-conditioning broadcasting, single-sample sparse-layer gradient handling, tree-splitting safeguards, offline license detection behavior, tensor JSON shape serialization, and activation-derivative rank tolerance.

• Tests

  • Added VideoCLIP tests, expanded regression and neural-network tests, relaxed diffusion determinism tolerance, and added many single-cluster clustering tests.

• Chores

  • Updated AiDotNet.Tensors package to v0.16.0

[coderabbitai]

Actionable comments posted: 20

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (3)

src/Models/Options/NGBoostRegressionOptions.cs (1)

63-67: ⚠️ Potential issue | 🟡 Minor

Orphaned XML documentation must be removed.

Lines 63-66 contain XML documentation (<summary> and <value> tags) for a property that no longer exists in this class. This will cause documentation tools to generate malformed output and confuses maintainers.

🧹 Proposed fix - remove orphaned docs

     public new int MinSamplesSplit { get; set; } = 2;
 
-    /// <summary>
-    /// Gets or sets the minimum number of samples required to be at a leaf node.
-    /// </summary>
-    /// <value>Default is 1.</value>
     // MinSamplesLeaf inherited from DecisionTreeOptions (default=1), matching NGBoost paper (Duan 2020)
 
     /// <summary>

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/Models/Options/NGBoostRegressionOptions.cs` around lines 63 - 67, Remove
the orphaned XML documentation block for the non-existent MinSamplesLeaf
property from the NGBoostRegressionOptions class: delete the <summary> and
<value> tags and the accompanying comment that refer to MinSamplesLeaf (the
inherited property from DecisionTreeOptions) so the class no longer contains
stale XML docs that don't match any member (search for NGBoostRegressionOptions
and the MinSamplesLeaf comment to locate the text).

src/Diffusion/NoisePredictors/DiffusionResBlock.cs (1)

212-221: 🧹 Nitpick | 🔵 Trivial

Pre-existing: Incomplete Backward implementation.

The backward pass only propagates gradients through the skip connection, ignoring the main ResBlock path (norm→SiLU→conv chains). While this wasn't introduced by this PR, given the PR's focus on "backward/gradient implementation fixes," this incomplete implementation deserves attention.

For a complete backward pass, gradients should flow through both the residual path AND the main conv blocks via the chain rule.

Would you like me to open an issue to track completing the backward pass for DiffusionResBlock?

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/Diffusion/NoisePredictors/DiffusionResBlock.cs` around lines 212 - 221,
The Backward method in DiffusionResBlock currently only routes gradients through
_skipConv and ignores the main residual path; update Backward(Tensor<T>
outputGradient) to compute and sum gradients from both paths: (1) propagate
outputGradient through the skip connection via _skipConv.Backward when _skipConv
!= null, (2) propagate outputGradient through the main res path by reversing the
forward sequence (e.g., last conv Backward, activation SiLU Backward,
normalization Backward, and previous conv Backward for each sublayer) using the
same layer instances used in Forward (identify the conv layers, norm layers, and
SiLU/op nodes in this class), and finally return the elementwise sum of the skip
gradient and the main path gradient (or outputGradient when skip is null);
ensure you respect any in-place ops and tensor shapes and call Backward on each
child layer in proper reverse order to implement the chain rule.

src/NeuralNetworks/VideoCLIPNeuralNetwork.cs (1)

1486-1516: ⚠️ Potential issue | 🔴 Critical

Blocking: backprop skips the derivative of the final normalization.

EncodeVideoNative() returns Normalize(pooled) at Line 1146, but Backward() sends the incoming loss gradient straight into _videoProjection.Backward(...). That treats normalization as identity and corrupts every downstream gradient. Cache the pre-normalized projection output during forward and backprop through Normalize() before entering the projection layer. As per coding guidelines, Backward must implement actual backpropagation through all layers.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/NeuralNetworks/VideoCLIPNeuralNetwork.cs` around lines 1486 - 1516,
Backward currently bypasses the final Normalize step from EncodeVideoNative;
update the forward pass (EncodeVideoNative) to cache the pre-normalized
projection output (e.g., store the pooled tensor before calling Normalize) and
in Backward(Tensor<T> gradient) first compute the gradient through Normalize
using that cached pre-normalized tensor (apply the normalization backward logic
or call the corresponding layer backward if you have a Normalization layer) to
produce the gradient w.r.t. the pooled/projection output, then continue the
existing reshape and call into _videoProjection.Backward(currentGradient).
Ensure the cached tensor is used by Backward() and that shape handling (the
Rank==1→[1,embeddingDim] conversion) is preserved when propagating the
normalized-gradient into _videoProjection.Backward.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@Directory.Packages.props`:
- Line 8: Update the PackageVersion entry for AiDotNet.Tensors to a published
version: replace Version="0.16.0" with Version="0.15.0" in the PackageVersion
element (the line containing PackageVersion Include="AiDotNet.Tensors"), or if
your code relies on a 0.16.0-only API like BroadcastAdd, either point to a valid
prerelease/internal feed that publishes 0.16.0 or refactor usages of
BroadcastAdd to the equivalent APIs available in 0.15.0 before changing the
package version.

In `@model_test_progress.md`:
- Around line 3-11: The section header "Completed — All Invariants Pass"
contradicts the table rows (e.g., NeuralNetworks/Layers at 94% and Regression at
99.9%), so update the document to accurately reflect status: either rename the
heading (for example to "Summary — Current Pass Rates") or split the table so
fully passing categories remain under "Completed — All Invariants Pass" and
partial-pass ones (NeuralNetworks/Layers, Regression) move to a new "Partial —
Some Invariants Fail" section; locate the heading and the table rows containing
the category names "NeuralNetworks/Layers" and "Regression" and make the
heading/table split or rename accordingly so the title matches the data.

In `@src/Helpers/DeserializationHelper.cs`:
- Around line 148-181: The deserializer block for PatchEmbeddingLayer<> assumes
inputShape has 3 elements and outputShape has 2, which can cause
IndexOutOfRangeException; update the code around the PatchEmbeddingLayer<>
handling to validate inputShape and outputShape lengths before indexing (e.g.
check inputShape.Length >= 3 and outputShape.Length >= 2), and throw a clear
InvalidOperationException or ArgumentException with context (mentioning type
name and missing metadata) if validation fails; keep using TryGetInt for
PatchSize and then proceed to find the ctor and invoke it (symbols to modify:
PatchEmbeddingLayer<>, inputShape, outputShape, TryGetInt, ctor, instance).

In `@src/Helpers/LicenseValidator.cs`:
- Around line 73-76: The current check in LicenseValidator that treats
_licenseKey.ServerUrl being null or empty as offline validation allows null to
bypass server validation and mark licenses Active; update the logic in the
synchronous and asynchronous validators (methods handling ValidateLicense /
ValidateLicenseAsync) to treat null ServerUrl as meaning use DefaultServerUrl
(or otherwise resolve to DefaultServerUrl) instead of immediately switching to
offline validation, and ensure the code trims and checks for empty string only
to decide offline mode; specifically, replace the branch that currently does "if
(_licenseKey.ServerUrl is null || _licenseKey.ServerUrl.Trim().Length == 0)"
with logic that assigns DefaultServerUrl when ServerUrl is null and only uses
offline behavior when ServerUrl is explicitly empty after trimming, so both the
sync and async flows reference DefaultServerUrl when appropriate and server
validation is not skipped for null values.

In `@src/Interfaces/IVideoCLIPModel.cs`:
- Line 50: You changed the IVideoCLIPModel<T> contract by replacing the string
TemporalAggregation with an enum TemporalAggregationType, which breaks external
implementers; add a compatibility shim and a clear migration note: in the
interface add a default-implemented legacy accessor like string
TemporalAggregationString => TemporalAggregation.ToString(); (or a getter with
that mapping) so existing consumers can still read a string, mark it [Obsolete]
with guidance, and update the PR/changelog with a breaking-change callout
explaining how to migrate from TemporalAggregationString (or the old string) to
the new TemporalAggregation (TemporalAggregationType) and include example
mapping.

In `@src/Models/Options/ConditionalInferenceTreeOptions.cs`:
- Around line 21-26: Add a copy constructor to ConditionalInferenceTreeOptions
that takes another ConditionalInferenceTreeOptions instance and copies all
properties (at minimum SignificanceLevel, StatisticalTest,
MaxDegreeOfParallelism, MinSamplesLeaf and any inherited options). If the base
Options class provides a copy constructor, call base(other); otherwise copy base
properties explicitly. Ensure any reference-type properties are cloned
appropriately so the new instance is a true independent copy.

In `@src/Models/Options/DecisionTreeOptions.cs`:
- Around line 47-48: The XML doc for DecisionTreeOptions.MinSamplesPerLeaf says
"defaulting to 5" but the property initializer is = 1; make them consistent by
updating the property initializer in DecisionTreeOptions (MinSamplesPerLeaf) to
= 5 to match the documentation, or if 1 is the intended default, update the
<value> XML text to state "defaulting to 1"; change only the MinSamplesPerLeaf
initializer or its XML comment accordingly so doc and code match.

In `@src/NeuralNetworks/Layers/SparseLinearLayer.cs`:
- Around line 292-324: The code performs an unnecessary 2D→1D→2D round-trip:
when outputWas1D you expand _lastOutput/outGrad to activOutput/outGrad (2D),
compute delta2D via ApplyActivationDerivativeFromOutput, then copy delta2D back
into a new 1D Tensor<T> delta only to convert it back to 2D later; instead, keep
delta as delta2D when outputWas1D (i.e., set delta = delta2D and remove the
allocation/copy that creates the 1D tensor), eliminating the extra element-wise
copies and allocations while preserving the existing downstream logic that
expects 2D; update references to delta so they use delta2D/delta consistently
(symbols: outputWas1D, activOutput, outGrad,
ApplyActivationDerivativeFromOutput, delta2D, delta).

In `@src/NeuralNetworks/VideoCLIPNeuralNetwork.cs`:
- Line 122: The serialized TemporalAggregation value is written but never
restored because the readonly field _temporalAggregation prevents assignment
during deserialization; update the class so that the deserialization path in
Deserialize/Restore (the code that complements SerializeNetworkSpecificData())
assigns the saved enum back into the instance: make _temporalAggregation
assignable (remove readonly or add an internal/private setter/constructor
overload), read and validate the enum value from the payload in the
deserialization routine, and set the instance's _temporalAggregation accordingly
(ensure you reference SerializeNetworkSpecificData(), Deserialize/Restore, and
CreateNewInstance() paths so the restored model uses the serialized aggregation
mode instead of the CreateNewInstance default).
- Around line 1541-1574: The loop that re-runs per-frame forward pass restores
activations but only calls _frameEncoderLayers[i].Backward(grad) and drops its
result, so gradients never flow back through PrependClsToken,
AddPositionalEmbeddings, or _patchEmbedding and no per-frame input gradients are
accumulated; fix by after each frame's reverse-iteration capture the backward
output from the first frame encoder layer, propagate that gradient through the
inverse operations (remove CLS token and split rows), accumulate the
CLS/positional gradients into the temporal gradient as needed, take rows
1..seqLen-1 (the patch rows) and call _patchEmbedding.Backward(patchGrad) to get
per-frame input grads, sum those into a cumulative input gradient tensor (e.g.,
inputGradients), and after the loop return that accumulated dL/dinput instead of
currentGradient; reference the symbols _cachedTrainingFrames,
_patchEmbedding.Forward/Backward, PrependClsToken, AddPositionalEmbeddings,
_frameEncoderLayers.Forward/Backward, currentGradient and ensure you preserve
gradient shapes when slicing rows 1..n and accumulating across frames.
- Around line 1633-1645: The current fallback silently ignores the configured
_optimizer and uses a hardcoded layer.UpdateParameters(0.001), which drops
optimizer state and never updates standalone trainable parameters returned by
GetParameters()/counted in ParameterCount; replace this by failing fast for
unsupported optimizer types (throw NotSupportedException or similar) or by
delegating to a model-wide update API on the configured _optimizer; specifically
remove the magic per-layer 0.001 path, ensure code paths call the optimizer's
model-level update (instead of layer.UpdateParameters) or explicitly enumerate
and pass all trainable parameters (Layers plus any standalone parameters) into
the optimizer, and keep optimizer state (do not reinitialize) until a proper
model-wide update implementation exists.

In `@src/Serialization/TensorJsonConverter.cs`:
- Around line 86-89: The reflection call to shapeToArray.Invoke(shapeObj, null)
is unsafely cast to (int[]) which can throw InvalidCastException; update the
TensorJsonConverter logic around shapeToArray, shapeObj and the shape variable
to validate the invoked result's runtime type, handle arrays of numeric element
types (e.g., long[], object[] with numeric values) by converting/transforming
elements into an int[] safely, and if conversion isn't possible throw the same
JsonSerializationException used elsewhere so failures remain friendly and
consistent.

In `@tests/AiDotNet.Tests/ModelFamilyTests/Clustering/BIRCHTests.cs`:
- Around line 17-18: CreateSingleClusterModel currently constructs a
BIRCH<double> with BIRCHOptions<double> that only sets NumClusters = 1, which
diverges from the tuned options used elsewhere; update CreateSingleClusterModel
to include the same Threshold (and any other tuned fields) on the
AiDotNet.Clustering.Options.BIRCHOptions<double> instance (e.g., set Threshold =
<same value> alongside NumClusters = 1) so the single-cluster factory validates
the same configuration as the main model setup.

In `@tests/AiDotNet.Tests/ModelFamilyTests/Clustering/CLARANSTests.cs`:
- Around line 17-19: CreateSingleClusterModel uses default CLARANS options and
omits the deterministic Seed; update the factory to pass a
CLARANSOptions<double> with Seed = 42 (matching CreateModel) so tests are
reproducible—i.e., modify CreateSingleClusterModel to construct new
CLARANS<double>(new CLARANSOptions<double> { NumClusters = 1, Seed = 42 }) (you
may also include NumLocal = 5 if you want exact parity with CreateModel).

In
`@tests/AiDotNet.Tests/ModelFamilyTests/Clustering/ConsensusClusteringTests.cs`:
- Around line 17-18: The CreateSingleClusterModel override constructs a
ConsensusClustering<double> with NumClusters = 1 but omits the seeded
configuration; update the instantiation of ConsensusClusteringOptions<double> to
preserve the same Seed used by the main test model (e.g., set the Seed property
alongside NumClusters = 1) so the single-cluster path remains reproducible;
modify the CreateSingleClusterModel method to pass the Seed into
ConsensusClusteringOptions<double> when creating new
ConsensusClustering<double>.

In `@tests/AiDotNet.Tests/ModelFamilyTests/Clustering/OnlineKMeansTests.cs`:
- Around line 20-21: The override CreateSingleClusterModel should only change
NumClusters to 1 and preserve the baseline training options; update the
OnlineKMeans instantiation so it builds its OnlineKMeansOptions<double> by
copying or reusing the same default/training options used elsewhere (preserving
Seed, learning/decay parameters, Iterations/MaxIterations, etc.) and then set
NumClusters = 1, rather than creating a fresh options object that resets other
fields; locate the CreateSingleClusterModel method and modify the
OnlineKMeansOptions construction to clone or apply the baseline options and only
override NumClusters.

In `@tests/AiDotNet.Tests/ModelFamilyTests/Clustering/SeededKMeansTests.cs`:
- Around line 17-18: CreateSingleClusterModel currently constructs a
SeededKMeans without setting the seed, breaking reproducibility; update
CreateSingleClusterModel to set the same seed used in CreateModel() by passing
the Seed value into the SeededKMeansOptions (e.g., new
SeededKMeansOptions<double> { NumClusters = 1, Seed =
<same-seed-variable-or-literal-used-in-CreateModel()> }) so both CreateModel and
CreateSingleClusterModel use the identical deterministic seed when instantiating
SeededKMeans.

In `@tests/AiDotNet.Tests/ModelFamilyTests/Clustering/SpectralClusteringTests.cs`:
- Around line 16-18: The CreateSingleClusterModel override currently constructs
SpectralClustering without a deterministic seed; update the SpectralOptions
passed to SpectralClustering in CreateSingleClusterModel to include Seed = 42
(matching CreateModel) so tests are consistent and deterministic; locate the
CreateSingleClusterModel method and add Seed = 42 to the new
AiDotNet.Clustering.Options.SpectralOptions<double> initializer.

In
`@tests/AiDotNet.Tests/ModelFamilyTests/Clustering/StreamingMiniBatchKMeansTests.cs`:
- Around line 18-19: CreateSingleClusterModel currently constructs
MiniBatchKMeans with only NumClusters set, which drops BatchSize and Seed and
changes behavior; update the factory to pass explicit BatchSize and Seed values
into the MiniBatchKMeansOptions (in addition to NumClusters = 1) so the
single-cluster test path preserves the same batch and RNG configuration as other
tests that rely on MiniBatchKMeans/ MiniBatchKMeansOptions settings.

In
`@tests/AiDotNet.Tests/ModelFamilyTests/NeuralNetworks/VideoCLIPNeuralNetworkTests.cs`:
- Around line 42-79: The test only exercises
TemporalAggregationType.TemporalTransformer; add a separate test that constructs
a VideoCLIPNeuralNetwork (same pattern as CreateNetwork) with a non-default
TemporalAggregationType (e.g., TemporalAggregationType.SomeOtherValue) to cover
the alternate aggregation path, then perform a serialize/deserialize round-trip
of that model using the project’s existing model serialization helpers and
assert the deserialized model matches the original (compare key properties like
embeddingDimension, numFrames, temporalAggregation, and any equality/metadata
checks) to ensure the new aggregation/serialization surface is
regression-tested.

---

Outside diff comments:
In `@src/Diffusion/NoisePredictors/DiffusionResBlock.cs`:
- Around line 212-221: The Backward method in DiffusionResBlock currently only
routes gradients through _skipConv and ignores the main residual path; update
Backward(Tensor<T> outputGradient) to compute and sum gradients from both paths:
(1) propagate outputGradient through the skip connection via _skipConv.Backward
when _skipConv != null, (2) propagate outputGradient through the main res path
by reversing the forward sequence (e.g., last conv Backward, activation SiLU
Backward, normalization Backward, and previous conv Backward for each sublayer)
using the same layer instances used in Forward (identify the conv layers, norm
layers, and SiLU/op nodes in this class), and finally return the elementwise sum
of the skip gradient and the main path gradient (or outputGradient when skip is
null); ensure you respect any in-place ops and tensor shapes and call Backward
on each child layer in proper reverse order to implement the chain rule.

In `@src/Models/Options/NGBoostRegressionOptions.cs`:
- Around line 63-67: Remove the orphaned XML documentation block for the
non-existent MinSamplesLeaf property from the NGBoostRegressionOptions class:
delete the <summary> and <value> tags and the accompanying comment that refer to
MinSamplesLeaf (the inherited property from DecisionTreeOptions) so the class no
longer contains stale XML docs that don't match any member (search for
NGBoostRegressionOptions and the MinSamplesLeaf comment to locate the text).

In `@src/NeuralNetworks/VideoCLIPNeuralNetwork.cs`:
- Around line 1486-1516: Backward currently bypasses the final Normalize step
from EncodeVideoNative; update the forward pass (EncodeVideoNative) to cache the
pre-normalized projection output (e.g., store the pooled tensor before calling
Normalize) and in Backward(Tensor<T> gradient) first compute the gradient
through Normalize using that cached pre-normalized tensor (apply the
normalization backward logic or call the corresponding layer backward if you
have a Normalization layer) to produce the gradient w.r.t. the pooled/projection
output, then continue the existing reshape and call into
_videoProjection.Backward(currentGradient). Ensure the cached tensor is used by
Backward() and that shape handling (the Rank==1→[1,embeddingDim] conversion) is
preserved when propagating the normalized-gradient into
_videoProjection.Backward.

🪄 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: 55f015bc-eeef-4354-8244-78c25ab08180

📥 Commits

Reviewing files that changed from the base of the PR and between 20b72a5 and 97e3f18.

📒 Files selected for processing (38)

• Directory.Packages.props
• model_test_progress.md
• src/Diffusion/NoisePredictors/DiffusionResBlock.cs
• src/Enums/TemporalAggregationType.cs
• src/GaussianProcesses/DeepGaussianProcess.cs
• src/Helpers/DeserializationHelper.cs
• src/Helpers/LicenseValidator.cs
• src/InferenceOptimization/IR/Common/IRTypes.cs
• src/Interfaces/IVideoCLIPModel.cs
• src/Models/Options/ConditionalInferenceTreeOptions.cs
• src/Models/Options/DecisionTreeOptions.cs
• src/Models/Options/NGBoostRegressionOptions.cs
• src/NeuralNetworks/Layers/PatchEmbeddingLayer.cs
• src/NeuralNetworks/Layers/SparseLinearLayer.cs
• src/NeuralNetworks/VideoCLIPNeuralNetwork.cs
• src/Regression/ConditionalInferenceTreeRegression.cs
• src/Serialization/TensorJsonConverter.cs
• tests/AiDotNet.Tests/ModelFamilyTests/Base/ClusteringModelTestBase.cs
• tests/AiDotNet.Tests/ModelFamilyTests/Base/DiffusionModelTestBase.cs
• tests/AiDotNet.Tests/ModelFamilyTests/Base/RegressionModelTestBase.cs
• tests/AiDotNet.Tests/ModelFamilyTests/Clustering/AgglomerativeClusteringTests.cs
• tests/AiDotNet.Tests/ModelFamilyTests/Clustering/BIRCHTests.cs
• tests/AiDotNet.Tests/ModelFamilyTests/Clustering/BisectingKMeansTests.cs
• tests/AiDotNet.Tests/ModelFamilyTests/Clustering/CLARANSTests.cs
• tests/AiDotNet.Tests/ModelFamilyTests/Clustering/CURETests.cs
• tests/AiDotNet.Tests/ModelFamilyTests/Clustering/ConsensusClusteringTests.cs
• tests/AiDotNet.Tests/ModelFamilyTests/Clustering/FuzzyCMeansTests.cs
• tests/AiDotNet.Tests/ModelFamilyTests/Clustering/GaussianMixtureModelTests.cs
• tests/AiDotNet.Tests/ModelFamilyTests/Clustering/KMeansTests.cs
• tests/AiDotNet.Tests/ModelFamilyTests/Clustering/KMedoidsTests.cs
• tests/AiDotNet.Tests/ModelFamilyTests/Clustering/MiniBatchKMeansTests.cs
• tests/AiDotNet.Tests/ModelFamilyTests/Clustering/OnlineKMeansTests.cs
• tests/AiDotNet.Tests/ModelFamilyTests/Clustering/SeededKMeansTests.cs
• tests/AiDotNet.Tests/ModelFamilyTests/Clustering/SpectralClusteringTests.cs
• tests/AiDotNet.Tests/ModelFamilyTests/Clustering/StreamingMiniBatchKMeansTests.cs
• tests/AiDotNet.Tests/ModelFamilyTests/NeuralNetworks/MixtureOfExpertsNeuralNetworkTests.cs
• tests/AiDotNet.Tests/ModelFamilyTests/NeuralNetworks/VideoCLIPNeuralNetworkTests.cs
• tests/AiDotNet.Tests/UnitTests/Serialization/LicenseKeyTests.cs

[coderabbitai]

Actionable comments posted: 5

♻️ Duplicate comments (3)

src/NeuralNetworks/VideoCLIPNeuralNetwork.cs (3)

1931-1931: ⚠️ Potential issue | 🔴 Critical

Blocking: validate the deserialized TemporalAggregationType before casting.

A corrupted or forward-incompatible payload can currently inject any integer into _temporalAggregation, and the model will accept it as if it were valid state. Reject unknown values here so deserialization fails loudly at the boundary.

🔒 Suggested fix

-        _temporalAggregation = (TemporalAggregationType)reader.ReadInt32();
+        int temporalAggregationValue = reader.ReadInt32();
+        if (!Enum.IsDefined(typeof(TemporalAggregationType), temporalAggregationValue))
+        {
+            throw new InvalidDataException(
+                $"Unknown temporal aggregation value: {temporalAggregationValue}");
+        }
+        _temporalAggregation = (TemporalAggregationType)temporalAggregationValue;

As per coding guidelines, "missing validation of external inputs" is blocking at system boundaries.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/NeuralNetworks/VideoCLIPNeuralNetwork.cs` at line 1931, The
deserialization directly casts reader.ReadInt32() into _temporalAggregation
(TemporalAggregationType) which accepts any integer; validate the integer first
and reject unknown values so deserialization fails loudly. Change the code
around _temporalAggregation to read the int into a temp (e.g., int raw =
reader.ReadInt32()), check it against the TemporalAggregationType enum (e.g.,
Enum.IsDefined(typeof(TemporalAggregationType), raw) or a switch/table of
allowed values), and only assign the cast value to _temporalAggregation if
valid; otherwise throw a descriptive exception (e.g., InvalidDataException) so
corrupted or forward-incompatible payloads are rejected at the boundary.

---

1634-1638: ⚠️ Potential issue | 🔴 Critical

Blocking: the optimizer path still freezes the standalone embedding matrices.

UpdateParameters(Layers) only touches layer instances. _visionClsToken, _visionPositionalEmbeddings, _temporalPositionalEmbeddings, and _textPositionalEmbeddings are still part of the model state and ParameterCount, but they never enter the optimizer path here. Until those parameters are included in a model-wide gradient/update flow, training only a subset of the model is silently incorrect.

As per coding guidelines, "Incomplete features: Half-implemented patterns where some code paths work but others silently do nothing" are blocking.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/NeuralNetworks/VideoCLIPNeuralNetwork.cs` around lines 1634 - 1638, The
optimizer path only updates layer instances via
gradOptimizer.UpdateParameters(Layers) and ignores standalone embedding tensors
(_visionClsToken, _visionPositionalEmbeddings, _temporalPositionalEmbeddings,
_textPositionalEmbeddings), so include those tensors in the optimizer's update
flow: modify the code that calls IGradientBasedOptimizer<T, Tensor<T>,
Tensor<T>>.UpdateParameters to pass a complete parameter set (either append the
four embedding tensors into the Layers/parameters collection before calling
UpdateParameters, or add/use an overload like
UpdateParameters(IEnumerable<Parameter> allParameters) or a ModelParameters
property that returns Layers plus the four embedding tensors), ensuring the
optimizer implementation consumes and updates these named symbols so they
participate in gradient updates and ParameterCount reflects the updated set.

---

1541-1575: ⚠️ Potential issue | 🔴 Critical

Blocking: the checkpointed frame backward still stops before dL/dinput.

This loop replays the frame encoder, but it still passes the full [CLS + patches] gradient into _patchEmbedding.Backward(...), drops the returned frame-input gradient, and then returns currentGradient from the temporal encoder instead of an input-shaped tensor. Drop the CLS row before patch-embedding backprop, accumulate gradients for the CLS/positional parameters, collect each frame’s input gradient, and return that accumulated dL/dinput tensor.

As per coding guidelines, "Backward Method ... must implement actual backpropagation through all layers."

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/NeuralNetworks/VideoCLIPNeuralNetwork.cs` around lines 1541 - 1575, The
replay loop currently backprops only through encoder layers but throws away the
per-frame input gradient and returns currentGradient; fix
VideoCLIPNeuralNetwork.cs by: after computing frameGrad and doing layer.Backward
to produce grad, split grad into clsRow = grad[0, :] and patchGrad = grad[1: ,
:]; accumulate clsRow into the gradients for _visionClsToken and add
contributions into _visionPositionalEmbeddings' gradient (so CLS/positional
params get updated), then call _patchEmbedding.Backward(patchGrad) and collect
its returned dL/dinput (per-frame image gradient) into an accumulator tensor
sized like inputs; repeat for each frame and finally return the accumulated
dL/dinput tensor instead of currentGradient; ensure you reference and update
symbols _cachedTrainingFrames, _patchEmbedding.Backward, _visionClsToken,
_visionPositionalEmbeddings, frameGrad, grad, and currentGradient when
implementing these changes.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@model_test_progress.md`:
- Around line 3-18: The markdown headings and tables (e.g., "## Test Coverage by
Category" and "## Blocked") lack required surrounding blank lines causing
markdownlint MD022/MD058; fix by adding a blank line before each "## ..."
heading and a blank line after the preceding paragraph, and ensure a blank line
before and after each table block so the table rows are separated from headings
and paragraphs (apply the same blank-line adjustments around the other affected
headings/tables such as the one at lines 20-30).

In `@src/Helpers/DeserializationHelper.cs`:
- Around line 166-167: The fallback calculation for patchSize in
DeserializationHelper.cs can divide by zero or produce NaN when numPatches or
imageWidth are zero or when the sqrt rounds to 0; update the logic around
TryGetInt(additionalParams, "PatchSize") to validate numPatches and imageWidth
before using them, compute a safe sqrtArg = (double)numPatches * imageHeight /
Math.Max(1, imageWidth), guard that sqrtArg > 0, compute denom =
(int)Math.Round(Math.Sqrt(sqrtArg)) and then use Math.Max(1, denom) when
dividing imageHeight to derive patchSize (or fall back to a sensible default
like 1), ensuring patchSize is never zero and no division by zero occurs;
reference TryGetInt, patchSize, numPatches, imageHeight, imageWidth, and the
fallback calculation in your changes.

In `@src/Models/Options/ConditionalInferenceTreeOptions.cs`:
- Around line 28-38: The copy constructor
ConditionalInferenceTreeOptions(ConditionalInferenceTreeOptions other) currently
only copies derived fields and drops inherited DecisionTreeOptions properties;
update it to preserve full state by either calling the base copy constructor
(e.g., base(other)) if DecisionTreeOptions exposes one, or explicitly copy
inherited properties MaxFeatures, SplitCriterion, UseSoftTree, and
SoftTreeTemperature from other after Guard.NotNull(other) so the clone contains
all inherited and derived settings (keep existing copies of SignificanceLevel,
StatisticalTest, MaxDegreeOfParallelism, MinSamplesLeaf, MinSamplesSplit,
MaxDepth, Seed).

In `@src/NeuralNetworks/VideoCLIPNeuralNetwork.cs`:
- Around line 1501-1516: The Backward implementation is skipping the derivative
of the final L2 Normalize applied in EncodeVideoNative: before calling
_videoProjection.Backward you must retrieve the cached pre-normalized vector
(e.g., the "pooled" or pre-normalize embedding saved in forward for
EncodeVideoNative/EncodeVideo) and apply the L2-normalization gradient
(d(normalize(x))/dx) to currentGradient (handle both 1-D and 2-D shapes), then
pass the resulting gradient into _videoProjection.Backward; ensure the cache key
used in forward matches the symbol used here so the pre-normalized embedding is
available for computing the normalization Jacobian.
- Around line 1905-1906: The deserializer for VideoCLIPNeuralNetwork currently
ignores the persisted _useNativeMode flag (written alongside
_temporalAggregation) causing an ONNX-backed instance to be recreated via the
native constructor path; update the load logic that pairs with the writer where
writer.Write(_useNativeMode) is written so that when deserializing the saved
_useNativeMode you either (A) persist and restore the ONNX-specific state needed
to reconstruct the ONNX constructor path (model paths/session info) so an ONNX
instance is recreated, or (B) if you cannot restore ONNX state, throw an
explicit exception/fail-fast when _useNativeMode == false instead of silently
constructing the native instance; update the code around the
VideoCLIPNeuralNetwork deserialization path to read and act on _useNativeMode
(and mirror any additional ONNX state you choose to persist) so behavior matches
the serialized flag.

---

Duplicate comments:
In `@src/NeuralNetworks/VideoCLIPNeuralNetwork.cs`:
- Line 1931: The deserialization directly casts reader.ReadInt32() into
_temporalAggregation (TemporalAggregationType) which accepts any integer;
validate the integer first and reject unknown values so deserialization fails
loudly. Change the code around _temporalAggregation to read the int into a temp
(e.g., int raw = reader.ReadInt32()), check it against the
TemporalAggregationType enum (e.g.,
Enum.IsDefined(typeof(TemporalAggregationType), raw) or a switch/table of
allowed values), and only assign the cast value to _temporalAggregation if
valid; otherwise throw a descriptive exception (e.g., InvalidDataException) so
corrupted or forward-incompatible payloads are rejected at the boundary.
- Around line 1634-1638: The optimizer path only updates layer instances via
gradOptimizer.UpdateParameters(Layers) and ignores standalone embedding tensors
(_visionClsToken, _visionPositionalEmbeddings, _temporalPositionalEmbeddings,
_textPositionalEmbeddings), so include those tensors in the optimizer's update
flow: modify the code that calls IGradientBasedOptimizer<T, Tensor<T>,
Tensor<T>>.UpdateParameters to pass a complete parameter set (either append the
four embedding tensors into the Layers/parameters collection before calling
UpdateParameters, or add/use an overload like
UpdateParameters(IEnumerable<Parameter> allParameters) or a ModelParameters
property that returns Layers plus the four embedding tensors), ensuring the
optimizer implementation consumes and updates these named symbols so they
participate in gradient updates and ParameterCount reflects the updated set.
- Around line 1541-1575: The replay loop currently backprops only through
encoder layers but throws away the per-frame input gradient and returns
currentGradient; fix VideoCLIPNeuralNetwork.cs by: after computing frameGrad and
doing layer.Backward to produce grad, split grad into clsRow = grad[0, :] and
patchGrad = grad[1: , :]; accumulate clsRow into the gradients for
_visionClsToken and add contributions into _visionPositionalEmbeddings' gradient
(so CLS/positional params get updated), then call
_patchEmbedding.Backward(patchGrad) and collect its returned dL/dinput
(per-frame image gradient) into an accumulator tensor sized like inputs; repeat
for each frame and finally return the accumulated dL/dinput tensor instead of
currentGradient; ensure you reference and update symbols _cachedTrainingFrames,
_patchEmbedding.Backward, _visionClsToken, _visionPositionalEmbeddings,
frameGrad, grad, and currentGradient when implementing these changes.

🪄 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: f3cbd026-d7ac-4d16-897f-c0d8e178654b

📥 Commits

Reviewing files that changed from the base of the PR and between 97e3f18 and 3c8a163.

📒 Files selected for processing (15)

• model_test_progress.md
• src/Helpers/DeserializationHelper.cs
• src/Models/Options/ConditionalInferenceTreeOptions.cs
• src/Models/Options/DecisionTreeOptions.cs
• src/NeuralNetworks/Layers/SparseLinearLayer.cs
• src/NeuralNetworks/VideoCLIPNeuralNetwork.cs
• src/Serialization/TensorJsonConverter.cs
• tests/AiDotNet.Tests/ModelFamilyTests/Base/DiffusionModelTestBase.cs
• tests/AiDotNet.Tests/ModelFamilyTests/Clustering/BIRCHTests.cs
• tests/AiDotNet.Tests/ModelFamilyTests/Clustering/CLARANSTests.cs
• tests/AiDotNet.Tests/ModelFamilyTests/Clustering/ConsensusClusteringTests.cs
• tests/AiDotNet.Tests/ModelFamilyTests/Clustering/OnlineKMeansTests.cs
• tests/AiDotNet.Tests/ModelFamilyTests/Clustering/SeededKMeansTests.cs
• tests/AiDotNet.Tests/ModelFamilyTests/Clustering/SpectralClusteringTests.cs
• tests/AiDotNet.Tests/ModelFamilyTests/Clustering/StreamingMiniBatchKMeansTests.cs

[coderabbitai]

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/Models/Options/ConditionalInferenceTreeOptions.cs (1)

3-18: 🛠️ Refactor suggestion | 🟠 Major

Missing <para><b>Reference:</b> in class documentation per golden pattern.

The coding guidelines require Options classes to include a <para><b>Reference:</b> section citing the original research paper. The constructor comment references "Hothorn et al. 2006" but this citation should appear in the class-level XML documentation for discoverability and consistency with the golden pattern.

📚 Proposed fix - add Reference section

 /// <para><b>For Beginners:</b> Conditional Inference Trees are a special type of decision tree that uses 
 /// statistics to make better decisions about how to split data. Regular decision trees sometimes favor 
 /// certain types of data unfairly (like preferring variables with more possible values). This approach is 
 /// like having a referee that makes sure the tree-building process is fair and statistically sound. 
 /// The result is often a more reliable model, especially for data where some variables have many possible 
 /// values and others have few.</para>
+/// <para><b>Reference:</b> Hothorn, T., Hornik, K., &amp; Zeileis, A. (2006). Unbiased Recursive Partitioning: 
+/// A Conditional Inference Framework. Journal of Computational and Graphical Statistics, 15(3), 651-674.</para>
 /// </remarks>

As per coding guidelines: "XML Documentation on class ... with <para><b>Reference:</b> citing the original research paper."

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/Models/Options/ConditionalInferenceTreeOptions.cs` around lines 3 - 18,
Add a class-level XML <para><b>Reference:</b></para> section to the
ConditionalInferenceTreeOptions class documentation that cites the original
paper (Hothorn et al., 2006) consistent with the constructor comment and the
project's golden pattern; update the top-of-file XML doc block for the
ConditionalInferenceTreeOptions class to include the Reference paragraph with
the full citation text used in the constructor so discoverability and
consistency are maintained.

♻️ Duplicate comments (4)

model_test_progress.md (2)

21-30: ⚠️ Potential issue | 🟡 Minor

Markdown formatting: Add blank line after line 20 and around "Fixes Applied" heading.

📝 Proposed fix

 | Clustering | Partial: KMeans 18/19 pass. Needs k=1 overrides for all k-based models' SingleCluster test. Builder tests crash (OOM). |
+

 ## Fixes Applied
+
 1. **DeepGaussianProcess.cs** — CI coverage 0% → PASS. Training variance floor. Per Damianou & Lawrence 2013.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@model_test_progress.md` around lines 21 - 30, Add a blank line after the
existing content before the "Fixes Applied" heading and ensure there's an empty
line both above and below the "Fixes Applied" heading in model_test_progress.md
so the Markdown renders correctly; locate the heading text "Fixes Applied" and
insert a single blank line before it and another blank line after it.

---

3-11: ⚠️ Potential issue | 🟡 Minor

Markdown formatting: Add blank lines around headings and tables.

Static analysis flags MD022 and MD058. The heading at line 3 needs a blank line before the table, and other headings (lines 21, 31) similarly need surrounding blank lines. This was supposedly addressed in a prior commit but appears to still be an issue.

📝 Proposed fix

 # Model Family Test Progress

 ## Test Coverage by Category
+
 | Category | Pass Rate | Notes |
 |---|---|---|
 | ActivationFunctions | 260/260 (100%) | Fixed in other PR |
 | LossFunctions | 36/36 (100%) | Fixed in other PR |
 | NeuralNetworks/Layers | ~860/912 (94%) | Fixed in other PR |
 | GaussianProcess | 128/128 (100%) | 1 fix: DeepGP CI variance floor |
 | Regression | 1253/1254 (99.9%) | 57 models × 22 tests. 1 ConditionalInferenceTree CoefficientSigns |
 | TimeSeries | 377/377 (100%) | 29 models × 13 invariants — ALL PASS |
+

 ## Blocked
+
 | Category | Status |

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@model_test_progress.md` around lines 3 - 11, Add blank lines around Markdown
headings and tables: ensure each heading (e.g., "## Test Coverage by Category"
and the other section headings referenced in the file) has a blank line before
and after it, and that any Markdown table blocks are preceded and followed by an
empty line so the file satisfies MD022/MD058; update the surrounding lines in
the markdown so there is a single blank line separating headings, paragraphs,
and table blocks throughout the document.

src/NeuralNetworks/VideoCLIPNeuralNetwork.cs (1)

1981-1982: ⚠️ Potential issue | 🟡 Minor

Missing validation for deserialized _temporalAggregation enum value.

The deserialization reads an integer and casts directly to TemporalAggregationType without validating it's a defined enum value. Corrupted or malicious data could set an invalid enum state.

🛡️ Proposed fix to validate the enum

-        _temporalAggregation = (TemporalAggregationType)reader.ReadInt32();
+        int temporalAggregationValue = reader.ReadInt32();
+        if (!Enum.IsDefined(typeof(TemporalAggregationType), temporalAggregationValue))
+        {
+            throw new InvalidDataException(
+                $"Unknown temporal aggregation value: {temporalAggregationValue}");
+        }
+        _temporalAggregation = (TemporalAggregationType)temporalAggregationValue;

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/NeuralNetworks/VideoCLIPNeuralNetwork.cs` around lines 1981 - 1982, The
deserialization currently casts reader.ReadInt32() directly to
TemporalAggregationType into _temporalAggregation which can produce invalid enum
values; change the code to read the int into a temporary variable, validate it
with Enum.IsDefined(typeof(TemporalAggregationType), value) (or Enum.TryParse)
and only assign to _temporalAggregation if valid, otherwise handle it by
throwing an InvalidDataException or assigning a safe default (e.g.,
TemporalAggregationType.None) and logging the error; update the block that
currently contains "_temporalAggregation =
(TemporalAggregationType)reader.ReadInt32();" and keep the subsequent
"_useNativeMode = reader.ReadBoolean();" intact.

src/Helpers/DeserializationHelper.cs (1)

172-177: ⚠️ Potential issue | 🟡 Minor

Edge case: potential division by zero when Math.Round(sqrtVal) equals 0.

The condition guards against numPatches > 0 && imageWidth > 0, but if imageHeight is very small relative to the calculation, Math.Round(sqrtVal) can still round to 0, causing imageHeight / 0 on line 176. The fallback : 16 only applies to sqrtVal > 0, not to the divisor being zero.

🛡️ Proposed fix to guard the divisor

             else if (numPatches > 0 && imageWidth > 0)
             {
                 // Derive: numPatches = (H/P) * (W/P) → P = H / sqrt(numPatches * H/W)
                 double sqrtVal = Math.Sqrt((double)numPatches * imageHeight / imageWidth);
-                patchSize = sqrtVal > 0 ? (imageHeight / (int)Math.Round(sqrtVal)) : 16;
+                int divisor = (int)Math.Round(sqrtVal);
+                patchSize = divisor > 0 ? (imageHeight / divisor) : 16;
             }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/Helpers/DeserializationHelper.cs` around lines 172 - 177, The calculation
for patchSize can divide by zero if Math.Round(sqrtVal) returns 0; update the
block that computes patchSize (referencing sqrtVal, numPatches, imageHeight,
imageWidth, Math.Round) to explicitly compute an int rounded =
(int)Math.Round(sqrtVal) and check if rounded > 0 before using imageHeight /
rounded, otherwise set patchSize to the safe fallback (e.g., 16); keep the
existing sqrtVal > 0 guard but ensure the divisor is validated to avoid
imageHeight / 0.

🤖 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/VideoCLIPNeuralNetwork.cs`:
- Around line 1998-2014: Add a defensive bounds check before redistributing
entries from Layers to prevent IndexOutOfRangeException: compute the expected
total as 1 (patch) + _numFrameEncoderLayers + _numTemporalLayers + 1
(videoProjection) + 1 (textTokenEmbedding) + _numTextLayers + 1 (textProjection)
+ 1 (captionHead), then verify Layers.Count >= expectedTotal; if not, throw a
clear descriptive exception (or handle fallback) stating the mismatch and
include Layers.Count and expectedTotal so callers can debug; apply this check
before using idx and only proceed to assign _patchEmbedding,
_frameEncoderLayers, _temporalEncoderLayers, _videoProjection,
_textTokenEmbedding, _textEncoderLayers, _textProjection, and _captionHead.

---

Outside diff comments:
In `@src/Models/Options/ConditionalInferenceTreeOptions.cs`:
- Around line 3-18: Add a class-level XML <para><b>Reference:</b></para> section
to the ConditionalInferenceTreeOptions class documentation that cites the
original paper (Hothorn et al., 2006) consistent with the constructor comment
and the project's golden pattern; update the top-of-file XML doc block for the
ConditionalInferenceTreeOptions class to include the Reference paragraph with
the full citation text used in the constructor so discoverability and
consistency are maintained.

---

Duplicate comments:
In `@model_test_progress.md`:
- Around line 21-30: Add a blank line after the existing content before the
"Fixes Applied" heading and ensure there's an empty line both above and below
the "Fixes Applied" heading in model_test_progress.md so the Markdown renders
correctly; locate the heading text "Fixes Applied" and insert a single blank
line before it and another blank line after it.
- Around line 3-11: Add blank lines around Markdown headings and tables: ensure
each heading (e.g., "## Test Coverage by Category" and the other section
headings referenced in the file) has a blank line before and after it, and that
any Markdown table blocks are preceded and followed by an empty line so the file
satisfies MD022/MD058; update the surrounding lines in the markdown so there is
a single blank line separating headings, paragraphs, and table blocks throughout
the document.

In `@src/Helpers/DeserializationHelper.cs`:
- Around line 172-177: The calculation for patchSize can divide by zero if
Math.Round(sqrtVal) returns 0; update the block that computes patchSize
(referencing sqrtVal, numPatches, imageHeight, imageWidth, Math.Round) to
explicitly compute an int rounded = (int)Math.Round(sqrtVal) and check if
rounded > 0 before using imageHeight / rounded, otherwise set patchSize to the
safe fallback (e.g., 16); keep the existing sqrtVal > 0 guard but ensure the
divisor is validated to avoid imageHeight / 0.

In `@src/NeuralNetworks/VideoCLIPNeuralNetwork.cs`:
- Around line 1981-1982: The deserialization currently casts reader.ReadInt32()
directly to TemporalAggregationType into _temporalAggregation which can produce
invalid enum values; change the code to read the int into a temporary variable,
validate it with Enum.IsDefined(typeof(TemporalAggregationType), value) (or
Enum.TryParse) and only assign to _temporalAggregation if valid, otherwise
handle it by throwing an InvalidDataException or assigning a safe default (e.g.,
TemporalAggregationType.None) and logging the error; update the block that
currently contains "_temporalAggregation =
(TemporalAggregationType)reader.ReadInt32();" and keep the subsequent
"_useNativeMode = reader.ReadBoolean();" intact.

🪄 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: 75b71dac-2a23-4b03-bf49-399248ef305b

📥 Commits

Reviewing files that changed from the base of the PR and between 3c8a163 and b949d46.

📒 Files selected for processing (4)

• model_test_progress.md
• src/Helpers/DeserializationHelper.cs
• src/Models/Options/ConditionalInferenceTreeOptions.cs
• src/NeuralNetworks/VideoCLIPNeuralNetwork.cs

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/NeuralNetworks/VideoCLIPNeuralNetwork.cs (1)

1591-1626: ⚠️ Potential issue | 🔴 Critical

BLOCKING: Backward method discards computed input gradients, violating the golden pattern.

The frame encoder's backward loop correctly calls _patchEmbedding.Backward(grad) at line 1621, but the return value is discarded. Per the neural network golden pattern, the Backward method "Must implement actual backpropagation through all layers"—which means input gradients must be accumulated and propagated, not thrown away.

The method returns currentGradient (the temporal encoder gradient), but the input gradients from _patchEmbedding.Backward() are lost. This is incomplete:

grad = _frameEncoderLayers[i].Backward(grad);
_patchEmbedding.Backward(grad);  // Return value discarded

Should be:

grad = _frameEncoderLayers[i].Backward(grad);
grad = _patchEmbedding.Backward(grad);  // Capture and accumulate
// Then return accumulated input gradients

Either accumulate and return the full input gradient chain, or document explicitly why gradients are intentionally not propagated beyond the frame encoder.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/NeuralNetworks/VideoCLIPNeuralNetwork.cs` around lines 1591 - 1626, In
the Backward method, you are discarding the input gradients returned by
`_patchEmbedding.Backward(grad)`; capture and propagate them instead: after the
loop that sets `grad = _frameEncoderLayers[i].Backward(grad)`, assign `grad =
_patchEmbedding.Backward(grad)` and then accumulate this into the overall input
gradient you will return (e.g., add into `currentGradient` or a new
`inputGradient` tensor per frame), so the method returns the full propagated
gradients rather than dropping the patch-embedding input gradients; ensure you
handle accumulation across frames and return that accumulated tensor instead of
the original `currentGradient`.

♻️ Duplicate comments (1)

src/NeuralNetworks/VideoCLIPNeuralNetwork.cs (1)

1981-1988: ⚠️ Potential issue | 🔴 Critical

Blocking: Missing enum validation and ONNX mode guard during deserialization.

Two issues remain unaddressed:

1. Missing enum validation: The TemporalAggregationType value read at line 1981 is not validated with Enum.IsDefined(). Corrupt or tampered data could produce an invalid enum state.

2. ONNX mode deserialization still broken: When _useNativeMode is false (line 1982), the subsequent matrix deserialization and layer redistribution will produce an inconsistent model state. The ONNX InferenceSession objects (_videoEncoder, _textEncoder) cannot be restored from this serialization format, yet no guard prevents this path.

Per coding guidelines, "Incomplete features: Half-implemented patterns where some code paths work but others silently do nothing" are blocking.

Proposed fix

-        _temporalAggregation = (TemporalAggregationType)reader.ReadInt32();
-        _useNativeMode = reader.ReadBoolean();
+        int temporalAggValue = reader.ReadInt32();
+        if (!Enum.IsDefined(typeof(TemporalAggregationType), temporalAggValue))
+        {
+            throw new InvalidDataException(
+                $"Invalid TemporalAggregationType value: {temporalAggValue}");
+        }
+        _temporalAggregation = (TemporalAggregationType)temporalAggValue;
+
+        bool useNativeMode = reader.ReadBoolean();
+        if (!useNativeMode)
+        {
+            throw new NotSupportedException(
+                "Deserializing ONNX-backed VideoCLIP models is not supported. " +
+                "ONNX session state cannot be restored from this serialization format.");
+        }
+        _useNativeMode = useNativeMode;

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/NeuralNetworks/VideoCLIPNeuralNetwork.cs` around lines 1981 - 1988, When
deserializing, validate the TemporalAggregationType value returned for
_temporalAggregation using Enum.IsDefined(TemporalAggregationType, value) and
throw a descriptive exception (e.g., InvalidDataException) if it's not valid to
avoid corrupt enum states; also add a guard around the ONNX path so that when
_useNativeMode is false you do NOT proceed to deserialize or redistribute
matrices for ONNX (i.e., skip or fail before calling DeserializeMatrix for
_visionClsToken, _visionPositionalEmbeddings, _temporalPositionalEmbeddings,
_textPositionalEmbeddings and before attempting to restore _videoEncoder or
_textEncoder), and instead throw a clear exception or set a flag indicating ONNX
restoration is unsupported so callers can't end up with an inconsistent model
state.

🤖 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/VideoCLIPNeuralNetwork.cs`:
- Around line 1591-1626: In the Backward method, you are discarding the input
gradients returned by `_patchEmbedding.Backward(grad)`; capture and propagate
them instead: after the loop that sets `grad =
_frameEncoderLayers[i].Backward(grad)`, assign `grad =
_patchEmbedding.Backward(grad)` and then accumulate this into the overall input
gradient you will return (e.g., add into `currentGradient` or a new
`inputGradient` tensor per frame), so the method returns the full propagated
gradients rather than dropping the patch-embedding input gradients; ensure you
handle accumulation across frames and return that accumulated tensor instead of
the original `currentGradient`.

---

Duplicate comments:
In `@src/NeuralNetworks/VideoCLIPNeuralNetwork.cs`:
- Around line 1981-1988: When deserializing, validate the
TemporalAggregationType value returned for _temporalAggregation using
Enum.IsDefined(TemporalAggregationType, value) and throw a descriptive exception
(e.g., InvalidDataException) if it's not valid to avoid corrupt enum states;
also add a guard around the ONNX path so that when _useNativeMode is false you
do NOT proceed to deserialize or redistribute matrices for ONNX (i.e., skip or
fail before calling DeserializeMatrix for _visionClsToken,
_visionPositionalEmbeddings, _temporalPositionalEmbeddings,
_textPositionalEmbeddings and before attempting to restore _videoEncoder or
_textEncoder), and instead throw a clear exception or set a flag indicating ONNX
restoration is unsupported so callers can't end up with an inconsistent model
state.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: dbd28109-7b67-40fb-aee1-5d06831ee2df

📥 Commits

Reviewing files that changed from the base of the PR and between b949d46 and cb19577.

📒 Files selected for processing (1)

• src/NeuralNetworks/VideoCLIPNeuralNetwork.cs

[vercel]

Deployment failed with the following error:

Resource is limited - try again in 24 hours (more than 100, code: "api-deployments-free-per-day").

Learn More: https://vercel.com/franklins-projects-02a0b5a0?upgradeToPro=build-rate-limit