fix: fix/issue 1310 factory stubs - #1318
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
<review_stack_artifact> </review_stack_artifact> ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 24
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/AiDotNet.Generators/TestScaffoldGenerator.cs`:
- Around line 685-689: The current logic wrongly adds non-constructible models
(when canConstruct is false) to autoGenerated and later moves them into
testedModels, which suppresses AIDN040 and misstates coverage; update the logic
in TestScaffoldGenerator around the canConstruct check so that when canConstruct
is false you do NOT add the model to autoGenerated or to testedModels (instead
record it as needing manual scaffold or leave it unmarked), and ensure any later
block that moves items from autoGenerated into testedModels skips models that
were non-constructible; adjust the handling of the model variable and the
autoGenerated and testedModels collections so non-constructible models remain
flagged for manual work and do not get suppressed by AIDN040.
In `@src/Helpers/LayerHelper.cs`:
- Around line 23512-23519: Add an explicit validation at the start of the
factory method(s) in LayerHelper that accept the parameter patchSize (the method
containing IActivationFunction<T> geluActivation / identityActivation and
variables visionFfnDim/decoderFfnDim) to ensure patchSize > 0 and throw an
ArgumentOutOfRangeException (or equivalent) with a clear message if not; apply
the same check to the other LayerHelper factory overloads referenced (the
methods at the other locations that accept patchSize) so invalid non‑positive
values are rejected immediately at the factory boundary.
In `@src/NeuralNetworks/SyntheticData/CopulaGANGenerator.cs`:
- Around line 670-685: The critic loss currently uses only lossTensor =
E[D(fake)] - E[D(real)] and omits the gradient penalty; update the loss to add
the gradient-penalty term scaled by _options.GradientPenaltyWeight (e.g.,
lossTensor = avgFake - avgReal + _options.GradientPenaltyWeight * gradPenalty)
where gradPenalty is computed from the real/fake inputs' interpolated gradients
(use the same gradient-penalty routine used elsewhere in this class or implement
a ComputeGradientPenalty method that returns the scalar penalty Tensor<T>);
ensure grads = tape.ComputeGradients(...) is computed from this new lossTensor,
lossValue is taken from the updated lossTensor, and RecomputeLoss also returns
the mean plus the same gradient-penalty term so the TapeStepContext and
_optimizer.Step(context) optimize the WGAN-GP objective.
- Around line 745-763: The real rows are being sampled with fresh
SampleConditionAndRow() calls while condBatch is sampled separately, so real
rows and conditions don't match; fix by sampling condition-row pairs once and
using the same pairs for both condBatch and realFlat. Concretely, change
SampleConditionalBatchTensor (or add a new method like
SampleConditionalBatchPairs) to return both the conditional tensor and the
corresponding row indices (or row pointers), build condBatch from that single
sample, and in the loop that fills realFlat use those returned row indices
instead of calling _sampler!.SampleConditionAndRow() again; ensure realSingles
is then constructed from the realFlat and the same condBatch so (real row,
condition) pairs match.
In `@src/NeuralNetworks/SyntheticData/CTGANGenerator.cs`:
- Around line 681-712: BuildPackedRealAndFakeBatches currently generates
condBatch independently and then samples real rows separately, causing
misaligned condition vectors; fix by sampling the condition+row pair once per
sample via _sampler.SampleConditionAndRow() and use that sampled condition
vector both when building the realFlat/realSingles and when constructing the
generator input (genInput) instead of the separately created condBatch.
Concretely: in BuildPackedRealAndFakeBatches, replace the call to
SampleConditionalBatchTensor and the independent loop that calls
_sampler.SampleConditionAndRow() for row indices with a single loop (or a
sampler call that returns both) that collects both the row index (for realFlat)
and the corresponding condition vector (to build condBatch used in genInput and
later concatenated into realSingles and fakeSingles), ensuring the shapes still
match for GenerateNoiseBatchTensor, Engine.TensorConcatenate(genInput), and the
subsequent Reshape into realPacked/fakePacked.
- Around line 590-606: The critic loss currently omits the WGAN-GP term —
restore use of _options.GradientPenaltyWeight by computing the gradient penalty
and adding it to lossTensor before computing grads and lossValue: after avgFake
and avgReal, compute an interpolated batch x_hat between realPacked and
fakePacked, run DiscriminatorForwardBatched(x_hat, true) to get d_hat, compute
gradients of d_hat w.r.t. x_hat, form gp = _options.GradientPenaltyWeight *
Mean((Norm(gradients, axis=...)-1)^2) and then set lossTensor =
TensorSubtract(avgFake, avgReal) + gp; ensure tape.ComputeGradients(lossTensor,
discParams) and lossValue use the updated lossTensor, and update RecomputeLoss
(the lambda used by TapeStepContext) to include the same gradient-penalty term
so the optimizer sees the identical loss in the replay step; reference
DiscriminatorForwardBatched, realPacked/fakePacked, lossTensor,
tape.ComputeGradients, RecomputeLoss, TapeStepContext<T>, and _optimizer.Step
when making these changes.
In `@src/NeuralNetworks/SyntheticData/DPCTGANGenerator.cs`:
- Around line 608-625: The current code calls tape.ComputeGradients which
returns an already-aggregated batch gradient and then calls
ClipAndNoiseGradients on those aggregated tensors, which is not DP-SGD; instead
you must compute per-example gradients, clip each example's per-parameter
gradient to ClipNorm, then aggregate (sum or average) those clipped per-example
gradients and finally add Gaussian noise scaled by ClipNorm × noiseMultiplier
before passing them to the optimizer; refactor the gradient computation flow so
that Tape/ComputeGradients (or a new method) is invoked per example (or
otherwise returns per-example grads), apply per-example clipping, aggregate the
clipped grads, call the existing noise-addition logic (or move
ClipAndNoiseGradients to operate on aggregated clipped grads only for noise),
and then construct the TapeStepContext (used by _optimizer.Step) with the
correctly noised aggregated gradients instead of clipping after aggregation.
- Around line 716-738: BuildPackedRealAndFakeBatches currently samples condBatch
independently of the real rows, causing mismatched condition-row pairs; fix it
by sampling a single SampleConditionAndRow() per example and using that sampled
condition both when forming condBatch (used for the fake inputs) and when
selecting the corresponding real row in realFlat. Concretely, replace the call
to SampleConditionalBatchTensor(total) with code that iterates s=0..total-1,
calls _sampler!.SampleConditionAndRow() once per s to get the condition vector
and RowIndex, writes the condition into the condBatch at index s, and copies the
matched transformedData row into realFlat[s,*]; keep the later concatenations
(fakeSingles and realSingles) and reshapes the same.
In `@src/NeuralNetworks/SyntheticData/TableGANGenerator.cs`:
- Around line 449-507: TrainGeneratorStepBatched currently builds lossTensor
from only the adversarial term and mean matching; update it to also include the
classification loss scaled by _options.ClassificationWeight and the
variance-matching term using _realVar scaled by _options.InformationWeight when
those targets are present. Concretely, after computing fakeActivated and before
tape.ComputeGradients, compute fake feature variance (reduce mean of (x -
mean)^2) using VectorToTensor(_realVar) and add
Engine.TensorMultiplyScalar(varianceLoss, infoWeight) to lossTensor when
_realVar != null and InformationWeight>0; likewise, if classification
labels/weights are configured (check _options.ClassificationWeight>0 and
whatever label branch the code uses), compute the generator classification loss
from the discriminator/classifier outputs on fakeActivated, scale it by
NumOps.FromDouble(_options.ClassificationWeight) and add it into lossTensor so
both classification and full information (mean+variance) objectives contribute
to the gradients used by _optimizer.Step.
In `@src/VisionLanguage/InstructionTuned/DeepSeekVL.cs`:
- Line 133: The ComputePatchSize method currently uses integer truncation which
can produce too-large patch counts; change its logic to compute the patch grid
size by taking the ceiling of the square root of MaxVisualTokens (i.e., use
Math.Ceiling on Math.Sqrt(_options.MaxVisualTokens)), then divide ImageSize by
that ceiling value (ensuring the result is at least 1) so patch size rounds down
correctly and the number of patches does not exceed MaxVisualTokens; update the
ComputePatchSize return expression to use the ceiling-based divisor and keep the
Math.Max(1, ...) guards around both the divisor cast and final result.
In `@src/VisionLanguage/InstructionTuned/DeepSeekVL2.cs`:
- Line 135: ComputePatchSize() currently uses integer truncation which can
produce patches that are too small and exceed MaxVisualTokens; change the
calculation to compute patchesPerSide = Math.Max(1,
(int)Math.Ceiling(Math.Sqrt(_options.MaxVisualTokens))) and then compute
patchSize = Math.Max(1, (int)Math.Ceiling((double)_options.ImageSize /
patchesPerSide)) so you use floating-point division and ceiling to avoid
producing too many visual tokens; update the ComputePatchSize method to follow
this approach, referring to the ComputePatchSize() method and
_options.ImageSize/_options.MaxVisualTokens fields.
In `@src/VisionLanguage/InstructionTuned/Gemma3.cs`:
- Line 132: ComputePatchSize currently truncates divisions and uses integer cast
on Math.Sqrt which can under-estimate patch size and produce too many visual
tokens; change the logic in ComputePatchSize to (1) compute patchesPerSide as
Math.Max(1, (int)Math.Ceiling(Math.Sqrt(_options.MaxVisualTokens))) and (2)
compute patchSize using Math.Ceiling on the division of _options.ImageSize by
patchesPerSide (then cast to int and Math.Max with 1) so the function rounds up
rather than truncating and thus respects the visual-token budget and tensor
shapes.
In `@src/VisionLanguage/InstructionTuned/InternVL.cs`:
- Line 134: The current ComputePatchSize uses (int)Math.Sqrt(...) which
truncates and can produce too-small patch sizes; change it to compute
patchesPerDim = (int)Math.Ceiling(Math.Sqrt(_options.MaxVisualTokens)) and then
return Math.Max(1, _options.ImageSize / Math.Max(1, patchesPerDim)) so the patch
size is rounded up in the token-dimension conversion and thus keeps the total
visual token count within _options.MaxVisualTokens; update the ComputePatchSize
method to use Math.Ceiling on the sqrt of _options.MaxVisualTokens and reference
_options.ImageSize/_options.MaxVisualTokens accordingly.
In `@src/VisionLanguage/InstructionTuned/InternVL2.cs`:
- Around line 130-135: ComputePatchSize currently coerces invalid
_options.ImageSize/_options.MaxVisualTokens to 1; instead validate inputs and
fail fast: in ComputePatchSize check that _options.ImageSize > 0 and
_options.MaxVisualTokens > 0 and throw ArgumentException (including values) if
not. In ComputeEncoderDecoderBoundary validate the computed _encoderLayerEnd
stays within valid range for Layers (e.g., >=0 and <= Layers.Count after layer
construction) and throw InvalidOperationException with context if out of bounds;
reference ComputePatchSize, ComputeEncoderDecoderBoundary, _encoderLayerEnd,
_options.ImageSize, and _options.MaxVisualTokens to locate and implement these
checks.
In `@src/VisionLanguage/InstructionTuned/InternVL25.cs`:
- Line 133: ComputePatchSize currently floors the patch size and can yield more
visual tokens than _options.MaxVisualTokens for non-divisible sizes; change the
logic to compute a target patches-per-side as
ceil(sqrt(_options.MaxVisualTokens)) and then set patchSize = Math.Max(1,
(int)Ceiling((double)_options.ImageSize / targetPatchesPerSide)) so the
resulting number of patches (ceil(ImageSize/patchSize)^2) cannot exceed
MaxVisualTokens; update the ComputePatchSize() implementation to use these
calculations and reference _options.ImageSize and _options.MaxVisualTokens.
In `@src/VisionLanguage/InstructionTuned/InternVL3.cs`:
- Line 133: ComputePatchSize currently uses integer truncation which can allow
more visual tokens than _options.MaxVisualTokens; change the calculation to use
a ceiling-based divisor so the number of patches per side is
ceil(sqrt(_options.MaxVisualTokens)). Specifically, in ComputePatchSize replace
the divisor Math.Max(1, (int)Math.Sqrt(_options.MaxVisualTokens)) with
Math.Max(1, (int)Math.Ceiling(Math.Sqrt(_options.MaxVisualTokens))) and keep the
outer Math.Max(1, _options.ImageSize / divisor) to ensure patch size >=1,
referencing the ComputePatchSize method and
_options.ImageSize/_options.MaxVisualTokens fields.
In `@src/VisionLanguage/InstructionTuned/Llama32Vision.cs`:
- Line 134: The current ComputePatchSize uses integer truncation which can yield
a patch size that creates more than _options.MaxVisualTokens patches; change the
calculation to use floating-point division and Math.Ceiling so the patch size is
large enough to guarantee the number of patches <= _options.MaxVisualTokens.
Specifically, update ComputePatchSize to cast to double, compute patchSize =
(int)Math.Max(1, Math.Ceiling((double)_options.ImageSize /
Math.Sqrt((double)Math.Max(1, _options.MaxVisualTokens)))); keep the Math.Max(1,
...) guard and the method name ComputePatchSize and fields
_options.ImageSize/_options.MaxVisualTokens to locate the change.
In `@src/VisionLanguage/InstructionTuned/Phi3Vision.cs`:
- Around line 129-136: Validate _options.ImageSize and _options.MaxVisualTokens
before calling ComputePatchSize: ensure both are positive integers (>=1), and
that Math.Sqrt(_options.MaxVisualTokens) won't be zero; if invalid, throw or
return a clear configuration error rather than deriving a patch size. After
Layers.AddRange(... LayerHelper<T>.CreateDefaultVisionAdapterLayers(...)) run
ComputeEncoderDecoderBoundary() validation: verify the computed _encoderLayerEnd
is within the valid range (e.g., >0 and <= Layers.Count) and adjust or fail with
a descriptive exception if out of bounds; update ComputePatchSize and
ComputeEncoderDecoderBoundary to perform/propagate these checks and use the same
symbols (_options.ImageSize, _options.MaxVisualTokens, ComputePatchSize,
ComputeEncoderDecoderBoundary, _encoderLayerEnd,
Layers.AddRange/LayerHelper.CreateDefaultVisionAdapterLayers) so callers get a
validated configuration before proceeding.
In `@src/VisionLanguage/InstructionTuned/Phi4Multimodal.cs`:
- Around line 130-136: ComputePatchSize and ComputeEncoderDecoderBoundary
currently silently accept invalid inputs and assume the helper's layer count;
add explicit validation to fail fast: in ComputePatchSize validate
_options.ImageSize > 0 and _options.MaxVisualTokens > 0 and throw an
ArgumentException with clear message if not, compute patchSize and ensure 1 <=
patchSize <= _options.ImageSize (throw if violated); in the code path after
calling LayerHelper<T>.CreateDefaultVisionAdapterLayers (or inside
ComputeEncoderDecoderBoundary) compute the expected boundary using the same
formula (use lpb = _options.DropoutRate > 0 ? 6 : 5 and expected = 2 +
_options.NumVisionLayers * lpb + 6) and compare it to the actual
Layers.Count/derived boundary, throwing an InvalidOperationException if they
differ and include both values in the message so mismatches between helper
output and boundary math are explicit.
In `@src/VisionLanguage/InstructionTuned/Pixtral.cs`:
- Around line 119-125: Harden ComputePatchSize by validating inputs: in
ComputePatchSize() throw ArgumentException if _options.ImageSize <= 0 or
_options.MaxVisualTokens <= 0, compute patchSize = _options.ImageSize /
Math.Max(1, (int)Math.Sqrt(_options.MaxVisualTokens)) and if patchSize < 1 throw
ArgumentException instead of returning 1; in the initialization path that calls
Layers.AddRange(..., patchSize: ComputePatchSize()) (the InitializeLayers/else
block that adds vision adapter layers) validate or catch these exceptions so
invalid configs fail fast; after adding layers call
ComputeEncoderDecoderBoundary() and validate the resulting _encoderLayerEnd is
<= Layers.Count and >= 0 (throw InvalidOperationException/ArgumentException if
not) to ensure the encoder/decoder boundary is sane.
In `@src/VisionLanguage/InstructionTuned/PixtralLarge.cs`:
- Around line 121-127: ComputePatchSize currently silently clamps to 1 and
ComputeEncoderDecoderBoundary computes _encoderLayerEnd without validating
inputs; update ComputePatchSize to validate _options.ImageSize and
_options.MaxVisualTokens (throw or log and reject if non-positive or absurd)
instead of silently falling back, and update ComputeEncoderDecoderBoundary to
validate _options.NumVisionLayers, _options.DropoutRate and the computed
_encoderLayerEnd (ensure it falls within a sensible range >= 2 and <=
Layers.Count or throw/log and adjust), using the methods ComputePatchSize and
ComputeEncoderDecoderBoundary to perform these checks and fail fast with clear
error messages when inputs are invalid.
In `@src/VisionLanguage/InstructionTuned/SmolVLM.cs`:
- Line 116: ComputePatchSize currently uses integer division which floors and
can produce a patch size that yields more visual tokens than
_options.MaxVisualTokens; change ComputePatchSize to compute patchesPerSide =
ceil(sqrt(_options.MaxVisualTokens)) and then compute patchSize = Math.Max(1,
ceil((double)_options.ImageSize / patchesPerSide)) so the division rounds up;
update the implementation in the ComputePatchSize method to use Math.Ceiling
with a double cast and reference _options.ImageSize and _options.MaxVisualTokens
accordingly to ensure the produced number of patches does not exceed
MaxVisualTokens.
In `@src/VisionLanguage/Reasoning/KimiVL.cs`:
- Around line 208-212: Validate patch-size inputs and encoder boundary: in
ComputePatchSize() check _options.ImageSize and _options.MaxVisualTokens are
positive and make sense (throw ArgumentException with clear message if not)
instead of silently clamping; in ComputeEncoderDecoderBoundary() validate
_options.NumVisionLayers, _options.NumDecoderLayers and DropoutRate assumptions
and compute a boundary that is >0; then in InitializeLayers() after calling
ComputeEncoderDecoderBoundary() guard and verify _encoderLayerEnd is within the
generated Layers range (or Layers.Count if layers were added) and throw/abort
with a descriptive error if the boundary is out of range to fail fast before
using _encoderLayerEnd. Use the existing symbols ComputePatchSize,
ComputeEncoderDecoderBoundary, InitializeLayers and _encoderLayerEnd/_options
fields for locating changes.
In `@src/VisionLanguage/Reasoning/KimiVLThinking.cs`:
- Around line 219-222: Validate _options.ImageSize and _options.MaxVisualTokens
at the start of InitializeLayers/ComputePatchSize: ensure ImageSize > 0 and
MaxVisualTokens > 0 (or clamp to sensible defaults) and throw or log an
ArgumentException when invalid to avoid divide-by-zero or negative-sqrt; in
ComputePatchSize keep the Math but guard against Math.Sqrt returning <1 by
validating MaxVisualTokens first. After either branch of InitializeLayers (both
the Architecture.Layers path and the helper-based path that calls
ComputeEncoderDecoderBoundary), verify _encoderLayerEnd is within a valid range
(>=0 and <= Layers.Count) and adjust/clamp or throw if out of bounds; update
ComputeEncoderDecoderBoundary to validate _options.NumVisionLayers and
_options.NumDecoderLayers and ensure the computed _encoderLayerEnd is reasonable
before assigning. Include references to InitializeLayers, ComputePatchSize,
ComputeEncoderDecoderBoundary, _options.ImageSize, _options.MaxVisualTokens, and
_encoderLayerEnd when making 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: 745d41f3-c759-4ef7-a75d-be2e5b7c126c
📒 Files selected for processing (28)
src/AiDotNet.Generators/TestScaffoldGenerator.cssrc/Helpers/LayerHelper.cssrc/NeuralNetworks/SyntheticData/CTGANGenerator.cssrc/NeuralNetworks/SyntheticData/CopulaGANGenerator.cssrc/NeuralNetworks/SyntheticData/DPCTGANGenerator.cssrc/NeuralNetworks/SyntheticData/TableGANGenerator.cssrc/VisionLanguage/InstructionTuned/DeepSeekVL.cssrc/VisionLanguage/InstructionTuned/DeepSeekVL2.cssrc/VisionLanguage/InstructionTuned/Gemma3.cssrc/VisionLanguage/InstructionTuned/InternVL.cssrc/VisionLanguage/InstructionTuned/InternVL2.cssrc/VisionLanguage/InstructionTuned/InternVL25.cssrc/VisionLanguage/InstructionTuned/InternVL3.cssrc/VisionLanguage/InstructionTuned/Llama32Vision.cssrc/VisionLanguage/InstructionTuned/Phi3Vision.cssrc/VisionLanguage/InstructionTuned/Phi4Multimodal.cssrc/VisionLanguage/InstructionTuned/Pixtral.cssrc/VisionLanguage/InstructionTuned/PixtralLarge.cssrc/VisionLanguage/InstructionTuned/SmolVLM.cssrc/VisionLanguage/Reasoning/KimiVL.cssrc/VisionLanguage/Reasoning/KimiVLThinking.cstests/AiDotNet.Tests/ModelFamilyTests/NeuralNetworks/ACEStepTests.cstests/AiDotNet.Tests/ModelFamilyTests/NeuralNetworks/DocumentReaderTests.cstests/AiDotNet.Tests/ModelFamilyTests/NeuralNetworks/EmotiVoiceTests.cstests/AiDotNet.Tests/ModelFamilyTests/NeuralNetworks/Phi3VisionTests.cstests/AiDotNet.Tests/ModelFamilyTests/NeuralNetworks/SmolVLMTests.cstests/AiDotNet.Tests/ModelFamilyTests/NeuralNetworks/TableGANGeneratorTests.cstests/AiDotNet.Tests/ModelFamilyTests/NeuralNetworks/TransfusionTests.cs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/NeuralNetworks/SyntheticData/CausalGANGenerator.cs (3)
305-318:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winBLOCKING: Train method is incomplete — gradient computed but never applied.
This method computes
gradientbut does nothing with it. The trailing comment// Backward through generatorconfirms this is unfinished work. Per coding guidelines, stubs and placeholder implementations are blocking issues.Production-ready code must:
- Backpropagate the gradient through the generator layers
- Update parameters using the optimizer
🔧 Proposed fix
public override void Train(Tensor<T> input, Tensor<T> expectedOutput) { var output = Predict(input); // Simple MSE gradient var gradient = new Tensor<T>(output._shape); for (int i = 0; i < output.Length && i < expectedOutput.Length; i++) { gradient[i] = NumOps.FromDouble( 2.0 * (NumOps.ToDouble(output[i]) - NumOps.ToDouble(expectedOutput[i]))); } - // Backward through generator + // Backward through generator using tape-based training + using var tape = new GradientTape<T>(); + var genParams = TapeTrainingStep<T>.CollectParameters(Layers.Cast<ILayer<T>>()); + + var fwdOutput = Predict(input); + var lossTensor = _lossFunction.ComputeLoss(fwdOutput, expectedOutput); + + var grads = tape.ComputeGradients(lossTensor, genParams); + T lossValue = lossTensor.Length > 0 ? lossTensor[0] : NumOps.Zero; + + Tensor<T> ComputeForward(Tensor<T> inp, Tensor<T> _) => Predict(inp); + Tensor<T> RecomputeLoss(Tensor<T> pred, Tensor<T> target) => _lossFunction.ComputeLoss(pred, target); + + var context = new TapeStepContext<T>( + genParams, grads, lossValue, + input, expectedOutput, ComputeForward, RecomputeLoss, + parameterBuffer: null); + _optimizer.Step(context); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/NeuralNetworks/SyntheticData/CausalGANGenerator.cs` around lines 305 - 318, The Train method in CausalGANGenerator computes a gradient but never applies it; implement full backprop by passing the computed gradient into the generator's backward method (call the generator/backpropagation routine such as Backward or BackProp on the model that produced Predict), accumulate gradients on each trainable layer, and then invoke the optimizer update routine (e.g., Optimizer.Step(), UpdateParameters(), or ApplyGradients on the generator's parameters) to adjust weights; ensure you clear/zero gradients before the next step and handle batch dimension/shape consistency between output, gradient, and layer gradients.
986-988:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftBLOCKING: Empty Gradient Penalty region — WGAN-GP is incomplete without it.
The
#region Gradient Penaltyis empty, but:
- Class docstring at line 50 explicitly mentions "WGAN-GP loss for stable adversarial training"
- Options include
GradientPenaltyWeight(serialized at line 1190)- The paper reference (Gulrajani et al. 2017) in
TrainDiscriminatorStepBatcheddocstring is specifically for WGAN-GPWithout gradient penalty, the discriminator isn't constrained to be 1-Lipschitz, which defeats the purpose of WGAN-GP and leads to training instability. This is a half-implemented feature that silently degrades correctness.
Do you want me to generate a tape-tracked gradient penalty implementation that computes the penalty on interpolated samples between real and fake batches?
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/NeuralNetworks/SyntheticData/CausalGANGenerator.cs` around lines 986 - 988, The Gradient Penalty region is empty but WGAN-GP requires adding a gradient penalty term: implement a tape-tracked gradient penalty inside the existing `#region` Gradient Penalty and incorporate it into TrainDiscriminatorStepBatched. Specifically, sample random interpolation alpha between real and fake batches, compute interpolates = alpha * real + (1-alpha) * fake, pass interpolates through the discriminator (use the same forward used in TrainDiscriminatorStepBatched), record gradients of discriminator outputs w.r.t. interpolates, compute gradient norms per-sample (L2 norm), form penalty = GradientPenaltyWeight * mean((norms - 1)^2), and add this penalty to the discriminator loss before backprop; use the existing GradientPenaltyWeight option and ensure gradients are computed with tape/Autograd to avoid stopping gradient flow for other terms. Ensure to reference and update the loss variables used in TrainDiscriminatorStepBatched so the penalty participates in optimizer.step.
992-1010: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueConsider removing these dead code methods.
UpdateGeneratorParametersandUpdateDiscriminatorParametersare no longer used. The training pipeline has migrated to tape-based gradient computation (TrainDiscriminatorStepBatchedandTrainGeneratorStepBatched), which routes all parameter updates through_optimizer.Step(context)instead of the manuallayer.UpdateParameters(learningRate)pattern. These methods can be safely deleted.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/NeuralNetworks/SyntheticData/CausalGANGenerator.cs` around lines 992 - 1010, Remove the now-dead methods UpdateGeneratorParameters(T learningRate) and UpdateDiscriminatorParameters(T learningRate) from the class since training uses tape-based gradient updates via TrainDiscriminatorStepBatched, TrainGeneratorStepBatched and _optimizer.Step(context) instead of manual layer.UpdateParameters(learningRate); delete both method definitions and any references to _genBNLayers or _discLayers updates in those methods to avoid dead code.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.gitignore:
- Around line 425-426: The PR claims extensive neural-network changes but the
diff only adds two .gitignore entries ("/tools/_profiles" and "/.ci-failures");
verify and fix by either (A) adding the missing implementation and test files
referenced in the description (e.g., Transformer.cs, DeepBeliefNetwork.cs,
NeuralNetworkModel.cs and the GAN/tape training files) into the branch/commit so
the diff matches the PR objectives, or (B) update the PR title, commit message
and description to accurately reflect that the only change is the .gitignore
additions and explain why those patterns were added; ensure the final commit
includes the correct files or corrected metadata before requesting review.
In `@src/NeuralNetworks/SyntheticData/CausalGANGenerator.cs`:
- Around line 747-752: The docstring incorrectly shows a Hadamard product (x' =
x ⊙ (I + A)) while the code uses matrix multiplication
(Engine.TensorMatMul(fakeBatch, iPlusA)); update the batch-method docstring in
CausalGANGenerator to reflect matrix multiplication (e.g., x' = x * (I + A) or
explicitly “each sample row x is multiplied by (I + A)”) and make it consistent
with the single-sample docstring that states y = (I + W^T) * x; ensure
references to iPlusA and Engine.TensorMatMul remain accurate.
---
Outside diff comments:
In `@src/NeuralNetworks/SyntheticData/CausalGANGenerator.cs`:
- Around line 305-318: The Train method in CausalGANGenerator computes a
gradient but never applies it; implement full backprop by passing the computed
gradient into the generator's backward method (call the
generator/backpropagation routine such as Backward or BackProp on the model that
produced Predict), accumulate gradients on each trainable layer, and then invoke
the optimizer update routine (e.g., Optimizer.Step(), UpdateParameters(), or
ApplyGradients on the generator's parameters) to adjust weights; ensure you
clear/zero gradients before the next step and handle batch dimension/shape
consistency between output, gradient, and layer gradients.
- Around line 986-988: The Gradient Penalty region is empty but WGAN-GP requires
adding a gradient penalty term: implement a tape-tracked gradient penalty inside
the existing `#region` Gradient Penalty and incorporate it into
TrainDiscriminatorStepBatched. Specifically, sample random interpolation alpha
between real and fake batches, compute interpolates = alpha * real + (1-alpha) *
fake, pass interpolates through the discriminator (use the same forward used in
TrainDiscriminatorStepBatched), record gradients of discriminator outputs w.r.t.
interpolates, compute gradient norms per-sample (L2 norm), form penalty =
GradientPenaltyWeight * mean((norms - 1)^2), and add this penalty to the
discriminator loss before backprop; use the existing GradientPenaltyWeight
option and ensure gradients are computed with tape/Autograd to avoid stopping
gradient flow for other terms. Ensure to reference and update the loss variables
used in TrainDiscriminatorStepBatched so the penalty participates in
optimizer.step.
- Around line 992-1010: Remove the now-dead methods UpdateGeneratorParameters(T
learningRate) and UpdateDiscriminatorParameters(T learningRate) from the class
since training uses tape-based gradient updates via
TrainDiscriminatorStepBatched, TrainGeneratorStepBatched and
_optimizer.Step(context) instead of manual layer.UpdateParameters(learningRate);
delete both method definitions and any references to _genBNLayers or _discLayers
updates in those methods to avoid dead code.
🪄 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: c7320df0-7a4e-49cb-a405-a8efb18261f2
📒 Files selected for processing (2)
.gitignoresrc/NeuralNetworks/SyntheticData/CausalGANGenerator.cs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (2)
src/Helpers/LayerHelper.cs (1)
23758-23771:⚠️ Potential issue | 🔴 CriticalBlocking: validate
patchSizein this factory too.This overload now takes external
patchSizeinput and feeds it straight intoPatchEmbeddingLayer<T>without the same fail-fast guard the neighboring factories use. Production-ready code should reject non-positive values at the factory boundary instead of deferring the failure to layer construction.Suggested fix
{ + ValidatePatchSize(patchSize); + IActivationFunction<T> geluActivation = new GELUActivation<T>(); IActivationFunction<T> identityActivation = new IdentityActivation<T>(); int visionFfnDim = visionDim * 4;As per coding guidelines "Production Readiness (CRITICAL - Flag as BLOCKING): ... missing validation of external inputs ..."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Helpers/LayerHelper.cs` around lines 23758 - 23771, The factory method that creates the PatchEmbeddingLayer<T> accepts an external patchSize but does not validate it; add a guard at the start of this method to ensure patchSize > 0 and throw an ArgumentOutOfRangeException (or ArgumentException) with a clear message if invalid so we fail fast instead of deferring to PatchEmbeddingLayer<T>; locate the method (the factory that declares double dropoutRate, int patchSize = 16 and creates geluActivation/identityActivation/visionFfnDim/decoderFfnDim) and insert the validation before yielding new PatchEmbeddingLayer<T>(patchSize, visionDim, expectedInputChannels: 3).src/NeuralNetworks/SyntheticData/DPCTGANGenerator.cs (1)
675-744:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftClip/noise is still applied per PacGAN pack, not per source row.
This is still not row-level DP-SGD.
ComputePerExampleNoisedGradients(...)treats each packed row as one “example”, butBuildPackedRealAndFakeBatches(...)constructs each packed row from_options.PacSizesource records. That means clipping and noise are calibrated to groups of rows whileComputeStepPrivacyCost(...)still accounts as if the mechanism were row-level. The advertised(ε, δ)guarantee is therefore unsound.Before merge, either disable packing in the DP path (
PacSize = 1) or move clipping/noise to true per-row gradients before any PacGAN packing.Also applies to: 755-778
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/NeuralNetworks/SyntheticData/DPCTGANGenerator.cs` around lines 675 - 744, ComputePerExampleNoisedGradients currently treats each packed PacGAN row as an “example”, so clipping/noise are applied per pack while ComputeStepPrivacyCost assumes per-source-row privacy; to fix, ensure DP is applied per source row by either forcing _options.PacSize = 1 when _options.DifferentialPrivacy is enabled (disable packing in the DP path) or by moving the clipping/noise logic into the code that iterates source rows before BuildPackedRealAndFakeBatches packs them (compute per-source-row gradients, clip and add noise, then aggregate into packs); update ComputePerExampleNoisedGradients (and related logic around BuildPackedRealAndFakeBatches and ComputeStepPrivacyCost) to reference _options.PacSize and apply the chosen approach consistently (also adjust the same logic applied around the code region referenced at 755-778).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/NeuralNetworks/SyntheticData/CausalGANGenerator.cs`:
- Around line 608-615: The replayed loss must match the loss used to compute
grads: change RecomputeLoss so it recomputes mean(D(fake)) - mean(D(real)) (the
same objective used to compute grads) rather than just mean(pred); ensure the
TapeStepContext<T> is constructed with the fake and real batches (or a
ComputeForward that returns both fake and real scores) so RecomputeLoss can call
DiscriminatorForwardBatched for the fake and real inputs and return
Engine.ReduceMean(fakeScores, allAxes) - Engine.ReduceMean(realScores, allAxes);
update the context creation (and possibly the ComputeForward signature) so the
optimizer Step receives a context whose recompute exactly matches the original
gradient computation (refs: DiscriminatorForwardBatched, ComputeForward,
RecomputeLoss, grads, TapeStepContext<T>, _optimizer.Step).
- Around line 600-606: The critic loss currently only uses lossTensor =
E[D(fake)] - E[D(real)], dropping the WGAN-GP gradient-penalty controlled by
_options.GradientPenaltyWeight; restore the GP term by computing the gradient
penalty and adding _options.GradientPenaltyWeight * gp to lossTensor before
computing grads: create interpolates between realScores and fakeScores, evaluate
critic outputs on interpolates (using the same path as real/fake), compute
gradients of those outputs wrt interpolates, compute gp = Mean((norm(gradients)
- 1)^2) (use Engine/NumOps helpers consistent with existing code), multiply by
_options.GradientPenaltyWeight and add to lossTensor, then call
tape.ComputeGradients(lossTensor, discParams) and set lossValue from lossTensor
as before.
In `@src/NeuralNetworks/SyntheticData/DPCTGANGenerator.cs`:
- Around line 607-614: The replay closure currently passed to TapeStepContext<T>
(ComputeForward and RecomputeLoss) only computes mean(D(real)) while lossTensor
and noisedGrads were produced from mean(D(fake)) - mean(D(real)), so the
optimizer replays the wrong objective; update the closures used in the
TapeStepContext<T> construction so they reproduce the full critic objective:
ensure ComputeForward (or a new paired forward) returns discriminator outputs
for both fake and real (using DiscriminatorForwardBatched or by capturing the
fakePacked and realPacked tensors) and make RecomputeLoss compute
ReduceMean(D(fake), allAxes) - ReduceMean(D(real), allAxes) (matching how
lossTensor was computed) so the replayed loss matches lossValue and noisedGrads.
In `@src/NeuralNetworks/SyntheticData/TableGANGenerator.cs`:
- Around line 587-603: BuildClassificationTargetTensor currently reads labels
from the transformed tensor (realBatch) using _options.LabelColumnIndex which
refers to the raw schema, causing wrong classes after VGM/encoding; change the
pipeline so that each sampled row carries the original raw label (or map the
original label index through the transformer metadata) and then in
BuildClassificationTargetTensor use that raw label value (or decode the correct
transformed slice via the transformer metadata/column mapping) to compute
targetClass and one-hot into targets. Update the caller that constructs
realBatch/samples to either attach a parallel rawLabel array or perform a
transformation-step that converts _options.LabelColumnIndex into the
corresponding transformed slice indices (using transformer metadata) and use
those decoded values inside BuildClassificationTargetTensor instead of
realBatch[b, labelIdx].
- Around line 440-446: RecomputeLoss must replay the exact critic objective used
for gradient computation: replace the current single-term
Engine.ReduceMean(pred, ...) with the critic loss mean(D(fake)) - mean(D(real));
update the closure passed into TapeStepContext (where ComputeForward is
DiscriminatorForwardBatched and RecomputeLoss is defined) so RecomputeLoss
computes Engine.ReduceMean(predFake, allAxes, keepDims:false) minus
Engine.ReduceMean(predReal, allAxes, keepDims:false) (using the same
realBatch/fakeBatch or the corresponding tensors produced by
DiscriminatorForwardBatched) so that the optimizer.Step replay uses the
identical loss as ComputeGradients.
In `@src/VisionLanguage/Reasoning/KimiVL.cs`:
- Around line 208-219: The InitializeLayers implementation duplicates
patch/boundary logic; extract ValidatePatchOptions, ComputePatchSize,
ComputeEncoderDecoderBoundary, and ValidateEncoderDecoderBoundary into
VisionLanguageModelBase<T> (or a new shared helper) and have
KimiVL.InitializeLayers call the base/helper methods (e.g.,
base.ValidatePatchOptions(), base.ComputePatchSize(),
base.ComputeEncoderDecoderBoundary(), base.ValidateEncoderDecoderBoundary()) and
remove the duplicated methods from KimiVL; ensure any KimiVL-specific constants
(like the lpb/resamplerLpb computation using _options.DropoutRate and
_options.NumVisionLayers) are parameterized so the base/helper can compute
_encoderLayerEnd or return it to the subclass, preserving the existing behavior
in InitializeLayers and keeping layer boundary assignment (_encoderLayerEnd and
Layers population) consistent.
---
Duplicate comments:
In `@src/Helpers/LayerHelper.cs`:
- Around line 23758-23771: The factory method that creates the
PatchEmbeddingLayer<T> accepts an external patchSize but does not validate it;
add a guard at the start of this method to ensure patchSize > 0 and throw an
ArgumentOutOfRangeException (or ArgumentException) with a clear message if
invalid so we fail fast instead of deferring to PatchEmbeddingLayer<T>; locate
the method (the factory that declares double dropoutRate, int patchSize = 16 and
creates geluActivation/identityActivation/visionFfnDim/decoderFfnDim) and insert
the validation before yielding new PatchEmbeddingLayer<T>(patchSize, visionDim,
expectedInputChannels: 3).
In `@src/NeuralNetworks/SyntheticData/DPCTGANGenerator.cs`:
- Around line 675-744: ComputePerExampleNoisedGradients currently treats each
packed PacGAN row as an “example”, so clipping/noise are applied per pack while
ComputeStepPrivacyCost assumes per-source-row privacy; to fix, ensure DP is
applied per source row by either forcing _options.PacSize = 1 when
_options.DifferentialPrivacy is enabled (disable packing in the DP path) or by
moving the clipping/noise logic into the code that iterates source rows before
BuildPackedRealAndFakeBatches packs them (compute per-source-row gradients, clip
and add noise, then aggregate into packs); update
ComputePerExampleNoisedGradients (and related logic around
BuildPackedRealAndFakeBatches and ComputeStepPrivacyCost) to reference
_options.PacSize and apply the chosen approach consistently (also adjust the
same logic applied around the code region referenced at 755-778).
🪄 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: 9657b848-f667-46d7-9c20-cf36c1d6ec04
📒 Files selected for processing (13)
src/AiDotNet.Generators/TestScaffoldGenerator.cssrc/Helpers/LayerHelper.cssrc/NeuralNetworks/SyntheticData/CTGANGenerator.cssrc/NeuralNetworks/SyntheticData/CausalGANGenerator.cssrc/NeuralNetworks/SyntheticData/CopulaGANGenerator.cssrc/NeuralNetworks/SyntheticData/DPCTGANGenerator.cssrc/NeuralNetworks/SyntheticData/TableGANGenerator.cssrc/VisionLanguage/InstructionTuned/Pixtral.cssrc/VisionLanguage/InstructionTuned/PixtralLarge.cssrc/VisionLanguage/InstructionTuned/SmolVLM.cssrc/VisionLanguage/Reasoning/KimiVL.cssrc/VisionLanguage/Reasoning/KimiVLThinking.cstests/AiDotNet.Tests/IntegrationTests/Helpers/LayerHelperIntegrationTests.cs
Contributor
There was a problem hiding this comment.
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)
tests/AiDotNet.Tests/IntegrationTests/SyntheticData/SyntheticTabularGeneratorIntegrationTests.cs (1)
143-167:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winTest assertion uses wrong expected column count.
Line 146 switches to
CreateImbalancedData()which returns a matrix with 4 columns, but line 166 validates againstTotalColswhich is 5. The test will fail with an assertion error.🐛 Proposed fix
- ValidateGeneratedData(generated, GenSamples, TotalCols, "CTGAN"); + ValidateGeneratedData(generated, GenSamples, data.Columns, "CTGAN");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/AiDotNet.Tests/IntegrationTests/SyntheticData/SyntheticTabularGeneratorIntegrationTests.cs` around lines 143 - 167, The test CTGANGenerator_FitAndGenerate_ProducesValidOutput uses CreateImbalancedData() which returns 4 columns but calls ValidateGeneratedData(..., TotalCols, ...) expecting 5; update the assertion to use the actual expected column count from the test data (e.g., use columns.Length or the known value 4) so ValidateGeneratedData is invoked with the correct expected column count; locate this in the test method CTGANGenerator_FitAndGenerate_ProducesValidOutput and replace TotalCols with columns.Length (or 4) to fix the failing assertion.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@tests/AiDotNet.Tests/IntegrationTests/SyntheticData/SyntheticTabularGeneratorIntegrationTests.cs`:
- Around line 304-346: The test method
TableGANGenerator_ClassificationTargets_UseTransformedLabelSlice contains an
unnecessary await Task.Yield() and is declared async but performs no async work;
remove the await Task.Yield() and make the method synchronous by changing the
signature from async Task to void (or remove async and use plain Task-returning
synchronous fact) and keep the [Fact(Timeout = 120000)] attribute, or
alternatively call an actual async API such as FitAsync on
TableGANGenerator<double> if you intend the test to remain async—update the
method signature and invocation (Fit -> FitAsync) accordingly.
---
Outside diff comments:
In
`@tests/AiDotNet.Tests/IntegrationTests/SyntheticData/SyntheticTabularGeneratorIntegrationTests.cs`:
- Around line 143-167: The test
CTGANGenerator_FitAndGenerate_ProducesValidOutput uses CreateImbalancedData()
which returns 4 columns but calls ValidateGeneratedData(..., TotalCols, ...)
expecting 5; update the assertion to use the actual expected column count from
the test data (e.g., use columns.Length or the known value 4) so
ValidateGeneratedData is invoked with the correct expected column count; locate
this in the test method CTGANGenerator_FitAndGenerate_ProducesValidOutput and
replace TotalCols with columns.Length (or 4) to fix the failing assertion.
🪄 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: 5275b7b2-64d4-4f01-a90a-e627f8cf3afe
📒 Files selected for processing (4)
src/NeuralNetworks/SyntheticData/CausalGANGenerator.cssrc/NeuralNetworks/SyntheticData/DPCTGANGenerator.cssrc/NeuralNetworks/SyntheticData/TableGANGenerator.cstests/AiDotNet.Tests/IntegrationTests/SyntheticData/SyntheticTabularGeneratorIntegrationTests.cs
Contributor
There was a problem hiding this comment.
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)
tests/AiDotNet.Tests/IntegrationTests/SyntheticData/SyntheticTabularGeneratorIntegrationTests.cs (1)
146-166:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winColumn count mismatch will cause test to fail.
CreateImbalancedData()returns a matrix with 4 columns, but line 166 validates againstTotalColswhich is 5. The generator trained on 4-column data produces 4-column output, soAssert.Equal(5, generated.Columns)will fail.🐛 Proposed fix
- ValidateGeneratedData(generated, GenSamples, TotalCols, "CTGAN"); + ValidateGeneratedData(generated, GenSamples, data.Columns, "CTGAN");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/AiDotNet.Tests/IntegrationTests/SyntheticData/SyntheticTabularGeneratorIntegrationTests.cs` around lines 146 - 166, CreateImbalancedData returns 4-column data but the test calls ValidateGeneratedData expecting TotalCols (5), causing a column-count assertion failure; update the test so the expected column count matches the training data by either changing the call to ValidateGeneratedData to use the actual column count from CreateImbalancedData (e.g., use data.Columns or columns.Count) or adjust TotalCols to 4, ensuring the rest of the test (CreateArchitecture(data.Columns,...), CTGANGenerator.Generate, and the generated.Columns check) all use the same column count consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@tests/AiDotNet.Tests/IntegrationTests/VisionLanguage/KimiVLReviewRegressionIntegrationTests.cs`:
- Around line 27-32: The test currently only asserts model.Layers is non-empty
but must actually verify the encoder/decoder boundary contract claimed by
Constructor_WithTinyNativeConfiguration_UsesSharedBoundaryHelpers: locate the
constructed KimiVL<double> instance (KimiVL<double> model) and add assertions
that the encoder and decoder reference the same boundary helper (e.g., compare
model.EncoderBoundaryHelper and model.DecoderBoundaryHelper for reference
equality or identical type) and that the boundary behavior is consistent (invoke
the boundary helper on a small synthetic input and assert encoder/decoder
boundary outputs match or meet expected values); update the test to perform
these concrete checks instead of the trivial layer-count assertion.
---
Outside diff comments:
In
`@tests/AiDotNet.Tests/IntegrationTests/SyntheticData/SyntheticTabularGeneratorIntegrationTests.cs`:
- Around line 146-166: CreateImbalancedData returns 4-column data but the test
calls ValidateGeneratedData expecting TotalCols (5), causing a column-count
assertion failure; update the test so the expected column count matches the
training data by either changing the call to ValidateGeneratedData to use the
actual column count from CreateImbalancedData (e.g., use data.Columns or
columns.Count) or adjust TotalCols to 4, ensuring the rest of the test
(CreateArchitecture(data.Columns,...), CTGANGenerator.Generate, and the
generated.Columns check) all use the same column count consistently.
🪄 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: 1a755678-bc05-4517-bcf1-d0198f5651ab
📒 Files selected for processing (4)
src/VisionLanguage/Reasoning/KimiVL.cssrc/VisionLanguage/VisionLanguageModelBase.cstests/AiDotNet.Tests/IntegrationTests/SyntheticData/SyntheticTabularGeneratorIntegrationTests.cstests/AiDotNet.Tests/IntegrationTests/VisionLanguage/KimiVLReviewRegressionIntegrationTests.cs
ooples
pushed a commit
that referenced
this pull request
May 15, 2026
Symptom: every Build & SonarCloud run on PR #1318 (and on master pushes, and on other branches) shows status=completed / conclusion=cancelled. No CI test results have surfaced for the branch despite the workflow being active. Root cause: the old `concurrency.group: build-${{ github.ref }}` + `cancel-in-progress: true` config groups all push events on a single ref into one slot. When two commits land on master within seconds, the new run cancels the previous in-flight run — but the canceller itself races against any subsequent event (e.g. a tag push or a force-push trigger), producing the observed all-cancelled state. Fix follows the GitHub Actions documented pattern for PR + push: - PR events: group by `github.event.pull_request.number` so a rapid synchronize / reopen cancels the previous run for that PR only. - Push events: group by `${ref}-${sha}` so each commit gets its own slot and back-to-back master merges complete independently. Also forces `cancel-in-progress: false` for push so a follow-on push can't kill an in-flight master build. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ooples
pushed a commit
that referenced
this pull request
May 15, 2026
…weep Single coherent commit applying the junior dev's complete in-progress work that was held in stash during prior session work on PR #1318. Full build verified clean on both net10.0 and net471 targets (0 errors, NU5128 packaging warning expected when building net10.0-only). Major content categories (12,150 insertions / 7,210 deletions): src/NeuralNetworks/Layers (16 files) Layer-level fixes including activation derivative restoration, tape-tracking on internal forward paths, and shape-resolution polish for the lazy-init migration. src/NeuralNetworks (~20 files at top level) Major paper-faithful model rewrites: Transformer, Word2Vec, Sparse, SelfOrganizingMap, WGANGP, UnifiedMultimodalNetwork, and several others. Each rewrite replaces manual indexed loops with tape-tracked Engine ops so backprop flows correctly under the autodiff migration. src/Diffusion/Conditioning (13 files) Text conditioner refactors: CLIP / SigLIP / SigLIP2 / T5 / Distilled-T5 / Qwen2 / Gemma / ChatGLM3 / Dual / Triple text conditioners. Common base via TextConditioningBase, paper-faithful tokenizer wiring. src/Audio (10 files) AST / CLAP / PANNs paper-faithful rewrites (massive — 1000+ line diffs), Whisper improvements, HiFi-GAN, NLMS echo cancellation, source separator round-trip. src/ReinforcementLearning (11 files) Agent improvements across the family — MarketMaking + others. src/Helpers (5 files) LayerHelper.cs — ODISE GroupNorm (Wu & He 2018 §3.1) replacing BatchNorm (B=1 instability), PANNs CNN14 + AST + EmotiVoice helper functions, ChooseGroupCount utility. TestScaffoldGenerator polish. src/Models/Options (6 files) Paper-faithful option defaults across multiple models. src/Optimizers (3 files) Adam / AdamW / GradientBasedOptimizerBase improvements. src/Deployment/Mobile (3 files) NNAPI Android backend hardening. src/AdversarialRobustness (2 files) RLHF Alignment + AdversarialTraining defenses. src/ComputerVision (2 files) SceneTextReader OCR + ODISE refactor. src/RetrievalAugmentedGeneration/Retrievers (4 files) Retriever improvements. tests/AiDotNet.Tests (18 files) Test scaffold updates aligned with the model changes — ModelFamily tests, ConditioningModule tests, an Issue1317 transformer custom layer validation regression test. tools/ResNetPerfHarness (1 file) Profiling harness improvements. Merge conflicts resolved in two files where the junior dev's stash overlapped with prior session work: - src/TextToSpeech/StyleEmotion/EmotiVoice.cs: kept the explicit _optimizer argument on TrainWithTape (paper-faithful — honours the EmotiVoiceOptions hyperparameters) plus prior comments explaining why eval-mode Predict matters for SpeakerConsistency / degenerate- output tests. - src/Helpers/LayerHelper.cs: kept the longer comment block in CreateDefaultStyleTTSLayers documenting the inputFeatureDim → encoder projection that prevents mel-spectrogram inputs from collapsing the encoder's MHA. This commit is large but atomic — it represents the full integrated state of the junior dev's work that I incorrectly held back during earlier session triage. The build is clean, no behavior regressions expected vs. the stash's authored state. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 25
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/NeuralNetworks/GenerativeAdversarialNetwork.cs (1)
958-974:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftKeep the gradient-penalty loss on the active tape.
This closure returns a brand-new constant tensor built from
T penaltyScalar. By that point the penalty has already been collapsed outsideTrainWithCustomLoss, so the discriminator step has no graph back to its parameters. In practice, enabling WGAN-GP here adds diagnostics but no regularizing update.Concrete fix
trainableDisc.TrainWithCustomLoss(_lastRealBatch, _ => { - T penaltyScalar = ComputeGradientPenalty( - _lastRealBatch, _lastFakeBatch, _gradientPenaltyLambda); - var lossTensor = new Tensor<T>([1]); - lossTensor[0] = penaltyScalar; - return lossTensor; + return ComputeGradientPenaltyTensor( + _lastRealBatch, + _lastFakeBatch, + _gradientPenaltyLambda); });
ComputeGradientPenaltyTensor(...)needs to build the interpolated samples and returnλ * mean((||∇D(x̂)|| - 1)^2)as a tensor on the same tape instead of converting it toT.As per coding guidelines, "Half-implemented patterns where some code paths work but others silently do nothing" are blocking.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/NeuralNetworks/GenerativeAdversarialNetwork.cs` around lines 958 - 974, The gradient-penalty is being collapsed to a scalar T before being returned to TrainWithCustomLoss, which severs the autograd tape; change ComputeGradientPenalty (or add ComputeGradientPenaltyTensor) to return a Tensor<T> constructed from the interpolated samples so the penalty is computed as λ * mean((||∇D(x̂)|| - 1)^2) on the same tape instead of returning a plain T, and update the TrainWithCustomLoss closure to return that Tensor<T> directly (do not create a constant Tensor from a detached T value).src/NeuralNetworks/Layers/DeconvolutionalLayer.cs (1)
879-885:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPersist the activation type here too.
This fixes the shape parameters, but clone/deserialize will still recreate any non-default
activationFunctionas the constructor defaultReLUbecause this override only emitsKernelSize/Stride/Padding.ConvolutionalLayer<T>already writesScalarActivationTypefor the same reason.♻️ Suggested fix
internal override Dictionary<string, string> GetMetadata() { var metadata = base.GetMetadata(); metadata["KernelSize"] = KernelSize.ToString(); metadata["Stride"] = Stride.ToString(); metadata["Padding"] = Padding.ToString(); + if (ScalarActivation is not null) + { + metadata["ScalarActivationType"] = ScalarActivation.GetType().AssemblyQualifiedName + ?? ScalarActivation.GetType().FullName ?? string.Empty; + } return metadata; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/NeuralNetworks/Layers/DeconvolutionalLayer.cs` around lines 879 - 885, GetMetadata in DeconvolutionalLayer currently copies KernelSize/Stride/Padding but omits the activation type, so cloning/deserialization will lose any non-default activationFunction; update the override of DeconvolutionalLayer.GetMetadata to call base.GetMetadata() and add metadata["ScalarActivationType"] = ScalarActivationType (or the equivalent property/name used by ConvolutionalLayer<T>) so the activation type is persisted alongside KernelSize/Stride/Padding and will be restored on clone/deserialize.src/NeuralNetworks/Layers/ConvolutionalLayer.cs (1)
1018-1065:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCanonicalize before lazy initialization.
Forward()now advertises rank-1/rank-2/rank>4 support, but Line 1021 still callsEnsureInitializedFromInput(input)before any reshaping. On the first call,OnFirstForward()only accepts rank-3/4 tensors, so these new branches are unreachable for an uninitialized layer and the call still throws before it gets here.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/NeuralNetworks/Layers/ConvolutionalLayer.cs` around lines 1018 - 1065, Forward currently calls EnsureInitializedFromInput(input) before reshaping, so lazy initialization (OnFirstForward) only sees the original rank and rejects rank-1/2/>4 inputs; move the lazy-init after you canonicalize into input4D (i.e., perform the reshaping logic in Forward first to produce the 4D tensor, then call EnsureInitializedFromInput(input4D) or call OnFirstForward with the canonicalized shape) so OnFirstForward/EnsureInitializedFromInput always receives a rank-3/4 tensor; update references inside Forward (input4D, _originalInputShape, _addedBatchDimension) accordingly.
♻️ Duplicate comments (8)
src/Helpers/KaimingInitHelper.cs (1)
30-30:⚠️ Potential issue | 🟠 Major | ⚡ Quick winBlocking: keep this helper out of the public API surface.
KaimingInitHelperis implementation plumbing, so exposing it aspublicexpands the supported surface beyond the facade contract. Make the typeinternal.♻️ Proposed fix
-public static class KaimingInitHelper +internal static class KaimingInitHelperAs per coding guidelines, "Users should ONLY interact with
AiModelBuilder.csandAiModelResult.cs" and helper/utility classes should preferinternal.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Helpers/KaimingInitHelper.cs` at line 30, The KaimingInitHelper class is currently declared public but should be internal to avoid expanding the public API; change the declaration of the KaimingInitHelper type from public to internal (i.e., update the class modifier on KaimingInitHelper) so it remains usable within the assembly but is not exposed to consumers, and run tests/build to ensure no external references rely on the public modifier.src/NeuralNetworks/DCGAN.cs (1)
428-429:⚠️ Potential issue | 🟠 Major | ⚡ Quick winTrack both spatial axes before the final
4x4logit conv.
currentSizecomes fromMath.Min(imageHeight, imageWidth), so this loop stops as soon as the smaller axis reaches 4 even if the other axis is still larger. For inputs like64x32, the last conv runs on an8x4map and emits multiple logits per sample instead of the declared[batch, 1].Either maintain
currentHeight/currentWidththrough the downsampling schedule, or reject sizes that cannot reach4x4on both axes before building the discriminator.As per coding guidelines, "missing validation of external inputs" is blocking.
Also applies to: 442-475
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/NeuralNetworks/DCGAN.cs` around lines 428 - 429, The discriminator currently computes targetSize/currentSize from Math.Min(imageHeight, imageWidth) which allows one spatial axis to reach 4 while the other remains larger, producing non-[batch,1] logits; update the DCGAN discriminator build to track both axes (e.g., currentHeight and currentWidth derived from imageHeight and imageWidth) through each downsampling step used in the conv loop (the variables currentSize/targetSize and the downsampling schedule) and only stop when both axes are <=4, or alternatively validate inputs up front and reject any imageHeight/imageWidth that cannot both reach 4x4 by repeated halving; ensure the final logit conv always receives a 4x4 feature map so outputs are [batch,1].src/NeuralNetworks/NeuralNetworkBase.cs (2)
5623-5646: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winKeep fused-training state out of the subclass API.
Making
_fusedTrainingDisabledprotectedexposes training-plumbing on a public base type and lets derived models desynchronize it from_fusedTrainingCommittedand compiled-plan state. Prefer a protected virtual opt-out hook and keep the backing field private.As per coding guidelines, "Prefer
privateoverinternalwhen the member is only used within its own class" and implementation details should stay hidden behind the facade-oriented API.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/NeuralNetworks/NeuralNetworkBase.cs` around lines 5623 - 5646, The field _fusedTrainingDisabled is exposed as protected; hide the implementation by making the backing field private and replace external subclass access with a protected virtual opt-out hook (e.g., a protected virtual bool ShouldDisableFusedTraining() or OnFusedTrainingDisabled override) so derived models can opt out without mutating internal state; update usages in TryTrainWithFusedOptimizer, ResetState, and InvalidateParameterCountCache to consult the new private field and the protected hook, and ensure consistency with _fusedTrainingCommitted and compiled-plan state (do not expose TensorCodecOptions.EnableCompilation toggles).
5350-5368:⚠️ Potential issue | 🟠 Major | ⚡ Quick winClip raw network-level trainables in the same global-norm pass.
This only clips
gradsfor layer-owned parameters. The later extras update still reads unclipped entries fromallGrads, so tensors surfaced byGetExtraTrainableTensors()bypass the new max-norm contract entirely.Suggested fix
- double maxGradNorm = MaxGradNorm; - if (maxGradNorm > 0.0 && grads.Count > 0) + double maxGradNorm = MaxGradNorm; + if (maxGradNorm > 0.0) { - ApplyGradientClipping(grads, maxGradNorm, trainableParams); + var clipOrder = trainableParams.Concat(extraTrainableTensors).ToList(); + var clipTargets = new Dictionary<Tensor<T>, Tensor<T>>( + Helpers.TensorReferenceComparer<Tensor<T>>.Instance); + foreach (var param in clipOrder) + { + if (allGrads.TryGetValue(param, out var grad)) + clipTargets[param] = grad; + } + if (clipTargets.Count > 0) + ApplyGradientClipping(clipTargets, maxGradNorm, clipOrder); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/NeuralNetworks/NeuralNetworkBase.cs` around lines 5350 - 5368, The gradient-clipping pass only clips the layer-owned gradients (`grads`) but not tensors returned by GetExtraTrainableTensors(), so extra trainables in `allGrads` bypass MaxGradNorm; modify the clipping step in NeuralNetworkBase (where ApplyGradientClipping is called) to include all trainable gradients together—e.g., build a deterministic-ordered collection that merges `grads` and the extra entries from `GetExtraTrainableTensors()` (or pass `allGrads` instead of `grads`) and then call ApplyGradientClipping(combinedGrads, maxGradNorm, trainableParams) so later updates read the clipped values; ensure the iteration-order key remains `trainableParams` to preserve determinism.src/NeuralNetworks/TransformerArchitecture.cs (1)
248-252:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRestore a forwarding
RandomSeedproperty onTransformerArchitecture<T>.Replacing the member with a comment removes
TransformerArchitecture<T>.RandomSeedfrom the derived type, which is a binary/reflection break, and the XML doc block above now binds toVocabularySizeinstead. Add a forwarding shim here, e.g.public new int? RandomSeed { get => base.RandomSeed; set => base.RandomSeed = value; }.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/NeuralNetworks/TransformerArchitecture.cs` around lines 248 - 252, Restore a forwarding RandomSeed property on TransformerArchitecture<T> so the derived type retains the member and the XML doc stays attached; add a public new int? RandomSeed { get => base.RandomSeed; set => base.RandomSeed = value; } property in the TransformerArchitecture<T> declaration (the ctor later that assigns into the inherited property can continue to operate on the base property) so reflection/binary compatibility and XML docbinding are preserved.src/Training/CompiledTapeTrainingStep.cs (1)
87-87:⚠️ Potential issue | 🟠 Major | ⚡ Quick winTrack
lrScheduleandmaxGradNormin the fused-plan config cache.Line 402 still snapshots only optimizer type/LR/betas/epsilon/weight decay. After the first fused configure, changes to Line 258 or Line 259 are ignored while the method continues returning
true, so the fused path can silently diverge from eager semantics.🛠 Proposed fix
- private static (int OptType, float Lr, float B1, float B2, float Eps, float Wd)? _configuredOptimizerConfig; + private static (int OptType, float Lr, float B1, float B2, float Eps, float Wd, double MaxGradNorm, AiDotNet.Tensors.Engines.Compilation.LrSchedule? Schedule)? _configuredOptimizerConfig; ... - var currentConfig = ((int)optimizerType, learningRate, beta1, beta2, epsilon, weightDecay); + var currentConfig = ((int)optimizerType, learningRate, beta1, beta2, epsilon, weightDecay, maxGradNorm, lrSchedule);Also applies to: 257-259, 402-455
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Training/CompiledTapeTrainingStep.cs` at line 87, The cached optimizer snapshot _configuredOptimizerConfig only records optimizer type/LR/betas/eps/wd; extend it to also store lrSchedule and maxGradNorm, update all places that set or compare _configuredOptimizerConfig (e.g., the fused-plan configuration and the method that returns true/false for fused configuration) to include those two fields, and ensure the cache is populated from the current lrSchedule and maxGradNorm values when configuring the fused plan so subsequent changes to those settings will cause reconfiguration rather than silently returning true.tests/AiDotNet.Tests/ModelFamilyTests/NeuralNetworks/DeepBeliefNetworkTests.cs (2)
181-195:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDispose cloned DBN instances in
MoreData_ShouldNotDegrade.Lines [181] and [194] allocate disposable networks without disposal, which can leak large tensors across tests.
Suggested fix
- var network1 = (DeepBeliefNetwork<double>)CreateNetwork(); + using var network1 = (DeepBeliefNetwork<double>)CreateNetwork(); ... - var network2 = (DeepBeliefNetwork<double>)network1.Clone(); + using var network2 = (DeepBeliefNetwork<double>)network1.Clone();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/AiDotNet.Tests/ModelFamilyTests/NeuralNetworks/DeepBeliefNetworkTests.cs` around lines 181 - 195, The test MoreData_ShouldNotDegrade creates disposable DeepBeliefNetwork<double> instances (network1 and network2 via CreateNetwork() and network1.Clone()) but never disposes them; update the test to ensure both network1 and network2 are disposed (e.g., wrap their lifetimes with using or call Dispose() in a finally), making sure to still call network1.PreTrain(...) before cloning so the clone gets the pretrained weights and then dispose both instances after assertions to avoid leaking large tensors.
82-89:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winRemove the conditional assertion gate; this can still pass when the metric is invalid.
Line [82] conditionally skips the core reduction assertion, so the test can pass instead of failing when loss values are invalid.
As per coding guidelines: “Always-passing tests with conditional assertions that skip verification when things fail” are blocking.Suggested fix
- if (!double.IsNaN(initialLoss) && !double.IsNaN(finalLoss)) - { - Assert.True(finalLoss <= initialLoss + TrainingLossReductionTolerance, - $"DBN training did not reduce loss after CD pre-training: " - + $"initial={initialLoss:F6}, final={finalLoss:F6}. " - + "Investigate whether CD-1 pretrain is escaping the vanishing-gradient " - + "regime or whether the supervised SGD+momentum step is mis-configured."); - } + Assert.False(double.IsNaN(initialLoss) || double.IsInfinity(initialLoss), + $"Initial loss is non-finite: {initialLoss}"); + Assert.False(double.IsNaN(finalLoss) || double.IsInfinity(finalLoss), + $"Final loss is non-finite: {finalLoss}"); + Assert.True(finalLoss <= initialLoss + TrainingLossReductionTolerance, + $"DBN training did not reduce loss after CD pre-training: " + + $"initial={initialLoss:F6}, final={finalLoss:F6}. " + + "Investigate whether CD-1 pretrain is escaping the vanishing-gradient " + + "regime or whether the supervised SGD+momentum step is mis-configured.");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/AiDotNet.Tests/ModelFamilyTests/NeuralNetworks/DeepBeliefNetworkTests.cs` around lines 82 - 89, Remove the conditional gate that skips the reduction check so the test cannot silently pass when metrics are invalid: delete the surrounding if (!double.IsNaN(initialLoss) && !double.IsNaN(finalLoss)) block and instead add explicit assertions that initialLoss and finalLoss are valid (e.g., Assert.False(double.IsNaN(initialLoss)) and Assert.False(double.IsNaN(finalLoss))) followed by the existing reduction assertion using TrainingLossReductionTolerance (Assert.True(finalLoss <= initialLoss + TrainingLossReductionTolerance, ...)) so the test always fails on invalid or non-reduced loss; update the message to reference initialLoss, finalLoss, and TrainingLossReductionTolerance as needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Audio/Fingerprinting/GraFPrint.cs`:
- Around line 157-180: The code currently hardcodes _fusedTrainingDisabled =
true in GraFPrint; change this to be driven by a new option property on the
model options (e.g., add DisableFusedOptimizerStep bool defaulting to false on
GraFPrintOptions) and initialize _fusedTrainingDisabled from that option (e.g.,
in the GraFPrint constructor or Init method read
options.DisableFusedOptimizerStep). Ensure the option defaults to false so
production keeps fused optimizer behavior, preserve existing behavior only when
callers explicitly set DisableFusedOptimizerStep=true, and update any
constructors/factory methods that create GraFPrint to accept or forward
GraFPrintOptions.
In `@src/NeuralNetworks/GenerativeAdversarialNetwork.cs`:
- Around line 1019-1039: The generator loss is incorrectly computed as LSGAN MSE
(ReduceMean((discScore - 1)^2)); replace that block inside the
trainableGen.TrainWithCustomLoss lambda (where discScore is produced by
iterating Discriminator.Layers) with the framework's configured adversarial
objective used by the discriminator. Specifically, keep the manual discriminator
forward pass (discScore) but compute and return the generator adversarial loss
via the same criterion the model is configured to use (e.g., when the
discriminator emits logits use softplus(-discScore) /
ReduceMean(softplus(-discScore), axes), or call the existing adversarial loss
helper method if one exists) instead of the diff/squared/ReduceMean path that
uses allRealLabels.
In `@src/NeuralNetworks/NeuralNetworkBase.cs`:
- Around line 294-335: The change accidentally renamed the protected
subclass-facing contract from MaxGradNorm to MaxGradNormField, breaking derived
code; restore a protected T member named MaxGradNorm (protected T MaxGradNorm)
so subclasses can continue to read/assign it, and rename the new public double
accessor to a distinct name (e.g., MaxGradNormValue or MaxGradNormDouble)
instead of MaxGradNorm; update MaxGradNormT to call NumOps.FromDouble(using the
new public double name) and ensure any ctor or usages that set the protected
field use the restored MaxGradNorm symbol and that all references to
MaxGradNormField are replaced accordingly.
In `@src/NeuralNetworks/SyntheticData/DPCTGANGenerator.cs`:
- Around line 389-400: The loop charges privacy only once but calls
TrainDiscriminatorStepBatchedDP(...) _options.DiscriminatorSteps times; update
the accounting so ComputeStepPrivacyCost(...) is applied per discriminator step:
either compute stepEpsilon once and add stepEpsilon inside the for-loop after
each TrainDiscriminatorStepBatchedDP call, or multiply stepEpsilon by
_options.DiscriminatorSteps before adding to _cumulativeEpsilon; do the same fix
for the analogous block around TrainGeneratorStepBatched (the 450-457 area) so
the early-stop guard and reported _cumulativeEpsilon reflect all discriminator
accesses to real data.
- Around line 597-605: The DP critic path currently computes lossTensor =
E[D(fake)] - E[D(real)] and then calls ComputePerExampleNoisedGradients without
the WGAN-GP term; restore the gradient-penalty by computing the per-example
gradient penalty and adding _options.GradientPenaltyWeight * gp to each
per-example critic loss before clipping/noising. Concretely, in the DP path use
DiscriminatorForwardBatched to get per-example scores, compute per-example
gradient of the critic w.r.t. interpolated inputs to form the per-example GP
(squared norm minus 1)^2, multiply by _options.GradientPenaltyWeight, add that
term to each per-example loss you pass into ComputePerExampleNoisedGradients (so
ComputePerExampleNoisedGradients receives E[D(fake)] - E[D(real)] + GP_weight*GP
on a per-example basis).
In `@src/NeuralNetworks/SyntheticData/MedSynthGenerator.cs`:
- Around line 528-535: The two closures passed into TapeStepContext<T>
(ComputeForward and RecomputeLoss) only replay the real-sample BCE but
ComputeGradients computed lossReal + lossFake; update the closures so they
capture and replay the same full discriminator objective used to compute
grads/noisedAvgGrads: have ComputeForward call DiscriminatorForwardBatched for
both real and fake inputs (or a combined batch) and have RecomputeLoss return
Engine.TensorNegate(Engine.ReduceMean(LogSigmoid(predReal), ...) +
Engine.ReduceMean(LogSigmoid(predFake), ...)) (i.e. lossReal+lossFake) so the
replayed loss matches the original gradient computation (also apply the same fix
to the other occurrence around the 627-634 region), or alternatively disable
replay for this TapeStepContext if you prefer not to replay at all.
In `@src/NeuralNetworks/SyntheticData/TableGANGenerator.cs`:
- Around line 405-453: The discriminator loss omits the WGAN-GP gradient
penalty; restore it in TrainDiscriminatorStepBatched by constructing
interpolated samples between realBatch and fakeBatch, having the tape watch
those interpolates, passing them through DiscriminatorForwardBatched to get
discInterp, computing gradients of discInterp w.r.t. the interpolates, computing
per-sample L2 norms, forming penalty = gpLambda * mean((norms - 1)^2) and adding
that penalty to lossTensor before computing parameter gradients; use existing
GradientTape<T> methods (e.g., tape.Watch / tape.ComputeGradients) and Engine
ops (ReduceSum/ReduceMean/Sqrt/TensorSubtract/TensorAdd) to compute norms and
aggregate the penalty, and pick a gpLambda (commonly 10) or the project's
configured gradient-penalty constant.
In `@src/NeuralNetworks/SyntheticData/TimeGANGenerator.cs`:
- Around line 731-739: RecomputeLoss currently returns only the adversarial
component; update the RecomputeLoss closure passed into TapeStepContext so it
recomputes the full training objective (adversarial + supervised/temporal
joint-phase term) exactly as used to produce lossValue—i.e. compute the forward
pass via GeneratorForwardBatched -> SupervisorForwardBatched ->
DiscriminatorForwardBatched (same as ComputeForward) and then combine
Engine.TensorNegate(Engine.ReduceMean(LogSigmoid(pred), advAxes,
keepDims:false)) with the supervised temporal term (the same supervised loss
expression used when constructing lossValue), ensuring any intermediate
tensors/axes (noise, advAxes, supervised weighting) are captured so optimizer
replay is consistent.
- Around line 655-662: RecomputeLoss currently returns only the real-sample BCE
term; change RecomputeLoss to replay both BCE terms so it recomputes lossReal +
lossFake (the same scalar that produced grads) — e.g. compute lossReal =
-ReduceMean(LogSigmoid(pred_real)) and lossFake =
-ReduceMean(LogSigmoid(Negate(pred_fake))) and return their sum; ensure the
signature used by TapeStepContext<T> (RecomputeLoss,
ComputeForward/DiscriminatorForwardBatched) receives the real and fake logits
needed so the closure passed to _optimizer.Step(...) exactly reproduces
lossTensor and avoids critic drift.
In `@src/TextToSpeech/StyleEmotion/EmotiVoice.cs`:
- Around line 42-43: The optimizer field (_optimizer) is currently readonly and
created in the constructor (via CreateDefaultOptimizer) so deserialized options
aren't applied; remove the readonly modifier from _optimizer, then in
DeserializeNetworkSpecificData() (the method that hydrates _options) recreate
the native optimizer by assigning _optimizer = CreateDefaultOptimizer() after
_options is loaded so the optimizer uses the serialized
LearningRate/OptimizerBeta1/OptimizerBeta2/OptimizerEpsilon values; update any
other spots that construct the optimizer (e.g., the EmotiVoice constructor and
any other initialization paths referenced at lines 89-90) to use the mutable
_optimizer.
In `@src/VisionLanguage/InstructionTuned/Phi3Vision.cs`:
- Around line 128-131: The call to
ValidateVisualPatchOptions(_options.ImageSize, _options.MaxVisualTokens) runs
even when a caller provided custom Architecture.Layers; move that validation
into the default-layer branch so it only runs when
LayerHelper<T>.CreateDefaultVisionAdapterLayers(...) is used. Concretely, remove
the standalone ValidateVisualPatchOptions call at the top of this block and
invoke ValidateVisualPatchOptions(_options.ImageSize, _options.MaxVisualTokens)
immediately before calling LayerHelper<T>.CreateDefaultVisionAdapterLayers(…) /
ComputePatchSize(), keeping the existing logic that populates Layers, sets
_encoderLayerEnd, calls ComputeEncoderDecoderBoundary(), and leaves
ValidateEncoderDecoderBoundary(_encoderLayerEnd) after the branches.
In `@src/VisionLanguage/InstructionTuned/Phi4Multimodal.cs`:
- Around line 129-132: The constructor currently calls
ValidateVisualPatchOptions(_options.ImageSize, _options.MaxVisualTokens) before
checking Architecture.Layers, which causes validation to run even when custom
Architecture.Layers are provided; move the ValidateVisualPatchOptions call into
the else branch that creates default vision adapter layers (the branch that
calls LayerHelper<T>.CreateDefaultVisionAdapterLayers and ComputePatchSize()),
so that patch option validation and ComputePatchSize() are only invoked when
default vision layers are used; ensure the existing branches still set Layers,
_encoderLayerEnd, and call ComputeEncoderDecoderBoundary() and
ValidateEncoderDecoderBoundary(_encoderLayerEnd) as before.
In `@src/VisionLanguage/InstructionTuned/Pixtral.cs`:
- Around line 118-121: Remove the unconditional call to ValidatePatchOptions()
so custom Architecture.Layers setups aren't rejected; instead call patch
validation only when you compute or use patch size (i.e., move or remove
ValidatePatchOptions() and ensure ComputePatchSize() remains responsible for
validating patch options in the default-layer branch that calls
LayerHelper<T>.CreateDefaultVisionAdapterLayers and
ComputeEncoderDecoderBoundary(); keep ValidateEncoderDecoderBoundary() as-is
after layer setup.
In `@src/VisionLanguage/InstructionTuned/PixtralLarge.cs`:
- Around line 120-123: The constructor currently calls ValidatePatchOptions()
and ComputePatchSize() even when Architecture.Layers is provided, causing valid
custom layer stacks to fail; change the logic so ValidatePatchOptions() and
ComputePatchOptions()/ComputePatchSize() are only executed in the default-layer
branch: if Architecture.Layers is not null/has items, only call
Layers.AddRange(Architecture.Layers) and set _encoderLayerEnd = Layers.Count / 2
(no patch validation or ComputePatchSize), otherwise run the existing
CreateDefaultVisionAdapterLayers(...) call (which may use ComputePatchSize())
and then ComputeEncoderDecoderBoundary(); keep the final
ValidateEncoderDecoderBoundary() call unchanged. Reference symbols:
ValidatePatchOptions, Architecture.Layers, ComputePatchSize, Layers.AddRange,
CreateDefaultVisionAdapterLayers, ComputeEncoderDecoderBoundary,
ValidateEncoderDecoderBoundary.
In `@src/VisionLanguage/InstructionTuned/SmolVLM.cs`:
- Around line 114-125: The InitializeLayers method currently calls
ValidatePatchOptions() before checking Architecture.Layers, causing custom layer
stacks to be validated against patch options they don't use; move the
ValidatePatchOptions() call (and any ComputePatchSize() invocation that relies
on it) into the default-projector branch so that ValidatePatchOptions() is only
executed when calling
LayerHelper<T>.CreateDefaultPixelShuffleProjectorLayers(...) (i.e., inside the
else branch that computes the patch projector and calls ComputePatchSize());
keep the ComputeEncoderDecoderBoundary/ValidateEncoderDecoderBoundary logic
unchanged and ensure _useNativeMode early return remains at the top of
InitializeLayers.
In `@src/VisionLanguage/Reasoning/KimiVL.cs`:
- Around line 208-211: The InitializeLayers method currently calls
ValidateVisualPatchOptions unconditionally which incorrectly rejects
caller-provided Architecture.Layers; move the patch-size validation so it only
runs when you build the default Kimi stack (the else branch). Concretely, remove
the unconditional call to ValidateVisualPatchOptions(_options.ImageSize,
_options.MaxVisualTokens) from InitializeLayers and invoke it inside the else
branch immediately before calling
ComputeKimiPatchSize()/LayerHelper<T>.CreateDefaultCrossAttentionResamplerVLMLayers
so the existing Architecture.Layers path is left untouched; keep
ComputeKimiPatchSize(), ComputeKimiEncoderDecoderBoundary(), and the assignment
to _encoderLayerEnd unchanged.
In `@src/VisionLanguage/Reasoning/KimiVLThinking.cs`:
- Around line 219-229: The InitializeLayers method calls ValidatePatchOptions()
unconditionally which breaks the custom-layer (Architecture.Layers) path; remove
the early ValidatePatchOptions() call and ensure patch sizing validation only
happens when creating the default resampler (i.e., keep
ComputePatchSize()/ValidatePatchOptions() invoked within the else branch that
calls LayerHelper<T>.CreateDefaultCrossAttentionResamplerVLMLayers), so custom
layers short-circuit without requiring image/max-token options.
In
`@tests/AiDotNet.Tests/IntegrationTests/Helpers/LayerHelperIntegrationTests.cs`:
- Around line 32-33: Replace the brittle message text assertion with a direct
assertion on the exception's ParamName: keep the
Assert.Throws<ArgumentOutOfRangeException>(factory) call that assigns to ex,
remove the Assert.Contains("greater than 0", ex.Message) line, and add an
assertion that ex.ParamName equals the actual parameter name used by the method
under test (e.g., Assert.Equal("<expectedParameterName>", ex.ParamName)),
referencing the exception variable ex from Assert.Throws.
In
`@tests/AiDotNet.Tests/IntegrationTests/SyntheticData/SyntheticTabularGeneratorIntegrationTests.cs`:
- Around line 337-341: The reflection lookup for BuildClassificationTargetTensor
is ambiguous because it uses GetMethod by name only; change it to resolve the
exact overload by specifying the parameter types (e.g., the two Tensor<double>
parameter types) or by enumerating
typeof(TableGANGenerator<double>).GetMethods(...) and selecting the method named
"BuildClassificationTargetTensor" whose ParameterTypes match (Tensor<double>,
Tensor<double>), then invoke that MethodInfo; reference the method name
BuildClassificationTargetTensor and the TableGANGenerator<double> type when
updating the reflection code.
In
`@tests/AiDotNet.Tests/IntegrationTests/VisionLanguage/KimiVLReviewRegressionIntegrationTests.cs`:
- Around line 16-17: These integration tests (e.g.,
Constructor_WithInvalidImageSize_ThrowsBeforeLayerInitialization and the other
tests at lines 31-32 and 46-47) need explicit timeouts to avoid hanging CI:
convert each [Fact] to an async Task test and run the test body as a Task that
you await with a timeout (for example using
Task.WaitAsync(TimeSpan.FromSeconds(30)) or a
CancellationTokenSource(TimeSpan.FromSeconds(30)) and passing the token to
awaited operations), and fail the test if the wait times out so the test cannot
hang indefinitely.
In
`@tests/AiDotNet.Tests/IntegrationTests/VisionLanguage/VisionLanguagePatchSizingReviewRegressionIntegrationTests.cs`:
- Around line 93-97: The helper AssertInvalidSizingRejected currently inspects
ArgumentOutOfRangeException.Message which is brittle; update it to assert the
parameter name via ex.ParamName equals the expected parameter name (or contains
expectedParamName) instead of matching message text. Locate the method
AssertInvalidSizingRejected and replace the Assert.Contains on ex.Message with
an assertion that ex.ParamName matches the expected parameter identifier passed
into the helper (adjust the helper signature to accept expectedParamName if
needed) so tests no longer rely on localized message formatting.
- Around line 21-30: The test allocates many heavyweight VLM instances
(DeepSeekVL, DeepSeekVL2, Gemma3, InternVL, InternVL2, InternVL25, InternVL3,
Llama32Vision, Phi3Vision, Phi4Multimodal) inside Assert.Equal calls without
disposing them; update each assertion to create the model in a disposable scope
and ensure Dispose is called (e.g., use "using" or assign to a local and call
Dispose) around the InvokeComputePatchSize(…) invocation so resources are
released after each check; keep InvokeComputePatchSize unchanged and apply this
pattern to every model instantiation in the listed assertions.
In
`@tests/AiDotNet.Tests/ModelFamilyTests/NeuralNetworks/DeepBeliefNetworkTests.cs`:
- Around line 213-216: The current guard only rejects NaN but allows Infinity;
update the invalid-loss assertion around lossShort and lossLong (the
Assert.False that checks double.IsNaN(...)) to also treat Infinity as invalid by
checking double.IsInfinity (or use double.IsFinite if available) for both
lossShort and lossLong so the test fails unconditionally on NaN or Infinity
before the subsequent Assert.True that compares lossLong, lossShort, and
MoreDataTolerance.
In
`@tests/AiDotNet.Tests/ModelFamilyTests/NeuralNetworks/TableGANGeneratorTests.cs`:
- Around line 80-83: The tests create TableGANGenerator<double> instances
without disposing them; update each creation (the calls to CreateGenerator()
that assign to gen in TableGANGeneratorTests) to use disposal, e.g. change "var
gen = CreateGenerator();" to "using var gen = CreateGenerator();" (or wrap with
a using block) so TableGANGenerator<double> is disposed after the test; apply
this change for the occurrences around the CreateGenerator usages indicated
(including the blocks at the other reported ranges).
- Around line 139-151: The test Fit_TinyDataset_MarksGeneratorAsFitted only
asserts metadata; extend it to verify actual training effect by (1) capturing a
fixed noise input (create a deterministic noise tensor) and calling the
generator's inference method before and after gen.Fit to assert the outputs
changed (e.g., different tensor values or L2 distance > 0), and (2) record
generator/discriminator training loss values (or compute dataset loss using the
generator) across epochs/steps and assert the loss decreased (final < initial)
or show monotonic decline; locate helpers CreateGenerator and the gen.Fit call
and use the generator's inference method (e.g., Generate/Sample/Forward) and any
exposed loss history or training callback hook to obtain losses, then add
assertions in the test alongside the existing IsFitted and Columns checks.
---
Outside diff comments:
In `@src/NeuralNetworks/GenerativeAdversarialNetwork.cs`:
- Around line 958-974: The gradient-penalty is being collapsed to a scalar T
before being returned to TrainWithCustomLoss, which severs the autograd tape;
change ComputeGradientPenalty (or add ComputeGradientPenaltyTensor) to return a
Tensor<T> constructed from the interpolated samples so the penalty is computed
as λ * mean((||∇D(x̂)|| - 1)^2) on the same tape instead of returning a plain T,
and update the TrainWithCustomLoss closure to return that Tensor<T> directly (do
not create a constant Tensor from a detached T value).
In `@src/NeuralNetworks/Layers/ConvolutionalLayer.cs`:
- Around line 1018-1065: Forward currently calls
EnsureInitializedFromInput(input) before reshaping, so lazy initialization
(OnFirstForward) only sees the original rank and rejects rank-1/2/>4 inputs;
move the lazy-init after you canonicalize into input4D (i.e., perform the
reshaping logic in Forward first to produce the 4D tensor, then call
EnsureInitializedFromInput(input4D) or call OnFirstForward with the
canonicalized shape) so OnFirstForward/EnsureInitializedFromInput always
receives a rank-3/4 tensor; update references inside Forward (input4D,
_originalInputShape, _addedBatchDimension) accordingly.
In `@src/NeuralNetworks/Layers/DeconvolutionalLayer.cs`:
- Around line 879-885: GetMetadata in DeconvolutionalLayer currently copies
KernelSize/Stride/Padding but omits the activation type, so
cloning/deserialization will lose any non-default activationFunction; update the
override of DeconvolutionalLayer.GetMetadata to call base.GetMetadata() and add
metadata["ScalarActivationType"] = ScalarActivationType (or the equivalent
property/name used by ConvolutionalLayer<T>) so the activation type is persisted
alongside KernelSize/Stride/Padding and will be restored on clone/deserialize.
---
Duplicate comments:
In `@src/Helpers/KaimingInitHelper.cs`:
- Line 30: The KaimingInitHelper class is currently declared public but should
be internal to avoid expanding the public API; change the declaration of the
KaimingInitHelper type from public to internal (i.e., update the class modifier
on KaimingInitHelper) so it remains usable within the assembly but is not
exposed to consumers, and run tests/build to ensure no external references rely
on the public modifier.
In `@src/NeuralNetworks/DCGAN.cs`:
- Around line 428-429: The discriminator currently computes
targetSize/currentSize from Math.Min(imageHeight, imageWidth) which allows one
spatial axis to reach 4 while the other remains larger, producing non-[batch,1]
logits; update the DCGAN discriminator build to track both axes (e.g.,
currentHeight and currentWidth derived from imageHeight and imageWidth) through
each downsampling step used in the conv loop (the variables
currentSize/targetSize and the downsampling schedule) and only stop when both
axes are <=4, or alternatively validate inputs up front and reject any
imageHeight/imageWidth that cannot both reach 4x4 by repeated halving; ensure
the final logit conv always receives a 4x4 feature map so outputs are [batch,1].
In `@src/NeuralNetworks/NeuralNetworkBase.cs`:
- Around line 5623-5646: The field _fusedTrainingDisabled is exposed as
protected; hide the implementation by making the backing field private and
replace external subclass access with a protected virtual opt-out hook (e.g., a
protected virtual bool ShouldDisableFusedTraining() or OnFusedTrainingDisabled
override) so derived models can opt out without mutating internal state; update
usages in TryTrainWithFusedOptimizer, ResetState, and
InvalidateParameterCountCache to consult the new private field and the protected
hook, and ensure consistency with _fusedTrainingCommitted and compiled-plan
state (do not expose TensorCodecOptions.EnableCompilation toggles).
- Around line 5350-5368: The gradient-clipping pass only clips the layer-owned
gradients (`grads`) but not tensors returned by GetExtraTrainableTensors(), so
extra trainables in `allGrads` bypass MaxGradNorm; modify the clipping step in
NeuralNetworkBase (where ApplyGradientClipping is called) to include all
trainable gradients together—e.g., build a deterministic-ordered collection that
merges `grads` and the extra entries from `GetExtraTrainableTensors()` (or pass
`allGrads` instead of `grads`) and then call
ApplyGradientClipping(combinedGrads, maxGradNorm, trainableParams) so later
updates read the clipped values; ensure the iteration-order key remains
`trainableParams` to preserve determinism.
In `@src/NeuralNetworks/TransformerArchitecture.cs`:
- Around line 248-252: Restore a forwarding RandomSeed property on
TransformerArchitecture<T> so the derived type retains the member and the XML
doc stays attached; add a public new int? RandomSeed { get => base.RandomSeed;
set => base.RandomSeed = value; } property in the TransformerArchitecture<T>
declaration (the ctor later that assigns into the inherited property can
continue to operate on the base property) so reflection/binary compatibility and
XML docbinding are preserved.
In `@src/Training/CompiledTapeTrainingStep.cs`:
- Line 87: The cached optimizer snapshot _configuredOptimizerConfig only records
optimizer type/LR/betas/eps/wd; extend it to also store lrSchedule and
maxGradNorm, update all places that set or compare _configuredOptimizerConfig
(e.g., the fused-plan configuration and the method that returns true/false for
fused configuration) to include those two fields, and ensure the cache is
populated from the current lrSchedule and maxGradNorm values when configuring
the fused plan so subsequent changes to those settings will cause
reconfiguration rather than silently returning true.
In
`@tests/AiDotNet.Tests/ModelFamilyTests/NeuralNetworks/DeepBeliefNetworkTests.cs`:
- Around line 181-195: The test MoreData_ShouldNotDegrade creates disposable
DeepBeliefNetwork<double> instances (network1 and network2 via CreateNetwork()
and network1.Clone()) but never disposes them; update the test to ensure both
network1 and network2 are disposed (e.g., wrap their lifetimes with using or
call Dispose() in a finally), making sure to still call network1.PreTrain(...)
before cloning so the clone gets the pretrained weights and then dispose both
instances after assertions to avoid leaking large tensors.
- Around line 82-89: Remove the conditional gate that skips the reduction check
so the test cannot silently pass when metrics are invalid: delete the
surrounding if (!double.IsNaN(initialLoss) && !double.IsNaN(finalLoss)) block
and instead add explicit assertions that initialLoss and finalLoss are valid
(e.g., Assert.False(double.IsNaN(initialLoss)) and
Assert.False(double.IsNaN(finalLoss))) followed by the existing reduction
assertion using TrainingLossReductionTolerance (Assert.True(finalLoss <=
initialLoss + TrainingLossReductionTolerance, ...)) so the test always fails on
invalid or non-reduced loss; update the message to reference initialLoss,
finalLoss, and TrainingLossReductionTolerance as needed.
🪄 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: 6718efee-596b-48a7-8353-5dcbd2ef6ed5
⛔ Files ignored due to path filters (1)
tests/AiDotNet.Tests/ModelFamilyTests/Generated/GraFPrintLossTraceTests.csis excluded by!**/generated/**
📒 Files selected for processing (60)
.github/workflows/sonarcloud.yml.gitignoreDirectory.Packages.propssrc/AiDotNet.Generators/TestScaffoldGenerator.cssrc/Audio/Fingerprinting/GraFPrint.cssrc/Audio/Fingerprinting/GraFPrintOptions.cssrc/Diffusion/FastGeneration/ConsistencyModel.cssrc/Diffusion/FastGeneration/Flux2SchnellModel.cssrc/Diffusion/Video/VideoCrafterModel.cssrc/Helpers/DeserializationHelper.cssrc/Helpers/KaimingInitHelper.cssrc/Helpers/LayerHelper.cssrc/NeuralNetworks/DCGAN.cssrc/NeuralNetworks/DeepBeliefNetwork.cssrc/NeuralNetworks/GenerativeAdversarialNetwork.cssrc/NeuralNetworks/Layers/BatchNormalizationLayer.cssrc/NeuralNetworks/Layers/ConvolutionalLayer.cssrc/NeuralNetworks/Layers/DeconvolutionalLayer.cssrc/NeuralNetworks/NeuralNetworkArchitecture.cssrc/NeuralNetworks/NeuralNetworkBase.cssrc/NeuralNetworks/ResNetNetwork.cssrc/NeuralNetworks/SyntheticData/CTGANGenerator.cssrc/NeuralNetworks/SyntheticData/CausalGANGenerator.cssrc/NeuralNetworks/SyntheticData/CopulaGANGenerator.cssrc/NeuralNetworks/SyntheticData/DPCTGANGenerator.cssrc/NeuralNetworks/SyntheticData/MedSynthGenerator.cssrc/NeuralNetworks/SyntheticData/TableGANGenerator.cssrc/NeuralNetworks/SyntheticData/TimeGANGenerator.cssrc/NeuralNetworks/TransformerArchitecture.cssrc/NeuralNetworks/UNet3D.cssrc/NeuralNetworks/VoxelCNN.cssrc/TextToSpeech/StyleEmotion/EmotiVoice.cssrc/TextToSpeech/StyleEmotion/EmotiVoiceOptions.cssrc/Training/CompiledTapeTrainingStep.cssrc/VisionLanguage/InstructionTuned/DeepSeekVL.cssrc/VisionLanguage/InstructionTuned/DeepSeekVL2.cssrc/VisionLanguage/InstructionTuned/Gemma3.cssrc/VisionLanguage/InstructionTuned/InternVL.cssrc/VisionLanguage/InstructionTuned/InternVL2.cssrc/VisionLanguage/InstructionTuned/InternVL25.cssrc/VisionLanguage/InstructionTuned/InternVL3.cssrc/VisionLanguage/InstructionTuned/Llama32Vision.cssrc/VisionLanguage/InstructionTuned/Phi3Vision.cssrc/VisionLanguage/InstructionTuned/Phi4Multimodal.cssrc/VisionLanguage/InstructionTuned/Pixtral.cssrc/VisionLanguage/InstructionTuned/PixtralLarge.cssrc/VisionLanguage/InstructionTuned/SmolVLM.cssrc/VisionLanguage/Reasoning/KimiVL.cssrc/VisionLanguage/Reasoning/KimiVLThinking.cssrc/VisionLanguage/VisionLanguageModelBase.cstests/AiDotNet.Tests/IntegrationTests/Helpers/LayerHelperIntegrationTests.cstests/AiDotNet.Tests/IntegrationTests/NeuralNetworks/AdvancedNeuralNetworkModelsIntegrationTests.cstests/AiDotNet.Tests/IntegrationTests/SyntheticData/SyntheticTabularGeneratorIntegrationTests.cstests/AiDotNet.Tests/IntegrationTests/VisionLanguage/KimiVLReviewRegressionIntegrationTests.cstests/AiDotNet.Tests/IntegrationTests/VisionLanguage/VisionLanguagePatchSizingReviewRegressionIntegrationTests.cstests/AiDotNet.Tests/ModelFamilyTests/Base/NeuralNetworkModelTestBase.cstests/AiDotNet.Tests/ModelFamilyTests/Base/OpticalFlowTestBase.cstests/AiDotNet.Tests/ModelFamilyTests/NeuralNetworks/DeepBeliefNetworkTests.cstests/AiDotNet.Tests/ModelFamilyTests/NeuralNetworks/EmotiVoiceTests.cstests/AiDotNet.Tests/ModelFamilyTests/NeuralNetworks/TableGANGeneratorTests.cs
Per CodeRabbit review on PR #1318: helper/utility classes that aren't user-facing should stay internal. KaimingInitHelper is initialization plumbing called by ConvolutionalLayer only; nothing outside the assembly references it. Resolves PRRT_kwDOKSXUF86CkKI2. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…Architecture CodeRabbit flagged this block as either an unattached docstring (it now visually documents VocabularySize because the property below it is just a comment-only marker) or a candidate for a forwarding-shim property. Project preference per [[feedback_no_obsolete_shims]] is clean breaking changes over no-op forwarders. RandomSeed is now declared on the NeuralNetworkArchitecture base class with its own full XML doc; the duplicate doc block on TransformerArchitecture had no purpose and was misleading the reader. Deleting the comment-only marker + its orphan docstring leaves the base class doc as the single source of truth. Resolves PRRT_kwDOKSXUF86CkKI9. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…a options CodeRabbit flagged the hardcoded _fusedTrainingDisabled = true on every GraFPrint instance as a test-context mitigation baked into production behaviour. Move it behind a new GraFPrintOptions.DisableFusedOptimizerStep flag (default false = production path), and have the model honour _options.DisableFusedOptimizerStep instead of unconditionally disabling. The GraFPrintLossTraceTests test base flips the new option to true while the underlying 30-iter divergence on the 53-layer BN pyramid is being traced; production callers run the full fused path unchanged. Resolves PRRT_kwDOKSXUF86ClRSI. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…tack branch CodeRabbit flagged 7 VLM InitializeLayers overrides where ValidatePatchOptions / ValidateVisualPatchOptions ran unconditionally — rejecting valid custom Architecture.Layers stacks on _options.ImageSize / _options.MaxVisualTokens values that the custom path never consumes. Move the validation inside the default-stack else-branch so custom layer stacks construct successfully. Files: Phi3Vision, Phi4Multimodal, Pixtral, PixtralLarge, SmolVLM, KimiVL, KimiVLThinking. Resolves PRRT_kwDOKSXUF86ClRSa, PRRT_kwDOKSXUF86ClRSd, PRRT_kwDOKSXUF86ClRSf, PRRT_kwDOKSXUF86ClRSg, PRRT_kwDOKSXUF86ClRSh, PRRT_kwDOKSXUF86ClRSi, PRRT_kwDOKSXUF86ClRSj. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…VLM/TableGAN instances Address 6 unresolved CodeRabbit comments on the test side of PR #1318: - LayerHelperIntegrationTests: assert ex.ParamName == "patchSize" instead of substring-matching the message text (stable against wording changes). - SyntheticTabularGeneratorIntegrationTests: disambiguate the reflected BuildClassificationTargetTensor lookup by binding the exact (Tensor<double>, Tensor<double>) signature — prevents future overload additions from triggering AmbiguousMatchException at runtime. No null- forgiving operators per global C# coding rule. - KimiVLReviewRegressionIntegrationTests: add [Fact(Timeout = 120000)] to all 3 tests so a hang can't block CI. - VisionLanguagePatchSizingReviewRegressionIntegrationTests: * Wrap each VLM constructor in a using-scoped factory helper (AssertPatchSizeIsEight) so the heavyweight model instances are disposed promptly instead of accumulating across 10 sequential allocations. * Switch AssertInvalidSizingRejected to assert ex.ParamName instead of substring-matching the message; the ValidateVisualPatchOptions throws use nameof(imageSize) / nameof(maxVisualTokens) so the expected names are now lowercase-camel. * Add [Fact(Timeout = 120000)]. - DeepBeliefNetworkTests: replace the IsNaN-only guard with two IsFinite asserts so Infinity counts as failure too (matches the invariant's intent — gradient explosion or numerical instability). - TableGANGeneratorTests: * Use 'using var' on every CreateGenerator() call across the file so the disposable generator's allocations don't accumulate. * Strengthen Fit_TinyDataset_MarksGeneratorAsFitted with an actual training-effect signal — capture generator output on a fixed-noise probe before and after Fit, and require either a shape change or a non-zero L2 distance. The prior bookkeeping-only assertions would pass even on a Fit that silently skipped the generator update. Resolves PRRT_kwDOKSXUF86ClRSl, PRRT_kwDOKSXUF86ClRSn, PRRT_kwDOKSXUF86ClRSr, PRRT_kwDOKSXUF86ClRSs, PRRT_kwDOKSXUF86ClRSt, PRRT_kwDOKSXUF86ClRSw, PRRT_kwDOKSXUF86ClRSy, PRRT_kwDOKSXUF86ClRS2. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The _optimizer field was readonly and built in the constructor (from CreateDefaultOptimizer using constructor-time _options defaults), so a reloaded native model kept those defaults instead of the persisted LearningRate / Beta1 / Beta2 / Epsilon / WeightDecay values that SerializeNetworkSpecificData writes and DeserializeNetworkSpecificData hydrates back. Remove the readonly modifier and have DeserializeNetworkSpecificData rebuild _optimizer via CreateDefaultOptimizer() once _options is fully populated (native-mode only — ONNX-mode models never use _optimizer). Resolves PRRT_kwDOKSXUF86ClRSY. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…AN closures CodeRabbit flagged three TapeStepContext RecomputeLoss closures that only replayed a partial objective compared to the loss that produced grads: - MedSynthGenerator.TrainDiscriminatorStepBatchedNonDP (line 528-530): lossTensor = lossReal + lossFake but RecomputeLoss only replayed the real-sample BCE. Now captures fakeBatch in the closure and replays both BCE terms. - MedSynthGenerator.TrainDiscriminatorStepPerExampleDPSGD (line 627-634): same issue in the DP-SGD path — per-example losses were lossReal + lossFake but RecomputeLoss only replayed real. BuildRealAndFakeBatches now keeps both sides; the lastFake batch is captured for the replay. - TimeGANGenerator.TrainDiscriminatorStepBatched (line 655-662): critic step had the same shape — lossReal + lossFake on lossTensor but RecomputeLoss replayed only lossReal. Captures fakeSup so the discriminator replay re-scores it. - TimeGANGenerator.TrainGeneratorStepBatched (line 731-739): joint Phase 3 loss combined advLoss + γ·supLoss when supervisedBatch > 0, but RecomputeLoss replayed only advLoss — silently dropping the temporal supervision term that the paper's joint phase exists to apply. Captures xt, xtNext, advAxes, and SupervisedWeight; replays the full adv + weighted-sup sum on every replay (and falls back to adv-only when supervisedBatch == 0, matching the forward branch). Without these fixes, an optimizer.Step that exercises the replay closure (e.g. for line search / trust-region) would update parameters against an objective that doesn't match the gradient — silent training drift. Resolves PRRT_kwDOKSXUF86ClRSO, PRRT_kwDOKSXUF86ClRSS, PRRT_kwDOKSXUF86ClRSV. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
CodeRabbit (critical): TrainDiscriminatorStepBatchedDP runs _options.DiscriminatorSteps times per outer batch (each touches real data and is its own DP-SGD privacy event under Abadi 2016 §3), but _cumulativeEpsilon was incremented only once per outer batch — under- counting reported epsilon and the early-stop guard by a factor of DiscriminatorSteps. Move the ComputeStepPrivacyCost call inside the inner critic loop so each critic step charges its own slot. Fixed in both Fit (line 388-401) and FitAsync (line 444-458). Resolves PRRT_kwDOKSXUF86ClRSM. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…CTGAN critic CodeRabbit (blocking): the WGAN critic loss in both TableGANGenerator and DPCTGANGenerator regressed from WGAN-GP to plain WGAN — the critic was optimizing E[D(fake)] − E[D(real)] with no 1-Lipschitz constraint, which diverges without weight clipping per Gulrajani et al. 2017 §3-§4. The options classes still exposed GradientPenaltyWeight but it was never consulted in the critic objective. - TableGANGenerator.TrainDiscriminatorStepBatched: compose lossTensor as (wasserstein + λ·GP); RecomputeLoss replays both terms so optimizer.Step replay stays tied to the same objective. - DPCTGANGenerator.TrainDiscriminatorStepBatchedDP: same composition for the reported lossValue and the replay closure (the reported scalar needs to match what the gradients minimize). - DPCTGANGenerator.ComputePerExampleNoisedGradients: per-example loss is now (wasserstein + λ·GP) on the (real_ex, fake_ex) pair BEFORE clipping, per Abadi 2016 §3 — DP-SGD's L2-sensitivity bound clips the full critic objective, so the GP term must enter inside the per-example tape scope. - Both files now have a private ComputeGradientPenalty(realBatch, fakeBatch) helper mirroring CausalGANGenerator.ComputeGradientPenalty: uniform-α interpolates, nested GradientTape to get d(D(interp))/d(interp), squared deviation of per-sample L2 norm from 1, mean over batch. Resolves PRRT_kwDOKSXUF86ClRSN, PRRT_kwDOKSXUF86ClRSP. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/Helpers/LayerHelper.cs (1)
1426-1452:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winBlocking: keep
CreateDefaultDeepBeliefNetworkLayersin the requiredyield returnform.This factory now stages layers in a
List<ILayer<T>>and emits them later, which breaks the LayerHelper contract forCreateDefault{ModelName}Layers. Here the final head input width is already known (rbmStackSizes[^1]), so the dense head can be constructed eagerly and yielded directly, which also removes the extraChainResolveLazyLayerspass.As per coding guidelines "Yield Return Pattern: Must use
yield returnfor each layer, not build a list".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Helpers/LayerHelper.cs` around lines 1426 - 1452, The CreateDefaultDeepBeliefNetworkLayers factory currently builds a List<ILayer<T>> and emits layers later, violating the "yield return" pattern; change it to yield each layer as it is constructed: iterate rbmStackSizes and yield return new RBMLayer<T>(...) for each stack pair (using sigmoidActivation), compute the finalActivation exactly as shown (softmaxActivation for MultiClassClassification else new IdentityActivation<T>()), then eagerly construct and yield return the DenseLayer using rbmStackSizes[^1] as the head input width and architecture.OutputSize for output, and remove the temporary List<ILayer<T>> and the ChainResolveLazyLayers call. Ensure references to RBMLayer, DenseLayer, rbmStackSizes, architecture.OutputSize, finalActivation and ChainResolveLazyLayers are updated/removed accordingly.src/NeuralNetworks/Layers/DeconvolutionalLayer.cs (1)
879-886:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix missing activation serialization in DeconvolutionalLayer.GetMetadata().
The
GetMetadata()implementation does not serialize the activation function type, whileConvolutionalLayerdoes. This breaks round-trip serialization forDeconvolutionalLayerinstances with non-default activations (e.g.,Identityfor flow heads in RAPIDFlow, custom activations in DCGAN/VAE models).When deserialized, the layer will always use the default
ReLUactivation instead of preserving the original, corrupting models that depend on specific activation semantics.Add this after line 885 to match
ConvolutionalLayer:if (ScalarActivation is not null) { metadata["ScalarActivationType"] = ScalarActivation.GetType().AssemblyQualifiedName ?? string.Empty; }(Note: The key name
"KernelSize"is correct—deserialization expects different keys for different layer types.)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/NeuralNetworks/Layers/DeconvolutionalLayer.cs` around lines 879 - 886, The DeconvolutionalLayer.GetMetadata() method omits serializing the activation type, causing deserialized layers to lose non-default activations; update GetMetadata (in class DeconvolutionalLayer, method GetMetadata) to include the ScalarActivation type in the returned metadata by checking if ScalarActivation is not null and then adding an entry "ScalarActivationType" with ScalarActivation.GetType().AssemblyQualifiedName (or empty string if null) so that activation information matches ConvolutionalLayer's serialization and preserves activations across round-trip serialization.src/NeuralNetworks/NeuralNetworkBase.cs (1)
5757-5801:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFused training still freezes raw network-level trainables.
This path only hands
trainableLayerstoTryStepWithFusedOptimizer. Unlike the eager path, it never includes or separately updates tensors returned byGetExtraTrainableTensors(), so models with raw trainables silently stop training those parameters whenever fused mode engages.Safe fallback
var trainableLayers = Training.TapeTrainingStep<T>.CollectTrainableLayers(Layers, _layerStructureVersion); if (trainableLayers.Length == 0) return EmitFusedMissAndFallback("no trainable layers"); + foreach (var extra in GetExtraTrainableTensors()) + { + if (extra is not null && extra.Length > 0) + return EmitFusedMissAndFallback("network-level extra trainables require eager optimizer path"); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/NeuralNetworks/NeuralNetworkBase.cs` around lines 5757 - 5801, The fused training path calls Training.CompiledTapeTrainingStep<T>.TryStepWithFusedOptimizer with only trainableLayers, which omits raw network-level trainables returned by GetExtraTrainableTensors() and thus they never get updated; update the logic that builds trainableLayers before calling TryStepWithFusedOptimizer to also include tensors from Training.TapeTrainingStep<T>.GetExtraTrainableTensors(Layers, _layerStructureVersion) (or pass them as an explicit extra-tensors parameter to TryStepWithFusedOptimizer if it supports it), ensuring the same ordering/identity used by the eager path so those raw trainables are included in the fused optimizer step and receive gradient updates.
♻️ Duplicate comments (4)
src/Diffusion/Video/VideoCrafterModel.cs (1)
224-237:⚠️ Potential issue | 🟠 Major | ⚡ Quick winBlocking: don’t hardcode a test-tuned inference budget in the production constructor default.
Line 237 forces
DefaultInferenceSteps = 4for every caller that omitsoptions, while the rationale is test-time budget control. Keep constructor defaults production-oriented and pass reduced inference steps explicitly from tests.Proposed fix
- options ?? new DiffusionModelOptions<T> { DefaultInferenceSteps = 4 }, + options ?? new DiffusionModelOptions<T>(),As per coding guidelines, “Production Readiness (CRITICAL - Flag as BLOCKING)… hardcoded values instead of proper logic.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Diffusion/Video/VideoCrafterModel.cs` around lines 224 - 237, The VideoCrafterModel constructor currently injects a test-tuned DefaultInferenceSteps = 4 by setting options ?? new DiffusionModelOptions<T> { DefaultInferenceSteps = 4 }, which hardcodes a reduced inference budget into production; change the constructor to use the normal production default (do not override DiffusionModelOptions<T>.DefaultInferenceSteps) when options is null (e.g., instantiate a plain new DiffusionModelOptions<T>() or accept null and let callers use defaults), and update the ScaledInput_ShouldChangeOutput test to pass an explicit DiffusionModelOptions with DefaultInferenceSteps = 4 when constructing VideoCrafterModel for test-time budget control.src/NeuralNetworks/NeuralNetworkBase.cs (2)
5350-5368:⚠️ Potential issue | 🟠 Major | ⚡ Quick winClip the raw network-level trainables too.
This only clips
grads, but the method later updates tensors fromGetExtraTrainableTensors()viaallGrads. That leaves raw trainables like cls/pos embeddings outside the global-norm cap, so the step no longer matches the advertised “all trainable parameters” behavior.Suggested fix
- double maxGradNorm = MaxGradNorm; - if (maxGradNorm > 0.0 && grads.Count > 0) + double maxGradNorm = MaxGradNorm; + if (maxGradNorm > 0.0) { - ApplyGradientClipping(grads, maxGradNorm, trainableParams); + var clipOrder = trainableParams.Concat(extraTrainableTensors).ToList(); + var clipTargets = new Dictionary<Tensor<T>, Tensor<T>>( + Helpers.TensorReferenceComparer<Tensor<T>>.Instance); + foreach (var param in clipOrder) + { + if (allGrads.TryGetValue(param, out var grad)) + clipTargets[param] = grad; + } + if (clipTargets.Count > 0) + ApplyGradientClipping(clipTargets, maxGradNorm, clipOrder); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/NeuralNetworks/NeuralNetworkBase.cs` around lines 5350 - 5368, The global-norm clipping only passes `grads` to ApplyGradientClipping, but later updates also use `allGrads` (which includes tensors from GetExtraTrainableTensors()), so raw network-level trainables (e.g. cls/pos embeddings) are not clipped; modify the clipping call to include those extra trainable gradients by merging `grads` with the gradients/tensors produced from GetExtraTrainableTensors() (or pass `allGrads` instead) and keep using `trainableParams` as the deterministic key, ensuring ApplyGradientClipping is invoked with the full set of trainable gradients when MaxGradNorm > 0.0.
294-335:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRestore the protected
MaxGradNormcontract name.Renaming the subclass-facing field from
MaxGradNormtoMaxGradNormFieldis a source-breaking change for derived models that read or assignMaxGradNorm; the new public property is read-only anddouble-typed, so it does not preserve that contract. Keep the protectedTmember name stable and give the new public accessor a distinct name instead.As per coding guidelines, "Public API Surface (CRITICAL)" changes on
src/**should be scrutinized carefully.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/NeuralNetworks/NeuralNetworkBase.cs` around lines 294 - 335, Restore the original protected T member name by renaming MaxGradNormField back to MaxGradNorm, rename the current public double property MaxGradNorm to a distinct name (e.g. MaxGradNormDouble or MaxGradNormValue) and update its XML docs accordingly, and update the protected accessor MaxGradNormT to call NumOps.FromDouble on the new public double property (NumOps.FromDouble(MaxGradNormDouble)); finally search and update any internal usages that referenced MaxGradNormField or the old MaxGradNorm property so they now use the restored protected MaxGradNorm or the new public double property name respectively.src/NeuralNetworks/SyntheticData/DPCTGANGenerator.cs (1)
604-628:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftRestore WGAN-GP in the DP critic path.
lossTensor,RecomputeLoss, and the per-example loss insideComputePerExampleNoisedGradients(...)are all plainE[D(fake)] - E[D(real)]._options.GradientPenaltyWeightis never applied here anymore, so this path no longer matches the documented WGAN-GP objective.💡 Suggested direction
- var lossTensor = Engine.TensorSubtract(avgFake, avgReal); + var gradientPenalty = ComputeGradientPenalty(realPacked, fakePacked); + var lossTensor = Engine.TensorAdd( + Engine.TensorSubtract(avgFake, avgReal), + Engine.TensorMultiplyScalar( + gradientPenalty, + NumOps.FromDouble(_options.GradientPenaltyWeight)));Apply the same term in
RecomputeLoss(...)and in each per-examplelossbefore clipping/noising, otherwise the replayed objective and the DP-SGD path still optimize plain WGAN.Also applies to: 688-758
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/NeuralNetworks/SyntheticData/DPCTGANGenerator.cs` around lines 604 - 628, The WGAN-GP gradient penalty term is missing: include the same _options.GradientPenaltyWeight * gradient_penalty term in the loss used for the replay path (lossTensor), in RecomputeLoss, and also add that term to each per-example loss computed inside ComputePerExampleNoisedGradients (before any clipping/noising) so the recomputed objective and the DP-SGD/noised gradients match the documented WGAN-GP objective; update uses of avgReal/avgFake (and any recomputedFakeScores) to include the computed gradient penalty contribution so lossTensor, RecomputeLoss, and the per-example loss all contain E[D(fake)] - E[D(real)] + _options.GradientPenaltyWeight * gradient_penalty.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Helpers/LayerHelper.cs`:
- Around line 32114-32138: The factory currently only projects when
inputFeatureDim > 0, leaving the no-projection path when callers omit the
argument; change the logic in the method in LayerHelper (the block that creates
DenseLayer<T> projection) to first derive an effectiveInputDim when
inputFeatureDim == 0 by calling architecture.GetInputShape() (or similar API
available on the architecture object), validate it (throw an
ArgumentException/InvalidOperationException if it cannot be inferred), and then
use effectiveInputDim in place of inputFeatureDim for the projection decision
(i.e., if effectiveInputDim > 0 && effectiveInputDim != encoderDim then yield
return new DenseLayer<T>(encoderDim, identityActivation)); retain the existing
negative-argument check for inputFeatureDim and preserve GELU/Identity
activation setup and exception behavior.
In `@src/NeuralNetworks/SyntheticData/DPCTGANGenerator.cs`:
- Around line 389-405: The auto-noise calibration currently treats each outer
batch as a single privacy event; update the calibration so that when
NoiseMultiplier is auto-derived (via ComputeNoiseMultiplier or similar), it
scales the required noise by _options.DiscriminatorSteps (i.e., treat each
TrainDiscriminatorStepBatchedDP call as a separate privacy event). Locate where
ComputeNoiseMultiplier(...) or the auto-derived NoiseMultiplier is computed and
multiply the effective number of steps or events by _options.DiscriminatorSteps
(or pass numPacks * pacSize * _options.DiscriminatorSteps into the privacy
accountant call) so the returned sigma accounts for every critic step; ensure
this same adjustment is applied in the analogous place referenced by lines
455-464.
- Around line 389-405: The loop charges _cumulativeEpsilon per discriminator
step but doesn't stop mid-epoch when the budget is exceeded, so modify the DP
loop in DPCTGANGenerator to check _options.Epsilon immediately after adding
ComputeStepPrivacyCost and abort further steps: before calling
TrainDiscriminatorStepBatchedDP, verify remaining budget and after incrementing
_cumulativeEpsilon if (_cumulativeEpsilon >= _options.Epsilon) break out of the
discriminator loop and prevent the subsequent TrainGeneratorStepBatched from
running (propagate the early-stop to the outer training loop or set a flag),
applying the same fix to the other occurrence around
TrainDiscriminatorStepBatchedDP / TrainGeneratorStepBatched at lines 455-464;
use the existing symbols TrainDiscriminatorStepBatchedDP,
TrainGeneratorStepBatched, ComputeStepPrivacyCost, _cumulativeEpsilon and
_options.Epsilon to locate and guard these spots.
In `@src/NeuralNetworks/SyntheticData/MedSynthGenerator.cs`:
- Around line 641-656: The closure passed into TapeStepContext<T> rebuilds
lastFake from fresh noise, breaking DP replay because avgLoss and noisedAvgGrads
were computed from the original per-example fake rows; update the code so the
RecomputeLoss/ComputeForward closures use the exact fake tensor captured during
the per-example loop (the same tensor used to compute avgLoss and
noisedAvgGrads) instead of reconstructing lastFake — e.g., ensure capturedFake
(or a new variable) references the original per-example fake batch and pass that
into DiscriminatorForwardBatched inside RecomputeLoss/ComputeForward when
constructing the TapeStepContext<T>; alternatively, if capturing the exact batch
is not possible, disable replay for this DP path by not providing a replay
RecomputeLoss to TapeStepContext<T>.
In `@src/NeuralNetworks/SyntheticData/TimeGANGenerator.cs`:
- Around line 278-310: The three phase loops use phaseDuration = Math.Max(1,
epochs / 3) and thus run 3*phaseDuration iterations, violating the caller's
epochs budget; change to explicitly split epochs across phases (e.g., base =
epochs / 3, rem = epochs % 3, then phaseEpochs0 = base + (rem>0?1:0),
phaseEpochs1 = base + (rem>1?1:0), phaseEpochs2 = base) and replace each loop to
run for phaseEpochs0/phaseEpochs1/phaseEpochs2 respectively, updating the loops
that call TrainEmbeddingStepBatched, TrainSupervisedStepBatched,
TrainDiscriminatorStepBatched, TrainGeneratorStepBatched so total iterations
equal epochs (alternatively, if you intend per-phase epochs, rename
phaseDuration to phaseEpochsPerPhase and document it).
In `@src/TextToSpeech/StyleEmotion/EmotiVoice.cs`:
- Around line 46-47: The constructor and CreateDefaultOptimizer currently build
an AdamOptimizer only with InitialLearningRate/Beta1/Beta2/Epsilon, ignoring
persisted WeightDecay and LearningRateSchedulerGamma; update
CreateDefaultOptimizer (and the EmotiVoice native-mode
construction/deserialization path that calls it) to set WeightDecay on the
AdamOptimizerOptions and to attach/configure a learning-rate scheduler using
LearningRateSchedulerGamma (or ensure the optimizer returned is
wrapped/configured to honor the scheduler setting), so the optimizer created by
CreateDefaultOptimizer and used in native mode will reflect _options.WeightDecay
and _options.LearningRateSchedulerGamma (and do the same fix for the duplicate
logic referenced around lines 93-94).
In `@src/VisionLanguage/VisionLanguageModelBase.cs`:
- Around line 205-269: The methods ValidateVisualPatchOptions,
ComputeVisualPatchSize, TransformerBlockLayerCount, ResamplerBlockLayerCount,
ComputeVisionLanguageBoundary, and ValidateEncoderDecoderBoundary are
implementation plumbing and should be narrowed from protected/protected static
to private protected (use private protected static for the static helpers) so
they are not part of the public subclass contract; update each method signature
to use the private protected accessibility modifier while keeping signatures,
return types, and behavior unchanged (e.g., change "protected static void
ValidateVisualPatchOptions(...)" to "private protected static void
ValidateVisualPatchOptions(...)" and similarly for the others).
In
`@tests/AiDotNet.Tests/ModelFamilyTests/NeuralNetworks/DeepBeliefNetworkTests.cs`:
- Around line 213-217: Replace usages of the unavailable double.IsFinite in
DeepBeliefNetworkTests (the assertions referencing lossShort and lossLong) with
a net471-compatible helper: add or reuse a private static bool IsFinite(double
value) => !double.IsNaN(value) && !double.IsInfinity(value) and call
IsFinite(lossShort) and IsFinite(lossLong) in place of double.IsFinite to ensure
the tests compile for the net471 target; you can extract the helper into the
test class or a shared test utilities location if desired.
In
`@tests/AiDotNet.Tests/ModelFamilyTests/NeuralNetworks/TableGANGeneratorTests.cs`:
- Around line 102-108: The test Predict_NoiseInput_ReturnsFiniteRow currently
only checks for non-null and finite values, allowing an empty array to pass; add
a strict size assertion before the finiteness loop (e.g.,
Assert.Equal(expectedRowWidth, output.Length) or Assert.True(output.Length ==
<expectedWidth>)) referencing the output variable in TableGANGeneratorTests so
the test fails if the returned row width is not the expected value; place this
assertion immediately after Assert.NotNull(output) and before the for-loop.
---
Outside diff comments:
In `@src/Helpers/LayerHelper.cs`:
- Around line 1426-1452: The CreateDefaultDeepBeliefNetworkLayers factory
currently builds a List<ILayer<T>> and emits layers later, violating the "yield
return" pattern; change it to yield each layer as it is constructed: iterate
rbmStackSizes and yield return new RBMLayer<T>(...) for each stack pair (using
sigmoidActivation), compute the finalActivation exactly as shown
(softmaxActivation for MultiClassClassification else new
IdentityActivation<T>()), then eagerly construct and yield return the DenseLayer
using rbmStackSizes[^1] as the head input width and architecture.OutputSize for
output, and remove the temporary List<ILayer<T>> and the ChainResolveLazyLayers
call. Ensure references to RBMLayer, DenseLayer, rbmStackSizes,
architecture.OutputSize, finalActivation and ChainResolveLazyLayers are
updated/removed accordingly.
In `@src/NeuralNetworks/Layers/DeconvolutionalLayer.cs`:
- Around line 879-886: The DeconvolutionalLayer.GetMetadata() method omits
serializing the activation type, causing deserialized layers to lose non-default
activations; update GetMetadata (in class DeconvolutionalLayer, method
GetMetadata) to include the ScalarActivation type in the returned metadata by
checking if ScalarActivation is not null and then adding an entry
"ScalarActivationType" with ScalarActivation.GetType().AssemblyQualifiedName (or
empty string if null) so that activation information matches
ConvolutionalLayer's serialization and preserves activations across round-trip
serialization.
In `@src/NeuralNetworks/NeuralNetworkBase.cs`:
- Around line 5757-5801: The fused training path calls
Training.CompiledTapeTrainingStep<T>.TryStepWithFusedOptimizer with only
trainableLayers, which omits raw network-level trainables returned by
GetExtraTrainableTensors() and thus they never get updated; update the logic
that builds trainableLayers before calling TryStepWithFusedOptimizer to also
include tensors from
Training.TapeTrainingStep<T>.GetExtraTrainableTensors(Layers,
_layerStructureVersion) (or pass them as an explicit extra-tensors parameter to
TryStepWithFusedOptimizer if it supports it), ensuring the same
ordering/identity used by the eager path so those raw trainables are included in
the fused optimizer step and receive gradient updates.
---
Duplicate comments:
In `@src/Diffusion/Video/VideoCrafterModel.cs`:
- Around line 224-237: The VideoCrafterModel constructor currently injects a
test-tuned DefaultInferenceSteps = 4 by setting options ?? new
DiffusionModelOptions<T> { DefaultInferenceSteps = 4 }, which hardcodes a
reduced inference budget into production; change the constructor to use the
normal production default (do not override
DiffusionModelOptions<T>.DefaultInferenceSteps) when options is null (e.g.,
instantiate a plain new DiffusionModelOptions<T>() or accept null and let
callers use defaults), and update the ScaledInput_ShouldChangeOutput test to
pass an explicit DiffusionModelOptions with DefaultInferenceSteps = 4 when
constructing VideoCrafterModel for test-time budget control.
In `@src/NeuralNetworks/NeuralNetworkBase.cs`:
- Around line 5350-5368: The global-norm clipping only passes `grads` to
ApplyGradientClipping, but later updates also use `allGrads` (which includes
tensors from GetExtraTrainableTensors()), so raw network-level trainables (e.g.
cls/pos embeddings) are not clipped; modify the clipping call to include those
extra trainable gradients by merging `grads` with the gradients/tensors produced
from GetExtraTrainableTensors() (or pass `allGrads` instead) and keep using
`trainableParams` as the deterministic key, ensuring ApplyGradientClipping is
invoked with the full set of trainable gradients when MaxGradNorm > 0.0.
- Around line 294-335: Restore the original protected T member name by renaming
MaxGradNormField back to MaxGradNorm, rename the current public double property
MaxGradNorm to a distinct name (e.g. MaxGradNormDouble or MaxGradNormValue) and
update its XML docs accordingly, and update the protected accessor MaxGradNormT
to call NumOps.FromDouble on the new public double property
(NumOps.FromDouble(MaxGradNormDouble)); finally search and update any internal
usages that referenced MaxGradNormField or the old MaxGradNorm property so they
now use the restored protected MaxGradNorm or the new public double property
name respectively.
In `@src/NeuralNetworks/SyntheticData/DPCTGANGenerator.cs`:
- Around line 604-628: The WGAN-GP gradient penalty term is missing: include the
same _options.GradientPenaltyWeight * gradient_penalty term in the loss used for
the replay path (lossTensor), in RecomputeLoss, and also add that term to each
per-example loss computed inside ComputePerExampleNoisedGradients (before any
clipping/noising) so the recomputed objective and the DP-SGD/noised gradients
match the documented WGAN-GP objective; update uses of avgReal/avgFake (and any
recomputedFakeScores) to include the computed gradient penalty contribution so
lossTensor, RecomputeLoss, and the per-example loss all contain E[D(fake)] -
E[D(real)] + _options.GradientPenaltyWeight * gradient_penalty.
🪄 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: 2b03ad34-0fc0-4e61-aeeb-f03d285d4c49
⛔ Files ignored due to path filters (1)
tests/AiDotNet.Tests/ModelFamilyTests/Generated/GraFPrintLossTraceTests.csis excluded by!**/generated/**
📒 Files selected for processing (60)
.github/workflows/sonarcloud.yml.gitignoreDirectory.Packages.propssrc/AiDotNet.Generators/TestScaffoldGenerator.cssrc/Audio/Fingerprinting/GraFPrint.cssrc/Audio/Fingerprinting/GraFPrintOptions.cssrc/Diffusion/FastGeneration/ConsistencyModel.cssrc/Diffusion/FastGeneration/Flux2SchnellModel.cssrc/Diffusion/Video/VideoCrafterModel.cssrc/Helpers/DeserializationHelper.cssrc/Helpers/KaimingInitHelper.cssrc/Helpers/LayerHelper.cssrc/NeuralNetworks/DCGAN.cssrc/NeuralNetworks/DeepBeliefNetwork.cssrc/NeuralNetworks/GenerativeAdversarialNetwork.cssrc/NeuralNetworks/Layers/BatchNormalizationLayer.cssrc/NeuralNetworks/Layers/ConvolutionalLayer.cssrc/NeuralNetworks/Layers/DeconvolutionalLayer.cssrc/NeuralNetworks/NeuralNetworkArchitecture.cssrc/NeuralNetworks/NeuralNetworkBase.cssrc/NeuralNetworks/ResNetNetwork.cssrc/NeuralNetworks/SyntheticData/CTGANGenerator.cssrc/NeuralNetworks/SyntheticData/CausalGANGenerator.cssrc/NeuralNetworks/SyntheticData/CopulaGANGenerator.cssrc/NeuralNetworks/SyntheticData/DPCTGANGenerator.cssrc/NeuralNetworks/SyntheticData/MedSynthGenerator.cssrc/NeuralNetworks/SyntheticData/TableGANGenerator.cssrc/NeuralNetworks/SyntheticData/TimeGANGenerator.cssrc/NeuralNetworks/TransformerArchitecture.cssrc/NeuralNetworks/UNet3D.cssrc/NeuralNetworks/VoxelCNN.cssrc/TextToSpeech/StyleEmotion/EmotiVoice.cssrc/TextToSpeech/StyleEmotion/EmotiVoiceOptions.cssrc/Training/CompiledTapeTrainingStep.cssrc/VisionLanguage/InstructionTuned/DeepSeekVL.cssrc/VisionLanguage/InstructionTuned/DeepSeekVL2.cssrc/VisionLanguage/InstructionTuned/Gemma3.cssrc/VisionLanguage/InstructionTuned/InternVL.cssrc/VisionLanguage/InstructionTuned/InternVL2.cssrc/VisionLanguage/InstructionTuned/InternVL25.cssrc/VisionLanguage/InstructionTuned/InternVL3.cssrc/VisionLanguage/InstructionTuned/Llama32Vision.cssrc/VisionLanguage/InstructionTuned/Phi3Vision.cssrc/VisionLanguage/InstructionTuned/Phi4Multimodal.cssrc/VisionLanguage/InstructionTuned/Pixtral.cssrc/VisionLanguage/InstructionTuned/PixtralLarge.cssrc/VisionLanguage/InstructionTuned/SmolVLM.cssrc/VisionLanguage/Reasoning/KimiVL.cssrc/VisionLanguage/Reasoning/KimiVLThinking.cssrc/VisionLanguage/VisionLanguageModelBase.cstests/AiDotNet.Tests/IntegrationTests/Helpers/LayerHelperIntegrationTests.cstests/AiDotNet.Tests/IntegrationTests/NeuralNetworks/AdvancedNeuralNetworkModelsIntegrationTests.cstests/AiDotNet.Tests/IntegrationTests/SyntheticData/SyntheticTabularGeneratorIntegrationTests.cstests/AiDotNet.Tests/IntegrationTests/VisionLanguage/KimiVLReviewRegressionIntegrationTests.cstests/AiDotNet.Tests/IntegrationTests/VisionLanguage/VisionLanguagePatchSizingReviewRegressionIntegrationTests.cstests/AiDotNet.Tests/ModelFamilyTests/Base/NeuralNetworkModelTestBase.cstests/AiDotNet.Tests/ModelFamilyTests/Base/OpticalFlowTestBase.cstests/AiDotNet.Tests/ModelFamilyTests/NeuralNetworks/DeepBeliefNetworkTests.cstests/AiDotNet.Tests/ModelFamilyTests/NeuralNetworks/EmotiVoiceTests.cstests/AiDotNet.Tests/ModelFamilyTests/NeuralNetworks/TableGANGeneratorTests.cs
💤 Files with no reviewable changes (1)
- src/NeuralNetworks/TransformerArchitecture.cs
CodeRabbit: the manual generator forward in the GAN base class computed ReduceMean((discScore − 1)²) which is the LSGAN (Mao 2017) objective — NOT the BCE-with-logits criterion that this base class deliberately wires into the Discriminator (see the long comment at lines 405-419 explaining why GetDefaultLossFunction(BinaryClassification) is BCE-with-logits, which avoids the sigmoid-then-clamp gradient-killing trap). Mixing LSGAN MSE on the generator with BCE-with-logits on the discriminator changed training semantics for every derived GAN that reaches this path. Now both halves of the minimax use the same adversarial objective. Replace the MSE form with the non-saturating BCE-with-logits generator term per Goodfellow 2014 §3: L_G = E[ -log σ(D(G(z))) ] = E[ softplus(-D(G(z))) ] Implementation uses Engine.Softplus on the negated discriminator logits, which is the numerically stable identity for -log σ(x). All ops are tape-tracked so the gradient flows back through the discriminator forward (which is run in EVAL mode but on the active tape) to the generator's parameters. Removed the now-unused allRealLabels tensor (the labels-of-1 vector isn't needed for the logit-form generator loss). Resolves PRRT_kwDOKSXUF86ClRSJ. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
src/NeuralNetworks/SyntheticData/DPCTGANGenerator.cs (2)
389-405:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winScale auto-noise calibration by
DiscriminatorSteps.This schedule now creates
_options.DiscriminatorStepsprivacy events per outer batch, butComputeNoiseMultiplier(...)still calibrates sigma as if there were one. WhenNoiseMultiplieris auto-derived, the run is under-noised and the reported(ε, δ)bound is too optimistic. Update the total-step calculation to multiply byMath.Max(1, _options.DiscriminatorSteps).Also applies to: 455-464
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/NeuralNetworks/SyntheticData/DPCTGANGenerator.cs` around lines 389 - 405, The auto-noise calibration currently treats each outer batch as a single privacy event, but the training loop runs _options.DiscriminatorSteps critic updates per outer batch; update the noise-calculation path (where ComputeNoiseMultiplier or the auto-derived NoiseMultiplier is computed) to multiply the total-step / event count by Math.Max(1, _options.DiscriminatorSteps) so the calibrated sigma accounts for all critic steps per outer batch; apply the same fix in the other duplicate calculation site referenced around the block at lines 455-464 so both noise-calibrations use Math.Max(1, _options.DiscriminatorSteps) when computing total steps for privacy accounting.
400-405:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winAbort the batch as soon as epsilon is exhausted.
After Line 403 and Line 462, the loop keeps running remaining critic steps and still calls
TrainGeneratorStepBatched(...). That spends privacy beyond_options.Epsiloneven though the run should stop immediately. Break out of the inner loops as soon as_cumulativeEpsiloncrosses the budget and skip the generator step for that batch.Also applies to: 459-464
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/NeuralNetworks/SyntheticData/DPCTGANGenerator.cs` around lines 400 - 405, The inner discriminator loop currently continues and the generator step is executed even after _cumulativeEpsilon exceeds _options.Epsilon; modify the loop that iterates dStep (and the surrounding batch logic using numPacks) to check _cumulativeEpsilon after each ComputeStepPrivacyCost call and immediately break out of the discriminator loop and skip calling TrainGeneratorStepBatched(transformedData, numPacks) for that batch when the budget is exhausted; specifically add a conditional using _options.Epsilon after computing the step cost (from ComputeStepPrivacyCost(data.Rows, numPacks * pacSize)) so TrainDiscriminatorStepBatchedDP and subsequent iterations stop and the generator step is not invoked if _cumulativeEpsilon > _options.Epsilon.src/NeuralNetworks/GenerativeAdversarialNetwork.cs (1)
958-974:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftKeep the gradient-penalty loss tape-connected.
ComputeGradientPenalty(...)reduces the penalty to a plainTand then wraps it in a fresh[1]tensor, so thisTrainWithCustomLosscall is still disconnected from the discriminator parameters. WithEnableGradientPenalty()on, the extra step changes diagnostics but contributes zero critic gradients. Return a discriminator-tape-connected penalty tensor here instead of materializing a scalar first.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/NeuralNetworks/GenerativeAdversarialNetwork.cs` around lines 958 - 974, ComputeGradientPenalty currently reduces the penalty to a plain T and then you wrap that scalar in a fresh Tensor, breaking the autodiff tape so no gradients flow to the discriminator; instead return and return a tape-connected Tensor<T> directly to TrainWithCustomLoss. Change usage in the TrainWithCustomLoss delegate for Discriminator so you call a variant of ComputeGradientPenalty that returns a Tensor<T> (or add an overload ComputeGradientPenaltyTensor) taking _lastRealBatch, _lastFakeBatch and _gradientPenaltyLambda and return that Tensor directly (do not materialize to T or construct new Tensor([1])); this preserves the gradient tape through NeuralNetworkBase<T>.TrainWithCustomLoss and ensures critic gradients include the penalty.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/NeuralNetworks/SyntheticData/TableGANGenerator.cs`:
- Around line 580-588: ComputeGeneratorLoss currently adds a classification loss
term (via ComputeClassificationLoss and _options.ClassificationWeight) but the
classifier layers (_classLayers and _classOutput) are never trained, so the
generator is optimizing against random logits; either remove the classification
term from ComputeGeneratorLoss or add a training path that updates the
classifier: implement a routine that computes classification loss on realBatch
(using the same ComputeClassificationLoss or a dedicated ComputeClassifierLoss),
backpropagates and performs an optimizer step for the classifier parameters
(_classLayers/_classOutput) every training iteration (or at a configurable
schedule), and only include the weighted classification term in the generator
loss when the classifier has been trained (or when an explicit TrainClassifier
flag is enabled); ensure optimizer state for the classifier is created and
stepped alongside generator/discriminator optimizers.
---
Duplicate comments:
In `@src/NeuralNetworks/GenerativeAdversarialNetwork.cs`:
- Around line 958-974: ComputeGradientPenalty currently reduces the penalty to a
plain T and then you wrap that scalar in a fresh Tensor, breaking the autodiff
tape so no gradients flow to the discriminator; instead return and return a
tape-connected Tensor<T> directly to TrainWithCustomLoss. Change usage in the
TrainWithCustomLoss delegate for Discriminator so you call a variant of
ComputeGradientPenalty that returns a Tensor<T> (or add an overload
ComputeGradientPenaltyTensor) taking _lastRealBatch, _lastFakeBatch and
_gradientPenaltyLambda and return that Tensor directly (do not materialize to T
or construct new Tensor([1])); this preserves the gradient tape through
NeuralNetworkBase<T>.TrainWithCustomLoss and ensures critic gradients include
the penalty.
In `@src/NeuralNetworks/SyntheticData/DPCTGANGenerator.cs`:
- Around line 389-405: The auto-noise calibration currently treats each outer
batch as a single privacy event, but the training loop runs
_options.DiscriminatorSteps critic updates per outer batch; update the
noise-calculation path (where ComputeNoiseMultiplier or the auto-derived
NoiseMultiplier is computed) to multiply the total-step / event count by
Math.Max(1, _options.DiscriminatorSteps) so the calibrated sigma accounts for
all critic steps per outer batch; apply the same fix in the other duplicate
calculation site referenced around the block at lines 455-464 so both
noise-calibrations use Math.Max(1, _options.DiscriminatorSteps) when computing
total steps for privacy accounting.
- Around line 400-405: The inner discriminator loop currently continues and the
generator step is executed even after _cumulativeEpsilon exceeds
_options.Epsilon; modify the loop that iterates dStep (and the surrounding batch
logic using numPacks) to check _cumulativeEpsilon after each
ComputeStepPrivacyCost call and immediately break out of the discriminator loop
and skip calling TrainGeneratorStepBatched(transformedData, numPacks) for that
batch when the budget is exhausted; specifically add a conditional using
_options.Epsilon after computing the step cost (from
ComputeStepPrivacyCost(data.Rows, numPacks * pacSize)) so
TrainDiscriminatorStepBatchedDP and subsequent iterations stop and the generator
step is not invoked if _cumulativeEpsilon > _options.Epsilon.
🪄 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: 417b4d5c-f93d-4423-b3b8-44f3e8f3ff1f
📒 Files selected for processing (3)
src/NeuralNetworks/GenerativeAdversarialNetwork.cssrc/NeuralNetworks/SyntheticData/DPCTGANGenerator.cssrc/NeuralNetworks/SyntheticData/TableGANGenerator.cs
After the first review-fix pass, CodeRabbit posted 9 follow-up issues — this batch addresses all of them: - LayerHelper.CreateDefaultStyleTTSLayers: 5 internal callers (PromptTTS, StyleTTS, StyleTTS2, StyleTTSZS, OpenVoice) were taking the default inputFeatureDim=0 = no-projection path even though their MelChannels input width doesn't match encoderDim. Wire inputFeatureDim explicitly to _options.MelChannels at every call site so the leading projection is emitted whenever it's actually needed. EmotiVoice already wired this correctly. - GenerativeAdversarialNetwork: the disc-step gradient-penalty closure called the scalar-T ComputeGradientPenalty(...) and wrapped the result in a fresh Tensor<T>([1]) — that wrapper had no recorded GradFn so the outer disc-training tape backprop'd zero signal into discriminator parameters (silent no-op enable). Replaced with new tape-connected BuildTapeTrackedGradientPenalty that does the GP via outer-tape-tracked engine ops + a nested gradient tape for d(D(interp))/d(interp), mirroring the inline pattern used in CausalGAN / TableGAN / DPCTGAN. - DPCTGANGenerator: (a) auto-noise calibration now scales totalSteps by DiscriminatorSteps so the auto-derived sigma accounts for each critic step being its own DP-SGD privacy event (otherwise the reported (ε, δ) bound is overstated when DiscriminatorSteps > 1); (b) added a mid-batch privacyBudgetExhausted break inside the critic loop so a single critic step that pushes the run past the (ε, δ) target stops immediately instead of letting the remaining steps in that epoch leak more budget. Both fixes applied to Fit and FitAsync. - MedSynthGenerator DP-SGD path: captured the actual per-example (real, fake) tensors during the per-example loop and concatenated them for the replay closure, replacing the BuildRealAndFakeBatches(...) fresh-noise rebuild that decoupled the replayed loss from the noisedAvgGrads. - TimeGANGenerator: replaced phaseDuration = max(1, epochs/3)*3 — which over-trains when epochs < 3 and drops the remainder for non-multiples of 3 — with an explicit (base + remainder) split that exactly honors the caller's epochs budget across phases. - EmotiVoice CreateDefaultOptimizer: switched from AdamOptimizer to AdamWOptimizer so persisted _options.WeightDecay actually applies (AdamOptimizerOptions has no WeightDecay setting), and attached an ExponentialLRScheduler with the persisted SchedulerGamma when gamma is in (0, 1). The serialization round-trip now actually honors both hyperparameters instead of silently dropping them. - VisionLanguageModelBase: narrowed 6 plumbing helpers (ValidateVisualPatchOptions, ComputeVisualPatchSize, TransformerBlockLayerCount, ResamplerBlockLayerCount, ComputeVisionLanguageBoundary, ValidateEncoderDecoderBoundary) from protected to private protected so they're not part of the external subclass contract. - DeepBeliefNetworkTests: double.IsFinite isn't in net471 (the test project multi-targets), so swapped back to the IsNaN || IsInfinity polyfill while keeping the Infinity-as-failure semantics. - TableGANGeneratorTests.Predict_NoiseInput_ReturnsFiniteRow: added a strict Assert.Equal(options.EmbeddingDimension, output.Length) before the finite-value loop so the test can't silently pass on an empty prediction. Resolves PRRT_kwDOKSXUF86Clvh2, PRRT_kwDOKSXUF86Clvh3, PRRT_kwDOKSXUF86Clvh5, PRRT_kwDOKSXUF86Clvh6, PRRT_kwDOKSXUF86Clvh8, PRRT_kwDOKSXUF86Clvh9, PRRT_kwDOKSXUF86ClviB, PRRT_kwDOKSXUF86ClviE, PRRT_kwDOKSXUF86ClviJ. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ier toggle CodeRabbit: ComputeGeneratorLoss adds a weighted classification term but this file never trains _classLayers / _classOutput on real (row, label) pairs — the classifier head's logits are effectively random, so the generator gradient driven by ClassificationWeight is noise rather than a label-fidelity-preserving signal. Add TableGANOptions.TrainClassifier (default false) and gate the classification-loss inclusion on it. Until a dedicated classifier-training routine lands that updates _classLayers/_classOutput against real labels, the classifier-aux term stays disabled by default; callers who later wire the training path can opt back in explicitly. Resolves PRRT_kwDOKSXUF86Clz8b. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/NeuralNetworks/GenerativeAdversarialNetwork.cs (1)
950-987:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFold GP into the discriminator loss instead of stepping twice.
The new WGAN-GP path still applies gradient penalty in a second discriminator optimizer step after the BCE step. That is not equivalent to optimizing
BCE + λ·GP: Adam/momentum state advances twice, and the GP gradient is evaluated at post-BCE weights. This changes training semantics and can destabilize derived GANs that enable_useGradientPenalty.Suggested direction
- Discriminator.Train(combinedImages, combinedLabels); - T combinedDiscLoss = Discriminator.GetLastLoss(); + T combinedDiscLoss; + var trainableDisc = (NeuralNetworkBase<T>)Discriminator; + combinedDiscLoss = trainableDisc.TrainWithCustomLoss(combinedImages, pred => + { + var advLoss = /* same combined real/fake adversarial loss used today */; + if (_useGradientPenalty && _lastRealBatch is not null && _lastFakeBatch is not null) + { + advLoss = Engine.TensorAdd( + advLoss, + BuildTapeTrackedGradientPenalty(_lastRealBatch, _lastFakeBatch, _gradientPenaltyLambda)); + } + return advLoss; + }); ... - if (_useGradientPenalty && _lastRealBatch is not null && _lastFakeBatch is not null) - { - ... - trainableDisc.TrainWithCustomLoss(realCaptured, _ => - BuildTapeTrackedGradientPenalty(realCaptured, fakeCaptured, lambdaCaptured)); - }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/NeuralNetworks/GenerativeAdversarialNetwork.cs` around lines 950 - 987, The current code applies gradient penalty in a separate discriminator optimizer step which advances optimizer state twice and evaluates GP at post-BCE weights; instead, when _useGradientPenalty is true and _lastRealBatch/_lastFakeBatch are available, fold the GP into the discriminator training closure so TrainWithCustomLoss computes combined loss = discriminator BCE loss + _gradientPenaltyLambda * BuildTapeTrackedGradientPenalty(real, fake, lambda) in one backward/optimizer step; update the TrainWithCustomLoss invocation on the Discriminator (NeuralNetworkBase<T>.TrainWithCustomLoss) to compute BCE and call BuildTapeTrackedGradientPenalty inside the same loss lambda (using _lastRealBatch/_lastFakeBatch and _gradientPenaltyLambda) and remove the separate try/catch block that runs a second optimizer step to ensure optimizer (Adam/momentum) state is updated only once.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/NeuralNetworks/SyntheticData/MedSynthGenerator.cs`:
- Around line 756-778: GenerateNoiseBatchTensor currently uses
Engine.TensorRandomUniformRange<T> (non-deterministic) to produce the Box–Muller
uniforms; replace that with seed-aware sampling from the instance RNG (_random)
so MedSynthOptions.Seed is honored. Concretely, produce two arrays of length
halfElements with values drawn via _random.NextDouble() (or the existing _random
helper used elsewhere), convert those arrays into tensors (instead of calling
Engine.TensorRandomUniformRange<T>), then continue computing u1 = 1 - u1Temp,
radius, theta, z1, z2 and interleave into noiseData as before; keep all other
math using Engine and return the Tensor<T> shaped [batchSize, latentDim] so
batched noise is deterministic when _options.Seed/_random is set.
In `@src/NeuralNetworks/SyntheticData/TimeGANGenerator.cs`:
- Around line 275-285: The code splits epochs into
phase1Epochs/phase2Epochs/phase3Epochs but does not validate the caller-supplied
epochs before setting IsFitted, allowing negative/zero epochs to mark an
untrained model as fitted; add a guard at the start of the training routine in
TimeGANGenerator (after computing baseEpochs/remainder or at method entry) that
throws an ArgumentOutOfRangeException (or returns an error) when epochs <= 0,
ensuring IsFitted is not set to true for invalid input, and update any
downstream logic around IsFitted so it only flips to true after at least one
training iteration has been executed.
- Around line 592-594: When BuildPairedSequenceBatch(...) returns zero pairs, do
not silently skip the temporal objective—detect this and fail fast: in the
TimeGANGenerator class (the method that calls BuildPairedSequenceBatch and
currently does "if (xt.Shape[0] == 0) return;") throw a descriptive exception
(e.g. ArgumentException or InvalidOperationException) indicating invalid
input/sequence length and include SequenceLength and sample info; also add
upfront validation for SequenceLength < 2 or single-row sequences to prevent
entering training at all. Apply the same change to the other location that
builds paired batches (the block around the 724-749 range) so both code paths
reject zero-pair training instead of marking IsFitted = true or continuing
silently.
- Around line 849-870: GenerateNoiseBatchTensor currently uses
Engine.TensorRandomUniformRange<T> (u2 and u1Temp) which bypasses the instance
RNG seeded from _options.Seed, making Fit nondeterministic; change
GenerateNoiseBatchTensor to draw uniform random values from the seeded _random
(or a RNG wrapper that uses _random) to produce u1 and u2 (or directly fill
noiseData) so the resulting radius/theta/z1/z2 are reproducible, then construct
the Tensor<T> as before; reference GenerateNoiseBatchTensor, _random,
_options.Seed, Engine.TensorRandomUniformRange<T>, and the final new
Tensor<T>(noiseData, ...) when implementing the deterministic sampling.
---
Outside diff comments:
In `@src/NeuralNetworks/GenerativeAdversarialNetwork.cs`:
- Around line 950-987: The current code applies gradient penalty in a separate
discriminator optimizer step which advances optimizer state twice and evaluates
GP at post-BCE weights; instead, when _useGradientPenalty is true and
_lastRealBatch/_lastFakeBatch are available, fold the GP into the discriminator
training closure so TrainWithCustomLoss computes combined loss = discriminator
BCE loss + _gradientPenaltyLambda * BuildTapeTrackedGradientPenalty(real, fake,
lambda) in one backward/optimizer step; update the TrainWithCustomLoss
invocation on the Discriminator (NeuralNetworkBase<T>.TrainWithCustomLoss) to
compute BCE and call BuildTapeTrackedGradientPenalty inside the same loss lambda
(using _lastRealBatch/_lastFakeBatch and _gradientPenaltyLambda) and remove the
separate try/catch block that runs a second optimizer step to ensure optimizer
(Adam/momentum) state is updated only once.
🪄 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: 35725e78-212f-402c-b3ae-7d6c3752d7c9
📒 Files selected for processing (13)
src/NeuralNetworks/GenerativeAdversarialNetwork.cssrc/NeuralNetworks/SyntheticData/DPCTGANGenerator.cssrc/NeuralNetworks/SyntheticData/MedSynthGenerator.cssrc/NeuralNetworks/SyntheticData/TimeGANGenerator.cssrc/TextToSpeech/DescriptionBased/PromptTTS.cssrc/TextToSpeech/StyleEmotion/EmotiVoice.cssrc/TextToSpeech/StyleEmotion/StyleTTS.cssrc/TextToSpeech/StyleEmotion/StyleTTS2.cssrc/TextToSpeech/StyleEmotion/StyleTTSZS.cssrc/TextToSpeech/VoiceCloning/OpenVoice.cssrc/VisionLanguage/VisionLanguageModelBase.cstests/AiDotNet.Tests/ModelFamilyTests/NeuralNetworks/DeepBeliefNetworkTests.cstests/AiDotNet.Tests/ModelFamilyTests/NeuralNetworks/TableGANGeneratorTests.cs
Contributor
There was a problem hiding this comment.
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 (3)
src/NeuralNetworks/SyntheticData/TableGANGenerator.cs (2)
938-941: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueConsider throwing
NotSupportedExceptionfor unsupported training path.The
Trainmethod is empty with only a comment. While the architectural decision to useFit/FitAsyncis valid for GAN training, an empty override may confuse callers who expect the standard training API to work.💡 Suggested explicit rejection
public override void Train(Tensor<T> input, Tensor<T> expectedOutput) { - // TableGAN uses its own specialized training via Fit/FitAsync. + throw new NotSupportedException( + "TableGAN uses GAN-style adversarial training via Fit() or FitAsync(). " + + "The standard Train() method is not applicable to generative adversarial networks."); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/NeuralNetworks/SyntheticData/TableGANGenerator.cs` around lines 938 - 941, The override of Train in TableGANGenerator is a no-op which can mislead callers; update the TableGANGenerator.Train(Tensor<T> input, Tensor<T> expectedOutput) method to throw a NotSupportedException with a clear message stating that standard Train is not supported and instructing callers to use Fit or FitAsync instead so the unsupported training path fails fast and documents the intended API.
914-923: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winRemove dead code:
UpdateGeneratorParametersandUpdateDiscriminatorParametersare unused after tape-based training migration.These methods (lines 914–923) call
layer.UpdateParameters(learningRate)directly, butFit()uses tape-based training withGradientTape+TapeStepContext+_optimizer.Step()pattern. The old methods are never invoked and would throw "Backward pass must be called before updating parameters" if called. Delete both methods.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/NeuralNetworks/SyntheticData/TableGANGenerator.cs` around lines 914 - 923, Remove the dead methods UpdateGeneratorParameters(T learningRate) and UpdateDiscriminatorParameters(T learningRate) from TableGANGenerator.cs: they are no longer used after the tape-based training migration (Fit uses GradientTape, TapeStepContext and _optimizer.Step()) and would call layer.UpdateParameters directly causing a "Backward pass must be called before updating parameters" if invoked; delete both methods and any references to them so parameter updates are handled only via the tape/_optimizer.Step() workflow.src/Models/Options/TableGANOptions.cs (1)
39-159:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftBLOCKING: Missing copy constructor per golden pattern.
TableGANOptions must implement a copy constructor matching the golden pattern:
public TableGANOptions(TableGANOptions<T> other). This constructor must:
- Throw
ArgumentNullExceptionif other is null- Copy ALL 12 properties (EmbeddingDimension, GeneratorDimensions, DiscriminatorDimensions, ClassifierDimensions, LabelColumnIndex, ClassificationWeight, TrainClassifier, InformationWeight, BatchSize, Epochs, LearningRate, VGMModes, DiscriminatorDropout, GradientPenaltyWeight, DiscriminatorSteps)
Missing this constructor causes silent data loss when cloning options and violates the required golden pattern for all Options classes.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Models/Options/TableGANOptions.cs` around lines 39 - 159, Add a copy constructor public TableGANOptions(TableGANOptions<T> other) to the TableGANOptions<T> class that throws ArgumentNullException when other is null and then copies all option fields from other (EmbeddingDimension, GeneratorDimensions, DiscriminatorDimensions, ClassifierDimensions, LabelColumnIndex, ClassificationWeight, TrainClassifier, InformationWeight, BatchSize, Epochs, LearningRate, VGMModes, DiscriminatorDropout, GradientPenaltyWeight, DiscriminatorSteps); ensure you perform deep copies for the int[] properties (GeneratorDimensions, DiscriminatorDimensions, ClassifierDimensions) rather than assigning references so mutations on the clone do not affect the source.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Models/Options/TableGANOptions.cs`:
- Around line 84-95: The TrainClassifier property lacks required XML
documentation elements; add a <value> element that succinctly states it is a
boolean controlling whether the generator loss includes the classification
auxiliary term, and add a <remarks> section containing a <para><b>For
Beginners:</b>...</para> that explains in plain language when to enable this
flag (e.g., only enable after wiring a classifier training routine that updates
_classLayers/_classOutput against real labels so ClassificationWeight is
meaningful) and any default behavior (defaults to false). Ensure the new XML
tags appear alongside the existing <summary> for the TrainClassifier property.
---
Outside diff comments:
In `@src/Models/Options/TableGANOptions.cs`:
- Around line 39-159: Add a copy constructor public
TableGANOptions(TableGANOptions<T> other) to the TableGANOptions<T> class that
throws ArgumentNullException when other is null and then copies all option
fields from other (EmbeddingDimension, GeneratorDimensions,
DiscriminatorDimensions, ClassifierDimensions, LabelColumnIndex,
ClassificationWeight, TrainClassifier, InformationWeight, BatchSize, Epochs,
LearningRate, VGMModes, DiscriminatorDropout, GradientPenaltyWeight,
DiscriminatorSteps); ensure you perform deep copies for the int[] properties
(GeneratorDimensions, DiscriminatorDimensions, ClassifierDimensions) rather than
assigning references so mutations on the clone do not affect the source.
In `@src/NeuralNetworks/SyntheticData/TableGANGenerator.cs`:
- Around line 938-941: The override of Train in TableGANGenerator is a no-op
which can mislead callers; update the TableGANGenerator.Train(Tensor<T> input,
Tensor<T> expectedOutput) method to throw a NotSupportedException with a clear
message stating that standard Train is not supported and instructing callers to
use Fit or FitAsync instead so the unsupported training path fails fast and
documents the intended API.
- Around line 914-923: Remove the dead methods UpdateGeneratorParameters(T
learningRate) and UpdateDiscriminatorParameters(T learningRate) from
TableGANGenerator.cs: they are no longer used after the tape-based training
migration (Fit uses GradientTape, TapeStepContext and _optimizer.Step()) and
would call layer.UpdateParameters directly causing a "Backward pass must be
called before updating parameters" if invoked; delete both methods and any
references to them so parameter updates are handled only via the
tape/_optimizer.Step() workflow.
🪄 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: 8cda0fd8-db70-4474-b32e-c5da8ad7532c
📒 Files selected for processing (2)
src/Models/Options/TableGANOptions.cssrc/NeuralNetworks/SyntheticData/TableGANGenerator.cs
…comments + SetMaxGradNorm shim CodeRabbit posted a second follow-up wave. Address all 9 plus restore the buildability of the solution against the currently-published AiDotNet.Tensors 0.81.0 (which was tagged from commit 1514cbf3, before Tensors PR #359 merged — SetMaxGradNorm isn't in that release): - NeuralNetworkBase: apply CodeRabbit's suggested MaxGradNorm rename. Protected backing field is renamed back to its historical name (T MaxGradNorm, was T MaxGradNormField); the public double-typed accessor is renamed to MaxGradNormValue. Updated all internal sites that read the value as double (NeuralNetworkBase eager + fused-tape paths, GraFPrint override, ResNetNetwork/UNet3D/VoxelCNN Clone calls). The historical protected-field assign-and-read contract is preserved for external subclassers per CodeRabbit's source-compat ask. - CompiledTapeTrainingStep: SetMaxGradNorm reflection shim. AiDotNet builds against any AiDotNet.Tensors 0.8x — when the underlying assembly pre-dates the SetMaxGradNorm addition (PR #359 didn't make it into the 0.81.0 cut), we silently skip the plan-side clip and let NeuralNetworkBase.TrainWithTape's eager clip handle the bound. Once a new Tensors release ships, the lazily-cached MethodInfo picks it up automatically with no further code change. - DPCTGAN/MedSynth/TimeGAN GenerateNoiseBatchTensor: replaced Engine.TensorRandomUniformRange Box-Muller paths with the seeded _random RNG so {DPCTGAN,MedSynth,TimeGAN}Options.Seed makes the batched training path reproducible (mirrors the seeded contract that Generate(...) and the rest of the sampler stack already honour). - TimeGAN Fit: reject epochs <= 0 and SequenceLength < 2 up front instead of silently producing a no-op training run that still flips IsFitted = true. Also reject the case where every prepared sequence has fewer than 2 timesteps (no (xt, xt+1) pairs → supervisor objective is dead, not "TimeGAN" anymore). - TableGANGeneratorTests.Fit_TinyDataset_MarksGeneratorAsFitted: rewrite the training-effect assertion to do TWO Fit calls so the comparison happens after the generator-rebuild-to-transformed-width step has already stabilised the architecture. The prior (shapeChanged || l2 > 0) form silently accepted a pure rebuild with no trainable-weight update. - TableGANOptions.TrainClassifier: complete the XML doc per the golden Options pattern — add <value> and <remarks> with the For-Beginners paragraph. - TableGAN test System.Random(seed: 7) → positional new System.Random(7) so the test builds on net471 too (the named param differs across TFMs). Resolves PRRT_kwDOKSXUF86ClRSK, PRRT_kwDOKSXUF86Cl7qY, PRRT_kwDOKSXUF86Cl7qa, PRRT_kwDOKSXUF86Cl7qg, PRRT_kwDOKSXUF86Cl7qk, PRRT_kwDOKSXUF86Cl7qn, PRRT_kwDOKSXUF86Cl7qr, PRRT_kwDOKSXUF86Cl-qe. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/NeuralNetworks/SyntheticData/MedSynthGenerator.cs (1)
182-198:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftCustom decoder overrides are being re-parsed as the internal full graph.
When
Architecture.Layersis supplied,InitializeLayers()stores only the caller's decoder stack inLayers, butExtractMedSynthLayerReferences()still consumes that list as[encoder..., mean, logvar, decoder..., discriminator...]. After this change,Predict,DecoderForward, and generator training read_decoderLayersexclusively, so custom decoder layers are skipped or misordered unless they accidentally match the internal FC+BN layout. In custom-layer mode, alias_decoderLayersdirectly fromArchitecture.Layersand build the auxiliary encoder/discriminator modules separately.Also applies to: 204-240
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/NeuralNetworks/SyntheticData/MedSynthGenerator.cs` around lines 182 - 198, Custom decoder layers from Architecture.Layers are being mis-parsed into the internal full graph because InitializeLayers() copies caller-provided layers into Layers but ExtractMedSynthLayerReferences() expects the full internal layout; change InitializeLayers() so when Architecture.Layers is non-null you set _decoderLayers directly from Architecture.Layers (alias or clone as appropriate) and set _usingCustomLayers=true, and then skip or alter ExtractMedSynthLayerReferences() to only build the missing auxiliary modules (encoder, mean/logvar, discriminator) separately rather than re-parsing Layers; ensure Predict, DecoderForward, and generator training continue to read from _decoderLayers and that auxiliary module construction uses LayerHelper<T>.CreateDefaultMedSynthLayers or equivalent when _usingCustomLayers is true.
♻️ Duplicate comments (2)
src/Training/CompiledTapeTrainingStep.cs (1)
280-282:⚠️ Potential issue | 🟠 Major | ⚡ Quick winTrack
maxGradNormandlrSchedulein the fused-plan config key.These new knobs are only applied on the first
ConfigureOptimizer(...)call. Because_configuredOptimizerConfigstill ignores both values, later calls on the same compiled plan can returntruewhile reusing stale clipping/schedule semantics. Extend the cached config tuple and equality check to includemaxGradNormandlrSchedule.Also applies to: 430-472
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Training/CompiledTapeTrainingStep.cs` around lines 280 - 282, The cached optimizer-config check currently ignores the new parameters maxGradNorm and lrSchedule, so ConfigureOptimizer(...) may reuse a stale _configuredOptimizerConfig; update the cached config tuple (the _configuredOptimizerConfig field) and its equality comparison to include maxGradNorm and lrSchedule so the fused-plan config key reflects these knobs, and ensure ConfigureOptimizer(...) uses the extended tuple when deciding whether to reconfigure the optimizer (also apply the same change in the other ConfigureOptimizer overloads mentioned around the 430–472 region).src/NeuralNetworks/NeuralNetworkBase.cs (1)
5351-5369:⚠️ Potential issue | 🟠 Major | ⚡ Quick winClip network-level extra trainables too.
This block only clips
grads/trainableParams, but the extra raw tensors are still updated fromallGradslater. That breaks the new global-norm contract and leavesGetExtraTrainableTensors()unbounded.Suggested fix
double maxGradNorm = MaxGradNormValue; - if (maxGradNorm > 0.0 && grads.Count > 0) + if (maxGradNorm > 0.0) { - // Pass trainableParams as the iteration-order key so the - // total-norm sum is computed in a deterministic order - // (Dictionary iteration order is bucket-order, which uses - // the per-process-randomized identity hash for tensor - // keys → different sum-order per process → different - // clip scale per process → non-deterministic training). - ApplyGradientClipping(grads, maxGradNorm, trainableParams); + var clipOrder = extraTrainableTensors.Count == 0 + ? trainableParams + : trainableParams.Concat(extraTrainableTensors).ToList(); + var clipTargets = new Dictionary<Tensor<T>, Tensor<T>>( + Helpers.TensorReferenceComparer<Tensor<T>>.Instance); + foreach (var param in clipOrder) + { + if (allGrads.TryGetValue(param, out var grad)) + clipTargets[param] = grad; + } + if (clipTargets.Count > 0) + ApplyGradientClipping(clipTargets, maxGradNorm, clipOrder); }Also applies to: 5392-5400
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/NeuralNetworks/NeuralNetworkBase.cs` around lines 5351 - 5369, The gradient clipping currently only passes grads and trainableParams to ApplyGradientClipping, leaving extra trainables from allGrads/GetExtraTrainableTensors() unclipped; fix by collecting the extra raw tensors returned by GetExtraTrainableTensors() (and their corresponding gradients from allGrads), append them to the existing grads and trainableParams sequences (preserving deterministic ordering), and then call ApplyGradientClipping with the combined lists so the global L2 norm includes both normal and extra trainables (also apply the same change to the other clipping call that mirrors this logic).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/NeuralNetworks/NeuralNetworkBase.cs`:
- Around line 329-335: Change the MaxGradNormT accessor to return the virtual
MaxGradNormValue cast to T instead of reading the backing field directly so
subclasses that override MaxGradNormValue (and the “return 0 to disable
clipping” contract) are honored; replace the current protected T MaxGradNormT =>
MaxGradNorm; with logic that retrieves MaxGradNormValue and casts it to T
(handling the cast safely per existing generic constraints). Apply the same fix
to the other T-typed accessor helpers in this file that currently read backing
fields (the other occurrences noted in the review) so they all delegate to their
corresponding double-typed virtual properties.
In `@src/NeuralNetworks/SyntheticData/DPCTGANGenerator.cs`:
- Around line 826-866: ComputeGradientPenalty currently calls
Engine.TensorRandomUniformRange<T> (line with epsilon) which bypasses the class
seeded RNG and makes GP nondeterministic; replace that call so epsilon is
generated from the instance seeded _random (respecting DPCTGANOptions.Seed) by
creating an epsilon tensor of shape [batchSize,1] (or flat then reshape) and
filling its values using _random (e.g., sampling _random.NextDouble() per
element and converting to T via NumOps) before broadcasting—ensure you use the
same Tensor creation and Engine.TensorFill/assign approach used elsewhere in
this class so the interpolation uses the seeded RNG rather than
Engine.TensorRandomUniformRange<T>.
In `@src/NeuralNetworks/SyntheticData/MedSynthGenerator.cs`:
- Around line 315-324: The current loop in Fit() only runs discriminator updates
and then TrainGeneratorStepBatched(), which drops all VAE training and the
clinical-constraint penalty; restore a batched VAE+constraint training step that
updates _encoderLayers, _meanHead, _logvarHead and the decoder/recovery heads
(_recovery*) and includes reconstruction, KL, and clinical-constraint terms in
the loss tensor before finishing the epoch. Concretely, either modify
TrainGeneratorStepBatched() to compute and minimize the non-saturating
adversarial loss plus the VAE reconstruction loss (reconstruct from encoder
outputs), the KL loss from _meanHead/_logvarHead, and the clinical-constraint
penalty, or add a new TrainVaeConstraintStepBatched(transformedData, b, end,
noiseMultiplier) and call it in the Fit() loop (after discriminator steps and
before marking training complete); ensure gradients apply to the encoder and
recovery/decoder variable scopes and that the constraint term is combined into
the single loss used for optimizer.minimize().
---
Outside diff comments:
In `@src/NeuralNetworks/SyntheticData/MedSynthGenerator.cs`:
- Around line 182-198: Custom decoder layers from Architecture.Layers are being
mis-parsed into the internal full graph because InitializeLayers() copies
caller-provided layers into Layers but ExtractMedSynthLayerReferences() expects
the full internal layout; change InitializeLayers() so when Architecture.Layers
is non-null you set _decoderLayers directly from Architecture.Layers (alias or
clone as appropriate) and set _usingCustomLayers=true, and then skip or alter
ExtractMedSynthLayerReferences() to only build the missing auxiliary modules
(encoder, mean/logvar, discriminator) separately rather than re-parsing Layers;
ensure Predict, DecoderForward, and generator training continue to read from
_decoderLayers and that auxiliary module construction uses
LayerHelper<T>.CreateDefaultMedSynthLayers or equivalent when _usingCustomLayers
is true.
---
Duplicate comments:
In `@src/NeuralNetworks/NeuralNetworkBase.cs`:
- Around line 5351-5369: The gradient clipping currently only passes grads and
trainableParams to ApplyGradientClipping, leaving extra trainables from
allGrads/GetExtraTrainableTensors() unclipped; fix by collecting the extra raw
tensors returned by GetExtraTrainableTensors() (and their corresponding
gradients from allGrads), append them to the existing grads and trainableParams
sequences (preserving deterministic ordering), and then call
ApplyGradientClipping with the combined lists so the global L2 norm includes
both normal and extra trainables (also apply the same change to the other
clipping call that mirrors this logic).
In `@src/Training/CompiledTapeTrainingStep.cs`:
- Around line 280-282: The cached optimizer-config check currently ignores the
new parameters maxGradNorm and lrSchedule, so ConfigureOptimizer(...) may reuse
a stale _configuredOptimizerConfig; update the cached config tuple (the
_configuredOptimizerConfig field) and its equality comparison to include
maxGradNorm and lrSchedule so the fused-plan config key reflects these knobs,
and ensure ConfigureOptimizer(...) uses the extended tuple when deciding whether
to reconfigure the optimizer (also apply the same change in the other
ConfigureOptimizer overloads mentioned around the 430–472 region).
🪄 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: e7ecc06c-af72-4216-9d67-c4ea18966709
📒 Files selected for processing (11)
src/Audio/Fingerprinting/GraFPrint.cssrc/Models/Options/TableGANOptions.cssrc/NeuralNetworks/NeuralNetworkBase.cssrc/NeuralNetworks/ResNetNetwork.cssrc/NeuralNetworks/SyntheticData/DPCTGANGenerator.cssrc/NeuralNetworks/SyntheticData/MedSynthGenerator.cssrc/NeuralNetworks/SyntheticData/TimeGANGenerator.cssrc/NeuralNetworks/UNet3D.cssrc/NeuralNetworks/VoxelCNN.cssrc/Training/CompiledTapeTrainingStep.cstests/AiDotNet.Tests/ModelFamilyTests/NeuralNetworks/TableGANGeneratorTests.cs
…ps + restore MedSynth VAE step
Address 3 new CodeRabbit follow-ups:
- NeuralNetworkBase.MaxGradNormT: route through the public MaxGradNormValue
virtual instead of reading the MaxGradNorm backing field directly so any
subclass that overrides MaxGradNormValue (e.g. GraFPrint's
options-driven override + the documented "return 0 to disable clipping"
contract) flows correctly into every T-typed clipping helper that uses
MaxGradNormT (TrainWithTape's ApplyGradientClipping calls, ClipTensorGradient,
ClipVectorGradient).
- DPCTGANGenerator.ComputeGradientPenalty: replaced the unseeded
Engine.TensorRandomUniformRange interpolation epsilon with _random.NextDouble
so DPCTGANOptions.Seed makes the GP path reproducible — matches the
seeded-noise refactor for GenerateNoiseBatchTensor.
- MedSynthGenerator: restore the VAE half of the VAE+GAN hybrid training
loop. The prior Fit() only ran discriminator + non-saturating generator
updates, leaving _encoderLayers / _meanHead / _logvarHead untrained and
silently regressing the model to a plain GAN with post-hoc clamping in
Generate(). Add TrainVaeStepBatched implementing:
* encoder → (μ, log σ²) heads → reparameterize z → decoder forward → recon;
* loss = MSE(real, recon) + KLWeight·KL(N(μ, σ²) ‖ N(0, I))
+ ConstraintWeight·ConstraintPenalty(recon)
where ConstraintPenalty is the squared violation of the per-column
[colMin, colMax] bounds learned in LearnConstraints, expressed via
tape-tracked Engine.ReLU on (recon − upper) and (lower − recon);
* KL formula 0.5 · Σ(σ² + μ² − 1 − log σ²) per Kingma 2013 §3;
* Reparameterization ε sampled from _random (seeded path);
* Replay-correct ComputeForward / RecomputeLoss closures that re-derive
the full composite loss from the captured (realBatch, ε, bounds) so
optimizer.Step replays stay tied to the same objective that produced
the gradients.
Plus a batched EncoderForwardBatched helper that mirrors the per-row
EncoderForward but on [batch, dataWidth] tensors with tape-tracked
Engine.ReLU. Fit() now interleaves VAE step + DP-SGD critic steps +
non-saturating generator step per batch, matching the documented hybrid.
Resolves PRRT_kwDOKSXUF86CmJkm, PRRT_kwDOKSXUF86CmJko,
PRRT_kwDOKSXUF86CmJkp.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/NeuralNetworks/SyntheticData/DPCTGANGenerator.cs (1)
1045-1064: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueRemove dead code — these methods are no longer called.
UpdateDiscriminatorParametersDPandUpdateGeneratorParametersare unused after the refactor to tape-based training via_optimizer.Step(context). Per coding guidelines, dead code should be removed.🧹 Proposed removal
- /// <summary> - /// Updates discriminator parameters with DP noise injection. - /// </summary> - private void UpdateDiscriminatorParametersDP(T learningRate) - { - foreach (var layer in _discLayers) - { - ClipAndNoiseGradient(layer); - layer.UpdateParameters(learningRate); - } - } - - private void UpdateGeneratorParameters(T learningRate) - { - foreach (var layer in Layers) - { - layer.UpdateParameters(learningRate); - } - foreach (var bn in _genBNLayers) - { - bn.UpdateParameters(learningRate); - } - }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/NeuralNetworks/SyntheticData/DPCTGANGenerator.cs` around lines 1045 - 1064, Remove the two now-unused methods UpdateDiscriminatorParametersDP and UpdateGeneratorParameters from the DPCTGANGenerator class: they are dead after switching to tape-based training (_optimizer.Step(context)). Delete both method definitions (including any private helpers only used by them like ClipAndNoiseGradient if it's otherwise unused) and ensure there are no remaining references; rely on the tape/_optimizer.Step(context) flow instead.src/NeuralNetworks/SyntheticData/MedSynthGenerator.cs (1)
300-332:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftBlocking: Privacy budget is never tracked or enforced when
EnablePrivacy=true.Unlike
DPCTGANGenerator,MedSynthGeneratorcomputes a noise multiplier but never:
- Tracks
_cumulativeEpsilonacross training steps- Stops training when the privacy budget is exhausted
- Accounts for
DiscriminatorStepsin noise calibration (line 1063 should multiplytotalStepsby_options.DiscriminatorSteps)This means when
EnablePrivacy=true, the model adds DP noise but the claimed(ε, δ)guarantee is not enforced — training continues past the budget, invalidating the privacy bound.Per coding guidelines: "Incomplete features" and "Simplified implementations" are blocking issues.
🔐 Suggested direction
+ private double _cumulativeEpsilon; + + public double CumulativeEpsilon => _cumulativeEpsilon; public void Fit(Matrix<T> data, IReadOnlyList<ColumnMetadata> columns, int epochs) { // ... existing setup ... + _cumulativeEpsilon = 0; + bool privacyBudgetExhausted = false; - for (int epoch = 0; epoch < epochs; epoch++) + for (int epoch = 0; epoch < epochs && !privacyBudgetExhausted; epoch++) { - for (int b = 0; b < data.Rows; b += batchSize) + for (int b = 0; b < data.Rows && !privacyBudgetExhausted; b += batchSize) { // ... TrainVaeStepBatched(transformedData, b, end); for (int d = 0; d < _options.DiscriminatorSteps; d++) { TrainDiscriminatorStepBatched(transformedData, b, end, noiseMultiplier); + if (_options.EnablePrivacy) + { + _cumulativeEpsilon += ComputeStepPrivacyCost(data.Rows, end - b); + if (_cumulativeEpsilon >= _options.Epsilon) + { + privacyBudgetExhausted = true; + break; + } + } } - TrainGeneratorStepBatched(end - b); + if (!privacyBudgetExhausted) + TrainGeneratorStepBatched(end - b); } } }And fix noise calibration:
private double ComputeNoiseMultiplier(int dataSize, int epochs) { // ... - int totalSteps = epochs * (dataSize / Math.Max(batchSize, 1)); + int totalSteps = epochs * (dataSize / Math.Max(batchSize, 1)) * Math.Max(1, _options.DiscriminatorSteps);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/NeuralNetworks/SyntheticData/MedSynthGenerator.cs` around lines 300 - 332, The privacy budget isn't tracked or enforced: when _options.EnablePrivacy is true you compute a noiseMultiplier via ComputeNoiseMultiplier but never update or check _cumulativeEpsilon and you don't account for discriminator update counts when calibrating noise; fix by (1) computing total DP steps as epochs * ceil(data.Rows / batchSize) * _options.DiscriminatorSteps (i.e. include DiscriminatorSteps in noise calibration), (2) updating _cumulativeEpsilon after each DP discriminator update inside the TrainDiscriminatorStepBatched loop using the same privacy-accounting routine used in DPCTGANGenerator (or a ComputeEpsilonForStep function), and (3) breaking out of the outer training loops (stop further VAE/G/Disc updates) once _cumulativeEpsilon >= target epsilon so training halts when the budget is exhausted; reference symbols: _options.EnablePrivacy, ComputeNoiseMultiplier, _cumulativeEpsilon, _options.DiscriminatorSteps, TrainDiscriminatorStepBatched, TrainVaeStepBatched, TrainGeneratorStepBatched, epochs, batchSize, data.Rows.src/NeuralNetworks/NeuralNetworkBase.cs (2)
5751-5843:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdvance the optimizer-side scheduler after fused steps.
The fused path now accepts
lrSchedule, but the success branch never callsStepSchedulerIfSupported(resolvedOptimizer). The compiled plan may follow the schedule whileresolvedOptimizer.GetCurrentLearningRate()and scheduler state stay at step 0, which leaves optimizer state, diagnostics, and any later reset/fallback path out of sync with the actual training step.Suggested fix
if (ran) { LastLoss = lossValue; // First successful fused step commits this model to the fused // path for the rest of the training session — Adam m/v are now // inside the compiled plan and transferring them to the eager // optimizer isn't possible without API we don't have. _fusedTrainingCommitted = true; + StepSchedulerIfSupported(resolvedOptimizer); // Emit diagnostic events for the fused-path hit. This is the // ONLY place we can observe that the fused path ran without🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/NeuralNetworks/NeuralNetworkBase.cs` around lines 5751 - 5843, The fused-path success branch after Training.CompiledTapeTrainingStep<T>.TryStepWithFusedOptimizer(...) marks the fused step as ran but never advances the optimizer-side scheduler; call StepSchedulerIfSupported(resolvedOptimizer) immediately after setting _fusedTrainingCommitted = true (and before emitting diagnostics) so the optimizer's lr/scheduler state matches the compiled plan's step; ensure you reference the same resolvedOptimizer instance used to build lrSched and don't swallow exceptions from StepSchedulerIfSupported.
5549-5605:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winSplit the XML docs for these helpers.
ApplyGradientClippingwas inserted insideGetOptimizerLearningRate's<summary>, andGetOptimizerLearningRatenow starts with a dangling</summary>. That leaves malformed XML docs and misleading generated documentation.Suggested fix
- /// <summary> - /// Best-effort read of the supplied optimizer's current learning - /// rate, used by the network-level extras update path. Returns the - /// optimizer-typed value when the optimizer is a recognised - /// Applies global gradient L2-norm clipping in-place across all gradient + /// <summary> + /// Applies global gradient L2-norm clipping in-place across all gradient /// tensors in the supplied dictionary. Computes /// <c>totalNorm = sqrt(sum_i sum(grad_i²))</c>; when /// <c>totalNorm > maxNorm</c>, scales every gradient by @@ } - /// <see cref="GradientBasedOptimizerBase{T,TInput,TOutput}"/>; falls - /// back to a conservative default for optimizers that don't expose - /// the rate. + /// <summary> + /// Best-effort read of the supplied optimizer's current learning + /// rate, used by the network-level extras update path. Returns the + /// optimizer-typed value when the optimizer is a recognised + /// <see cref="GradientBasedOptimizerBase{T,TInput,TOutput}"/>; falls + /// back to a conservative default for optimizers that don't expose + /// the rate. /// </summary> private static double GetOptimizerLearningRate(🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/NeuralNetworks/NeuralNetworkBase.cs` around lines 5549 - 5605, The XML documentation for ApplyGradientClipping was accidentally embedded inside GetOptimizerLearningRate's <summary>, leaving GetOptimizerLearningRate with a dangling </summary>; split them so each method has its own proper XML doc block: move the ApplyGradientClipping summary/comments to immediately precede the ApplyGradientClipping method (ensuring it has starting <summary>...closing </summary>), and restore GetOptimizerLearningRate's own <summary> block above the GetOptimizerLearningRate method (remove any stray text that was moved into it and ensure its opening and closing summary tags are present and contain the correct description).
♻️ Duplicate comments (1)
src/NeuralNetworks/NeuralNetworkBase.cs (1)
5354-5372:⚠️ Potential issue | 🟠 Major | ⚡ Quick winClip the extra network-level trainables too.
This only clips
gradsfor layer-owned parameters.extraTrainableTensorsare still updated later fromallGrads, so raw tensors like cls/pos embeddings bypass the advertised global norm cap.Suggested fix
double maxGradNorm = MaxGradNormValue; - if (maxGradNorm > 0.0 && grads.Count > 0) + if (maxGradNorm > 0.0) { - // Pass trainableParams as the iteration-order key so the - // total-norm sum is computed in a deterministic order - // (Dictionary iteration order is bucket-order, which uses - // the per-process-randomized identity hash for tensor - // keys → different sum-order per process → different - // clip scale per process → non-deterministic training). - ApplyGradientClipping(grads, maxGradNorm, trainableParams); + var clipOrder = extraTrainableTensors.Count == 0 + ? trainableParams.ToList() + : trainableParams.Concat(extraTrainableTensors).ToList(); + var clipTargets = new Dictionary<Tensor<T>, Tensor<T>>( + Helpers.TensorReferenceComparer<Tensor<T>>.Instance); + foreach (var param in clipOrder) + { + if (allGrads.TryGetValue(param, out var grad)) + clipTargets[param] = grad; + } + if (clipTargets.Count > 0) + ApplyGradientClipping(clipTargets, maxGradNorm, clipOrder); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/NeuralNetworks/NeuralNetworkBase.cs` around lines 5354 - 5372, The global gradient-norm clipping currently only receives grads (layer-owned gradients) so extraTrainableTensors (raw tensors like cls/pos embeddings updated from allGrads) bypass MaxGradNormValue; modify the clipping call in the block that checks MaxGradNormValue to include extraTrainableTensors in the total-norm computation (e.g., build a combined gradient list from grads and the gradients corresponding to extraTrainableTensors or pass allGrads filtered/ordered to ApplyGradientClipping) and ensure you still use trainableParams as the deterministic key ordering; update any call-sites or bookkeeping around ApplyGradientClipping, grads, allGrads, and extraTrainableTensors so the same scaled values are applied to those extra tensors.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/NeuralNetworks/NeuralNetworkBase.cs`:
- Around line 5751-5843: The fused-path success branch after
Training.CompiledTapeTrainingStep<T>.TryStepWithFusedOptimizer(...) marks the
fused step as ran but never advances the optimizer-side scheduler; call
StepSchedulerIfSupported(resolvedOptimizer) immediately after setting
_fusedTrainingCommitted = true (and before emitting diagnostics) so the
optimizer's lr/scheduler state matches the compiled plan's step; ensure you
reference the same resolvedOptimizer instance used to build lrSched and don't
swallow exceptions from StepSchedulerIfSupported.
- Around line 5549-5605: The XML documentation for ApplyGradientClipping was
accidentally embedded inside GetOptimizerLearningRate's <summary>, leaving
GetOptimizerLearningRate with a dangling </summary>; split them so each method
has its own proper XML doc block: move the ApplyGradientClipping
summary/comments to immediately precede the ApplyGradientClipping method
(ensuring it has starting <summary>...closing </summary>), and restore
GetOptimizerLearningRate's own <summary> block above the
GetOptimizerLearningRate method (remove any stray text that was moved into it
and ensure its opening and closing summary tags are present and contain the
correct description).
In `@src/NeuralNetworks/SyntheticData/DPCTGANGenerator.cs`:
- Around line 1045-1064: Remove the two now-unused methods
UpdateDiscriminatorParametersDP and UpdateGeneratorParameters from the
DPCTGANGenerator class: they are dead after switching to tape-based training
(_optimizer.Step(context)). Delete both method definitions (including any
private helpers only used by them like ClipAndNoiseGradient if it's otherwise
unused) and ensure there are no remaining references; rely on the
tape/_optimizer.Step(context) flow instead.
In `@src/NeuralNetworks/SyntheticData/MedSynthGenerator.cs`:
- Around line 300-332: The privacy budget isn't tracked or enforced: when
_options.EnablePrivacy is true you compute a noiseMultiplier via
ComputeNoiseMultiplier but never update or check _cumulativeEpsilon and you
don't account for discriminator update counts when calibrating noise; fix by (1)
computing total DP steps as epochs * ceil(data.Rows / batchSize) *
_options.DiscriminatorSteps (i.e. include DiscriminatorSteps in noise
calibration), (2) updating _cumulativeEpsilon after each DP discriminator update
inside the TrainDiscriminatorStepBatched loop using the same privacy-accounting
routine used in DPCTGANGenerator (or a ComputeEpsilonForStep function), and (3)
breaking out of the outer training loops (stop further VAE/G/Disc updates) once
_cumulativeEpsilon >= target epsilon so training halts when the budget is
exhausted; reference symbols: _options.EnablePrivacy, ComputeNoiseMultiplier,
_cumulativeEpsilon, _options.DiscriminatorSteps, TrainDiscriminatorStepBatched,
TrainVaeStepBatched, TrainGeneratorStepBatched, epochs, batchSize, data.Rows.
---
Duplicate comments:
In `@src/NeuralNetworks/NeuralNetworkBase.cs`:
- Around line 5354-5372: The global gradient-norm clipping currently only
receives grads (layer-owned gradients) so extraTrainableTensors (raw tensors
like cls/pos embeddings updated from allGrads) bypass MaxGradNormValue; modify
the clipping call in the block that checks MaxGradNormValue to include
extraTrainableTensors in the total-norm computation (e.g., build a combined
gradient list from grads and the gradients corresponding to
extraTrainableTensors or pass allGrads filtered/ordered to
ApplyGradientClipping) and ensure you still use trainableParams as the
deterministic key ordering; update any call-sites or bookkeeping around
ApplyGradientClipping, grads, allGrads, and extraTrainableTensors so the same
scaled values are applied to those extra tensors.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6e46935a-9d68-4572-acae-8836b78f26b7
📒 Files selected for processing (3)
src/NeuralNetworks/NeuralNetworkBase.cssrc/NeuralNetworks/SyntheticData/DPCTGANGenerator.cssrc/NeuralNetworks/SyntheticData/MedSynthGenerator.cs
3 tasks
ooples
pushed a commit
that referenced
this pull request
Jun 8, 2026
…endly) Flip cancel-in-progress=true for master push events too (not just PRs). Combined with the per-ref (no SHA) group key, this means each new master commit cancels the prior in-flight master Build & SonarCloud run. Reflects how releases actually work: a release is several PR merges to master in rapid succession; only the latest tip's green signal matters. The intermediate SHAs are never deployed to. Letting the prior 45-minute run finish only to be superseded wastes the slot. The previous attempt's reverted-from PR #1318 motivation (no green signal) is most likely PR #1318 checking an INTERMEDIATE SHA's status — those do get lost under cancel-in-progress=true. The LATEST master tip's signal is always preserved (the freshly-triggered run runs to completion), which is what branch protection / PR mergeability / release tagging consume.
ooples
added a commit
that referenced
this pull request
Jun 8, 2026
… runs on merge (#1547) * ci: serialize master Build & SonarCloud runs to stop FIFO starvation of PR runs With a 49-shard test matrix and the GitHub free-tier 20-concurrent-job cap, every dependabot/master commit was spawning its own per-SHA concurrency group (group includes both ref AND sha). The runs piled up FIFO-style — on 2026-06-08 we observed 11 queued master Build & SonarCloud runs holding ~700 matrix jobs in the queue ahead of 4 PR runs, with the PR runs unable to dispatch a single matrix job for 3+ hours. Drop the per-SHA suffix so all master pushes share one group key. With cancel-in-progress=false the running master run keeps going and posts its signal; new master pushes wait in the single "pending" slot. GitHub keeps only the most-recently-queued pending run, so a dependabot flood of N back-to-back commits validates the first + the last instead of all N. The previous pattern (1906d05) used cancel-in-progress=TRUE with the ref-only group, which DID cancel in-flight master runs and dropped their signal — that's the problem the per-SHA grouping was reverting away from. Setting cancel-in-progress=FALSE keeps the in-flight run alive, so we get the back-pressure without the dropped-signal problem. * ci: cancel old master runs when newer commits land (release-merge friendly) Flip cancel-in-progress=true for master push events too (not just PRs). Combined with the per-ref (no SHA) group key, this means each new master commit cancels the prior in-flight master Build & SonarCloud run. Reflects how releases actually work: a release is several PR merges to master in rapid succession; only the latest tip's green signal matters. The intermediate SHAs are never deployed to. Letting the prior 45-minute run finish only to be superseded wastes the slot. The previous attempt's reverted-from PR #1318 motivation (no green signal) is most likely PR #1318 checking an INTERMEDIATE SHA's status — those do get lost under cancel-in-progress=true. The LATEST master tip's signal is always preserved (the freshly-triggered run runs to completion), which is what branch protection / PR mergeability / release tagging consume. * ci: add cancel-on-pr-close — kill orphaned PR runs after merge After a PR merges, the last pull_request:synchronize run on the PR's HEAD keeps executing for ~45min on a SHA that's now dead (auto-delete-branch typically removes the PR branch on merge). Burns a CI slot that other PR runs could use. Add a small workflow that listens for pull_request.closed and cancels any in_progress or queued runs whose head_sha matches the closed PR's head_sha. The head_sha check is defense-in-depth — guards against false-positives if the same branch name is reused for a different PR later. --------- Co-authored-by: franklinic <franklin@ivorycloud.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
.gitignore.Closes #1310
Verification
dotnet build tests\AiDotNet.Tests\AiDotNetTests.csproj --no-restore --framework net10.0 -v:quiet -clp:ErrorsOnly -m:1 /nodeReuse:falsedotnet test tests\AiDotNet.Tests\AiDotNetTests.csproj --no-restore --no-build --framework net10.0 --filter FullyQualifiedName=AiDotNet.Tests.IntegrationTests.SyntheticData.SyntheticTabularGeneratorIntegrationTests.DPCTGANGenerator_FitAndGenerate_ProducesValidOutput|FullyQualifiedName=AiDotNet.Tests.IntegrationTests.SyntheticData.SyntheticTabularGeneratorIntegrationTests.TableGANGenerator_FitAndGenerate_ProducesValidOutput|FullyQualifiedName=AiDotNet.Tests.IntegrationTests.SyntheticData.SyntheticTabularGeneratorIntegrationTests.TableGANGenerator_ClassificationTargets_UseTransformedLabelSlice|FullyQualifiedName=AiDotNet.Tests.IntegrationTests.SyntheticData.SyntheticTabularGeneratorIntegrationTests.CausalGANGenerator_FitAndGenerate_ProducesValidOutput --nologo -v:minimaldotnet build tests\AiDotNet.Tests\AiDotNetTests.csproj --no-restore --framework net471 -v:quiet -clp:ErrorsOnly -m:1 /nodeReuse:falsedotnet test tests\AiDotNet.Tests\AiDotNetTests.csproj --no-restore --no-build --framework net471 --filter FullyQualifiedName=AiDotNet.Tests.IntegrationTests.SyntheticData.SyntheticTabularGeneratorIntegrationTests.DPCTGANGenerator_FitAndGenerate_ProducesValidOutput|FullyQualifiedName=AiDotNet.Tests.IntegrationTests.SyntheticData.SyntheticTabularGeneratorIntegrationTests.TableGANGenerator_FitAndGenerate_ProducesValidOutput|FullyQualifiedName=AiDotNet.Tests.IntegrationTests.SyntheticData.SyntheticTabularGeneratorIntegrationTests.TableGANGenerator_ClassificationTargets_UseTransformedLabelSlice|FullyQualifiedName=AiDotNet.Tests.IntegrationTests.SyntheticData.SyntheticTabularGeneratorIntegrationTests.CausalGANGenerator_FitAndGenerate_ProducesValidOutput --nologo -v:minimaldotnet build AiDotNet.sln --no-restore -v:quiet -clp:ErrorsOnly -m:1 /nodeReuse:falseSummary by CodeRabbit
New Features
Bug Fixes
Tests
Chores