fix: fix issue 418 and update info - #431
Conversation
|
Warning Rate limit exceeded@ooples has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 15 minutes and 36 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (12)
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds a full uncertainty quantification module: Bayesian neural networks, MC dropout, deep ensembles, calibration (temperature/Platt/isotonic), conformal prediction, Bayesian layers/interfaces, result types/options, builder wiring, dataset-level uncertainty stats, and broad RandomHelper refactor across many files. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Result as PredictionModelResult
participant Normalizer
participant Orchestrator as UQ Orchestrator
participant MC as MCDropoutLayer/Estimator
participant BE as BayesianNN/Estimator
participant DE as DeepEnsemble
participant Conformal as ConformalPredictor
Client->>Result: PredictWithUncertainty(input, numSamples?)
Result->>Normalizer: Normalize(input)
Normalizer-->>Result: normalizedInput
alt Method = MonteCarloDropout
Result->>MC: enable MC mode
loop samples
Result->>MC: Predict(normalizedInput)
end
MC-->>Result: predictions[]
else Method = BayesianNeuralNetwork
Result->>BE: Sample weights and Predict multiple times
BE-->>Result: predictions[]
else Method = DeepEnsemble
Result->>DE: Collect predictions from ensemble members
DE-->>Result: predictions[]
else Method = ConformalPrediction
Result->>Conformal: Compute threshold from calibration
Conformal-->>Result: prediction set / interval
end
Result->>Result: Aggregate mean & variance, compute metrics (entropy, MI)
Result->>Normalizer: Denormalize(mean, variance)
Normalizer-->>Result: denormalized outputs
Result-->>Client: UncertaintyPredictionResult(prediction, variance, metrics, conformal)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60–75 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull Request Overview
This pull request implements a comprehensive Uncertainty Quantification module for AiDotNet, addressing Issue #418. The implementation includes Bayesian neural networks (Monte Carlo Dropout, Variational Inference, Deep Ensembles), calibration methods (Temperature Scaling, Expected Calibration Error), and conformal prediction techniques.
Key changes:
- Bayesian neural network implementations with uncertainty estimation capabilities
- Post-training calibration methods for improving probability estimates
- Conformal prediction for guaranteed coverage intervals
Reviewed Changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 16 comments.
Show a summary per file
| File | Description |
|---|---|
| MCDropoutLayer.cs | Implements dropout layer that stays active during inference for uncertainty estimation |
| BayesianDenseLayer.cs | Implements fully-connected layer with weight distributions for variational inference |
| IUncertaintyEstimator.cs | Interface contract for models providing uncertainty estimates |
| IBayesianLayer.cs | Interface for Bayesian layers supporting weight sampling and KL divergence |
| SplitConformalPredictor.cs | Regression conformal prediction with guaranteed coverage intervals |
| ConformalClassifier.cs | Classification conformal prediction returning prediction sets |
| TemperatureScaling.cs | Post-training calibration using temperature parameter on logits |
| ExpectedCalibrationError.cs | Metric for evaluating probability calibration quality |
| MCDropoutNeuralNetwork.cs | Neural network wrapper using MC Dropout for uncertainty |
| DeepEnsemble.cs | Ensemble-based uncertainty estimation using multiple models |
| BayesianNeuralNetwork.cs | Full Bayesian neural network with weight distributions |
| TemperatureScalingTests.cs | Unit tests for temperature scaling calibration |
| MCDropoutLayerTests.cs | Unit tests for Monte Carlo dropout layer |
| ExpectedCalibrationErrorTests.cs | Unit tests for ECE metric computation |
| README.md | Comprehensive documentation with examples and references |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
src/UncertaintyQuantification/BayesianNeuralNetworks/BayesianNeuralNetwork.cs(1 hunks)src/UncertaintyQuantification/BayesianNeuralNetworks/DeepEnsemble.cs(1 hunks)src/UncertaintyQuantification/BayesianNeuralNetworks/MCDropoutNeuralNetwork.cs(1 hunks)src/UncertaintyQuantification/Calibration/ExpectedCalibrationError.cs(1 hunks)src/UncertaintyQuantification/Calibration/TemperatureScaling.cs(1 hunks)src/UncertaintyQuantification/ConformalPrediction/ConformalClassifier.cs(1 hunks)src/UncertaintyQuantification/ConformalPrediction/SplitConformalPredictor.cs(1 hunks)src/UncertaintyQuantification/Interfaces/IBayesianLayer.cs(1 hunks)src/UncertaintyQuantification/Interfaces/IUncertaintyEstimator.cs(1 hunks)src/UncertaintyQuantification/Layers/BayesianDenseLayer.cs(1 hunks)src/UncertaintyQuantification/Layers/MCDropoutLayer.cs(1 hunks)src/UncertaintyQuantification/README.md(1 hunks)tests/AiDotNet.Tests/UnitTests/UncertaintyQuantification/ExpectedCalibrationErrorTests.cs(1 hunks)tests/AiDotNet.Tests/UnitTests/UncertaintyQuantification/MCDropoutLayerTests.cs(1 hunks)tests/AiDotNet.Tests/UnitTests/UncertaintyQuantification/TemperatureScalingTests.cs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (14)
tests/AiDotNet.Tests/UnitTests/UncertaintyQuantification/ExpectedCalibrationErrorTests.cs (1)
src/UncertaintyQuantification/Calibration/ExpectedCalibrationError.cs (2)
ExpectedCalibrationError(28-158)ExpectedCalibrationError(41-48)
tests/AiDotNet.Tests/UnitTests/UncertaintyQuantification/MCDropoutLayerTests.cs (1)
src/UncertaintyQuantification/Layers/MCDropoutLayer.cs (2)
MCDropoutLayer(25-159)MCDropoutLayer(57-66)
src/UncertaintyQuantification/ConformalPrediction/ConformalClassifier.cs (2)
src/UncertaintyQuantification/ConformalPrediction/SplitConformalPredictor.cs (3)
T(125-145)T(153-167)Calibrate(77-100)src/Helpers/MathHelper.cs (2)
INumericOperations(33-61)MathHelper(16-987)
src/UncertaintyQuantification/Interfaces/IUncertaintyEstimator.cs (4)
src/UncertaintyQuantification/BayesianNeuralNetworks/BayesianNeuralNetwork.cs (6)
T(224-237)Tensor(55-80)Tensor(91-122)Tensor(134-163)Tensor(168-189)Tensor(194-214)src/UncertaintyQuantification/Interfaces/IBayesianLayer.cs (1)
T(42-42)src/UncertaintyQuantification/BayesianNeuralNetworks/DeepEnsemble.cs (5)
Tensor(72-87)Tensor(99-114)Tensor(126-140)Tensor(164-185)Tensor(190-210)src/UncertaintyQuantification/BayesianNeuralNetworks/MCDropoutNeuralNetwork.cs (5)
Tensor(52-79)Tensor(91-106)Tensor(117-130)Tensor(149-170)Tensor(175-195)
src/UncertaintyQuantification/Calibration/TemperatureScaling.cs (1)
src/Helpers/MathHelper.cs (2)
INumericOperations(33-61)MathHelper(16-987)
src/UncertaintyQuantification/Calibration/ExpectedCalibrationError.cs (1)
src/Helpers/MathHelper.cs (2)
INumericOperations(33-61)MathHelper(16-987)
src/UncertaintyQuantification/BayesianNeuralNetworks/MCDropoutNeuralNetwork.cs (3)
src/UncertaintyQuantification/BayesianNeuralNetworks/BayesianNeuralNetwork.cs (6)
T(224-237)Tensor(55-80)Tensor(91-122)Tensor(134-163)Tensor(168-189)Tensor(194-214)src/UncertaintyQuantification/Interfaces/IUncertaintyEstimator.cs (3)
Tensor(31-31)Tensor(43-43)Tensor(55-55)src/UncertaintyQuantification/Layers/MCDropoutLayer.cs (2)
MCDropoutLayer(25-159)MCDropoutLayer(57-66)
src/UncertaintyQuantification/BayesianNeuralNetworks/BayesianNeuralNetwork.cs (4)
src/UncertaintyQuantification/Interfaces/IBayesianLayer.cs (2)
T(42-42)SampleWeights(29-29)src/UncertaintyQuantification/Layers/BayesianDenseLayer.cs (4)
T(154-206)Tensor(211-237)Tensor(242-298)SampleWeights(119-143)src/UncertaintyQuantification/BayesianNeuralNetworks/DeepEnsemble.cs (6)
Tensor(72-87)Tensor(99-114)Tensor(126-140)Tensor(164-185)Tensor(190-210)List(151-159)src/UncertaintyQuantification/Interfaces/IUncertaintyEstimator.cs (3)
Tensor(31-31)Tensor(43-43)Tensor(55-55)
src/UncertaintyQuantification/ConformalPrediction/SplitConformalPredictor.cs (2)
src/UncertaintyQuantification/ConformalPrediction/ConformalClassifier.cs (2)
T(166-178)Calibrate(68-96)src/Helpers/MathHelper.cs (2)
INumericOperations(33-61)MathHelper(16-987)
src/UncertaintyQuantification/Interfaces/IBayesianLayer.cs (2)
src/UncertaintyQuantification/BayesianNeuralNetworks/BayesianNeuralNetwork.cs (1)
T(224-237)src/UncertaintyQuantification/Layers/BayesianDenseLayer.cs (2)
T(154-206)SampleWeights(119-143)
src/UncertaintyQuantification/Layers/BayesianDenseLayer.cs (2)
src/UncertaintyQuantification/BayesianNeuralNetworks/BayesianNeuralNetwork.cs (6)
T(224-237)Tensor(55-80)Tensor(91-122)Tensor(134-163)Tensor(168-189)Tensor(194-214)src/UncertaintyQuantification/Interfaces/IBayesianLayer.cs (2)
T(42-42)SampleWeights(29-29)
src/UncertaintyQuantification/BayesianNeuralNetworks/DeepEnsemble.cs (4)
src/UncertaintyQuantification/BayesianNeuralNetworks/BayesianNeuralNetwork.cs (6)
T(224-237)Tensor(55-80)Tensor(91-122)Tensor(134-163)Tensor(168-189)Tensor(194-214)src/Helpers/MathHelper.cs (2)
INumericOperations(33-61)MathHelper(16-987)src/UncertaintyQuantification/BayesianNeuralNetworks/MCDropoutNeuralNetwork.cs (5)
Tensor(52-79)Tensor(91-106)Tensor(117-130)Tensor(149-170)Tensor(175-195)src/UncertaintyQuantification/Interfaces/IUncertaintyEstimator.cs (3)
Tensor(31-31)Tensor(43-43)Tensor(55-55)
tests/AiDotNet.Tests/UnitTests/UncertaintyQuantification/TemperatureScalingTests.cs (1)
src/UncertaintyQuantification/Calibration/TemperatureScaling.cs (5)
TemperatureScaling(29-197)TemperatureScaling(56-60)Tensor(71-79)Vector(169-196)Calibrate(101-121)
src/UncertaintyQuantification/Layers/MCDropoutLayer.cs (1)
src/UncertaintyQuantification/BayesianNeuralNetworks/MCDropoutNeuralNetwork.cs (5)
Tensor(52-79)Tensor(91-106)Tensor(117-130)Tensor(149-170)Tensor(175-195)
🪛 GitHub Actions: Build
src/UncertaintyQuantification/BayesianNeuralNetworks/BayesianNeuralNetwork.cs
[error] 23-23: CS0534: 'BayesianNeuralNetwork' does not implement inherited abstract member 'NeuralNetworkBase.Predict(Tensor)'
🪛 GitHub Actions: Quality Gates (.NET)
src/UncertaintyQuantification/BayesianNeuralNetworks/BayesianNeuralNetwork.cs
[error] 23-23: CS0534: 'BayesianNeuralNetwork' does not implement inherited abstract member 'NeuralNetworkBase.Train(Tensor, Tensor)'
🪛 GitHub Check: Build All Frameworks
src/UncertaintyQuantification/BayesianNeuralNetworks/MCDropoutNeuralNetwork.cs
[failure] 24-24:
'MCDropoutNeuralNetwork' does not implement inherited abstract member 'NeuralNetworkBase.UpdateParameters(Vector)'
[failure] 24-24:
'MCDropoutNeuralNetwork' does not implement inherited abstract member 'NeuralNetworkBase.CreateNewInstance()'
src/UncertaintyQuantification/BayesianNeuralNetworks/BayesianNeuralNetwork.cs
[failure] 23-23:
'BayesianNeuralNetwork' does not implement inherited abstract member 'NeuralNetworkBase.InitializeLayers()'
[failure] 23-23:
'BayesianNeuralNetwork' does not implement inherited abstract member 'NeuralNetworkBase.Train(Tensor, Tensor)'
[failure] 23-23:
'BayesianNeuralNetwork' does not implement inherited abstract member 'NeuralNetworkBase.SerializeNetworkSpecificData(BinaryWriter)'
[failure] 23-23:
'BayesianNeuralNetwork' does not implement inherited abstract member 'NeuralNetworkBase.GetModelMetadata()'
[failure] 23-23:
'BayesianNeuralNetwork' does not implement inherited abstract member 'NeuralNetworkBase.DeserializeNetworkSpecificData(BinaryReader)'
[failure] 23-23:
'BayesianNeuralNetwork' does not implement inherited abstract member 'NeuralNetworkBase.UpdateParameters(Vector)'
[failure] 23-23:
'BayesianNeuralNetwork' does not implement inherited abstract member 'NeuralNetworkBase.CreateNewInstance()'
[failure] 23-23:
'BayesianNeuralNetwork' does not implement inherited abstract member 'NeuralNetworkBase.Predict(Tensor)'
🪛 GitHub Check: Publish Size Analysis
src/UncertaintyQuantification/BayesianNeuralNetworks/MCDropoutNeuralNetwork.cs
[failure] 24-24:
'MCDropoutNeuralNetwork' does not implement inherited abstract member 'NeuralNetworkBase.GetModelMetadata()'
[failure] 24-24:
'MCDropoutNeuralNetwork' does not implement inherited abstract member 'NeuralNetworkBase.Train(Tensor, Tensor)'
src/UncertaintyQuantification/BayesianNeuralNetworks/BayesianNeuralNetwork.cs
[failure] 23-23:
'BayesianNeuralNetwork' does not implement inherited abstract member 'NeuralNetworkBase.UpdateParameters(Vector)'
[failure] 23-23:
'BayesianNeuralNetwork' does not implement inherited abstract member 'NeuralNetworkBase.Predict(Tensor)'
[failure] 23-23:
'BayesianNeuralNetwork' does not implement inherited abstract member 'NeuralNetworkBase.InitializeLayers()'
[failure] 23-23:
'BayesianNeuralNetwork' does not implement inherited abstract member 'NeuralNetworkBase.SerializeNetworkSpecificData(BinaryWriter)'
[failure] 23-23:
'BayesianNeuralNetwork' does not implement inherited abstract member 'NeuralNetworkBase.CreateNewInstance()'
[failure] 23-23:
'BayesianNeuralNetwork' does not implement inherited abstract member 'NeuralNetworkBase.DeserializeNetworkSpecificData(BinaryReader)'
[failure] 23-23:
'BayesianNeuralNetwork' does not implement inherited abstract member 'NeuralNetworkBase.GetModelMetadata()'
[failure] 23-23:
'BayesianNeuralNetwork' does not implement inherited abstract member 'NeuralNetworkBase.Train(Tensor, Tensor)'
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: CodeQL analysis (csharp)
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (5)
tests/AiDotNet.Tests/UnitTests/UncertaintyQuantification/TemperatureScalingTests.cs (1)
101-127: LGTM! Consider adding input validation test.The test validates that Calibrate successfully updates temperature while maintaining positivity. However, the Calibrate method's input validation (requiring
logits.Rows == labels.Length) lacks test coverage.Consider adding a test case for mismatched input dimensions:
[Fact] public void Calibrate_WithMismatchedLengths_ThrowsException() { // Arrange var tempScaling = new TemperatureScaling<double>(); var logits = new Matrix<double>(5, 3); // 5 samples var labels = new Vector<int>(new int[] { 0, 1, 2 }); // 3 labels // Act & Assert Assert.Throws<ArgumentException>(() => tempScaling.Calibrate(logits, labels)); }src/UncertaintyQuantification/ConformalPrediction/ConformalClassifier.cs (1)
44-50: Consider validating numClasses parameter.The constructor accepts
numClassesbut doesn't validate it's positive. While the calling code should ensure this, defensive validation would prevent subtle bugs if a caller passes 0 or negative values.Apply this diff:
public ConformalClassifier(INeuralNetwork<T> model, int numClasses) { _model = model ?? throw new ArgumentNullException(nameof(model)); + if (numClasses <= 0) + throw new ArgumentOutOfRangeException(nameof(numClasses), "Number of classes must be positive."); _numClasses = numClasses;src/UncertaintyQuantification/Layers/MCDropoutLayer.cs (1)
112-128: Consider reordering checks for clearer semantics.If
Forwardwas called in inference mode (no dropout applied),_dropoutMaskwill benull. The current code throws before checking the mode. While this is technically safe (Backward shouldn't be called in pure inference), reordering improves clarity:public override Tensor<T> Backward(Tensor<T> outputGradient) { - if (_lastInput == null || _dropoutMask == null) - throw new InvalidOperationException("Forward pass must be called before backward pass."); - if (!IsTrainingMode && !_mcMode) return outputGradient; + if (_lastInput == null || _dropoutMask == null) + throw new InvalidOperationException("Forward pass must be called before backward pass."); + var inputGradient = new Tensor<T>(_lastInput.Shape);src/UncertaintyQuantification/BayesianNeuralNetworks/BayesianNeuralNetwork.cs (1)
90-156: Avoid redundant sampling loops.Both
EstimateAleatoricUncertaintyandEstimateEpistemicUncertaintyduplicate the full sampling loop. Consider reusingPredictWithUncertainty:public Tensor<T> EstimateAleatoricUncertainty(Tensor<T> input) { - // For simplicity, we estimate aleatoric uncertainty as the average of individual prediction variances - var predictions = new List<Tensor<T>>(); - - for (int i = 0; i < _numSamples; i++) - { - foreach (var bayesianLayer in Layers.OfType<IBayesianLayer<T>>()) - { - bayesianLayer.SampleWeights(); - } - - predictions.Add(Predict(input)); - } - - var mean = ComputeMean(predictions); - var variance = ComputeVariance(predictions, mean); + var (_, variance) = PredictWithUncertainty(input); // Aleatoric is approximated as a portion of total varianceThe same applies to
EstimateEpistemicUncertainty. This reduces code duplication and ensures consistent sampling behavior.src/UncertaintyQuantification/BayesianNeuralNetworks/MCDropoutNeuralNetwork.cs (1)
148-194: Consider extracting shared utility methods.
ComputeMeanandComputeVarianceare duplicated betweenMCDropoutNeuralNetworkandBayesianNeuralNetwork. Extract these to a shared utility class or base class to reduce code duplication.For example, create a static helper:
internal static class UncertaintyUtils<T> { public static Tensor<T> ComputeMean(List<Tensor<T>> predictions, INumericOperations<T> numOps) { ... } public static Tensor<T> ComputeVariance(List<Tensor<T>> predictions, Tensor<T> mean, INumericOperations<T> numOps) { ... } }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
src/LinearAlgebra/VectorBase.cs(5 hunks)src/Optimizers/CMAESOptimizer.cs(3 hunks)src/UncertaintyQuantification/BayesianNeuralNetworks/BayesianNeuralNetwork.cs(1 hunks)src/UncertaintyQuantification/BayesianNeuralNetworks/MCDropoutNeuralNetwork.cs(1 hunks)src/UncertaintyQuantification/Calibration/ExpectedCalibrationError.cs(1 hunks)src/UncertaintyQuantification/Calibration/TemperatureScaling.cs(1 hunks)src/UncertaintyQuantification/ConformalPrediction/ConformalClassifier.cs(1 hunks)src/UncertaintyQuantification/ConformalPrediction/SplitConformalPredictor.cs(1 hunks)src/UncertaintyQuantification/Layers/BayesianDenseLayer.cs(1 hunks)src/UncertaintyQuantification/Layers/MCDropoutLayer.cs(1 hunks)tests/AiDotNet.Tests/UnitTests/UncertaintyQuantification/ExpectedCalibrationErrorTests.cs(1 hunks)tests/AiDotNet.Tests/UnitTests/UncertaintyQuantification/MCDropoutLayerTests.cs(1 hunks)tests/AiDotNet.Tests/UnitTests/UncertaintyQuantification/TemperatureScalingTests.cs(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- tests/AiDotNet.Tests/UnitTests/UncertaintyQuantification/MCDropoutLayerTests.cs
🧰 Additional context used
🧬 Code graph analysis (7)
tests/AiDotNet.Tests/UnitTests/UncertaintyQuantification/ExpectedCalibrationErrorTests.cs (1)
src/UncertaintyQuantification/Calibration/ExpectedCalibrationError.cs (2)
ExpectedCalibrationError(30-154)ExpectedCalibrationError(43-50)
src/UncertaintyQuantification/BayesianNeuralNetworks/BayesianNeuralNetwork.cs (3)
src/UncertaintyQuantification/Layers/BayesianDenseLayer.cs (4)
T(156-208)Tensor(213-239)Tensor(244-300)SampleWeights(123-145)src/UncertaintyQuantification/Interfaces/IBayesianLayer.cs (2)
T(42-42)SampleWeights(29-29)src/UncertaintyQuantification/Interfaces/IUncertaintyEstimator.cs (3)
Tensor(31-31)Tensor(43-43)Tensor(55-55)
src/UncertaintyQuantification/ConformalPrediction/ConformalClassifier.cs (1)
src/UncertaintyQuantification/ConformalPrediction/SplitConformalPredictor.cs (3)
T(133-153)T(161-175)Calibrate(81-108)
tests/AiDotNet.Tests/UnitTests/UncertaintyQuantification/TemperatureScalingTests.cs (1)
src/UncertaintyQuantification/Calibration/TemperatureScaling.cs (5)
TemperatureScaling(29-200)TemperatureScaling(56-63)Tensor(74-82)Vector(172-199)Calibrate(104-124)
src/UncertaintyQuantification/Calibration/ExpectedCalibrationError.cs (1)
src/Helpers/MathHelper.cs (2)
INumericOperations(33-61)MathHelper(16-987)
src/UncertaintyQuantification/Layers/BayesianDenseLayer.cs (2)
src/UncertaintyQuantification/BayesianNeuralNetworks/BayesianNeuralNetwork.cs (1)
T(217-227)src/UncertaintyQuantification/Interfaces/IBayesianLayer.cs (2)
T(42-42)SampleWeights(29-29)
src/UncertaintyQuantification/Layers/MCDropoutLayer.cs (3)
src/UncertaintyQuantification/BayesianNeuralNetworks/BayesianNeuralNetwork.cs (6)
T(217-227)Tensor(57-79)Tensor(90-118)Tensor(130-156)Tensor(161-182)Tensor(187-207)src/UncertaintyQuantification/Layers/BayesianDenseLayer.cs (4)
T(156-208)Tensor(213-239)Tensor(244-300)Vector(343-368)src/UncertaintyQuantification/BayesianNeuralNetworks/MCDropoutNeuralNetwork.cs (5)
Tensor(54-81)Tensor(93-108)Tensor(119-132)Tensor(148-169)Tensor(174-194)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build All Frameworks
🔇 Additional comments (50)
src/LinearAlgebra/VectorBase.cs (4)
32-32: Documentation correctly updated.The exception documentation now accurately reflects the new validation behavior that allows zero-length vectors.
322-322: Documentation example improved.The L2 norm example now displays proper squared notation (3², 4²), making the mathematical formula clearer for users.
370-370: Documentation example enhanced.The Transform overload example now uses the proper multiplication symbol (×), making the index-based transformation clearer.
40-41: Allow zero-length vectors – fixes broken Empty() method and enables critical functionality.This change is essential. The old validation prevented
Vector<T>.Empty()(line 153) from working, and the codebase has 23+ direct uses ofnew Vector<T>(0)across TimeSeries models, UncertaintyQuantification layers, and other components that initialize empty vectors for lazy loading. No existing code relies on the ArgumentException being thrown for zero-length vectors – all ArgumentException catches in the codebase target different validation issues (length mismatch, deserialization errors). The relaxed validation unblocks intended functionality.[no_further_verification_needed]
src/Optimizers/CMAESOptimizer.cs (1)
2-2: LGTM!The addition of
using System.Linqis required for the LINQ-based sorting implementation used in theUpdateDistributionmethod.tests/AiDotNet.Tests/UnitTests/UncertaintyQuantification/ExpectedCalibrationErrorTests.cs (4)
10-41: LGTM!The constructor tests comprehensively validate default initialization, custom bin counts, and edge cases (zero/negative bins). The validation tests correctly expect
ArgumentExceptionfor invalid inputs.
43-77: LGTM!The Compute tests effectively validate ECE behavior for both perfectly calibrated data (ECE ≈ 0) and overconfident predictions (ECE > 0.5). The test data is well-chosen to isolate calibration quality.
79-90: LGTM!The test properly validates that mismatched input lengths trigger an
ArgumentException, ensuring robust input validation.
92-115: LGTM!The reliability diagram test validates output structure and value ranges (confidence/accuracy in [0,1], positive counts). The test data spans multiple bins, ensuring binning logic is exercised.
tests/AiDotNet.Tests/UnitTests/UncertaintyQuantification/TemperatureScalingTests.cs (3)
10-27: LGTM!The constructor tests validate both default initialization (T=1.0) and custom temperature values, confirming the Temperature property is correctly set.
29-47: LGTM!The temperature validation tests ensure the Temperature property correctly rejects non-positive values with
ArgumentException, enforcing the critical constraint that temperature must be positive to avoid division-by-zero or sign-flipped scaling.
49-99: LGTM!The ScaleLogits tests comprehensively validate temperature scaling behavior across three scenarios (T=1, T=2, T=0.5), confirming correct element-wise division by temperature.
src/UncertaintyQuantification/Calibration/TemperatureScaling.cs (5)
56-63: LGTM!The constructor correctly validates that
initialTemperature > 0before assignment, preventing divide-by-zero inScaleLogits. The use of theTemperatureproperty ensures consistent validation. This addresses the past review concern about non-positive initial temperatures.
41-50: LGTM!The Temperature property correctly enforces positivity constraints, throwing
ArgumentExceptionwhen attempting to set non-positive values.
74-82: LGTM!The ScaleLogits method correctly performs element-wise division by temperature. Division-by-zero is prevented by the Temperature property's positivity constraint.
104-124: LGTM!The Calibrate method properly validates input dimensions, performs gradient descent optimization, and clamps temperature to a minimum of 0.01 to prevent numerical instability.
129-199: LGTM!The ComputeGradient method correctly computes the gradient of negative log-likelihood with respect to temperature using the standard formula. The Softmax helper employs the max-subtraction technique for numerical stability, preventing overflow in exponential calculations.
src/UncertaintyQuantification/Calibration/ExpectedCalibrationError.cs (5)
43-50: LGTM!The constructor correctly validates that
numBins >= 1with an appropriate error message, ensuring valid bin configuration for ECE computation.
67-95: LGTM!The Compute method correctly implements the ECE formula: weighted average of absolute differences between bin accuracy and confidence. Input validation and explicit
Wherefiltering (addressing past feedback) ensure robust computation.
109-124: LGTM!The GetReliabilityDiagram method correctly generates per-bin calibration data for visualization. The explicit
Wherefiltering (line 115) addresses past feedback. TheConvert.ToDoubleconversion (line 117) is appropriate for display purposes despite potential precision loss.
129-153: LGTM!The CreateBins method correctly assigns predictions to bins by confidence level. Edge case handling (lines 144-145) prevents out-of-bounds access for probabilities of exactly 0.0 or 1.0.
159-181: LGTM!The CalibrationBin class provides a clean abstraction for per-bin statistics, correctly tracking confidence sum, sample count, and correct prediction count.
src/UncertaintyQuantification/ConformalPrediction/SplitConformalPredictor.cs (4)
38-58: LGTM! Clean initialization.The constructor properly validates the model parameter and initializes all fields correctly. The use of nullable
_calibrationScoresclearly indicates the uncalibrated state.
161-175: LGTM! Correct quantile computation.The adjusted quantile formula using
ceil((n+1)*alpha)/nis correct for split conformal prediction theory and ensures proper coverage guarantees. The bounds checking is appropriate.
189-211: LGTM! Correct coverage evaluation.The empirical coverage computation correctly validates each prediction interval and uses proper numeric operations for comparisons.
223-241: LGTM! Correct interval width computation.The average interval width calculation properly computes widths using numeric operations and converts to double for accurate averaging.
src/UncertaintyQuantification/ConformalPrediction/ConformalClassifier.cs (3)
168-180: LGTM! Correct threshold computation for classification.The threshold computation correctly uses
1.0 - confidenceLevelandFloorto select a conservative quantile, ensuring proper coverage guarantees for classification. The use ofFloor(vsCeilingin the regression predictor) is intentional and appropriate for classification conformal prediction.
193-213: LGTM! Correct coverage evaluation.The empirical coverage computation correctly validates whether each test label is contained in the corresponding prediction set.
227-243: LGTM! Correct average set size computation.The average set size calculation correctly sums prediction set sizes and computes the mean.
src/UncertaintyQuantification/Layers/MCDropoutLayer.cs (4)
1-6: LGTM!Imports and namespace declaration are appropriate for the layer's functionality.
29-72: LGTM!Constructor properly validates dropout rate, initializes the RNG with optional seed (addressing the past review concern), and correctly computes the inverted dropout scale factor.
79-105: LGTM!Forward pass correctly implements inverted dropout: stores input, applies mask when in training or MC mode, and scales kept activations by
1/(1-dropoutRate)to maintain expected values during inference.
130-165: LGTM!Parameter management and state reset methods are correctly implemented for a non-trainable dropout layer.
src/UncertaintyQuantification/Layers/BayesianDenseLayer.cs (9)
1-6: LGTM!Imports and namespace are appropriate for the Bayesian layer implementation.
27-79: LGTM!Class fields and constructor properly implement thread-safe RNG (addressing past review concerns), store variational parameters (mean and log-variance for weights and biases), and initialize everything correctly.
81-118: LGTM!Parameter initialization follows best practices: Xavier initialization for weight means, small initial variance (log-var = -5), and zero-initialized biases. Thread-safe RNG usage via
NextGaussian.
120-145: LGTM!Weight sampling correctly implements the reparameterization trick (
w = μ + σ * ε) using the thread-safe class-level RNG, enabling gradient flow through the stochastic sampling process.
156-208: LGTM!KL divergence computation correctly implements the closed-form formula for KL between two Gaussians. The formula
KL = 0.5 * (σ²/σ_p² + μ²/σ_p² - 1 - log(σ²/σ_p²))is correctly decomposed and computed for all weights and biases.
213-239: LGTM!Forward pass correctly stores input for backprop, lazily samples weights, and computes the linear transformation using sampled parameters.
305-338: LGTM!Parameter updates follow standard gradient descent and properly clear gradients after the update (addressing past review concerns).
343-398: LGTM!Parameter packing/unpacking maintains consistent ordering and validates expected parameter count.
403-453: LGTM!State management, thread-safe Gaussian sampling, and gradient clearing are correctly implemented. The Box-Muller transform in
RandomExtensionsis a standard implementation for Gaussian sampling.src/UncertaintyQuantification/BayesianNeuralNetworks/BayesianNeuralNetwork.cs (4)
1-46: LGTM!Class declaration, imports, and constructor are well-structured. The inheritance from
NeuralNetwork<T>(addressing past review concerns) provides the required base implementations, and parameter validation is appropriate.
57-79: LGTM!
PredictWithUncertaintycorrectly samples weights from Bayesian layers before each forward pass, collects predictions, and computes mean and variance for uncertainty estimation.
161-207: LGTM!Mean and variance computations are correctly implemented. Population variance (dividing by n) is used, which is appropriate given the large default sample count (30).
217-227: LGTM!KL divergence aggregation correctly sums contributions from all Bayesian layers using explicit type filtering.
src/UncertaintyQuantification/BayesianNeuralNetworks/MCDropoutNeuralNetwork.cs (4)
1-47: LGTM!Class declaration, imports, and constructor are well-structured. The inheritance from
NeuralNetwork<T>(addressing past review concerns) and parameter validation are appropriate.
54-81: LGTM!
PredictWithUncertaintycorrectly manages MC mode with try-finally for exception safety, collects stochastic predictions, and computes mean/variance.
93-132: LGTM!Aleatoric and epistemic uncertainty estimation correctly reuses
PredictWithUncertainty(unlike the duplicate loops inBayesianNeuralNetwork). The 80/20 split acknowledges that MC Dropout primarily captures epistemic uncertainty.
137-143: LGTM!
EnableMCModecorrectly uses explicit type filtering viaOfType<MCDropoutLayer<T>>()to toggle MC mode on relevant layers.
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 (2)
src/Models/Results/PredictionModelResult.cs (2)
1317-1325: Missing UncertaintyQuantificationOptions in deserialization.The
Deserializemethod copies properties from the deserialized object but omitsUncertaintyQuantificationOptions. This breaks the serialization round-trip—a model saved with UQ enabled will lose that configuration when loaded.if (deserializedObject != null) { Model = deserializedObject.Model; OptimizationResult = deserializedObject.OptimizationResult; NormalizationInfo = deserializedObject.NormalizationInfo; ModelMetaData = deserializedObject.ModelMetaData; BiasDetector = deserializedObject.BiasDetector; FairnessEvaluator = deserializedObject.FairnessEvaluator; + UncertaintyQuantificationOptions = deserializedObject.UncertaintyQuantificationOptions; }
1103-1111: UncertaintyQuantificationOptions not preserved in WithParameters.When creating a new instance via
WithParameters, theUncertaintyQuantificationOptionsis not passed to the constructor, causing it to be lost. The same issue affectsDeepCopyat lines 1198-1207.return new PredictionModelResult<T, TInput, TOutput>( updatedOptimizationResult, NormalizationInfo, BiasDetector, FairnessEvaluator, RagRetriever, RagReranker, RagGenerator, - QueryProcessors); + QueryProcessors, + loraConfiguration: LoRAConfiguration, + crossValidationResult: CrossValidationResult, + uncertaintyQuantificationOptions: UncertaintyQuantificationOptions);Apply the same fix to
DeepCopyat line 1198.
🧹 Nitpick comments (8)
src/UncertaintyQuantification/ConformalPrediction/ConformalClassifier.cs (2)
172-184: Consider aligning quantile computation with coverage guarantees.The threshold computation uses a simpler formula than
SplitConformalPredictor.ComputeQuantile, which applies the adjusted quantileceil((n+1)*alpha)/nper conformal prediction theory to ensure finite-sample coverage guarantees.For consistency and stronger theoretical guarantees, consider using the same adjusted formula:
private T ComputeThreshold(double confidenceLevel) { var n = _calibrationScores!.Length; - // Compute quantile index (using floor for conservative estimates) - var quantileLevel = 1.0 - confidenceLevel; - var index = (int)Math.Floor(n * quantileLevel); + // Adjusted quantile level to ensure coverage guarantee + // Using floor((n+1)*(1-alpha))/n as per conformal prediction theory + var quantileLevel = 1.0 - confidenceLevel; + var adjustedLevel = Math.Floor((n + 1) * quantileLevel) / n; + if (adjustedLevel < 0) adjustedLevel = 0; + + var index = (int)Math.Floor(n * adjustedLevel); if (index < 0) index = 0; if (index >= n) index = n - 1; return _calibrationScores[index]; }
197-217: Guard against empty test sets.Both
EvaluateCoverageandComputeAverageSetSizewill divide by zero iftestInputs.Rows == 0. Consider adding validation:public double EvaluateCoverage(Matrix<T> testInputs, Vector<int> testLabels, double confidenceLevel = 0.9) { if (!_isCalibrated) throw new InvalidOperationException("Must calibrate classifier before evaluation"); + if (testInputs.Rows == 0) + throw new ArgumentException("Test set must contain at least one sample.", nameof(testInputs)); + int coveredCount = 0;public double ComputeAverageSetSize(Matrix<T> testInputs, double confidenceLevel = 0.9) { if (!_isCalibrated) throw new InvalidOperationException("Must calibrate classifier before computing set size"); + if (testInputs.Rows == 0) + throw new ArgumentException("Test set must contain at least one sample.", nameof(testInputs)); + int totalSize = 0;Also applies to: 231-247
src/UncertaintyQuantification/ConformalPrediction/SplitConformalPredictor.cs (1)
193-215: Guard against empty test sets.Similar to
ConformalClassifier, both methods will divide by zero iftestInputs.Rows == 0:public double EvaluateCoverage(Matrix<T> testInputs, Vector<T> testTargets, double confidenceLevel = 0.9) { if (!_isCalibrated) throw new InvalidOperationException("Must calibrate predictor before evaluation"); + if (testInputs.Rows == 0) + throw new ArgumentException("Test set must contain at least one sample.", nameof(testInputs)); + int coveredCount = 0;public double ComputeAverageIntervalWidth(Matrix<T> testInputs, double confidenceLevel = 0.9) { if (!_isCalibrated) throw new InvalidOperationException("Must calibrate predictor before computing interval width"); + if (testInputs.Rows == 0) + throw new ArgumentException("Test set must contain at least one sample.", nameof(testInputs)); + double totalWidth = 0.0;Also applies to: 227-245
tests/AiDotNet.Tests/IntegrationTests/UncertaintyQuantificationFacadeTests.cs (1)
30-98: Good integration coverage; consider strengthening variance assertions.The test validates the full UQ workflow including layer injection, stochastic predictions, and shape consistency.
One observation: Line 97 uses
Assert.Contains(variance, v => v > 0.0)which passes if any element is positive. For stronger validation of variance computation, consider asserting that all variance values are non-negative (variance should never be negative mathematically):-Assert.Contains(variance, v => v > 0.0); +Assert.All(variance, v => Assert.True(v >= 0.0, "Variance must be non-negative")); +Assert.Contains(variance, v => v > 0.0); // At least some positive variance expectedsrc/PredictionModelBuilder.cs (2)
365-386: Collection modification during iteration may cause layer skipping.The loop iterates over
layers.CountwhileInsertLayerIntoCollectionmodifies the collection. Althoughi++at line 384 compensates, this pattern can be error-prone and the semantics ofInsertLayerIntoCollectionaren't fully visible here.Consider iterating in reverse order or building a list of insertion points first, then applying them:
-for (int i = 0; i < layers.Count - 1; i++) -{ - if (layers[i] is not ActivationLayer<T>) - { - continue; - } - - if (i >= layers.Count - 2) - { - continue; - } - - if (layers[i + 1] is DropoutLayer<T> || layers[i + 1] is MCDropoutLayer<T>) - { - continue; - } - - int? seed = options.RandomSeed.HasValue ? options.RandomSeed.Value + i : (int?)null; - neuralNetworkModel.Network.InsertLayerIntoCollection(i + 1, new MCDropoutLayer<T>(options.MonteCarloDropoutRate, mcMode: false, randomSeed: seed)); - i++; -} +var insertionPoints = new List<int>(); +for (int i = 0; i < layers.Count - 2; i++) +{ + if (layers[i] is ActivationLayer<T> && + layers[i + 1] is not DropoutLayer<T> && + layers[i + 1] is not MCDropoutLayer<T>) + { + insertionPoints.Add(i + 1); + } +} + +// Insert in reverse order to preserve indices +for (int j = insertionPoints.Count - 1; j >= 0; j--) +{ + int insertIdx = insertionPoints[j]; + int? seed = options.RandomSeed.HasValue ? options.RandomSeed.Value + j : (int?)null; + neuralNetworkModel.Network.InsertLayerIntoCollection(insertIdx, new MCDropoutLayer<T>(options.MonteCarloDropoutRate, mcMode: false, randomSeed: seed)); +}This avoids index shifting concerns and makes the intent clearer.
360-363: Consider using a dedicated property-specific exception message.The
nameof(options)refers to the method parameter, but the actual issue is withoptions.MonteCarloDropoutRate. Consider clarifying:-throw new ArgumentException("MonteCarloDropoutRate must be between 0 and 1.", nameof(options)); +throw new ArgumentOutOfRangeException( + nameof(options.MonteCarloDropoutRate), + options.MonteCarloDropoutRate, + "MonteCarloDropoutRate must be between 0 (exclusive) and 1 (exclusive).");src/UncertaintyQuantification/Layers/BayesianDenseLayer.cs (1)
69-79: Add input validation for constructor parameters.The constructor accepts
inputSizeandoutputSizewithout validating they are positive. Negative or zero values would cause issues in matrix/vector allocation and loop logic.public BayesianDenseLayer(int inputSize, int outputSize, double priorSigma = 1.0, int? randomSeed = null) : base([inputSize], [outputSize]) { + if (inputSize <= 0) + throw new ArgumentOutOfRangeException(nameof(inputSize), "Input size must be positive."); + if (outputSize <= 0) + throw new ArgumentOutOfRangeException(nameof(outputSize), "Output size must be positive."); + if (priorSigma <= 0) + throw new ArgumentOutOfRangeException(nameof(priorSigma), "Prior sigma must be positive."); + _inputSize = inputSize; _outputSize = outputSize;src/Models/Results/PredictionModelResult.cs (1)
682-685: Consider using sample variance for unbiased estimation.The variance computation divides by
n(population variance) rather thann-1(sample variance), which introduces slight downward bias. For typical MC dropout sample sizes (10-100), this is negligible, but using Bessel's correction would provide an unbiased estimate.+var countMinusOne = numOps.FromDouble(Math.Max(1, samples.Count - 1)); for (int i = 0; i < length; i++) { - varianceVector[i] = numOps.Divide(varianceVector[i], count); + varianceVector[i] = numOps.Divide(varianceVector[i], countMinusOne); }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
src/DataProcessor/DefaultDataPreprocessor.cs(2 hunks)src/Enums/UncertaintyQuantificationMethod.cs(1 hunks)src/Interfaces/IPredictionModelBuilder.cs(2 hunks)src/LinearAlgebra/Tensor.cs(24 hunks)src/Models/Options/UncertaintyQuantificationOptions.cs(1 hunks)src/Models/Results/PredictionModelResult.cs(6 hunks)src/NeuralNetworks/Layers/LayerBase.cs(2 hunks)src/NeuralNetworks/NeuralNetworkBase.cs(3 hunks)src/Optimizers/CMAESOptimizer.cs(3 hunks)src/PredictionModelBuilder.cs(5 hunks)src/Statistics/ErrorStats.cs(2 hunks)src/Statistics/PredictionStats.cs(5 hunks)src/UncertaintyQuantification/ConformalPrediction/ConformalClassifier.cs(1 hunks)src/UncertaintyQuantification/ConformalPrediction/SplitConformalPredictor.cs(1 hunks)src/UncertaintyQuantification/Layers/BayesianDenseLayer.cs(1 hunks)src/UncertaintyQuantification/Layers/MCDropoutLayer.cs(1 hunks)src/UncertaintyQuantification/README.md(1 hunks)tests/AiDotNet.Tests/IntegrationTests/UncertaintyQuantificationFacadeTests.cs(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/NeuralNetworks/Layers/LayerBase.cs
🚧 Files skipped from review as they are similar to previous changes (3)
- src/UncertaintyQuantification/README.md
- src/UncertaintyQuantification/Layers/MCDropoutLayer.cs
- src/Optimizers/CMAESOptimizer.cs
🧰 Additional context used
🧬 Code graph analysis (4)
src/NeuralNetworks/NeuralNetworkBase.cs (1)
src/NeuralNetworks/Layers/LayerBase.cs (2)
T(1098-1109)SetTrainingMode(472-478)
src/PredictionModelBuilder.cs (1)
src/Models/Options/UncertaintyQuantificationOptions.cs (1)
UncertaintyQuantificationOptions(17-61)
src/UncertaintyQuantification/ConformalPrediction/ConformalClassifier.cs (1)
src/UncertaintyQuantification/ConformalPrediction/SplitConformalPredictor.cs (2)
T(135-157)T(165-179)
src/Interfaces/IPredictionModelBuilder.cs (2)
src/PredictionModelBuilder.cs (1)
TOutput(402-405)src/Models/Options/UncertaintyQuantificationOptions.cs (1)
UncertaintyQuantificationOptions(17-61)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build All Frameworks
🔇 Additional comments (28)
src/DataProcessor/DefaultDataPreprocessor.cs (1)
94-94: LGTM! Normalizer reference now accessible for downstream operations.The assignment of the Normalizer into the normInfo object enables downstream components to access the normalizer for denormalization and calibration operations, which aligns well with the uncertainty quantification features being added in this PR. Downstream code in PredictionModelResult correctly uses this reference with appropriate null checks before calling NormalizeInput() and Denormalize() methods.
src/LinearAlgebra/Tensor.cs (2)
359-374: LGTM! 2D tensor-vector addition with broadcasting.The implementation correctly adds a vector to each row of a 2D tensor by broadcasting along the last dimension. The dimension check at line 361 ensures the vector length matches the tensor's last dimension.
981-1002: LGTM! 2D tensor-matrix multiplication.The implementation correctly performs matrix multiplication for 2D tensors with proper dimension validation and result shape calculation.
src/UncertaintyQuantification/ConformalPrediction/ConformalClassifier.cs (3)
44-50: LGTM! Constructor with proper validation.The constructor correctly validates the model parameter, initializes numeric operations, and sets the calibration flag to false.
68-100: LGTM! Calibration with comprehensive validation.The method properly validates inputs, handles empty calibration sets, validates model outputs and label ranges, and correctly computes and sorts conformity scores.
124-167: LGTM! Prediction set construction with proper safeguards.The method correctly validates state and inputs, builds the prediction set based on threshold comparison, and ensures a non-empty result by including the highest-probability class as a fallback.
src/UncertaintyQuantification/ConformalPrediction/SplitConformalPredictor.cs (4)
53-58: LGTM! Constructor with proper validation.The constructor correctly validates the model parameter and initializes the numeric operations.
81-110: LGTM! Calibration with addressed review concerns.The method properly validates inputs, handles empty calibration sets, validates scalar model output with descriptive error message, and correctly persists sorted calibration scores.
135-157: LGTM! Prediction with interval construction.The method correctly validates state and inputs, validates scalar model output, and constructs symmetric prediction intervals using the calibration quantile.
165-179: LGTM! Proper conformal quantile computation.The implementation correctly applies the adjusted quantile formula
ceil((n+1)*alpha)/nper conformal prediction theory to ensure finite-sample coverage guarantees, with proper bounds protection.src/Statistics/ErrorStats.cs (1)
426-429: Defensive guard mirrors PredictionStats pattern.The early-exit guard is correctly implemented and consistent with the same pattern in
PredictionStats.cs. All properties are properly initialized before the potential early return. Length mismatch validation betweenActualandPredictedarrays is enforced in the called StatisticsHelper methods:Vector.Subtract()validates inCalculateMedianAbsoluteError,CalculateMaxError, andCalculateResiduals, while methods likeCalculateMeanAbsolutePercentageError,CalculatePrecisionRecallAUC, andCalculateSymmetricMeanAbsolutePercentageErrorinclude explicit length checks.src/NeuralNetworks/NeuralNetworkBase.cs (3)
42-49: LGTM - Good encapsulation for internal integrations.The internal read-only accessor properly exposes the layer collection without allowing mutation from external integrations. This supports the uncertainty quantification features (e.g., MC Dropout, Bayesian layers) added in this PR while maintaining encapsulation.
51-61: Implementation correct and verified in uncertainty quantification context.The method is used properly in
TryInjectMonteCarloDropoutLayersto inject MC Dropout layers after activation layers. The index management is sound: insertions occur ati + 1with subsequenti++to skip the newly inserted layer, and the loop's live collection reference adapts as the count grows. Cache invalidation is maintained correctly.
823-831: Design supports MC Dropout by decoupling training mode from training capability.The unconditional setting of
IsTrainingModeat the network level allowsIsTrainingModeto betrueeven whenSupportsTrainingisfalse. This is intentional and safe:
- Each layer guards its own
IsTrainingModeupdate with aSupportsTrainingcheck (LayerBase line 474-476), preventing layers that don't support training from being put into training mode.- Critical methods (
Backpropagateline 355-363,ForwardWithMemoryline 410-412) validateSupportsTrainingindependently, preventing actual training operations on non-trainable networks.IsTrainingModeis used for behavioral control (dropout application, batch normalization statistics) within layers, not for capability validation.This design correctly enables MC Dropout and Bayesian inference patterns, where dropout must remain active during inference for uncertainty estimation. No code changes needed.
src/Enums/UncertaintyQuantificationMethod.cs (1)
1-25: LGTM!The enum is clean, well-documented, and appropriately scoped for the current implementation. The
Autofallback mechanism and extensibility for future UQ methods (Deep Ensembles, Variational Inference, etc. from Issue #418) are well-supported by this design.src/Interfaces/IPredictionModelBuilder.cs (1)
356-369: LGTM!The new interface method follows the established fluent builder pattern, with consistent documentation style and appropriate parameter typing.
tests/AiDotNet.Tests/IntegrationTests/UncertaintyQuantificationFacadeTests.cs (2)
17-28: LGTM!Good unit test validating that MC Dropout produces stochastic outputs when
mcMode: true. The fixed seed ensures reproducibility while the subtraction check verifies output variability.
100-149: Passthrough optimizer is appropriate for test isolation.The helper correctly bypasses training to focus on UQ layer injection and prediction behavior. The
NotSupportedExceptionfor file operations and empty serialization are acceptable for this test-only utility.src/PredictionModelBuilder.cs (3)
12-14: LGTM!Appropriate using directives added for the new UQ functionality.
322-348: LGTM!The method cleanly handles the Auto → MonteCarloDropout fallback and appropriately restricts UQ to neural network models with a clear error message.
531-540: LGTM!Clean implementation with null validation and fluent return pattern.
src/Models/Options/UncertaintyQuantificationOptions.cs (1)
1-62: LGTM!Well-structured options class with sensible defaults:
NumSamples = 30balances stability and latencyMonteCarloDropoutRate = 0.1is conservative for injected layersDenormalizeUncertainty = trueprovides user-friendly output by defaultThe validation for
NumSamples >= 1is appropriately handled at the call site (PredictWithUncertainty), andMonteCarloDropoutRatebounds are validated inTryInjectMonteCarloDropoutLayers.src/UncertaintyQuantification/Layers/BayesianDenseLayer.cs (3)
156-208: KL divergence implementation is correct.The formula correctly implements the analytical KL divergence between the variational posterior N(μ, σ²) and the isotropic Gaussian prior N(0, σ_prior²). The computation properly handles log-space variance representation.
261-286: Log-variance gradient computation is now correct.The gradient w.r.t. log-variance correctly uses
weightStd(= σ = exp(log_var/2)) rather than exp(log_var). This properly implements the chain rule through the reparameterization trick: ∂w/∂(log_var) = 0.5 · σ · ε.
447-454: Box-Muller implementation is correct.Using
1.0 - random.NextDouble()properly handles the edge case whereNextDouble()could return 0, which would causeMath.Log(0)to return negative infinity.src/Models/Results/PredictionModelResult.cs (3)
591-617: Good use of try/finally for MC mode cleanup.The implementation correctly uses try/finally to ensure Monte Carlo mode is disabled even if an exception occurs during sampling. The graceful fallback when no MC dropout layers are found is also appropriate.
721-747: Type conversion logic is well-structured.The method handles the three expected output types cleanly with appropriate error messaging for unsupported types.
795-797: Verify GlobalContrast scale factor corresponds to normalization implementation.The scale factor
2 * StdDevfor GlobalContrast denormalization is questionable. Standard global contrast normalization divides by standard deviation, so the inverse should multiply by standard deviation alone (factor of 1). Verify this factor of 2 is intentional and matches the corresponding normalization code.
There was a problem hiding this comment.
Actionable comments posted: 10
♻️ Duplicate comments (5)
src/UncertaintyQuantification/BayesianNeuralNetworks/MCDropoutNeuralNetwork.cs (2)
204-219: Duplicated CreateDefaultMetrics logic.This method is also duplicated across
DeepEnsemble.cs,MCDropoutNeuralNetwork.cs, and appears in other UQ files. The batch inference logic (lines 206-211) is repeated verbatim. Consolidate into a shared helper to ensure consistent metric initialization across all UQ estimators.
101-116: Same arbitrary aleatoric/epistemic decomposition as DeepEnsemble.MC Dropout captures epistemic uncertainty through stochastic forward passes. The 20/80 split is marginally better justified than DeepEnsemble's 30/70 (since MC Dropout is more clearly epistemic), but it's still a heuristic placeholder. For true aleatoric uncertainty, you'd need the network to model output variance explicitly. Consider documenting this limitation prominently or deferring aleatoric estimation until proper variance modeling is added.
src/Models/Results/PredictionModelResult.cs (2)
1267-1329: ComputeMeanAndVariance is duplicated across multiple files.This method is nearly identical to the versions in
DeepEnsemble.csandMCDropoutNeuralNetwork.cs. All three compute element-wise mean and variance over a list of tensors. As noted in the MCDropoutNeuralNetwork review, consolidate these into a sharedUncertaintyStatisticsHelper<T>to eliminate duplication.
605-627: FitTemperatureFromProbabilities in PredictionModelBuilder.cs duplicates conversion logic here.The probability-to-logit conversion (lines 616-622 in PredictionModelBuilder.cs) is repeated in
ApplyTemperatureScalingToProbabilityTensorhere (lines 1142-1146). Both clamp probabilities toeps, computelog(p), and divide by temperature. Extract a sharedProbabilitiesToLogitshelper to avoid duplication and ensure consistent epsilon values.src/UncertaintyQuantification/BayesianNeuralNetworks/DeepEnsemble.cs (1)
134-148: Same concern: epistemic split is arbitrary without proper decomposition.Similarly, the 70% epistemic factor is a placeholder. In proper ensemble uncertainty quantification, epistemic uncertainty is the variance of the ensemble means, not a fixed fraction of total variance. If you want to provide both types, you need the variance within each model (aleatoric) and variance between models (epistemic). Without individual model variance outputs, returning only total variance would be more honest.
🧹 Nitpick comments (14)
src/Statistics/ErrorStats.cs (1)
426-429: Good defensive guard for empty inputs.The early return prevents calculations on empty data and ensures all metrics remain at their zero-initialized defaults, which aligns well with the
Empty()factory method pattern.Consider adding a length mismatch validation to catch mismatched input arrays early:
🔎 Optional: Add length mismatch validation
if (inputs.Actual.Length == 0 || inputs.Predicted.Length == 0) { return; } + +if (inputs.Actual.Length != inputs.Predicted.Length) +{ + throw new ArgumentException( + $"Actual and Predicted arrays must have the same length. " + + $"Actual: {inputs.Actual.Length}, Predicted: {inputs.Predicted.Length}"); +}src/NeuralNetworks/Layers/ActivationLayer.cs (1)
8-8: Minor: Encoding artifact in documentation.The em-dash character appears as a replacement character (
�). Consider using a standard hyphen or ensuring UTF-8 encoding is preserved.-/// Without non-linearity, a neural network—no matter how many layers—would behave just like a single layer, +/// Without non-linearity, a neural network - no matter how many layers - would behave just like a single layer,src/Models/Results/UncertaintyCalibrationArtifacts.cs (1)
12-19: Consider documenting the access contract for value properties.The
default!initialization suppresses nullability warnings, but consumers should only access values (e.g.,ConformalRegressionQuantile) when the corresponding flag (HasConformalRegression) is true. Since this is internal, usage is controlled, but a brief inline comment could prevent future misuse.+ // Only access when HasConformalRegression is true internal T ConformalRegressionQuantile { get; set; } = default!;src/Models/UncertaintyStats.cs (1)
20-24: Consider pre-populating known metric keys for API consistency.The documentation states "Keys are stable so consumers do not have to branch on missing keys." However,
Empty()returns a dictionary with no keys. Consumers calling code likestats.UncertaintyStats.Metrics["predictive_entropy"]on an empty stats object would still getKeyNotFoundException.This is a minor inconsistency between the documented contract and the implementation—consider documenting that empty stats have no keys, or pre-populating known keys with default values.
src/Models/Results/RegressionConformalInterval.cs (1)
30-34: Consider null validation for consistency.The constructor doesn't validate
lowerandupperfor null, whileClassificationConformalPredictionSetvalidates its constructor parameter. For consistency and to prevent invalid state whenTOutputis a reference type, consider adding null checks.🔎 Proposed null validation
public RegressionConformalInterval(TOutput lower, TOutput upper) { - Lower = lower; - Upper = upper; + Lower = lower ?? throw new ArgumentNullException(nameof(lower)); + Upper = upper ?? throw new ArgumentNullException(nameof(upper)); }Note: If
TOutputis constrained to non-nullable types in practice, this may be unnecessary.src/Helpers/LayerHelper.cs (1)
1992-2063: Consider supporting regression task type with appropriate activation.The method always uses
SoftmaxActivationfor the output layer, which is suitable for classification but not for regression tasks. Other factory methods in this file (e.g.,CreateDefaultGNNLayersat lines 1297-1310) checkarchitecture.TaskTypeto select the appropriate activation. Consider adding similar task-type handling for consistency.🔎 Proposed enhancement for task-type-aware activation
- if (hiddenLayerSizes.Count > 0) - { - int lastHiddenLayerSize = hiddenLayerSizes[hiddenLayerSizes.Count - 1]; - yield return new AiDotNet.UncertaintyQuantification.Layers.BayesianDenseLayer<T>(lastHiddenLayerSize, outputSize, new SoftmaxActivation<T>() as IActivationFunction<T>); - } - else - { - yield return new AiDotNet.UncertaintyQuantification.Layers.BayesianDenseLayer<T>(inputSize, outputSize, new SoftmaxActivation<T>() as IActivationFunction<T>); - } - - yield return new ActivationLayer<T>(new[] { outputSize }, new SoftmaxActivation<T>() as IActivationFunction<T>); + IActivationFunction<T> outputActivation = architecture.TaskType switch + { + NeuralNetworkTaskType.Regression => new IdentityActivation<T>(), + NeuralNetworkTaskType.BinaryClassification => new SigmoidActivation<T>(), + _ => new SoftmaxActivation<T>() + }; + + if (hiddenLayerSizes.Count > 0) + { + int lastHiddenLayerSize = hiddenLayerSizes[hiddenLayerSizes.Count - 1]; + yield return new AiDotNet.UncertaintyQuantification.Layers.BayesianDenseLayer<T>(lastHiddenLayerSize, outputSize, outputActivation); + } + else + { + yield return new AiDotNet.UncertaintyQuantification.Layers.BayesianDenseLayer<T>(inputSize, outputSize, outputActivation); + } + + yield return new ActivationLayer<T>(new[] { outputSize }, outputActivation);Note: The same issue exists in
CreateDefaultNeuralNetworkLayers(lines 1980-1989) and could be addressed in a follow-up.src/Models/Results/UncertaintyPredictionResult.cs (1)
19-72: Well-designed immutable result container.The class follows good practices for a result type: immutable properties, clear documentation, and appropriate null validation for
metrics. The generic design withTfor numeric type andTOutputfor prediction output provides flexibility across different UQ implementations.One consideration:
Predictionis not validated for null. IfTOutputis a reference type, a null prediction could slip through. Depending on the contract, you may want to add validation:Optional: Add null validation for Prediction
public UncertaintyPredictionResult( UncertaintyQuantificationMethod methodUsed, TOutput prediction, TOutput? variance, IReadOnlyDictionary<string, Tensor<T>> metrics, RegressionConformalInterval<TOutput>? regressionInterval = null, ClassificationConformalPredictionSet? classificationSet = null) { MethodUsed = methodUsed; - Prediction = prediction; + Prediction = prediction ?? throw new ArgumentNullException(nameof(prediction)); Variance = variance; Metrics = metrics ?? throw new ArgumentNullException(nameof(metrics)); RegressionInterval = regressionInterval; ClassificationSet = classificationSet; }src/Models/Options/UncertaintyQuantificationOptions.cs (1)
17-94: Consider adding validation for configuration properties.The defaults are well-chosen and align with industry standards. However, several properties could benefit from validation to prevent invalid configurations at runtime:
Property Valid Range Risk if Invalid NumSamples≥ 1 Division by zero or empty aggregation MonteCarloDropoutRate[0, 1) Invalid dropout behavior ConformalConfidenceLevel(0, 1) Invalid coverage guarantees DeepEnsembleSize≥ 1 Empty ensemble Since this is an MVP, you could defer validation to the consuming code (builder/strategy), but documenting valid ranges in the XML comments would help users avoid mistakes.
Optional: Add property validation
+ private int _numSamples = 30; + private double _monteCarloDropoutRate = 0.1; + private double _conformalConfidenceLevel = 0.9; + private int _deepEnsembleSize = 5; - public int NumSamples { get; set; } = 30; + public int NumSamples + { + get => _numSamples; + set => _numSamples = value >= 1 ? value : throw new ArgumentOutOfRangeException(nameof(value), "NumSamples must be at least 1"); + }src/UncertaintyQuantification/BayesianNeuralNetworks/DeepEnsemble.cs (1)
220-236: Verify batch dimension inference logic in CreateDefaultMetrics.Line 223 computes
batchwith a complex formula involving rank, length, and last dimension. For rank-1 tensors, it defaults to 1 (correct). For higher ranks, it divides total length by the last dimension. This assumes the prediction is shaped as[batch, features]or similar. However, ifprediction.Shape[prediction.Shape.Length - 1]is zero or negative,Math.Max(1, ...)prevents division by zero but the logic may still be incorrect. Consider adding validation or comments explaining the expected tensor layout.src/UncertaintyQuantification/BayesianNeuralNetworks/MCDropoutNeuralNetwork.cs (1)
156-202: Duplicated mean/variance computation across DeepEnsemble and MCDropoutNeuralNetwork.
ComputeMeanandComputeVariancein this file are nearly identical to those inDeepEnsemble.cs. Both iterate overList<Tensor<T>>predictions and compute element-wise statistics. Extract these into a shared utility class (e.g.,UncertaintyStatisticsHelper<T>) to eliminate duplication and ensure consistency.🔧 Proposed refactor
Create
src/UncertaintyQuantification/Helpers/UncertaintyStatisticsHelper.cs:public static class UncertaintyStatisticsHelper<T> { public static Tensor<T> ComputeMean(List<Tensor<T>> predictions) { if (predictions.Count == 0) throw new ArgumentException("Cannot compute mean of empty prediction list"); var numOps = MathHelper.GetNumericOperations<T>(); var sum = new Tensor<T>(predictions[0].Shape); foreach (var pred in predictions) { for (int i = 0; i < pred.Length; i++) { sum[i] = numOps.Add(sum[i], pred[i]); } } var count = numOps.FromDouble(predictions.Count); for (int i = 0; i < sum.Length; i++) { sum[i] = numOps.Divide(sum[i], count); } return sum; } public static Tensor<T> ComputeVariance(List<Tensor<T>> predictions, Tensor<T> mean) { var numOps = MathHelper.GetNumericOperations<T>(); var variance = new Tensor<T>(mean.Shape); foreach (var pred in predictions) { for (int i = 0; i < pred.Length; i++) { var diff = numOps.Subtract(pred[i], mean[i]); variance[i] = numOps.Add(variance[i], numOps.Multiply(diff, diff)); } } var count = numOps.FromDouble(predictions.Count); for (int i = 0; i < variance.Length; i++) { variance[i] = numOps.Divide(variance[i], count); } return variance; } }Then replace both implementations with calls to the helper.
src/PredictionModelBuilder.cs (2)
407-407: Magic constant10007in seed mixing lacks justification.Line 407 uses
(memberIndex + 1) * 10007to mix seeds. While large primes are common for hashing, this specific choice isn't explained. Consider using a well-known mixing function or documenting why 10007 was chosen. Minor issue, but clarity helps maintainability.
516-535: ComputeConformalRegressionQuantile duplicates logic in SplitConformalPredictor.The quantile computation here (lines 524-534) is nearly identical to
ComputeQuantileinsrc/UncertaintyQuantification/Calibration/SplitConformalPredictor.cs(per relevant code snippets). Both use the same adjusted-level formula and index clamping. Extract into a shared utility to avoid drift and ensure consistent conformal guarantees across the codebase.src/UncertaintyQuantification/Layers/BayesianDenseLayer.cs (1)
131-153: SampleWeights uses instance RNG correctly, but lacks thread-safety documentation.The method uses
NextGaussian()which locks_rngLock(lines 508-511). This prevents concurrent sampling corruption. However, ifSampleWeights()is called concurrently from multiple threads (e.g., in a multi-threaded inference scenario), the lock on_rngLockserializes access but doesn't prevent interleaved sampling across threads. Document whether concurrentSampleWeights()calls are supported, or if the layer assumes single-threaded usage.src/Models/Results/PredictionModelResult.cs (1)
1017-1091: ComputeMonteCarloMomentsAndMetrics duplicates ensemble logic from PredictWithDeepEnsemble.Lines 1050-1088 compute mean/variance and entropy/MI for MC dropout samples, which is nearly identical to the logic in
PredictWithDeepEnsemble(lines 775-833). Both:
- Compute mean and variance from samples (via
ComputeMeanAndVariance).- Check if samples are probabilities.
- Compute expected entropy and predictive entropy.
- Compute mutual information as
predictive_entropy - expected_entropy.Extract this into a shared
ComputeProbabilisticMetricshelper that takes samples and returns (mean, variance, predictive_entropy, mutual_information). This eliminates ~80 lines of duplication and ensures consistency.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (37)
docs/UncertaintyQuantification/PR431_GAP_ANALYSIS_AND_IMPLEMENTATION_PLAN.md(1 hunks)docs/UncertaintyQuantification/PR431_TODO.md(1 hunks)src/DataProcessor/DefaultDataPreprocessor.cs(9 hunks)src/Enums/UncertaintyQuantificationMethod.cs(1 hunks)src/Evaluation/DefaultModelEvaluator.cs(3 hunks)src/Helpers/LayerHelper.cs(2 hunks)src/Interfaces/IPredictionModelBuilder.cs(2 hunks)src/LinearAlgebra/Tensor.cs(28 hunks)src/LinearAlgebra/VectorBase.cs(5 hunks)src/Models/DataSetStats.cs(2 hunks)src/Models/Options/UncertaintyQuantificationOptions.cs(1 hunks)src/Models/Results/ClassificationConformalPredictionSet.cs(1 hunks)src/Models/Results/PredictionModelResult.cs(8 hunks)src/Models/Results/RegressionConformalInterval.cs(1 hunks)src/Models/Results/UncertaintyCalibrationArtifacts.cs(1 hunks)src/Models/Results/UncertaintyPredictionResult.cs(1 hunks)src/Models/UncertaintyStats.cs(1 hunks)src/NeuralNetworks/Layers/ActivationLayer.cs(3 hunks)src/NeuralNetworks/Layers/LayerBase.cs(4 hunks)src/NeuralNetworks/NeuralNetworkBase.cs(16 hunks)src/Optimizers/CMAESOptimizer.cs(4 hunks)src/PredictionModelBuilder.cs(8 hunks)src/Statistics/ErrorStats.cs(4 hunks)src/Statistics/PredictionStats.cs(6 hunks)src/UncertaintyQuantification/BayesianNeuralNetworks/BayesianNeuralNetwork.cs(1 hunks)src/UncertaintyQuantification/BayesianNeuralNetworks/DeepEnsemble.cs(1 hunks)src/UncertaintyQuantification/BayesianNeuralNetworks/MCDropoutNeuralNetwork.cs(1 hunks)src/UncertaintyQuantification/Interfaces/IBayesianLayer.cs(1 hunks)src/UncertaintyQuantification/Interfaces/IUncertaintyEstimator.cs(1 hunks)src/UncertaintyQuantification/Layers/BayesianDenseLayer.cs(1 hunks)src/UncertaintyQuantification/Layers/MCDropoutLayer.cs(1 hunks)src/UncertaintyQuantification/README.md(1 hunks)tests/AiDotNet.Tests/IntegrationTests/NonParallelIntegrationCollection.cs(1 hunks)tests/AiDotNet.Tests/IntegrationTests/UncertaintyQuantificationFacadeTests.cs(1 hunks)tests/AiDotNet.Tests/UnitTests/UncertaintyQuantification/ExpectedCalibrationErrorTests.cs(1 hunks)tests/AiDotNet.Tests/UnitTests/UncertaintyQuantification/MCDropoutLayerTests.cs(1 hunks)tests/AiDotNet.Tests/UnitTests/UncertaintyQuantification/TemperatureScalingTests.cs(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- docs/UncertaintyQuantification/PR431_TODO.md
🚧 Files skipped from review as they are similar to previous changes (9)
- tests/AiDotNet.Tests/UnitTests/UncertaintyQuantification/TemperatureScalingTests.cs
- src/Optimizers/CMAESOptimizer.cs
- tests/AiDotNet.Tests/UnitTests/UncertaintyQuantification/ExpectedCalibrationErrorTests.cs
- src/LinearAlgebra/VectorBase.cs
- src/UncertaintyQuantification/BayesianNeuralNetworks/BayesianNeuralNetwork.cs
- src/UncertaintyQuantification/README.md
- src/NeuralNetworks/NeuralNetworkBase.cs
- src/DataProcessor/DefaultDataPreprocessor.cs
- src/LinearAlgebra/Tensor.cs
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-12-18T08:49:25.295Z
Learnt from: ooples
Repo: ooples/AiDotNet PR: 444
File: src/Interfaces/IPruningMask.cs:1-102
Timestamp: 2025-12-18T08:49:25.295Z
Learning: In the AiDotNet repository, the project-level global using includes AiDotNet.Tensors.LinearAlgebra via AiDotNet.csproj. Therefore, Vector<T>, Matrix<T>, and Tensor<T> are available without per-file using directives. Do not flag missing using directives for these types in any C# files within this project. Apply this guideline broadly to all C# files (not just a single file) to avoid false positives. If a file uses a type from a different namespace not covered by the global using, flag as usual.
Applied to files:
src/Enums/UncertaintyQuantificationMethod.cssrc/Statistics/ErrorStats.cssrc/UncertaintyQuantification/Interfaces/IUncertaintyEstimator.cssrc/Models/Results/ClassificationConformalPredictionSet.cssrc/Models/Results/UncertaintyPredictionResult.cssrc/Models/Results/RegressionConformalInterval.cssrc/Evaluation/DefaultModelEvaluator.cssrc/NeuralNetworks/Layers/LayerBase.cstests/AiDotNet.Tests/IntegrationTests/UncertaintyQuantificationFacadeTests.cstests/AiDotNet.Tests/IntegrationTests/NonParallelIntegrationCollection.cssrc/Models/DataSetStats.cssrc/Models/Options/UncertaintyQuantificationOptions.cssrc/UncertaintyQuantification/Layers/BayesianDenseLayer.cssrc/Helpers/LayerHelper.cssrc/NeuralNetworks/Layers/ActivationLayer.cssrc/UncertaintyQuantification/BayesianNeuralNetworks/DeepEnsemble.cssrc/UncertaintyQuantification/Interfaces/IBayesianLayer.cssrc/Statistics/PredictionStats.cssrc/Models/UncertaintyStats.cssrc/Models/Results/PredictionModelResult.cssrc/Models/Results/UncertaintyCalibrationArtifacts.cssrc/UncertaintyQuantification/Layers/MCDropoutLayer.cssrc/PredictionModelBuilder.cssrc/UncertaintyQuantification/BayesianNeuralNetworks/MCDropoutNeuralNetwork.cstests/AiDotNet.Tests/UnitTests/UncertaintyQuantification/MCDropoutLayerTests.cssrc/Interfaces/IPredictionModelBuilder.cs
📚 Learning: 2025-12-18T08:49:53.103Z
Learnt from: ooples
Repo: ooples/AiDotNet PR: 444
File: src/Interfaces/IPruningStrategy.cs:1-4
Timestamp: 2025-12-18T08:49:53.103Z
Learning: In this repository, global using directives are declared in AiDotNet.csproj for core namespaces (AiDotNet.Tensors.* and AiDotNet.*) and common system types. When reviewing C# files, assume these global usings are in effect; avoid adding duplicate using statements for these namespaces and for types like Vector<T>, Matrix<T>, Tensor<T>, etc. If a type is not found, verify the global usings or consider adding a file-scoped using if needed. Prefer relying on global usings to reduce boilerplate.
Applied to files:
src/Enums/UncertaintyQuantificationMethod.cssrc/Statistics/ErrorStats.cssrc/UncertaintyQuantification/Interfaces/IUncertaintyEstimator.cssrc/Models/Results/ClassificationConformalPredictionSet.cssrc/Models/Results/UncertaintyPredictionResult.cssrc/Models/Results/RegressionConformalInterval.cssrc/Evaluation/DefaultModelEvaluator.cssrc/NeuralNetworks/Layers/LayerBase.cstests/AiDotNet.Tests/IntegrationTests/UncertaintyQuantificationFacadeTests.cstests/AiDotNet.Tests/IntegrationTests/NonParallelIntegrationCollection.cssrc/Models/DataSetStats.cssrc/Models/Options/UncertaintyQuantificationOptions.cssrc/UncertaintyQuantification/Layers/BayesianDenseLayer.cssrc/Helpers/LayerHelper.cssrc/NeuralNetworks/Layers/ActivationLayer.cssrc/UncertaintyQuantification/BayesianNeuralNetworks/DeepEnsemble.cssrc/UncertaintyQuantification/Interfaces/IBayesianLayer.cssrc/Statistics/PredictionStats.cssrc/Models/UncertaintyStats.cssrc/Models/Results/PredictionModelResult.cssrc/Models/Results/UncertaintyCalibrationArtifacts.cssrc/UncertaintyQuantification/Layers/MCDropoutLayer.cssrc/PredictionModelBuilder.cssrc/UncertaintyQuantification/BayesianNeuralNetworks/MCDropoutNeuralNetwork.cstests/AiDotNet.Tests/UnitTests/UncertaintyQuantification/MCDropoutLayerTests.cssrc/Interfaces/IPredictionModelBuilder.cs
🧬 Code graph analysis (14)
src/Enums/UncertaintyQuantificationMethod.cs (2)
src/UncertaintyQuantification/BayesianNeuralNetworks/DeepEnsemble.cs (2)
DeepEnsemble(37-237)DeepEnsemble(56-63)src/UncertaintyQuantification/BayesianNeuralNetworks/BayesianNeuralNetwork.cs (2)
BayesianNeuralNetwork(28-331)BayesianNeuralNetwork(44-51)
src/Statistics/ErrorStats.cs (1)
src/Helpers/StatisticsHelper.cs (1)
StatisticsHelper(17-6685)
src/Models/Results/UncertaintyPredictionResult.cs (6)
src/UncertaintyQuantification/BayesianNeuralNetworks/BayesianNeuralNetwork.cs (6)
UncertaintyPredictionResult(133-160)T(303-313)Tensor(171-199)Tensor(211-237)Tensor(242-265)Tensor(270-293)src/UncertaintyQuantification/BayesianNeuralNetworks/DeepEnsemble.cs (5)
UncertaintyPredictionResult(75-95)Tensor(107-122)Tensor(134-148)Tensor(172-193)Tensor(198-218)src/UncertaintyQuantification/BayesianNeuralNetworks/MCDropoutNeuralNetwork.cs (4)
UncertaintyPredictionResult(57-89)Tensor(101-116)Tensor(127-140)Tensor(156-177)src/UncertaintyQuantification/Interfaces/IUncertaintyEstimator.cs (1)
UncertaintyPredictionResult(33-33)src/Models/Results/RegressionConformalInterval.cs (2)
RegressionConformalInterval(13-35)RegressionConformalInterval(30-34)src/Models/Results/ClassificationConformalPredictionSet.cs (2)
ClassificationConformalPredictionSet(15-34)ClassificationConformalPredictionSet(30-33)
src/Evaluation/DefaultModelEvaluator.cs (2)
src/Models/DataSetStats.cs (2)
DataSetStats(35-129)DataSetStats(125-128)src/Models/UncertaintyStats.cs (2)
UncertaintyStats(15-33)UncertaintyStats(29-32)
src/Models/DataSetStats.cs (1)
src/Models/UncertaintyStats.cs (2)
UncertaintyStats(15-33)UncertaintyStats(29-32)
src/UncertaintyQuantification/Layers/BayesianDenseLayer.cs (1)
src/UncertaintyQuantification/Interfaces/IBayesianLayer.cs (3)
T(42-42)SampleWeights(29-29)AddKLDivergenceGradients(48-48)
src/Helpers/LayerHelper.cs (4)
src/UncertaintyQuantification/BayesianNeuralNetworks/BayesianNeuralNetwork.cs (1)
T(303-313)src/UncertaintyQuantification/Interfaces/IBayesianLayer.cs (1)
T(42-42)src/UncertaintyQuantification/Layers/BayesianDenseLayer.cs (4)
T(164-216)BayesianDenseLayer(27-530)BayesianDenseLayer(70-73)BayesianDenseLayer(75-85)src/NeuralNetworks/Layers/ActivationLayer.cs (3)
ActivationLayer(19-471)ActivationLayer(110-114)ActivationLayer(158-162)
src/UncertaintyQuantification/BayesianNeuralNetworks/DeepEnsemble.cs (4)
src/UncertaintyQuantification/Interfaces/IBayesianLayer.cs (1)
T(42-42)src/Models/Results/UncertaintyPredictionResult.cs (2)
UncertaintyPredictionResult(19-72)UncertaintyPredictionResult(57-71)src/UncertaintyQuantification/Interfaces/IUncertaintyEstimator.cs (3)
UncertaintyPredictionResult(33-33)Tensor(45-45)Tensor(57-57)src/UncertaintyQuantification/Layers/MCDropoutLayer.cs (1)
Vector(151-154)
src/UncertaintyQuantification/Interfaces/IBayesianLayer.cs (2)
src/UncertaintyQuantification/BayesianNeuralNetworks/BayesianNeuralNetwork.cs (1)
T(303-313)src/UncertaintyQuantification/Layers/BayesianDenseLayer.cs (3)
T(164-216)SampleWeights(131-153)AddKLDivergenceGradients(363-387)
src/Models/UncertaintyStats.cs (2)
src/Models/Results/PredictionModelResult.cs (3)
AiDotNet(2134-2178)Dictionary(963-981)Dictionary(1796-1804)src/UncertaintyQuantification/BayesianNeuralNetworks/BayesianNeuralNetwork.cs (1)
T(303-313)
src/Models/Results/UncertaintyCalibrationArtifacts.cs (1)
src/PredictionModelBuilder.cs (3)
T(516-535)T(605-627)T(680-694)
src/UncertaintyQuantification/Layers/MCDropoutLayer.cs (1)
src/NeuralNetworks/Layers/LayerBase.cs (14)
T(1098-1109)LayerBase(26-1427)LayerBase(303-309)LayerBase(331-335)LayerBase(359-364)LayerBase(387-393)LayerBase(416-420)LayerBase(444-449)LayerBase(1022-1041)Vector(500-508)Vector(864-878)Vector(1221-1225)Vector(1367-1367)SetTrainingMode(472-478)
src/UncertaintyQuantification/BayesianNeuralNetworks/MCDropoutNeuralNetwork.cs (6)
src/PredictionModelBuilder.cs (3)
T(516-535)T(605-627)T(680-694)src/UncertaintyQuantification/BayesianNeuralNetworks/BayesianNeuralNetwork.cs (6)
T(303-313)UncertaintyPredictionResult(133-160)Tensor(171-199)Tensor(211-237)Tensor(242-265)Tensor(270-293)src/UncertaintyQuantification/Interfaces/IBayesianLayer.cs (1)
T(42-42)src/UncertaintyQuantification/Calibration/ExpectedCalibrationError.cs (4)
T(67-95)List(109-124)List(129-153)Add(175-180)src/UncertaintyQuantification/Interfaces/IUncertaintyEstimator.cs (3)
UncertaintyPredictionResult(33-33)Tensor(45-45)Tensor(57-57)src/UncertaintyQuantification/Layers/MCDropoutLayer.cs (3)
MCDropoutLayer(28-182)MCDropoutLayer(62-73)Vector(151-154)
src/Interfaces/IPredictionModelBuilder.cs (1)
src/Models/Options/UncertaintyQuantificationOptions.cs (1)
UncertaintyQuantificationOptions(17-94)
🪛 LanguageTool
docs/UncertaintyQuantification/PR431_GAP_ANALYSIS_AND_IMPLEMENTATION_PLAN.md
[style] ~233-~233: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...an be injected safely) → MC Dropout. 4. Else fallback → bootstrap ensemble (generic ...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build All Frameworks
🔇 Additional comments (30)
src/Statistics/PredictionStats.cs (2)
255-264: LGTM!Clean alias implementation providing naming flexibility for consumers. The expression-bodied getter has no overhead and the documentation clearly explains its purpose.
522-533: Input validation addresses past review concern; verify edge case behavior.The length mismatch validation at lines 527-530 correctly addresses the previous review feedback. However, note that when one array is empty and the other is not (e.g.,
Actual.Length == 0butPredicted.Length == 5), the early return at line 524 bypasses the length check—silently returning zero-initialized stats rather than throwing.If this is intentional (treat any empty input as "no data"), the current logic is fine. If mismatched inputs should always fail, consider:
-if (inputs.Actual.Length == 0 || inputs.Predicted.Length == 0) +if (inputs.Actual.Length == 0 && inputs.Predicted.Length == 0) { return; }src/NeuralNetworks/Layers/LayerBase.cs (1)
828-841: LGTM! Clean shape-preserving activation logic.The refactored
ApplyActivationmethod correctly prioritizes vector activation when available, falls back to element-wise scalar activation viaTransform, and returns input unchanged as identity. This preserves tensor shape throughout, which aligns well with the uncertainty-related layers (MCDropout, BayesianDenseLayer) that rely on consistent tensor dimensions.src/NeuralNetworks/Layers/ActivationLayer.cs (1)
305-310: LGTM! Vectorized gradient access approach.The refactored
BackwardScalarActivationconvertsoutputGradientto a vector first, then uses index-based access withinTransform. This works correctly because:
- Shape matching is validated at line 259 before this method is called
ToVector()flattens in the same order thatTransform's indexiiteratesThis aligns with the vectorized gradient handling patterns used elsewhere in the uncertainty quantification components.
src/Models/DataSetStats.cs (1)
72-79: LGTM! Well-integrated uncertainty stats property.The new
UncertaintyStats<T>property follows the established pattern of other stats properties in this class:
- Consistent initialization with
Empty()factory method- Proper XML documentation
- Logical placement after
PredictionStatsThis cleanly integrates uncertainty quantification diagnostics into the dataset statistics infrastructure.
tests/AiDotNet.Tests/IntegrationTests/NonParallelIntegrationCollection.cs (1)
1-9: LGTM! Standard xUnit collection definition for non-parallel tests.This follows the correct xUnit pattern for disabling parallelization. Tests that require deterministic or sequential execution (e.g., those with stochastic UQ components like Monte Carlo Dropout) should be decorated with
[Collection("NonParallelIntegration")]to use this collection.src/Models/Results/UncertaintyCalibrationArtifacts.cs (1)
9-20: LGTM! Clean artifact container for calibration state.The internal sealed class appropriately encapsulates build-time calibration artifacts. The
Has*flag pattern paired with corresponding values is a clear design choice that aligns with howPredictionModelBuildercomputes these values (per the relevant snippets showingComputeConformalRegressionQuantile,FitTemperatureFromProbabilities, etc.).src/Evaluation/DefaultModelEvaluator.cs (3)
131-145: LGTM!The
MeanOfhelper correctly handles the empty tensor edge case and uses the numeric operations abstraction appropriately for generic type support.
96-109: LGTM!Clean refactoring that separates UQ population logic. The use of a local
statsvariable with a separateTryPopulateUncertaintyStatsmethod improves readability and maintainability.
126-128: The direct dictionary access is safe and follows the documented design of the codebase. TheUncertaintyPredictionResult<T, TOutput>class explicitly guarantees that the"predictive_entropy"and"mutual_information"keys always exist in theMetricsdictionary. TheCreateDefaultUncertaintyMetricsmethod ensures these keys are initialized in every code path (initialized with default zero tensors if not explicitly set), and the class documentation states "Keys are stable so downstream consumers do not need to branch on missing keys." DefensiveTryGetValuechecks are unnecessary and would contradict the established contract.Likely an incorrect or invalid review comment.
src/Enums/UncertaintyQuantificationMethod.cs (1)
1-39: LGTM!The enum cleanly captures the supported uncertainty quantification strategies outlined in Issue #418. The
Autooption provides flexibility for automatic method selection, and the XML documentation is thorough.src/Models/UncertaintyStats.cs (1)
15-33: LGTM!The
UncertaintyStats<T>class is a clean, minimal container that integrates well with the existing evaluation pipeline alongsideErrorStats<T>andPredictionStats<T>.src/Models/Results/RegressionConformalInterval.cs (1)
13-23: LGTM!The immutable design with get-only properties is appropriate for a data container representing a prediction interval. Documentation clearly explains the conformal prediction concept.
src/Models/Results/ClassificationConformalPredictionSet.cs (2)
30-33: LGTM!Good defensive programming with null validation. The jagged array structure appropriately supports variable-size prediction sets per sample.
15-24: LGTM!The
ClassIndicesproperty design with a jagged array is well-suited for conformal prediction sets where different samples may have different numbers of candidate classes based on model confidence.tests/AiDotNet.Tests/UnitTests/UncertaintyQuantification/MCDropoutLayerTests.cs (1)
1-134: LGTM! Comprehensive test coverage for MCDropoutLayer.The test suite covers all key behaviors: constructor validation, dropout application in training and MC modes, passthrough in inference mode, and property toggling. The retry loop in
Forward_InTrainingMode_AppliesDropoutproperly handles randomness-induced flakiness.src/UncertaintyQuantification/Interfaces/IUncertaintyEstimator.cs (1)
1-58: Well-designed interface with excellent documentation.The
IUncertaintyEstimator<T>interface cleanly separates the uncertainty estimation contract with appropriate methods for combined prediction, aleatoric, and epistemic uncertainty. The beginner-friendly XML documentation will aid API discoverability.src/UncertaintyQuantification/Interfaces/IBayesianLayer.cs (1)
1-49: Clean interface design for Bayesian layers.The
IBayesianLayer<T>interface properly captures the essential operations for variational inference: weight sampling, KL divergence computation, and gradient accumulation. The documentation effectively explains the probabilistic concepts for users unfamiliar with Bayesian methods.tests/AiDotNet.Tests/IntegrationTests/UncertaintyQuantificationFacadeTests.cs (3)
1-101: Comprehensive integration test coverage for uncertainty quantification.The tests effectively validate the end-to-end UQ workflow including MC Dropout layer injection, stochastic output verification, variance shape matching, and metric computation (predictive_entropy, mutual_information). The deterministic parameter setup ensures reproducible test conditions.
448-605: Well-designed test helper classes.The
PassthroughOptimizer,SingleStepTrainOptimizer, andDeterministicParameterOptimizerclasses provide appropriate test isolation by controlling the optimization behavior. This allows tests to focus on UQ functionality rather than training dynamics.
398-446: Good concurrency smoke test.The thread safety test validates that concurrent
PredictWithUncertaintycalls complete without exceptions and produce valid results. This provides confidence in thread-safe usage patterns.src/Helpers/LayerHelper.cs (1)
2039-2041: LGTM - Bayesian layer integration follows established patterns.The
CreateDefaultBayesianNeuralNetworkLayersmethod correctly mirrors the structure ofCreateDefaultNeuralNetworkLayers, substitutingBayesianDenseLayerforDenseLayerto enable epistemic uncertainty estimation through weight distributions.docs/UncertaintyQuantification/PR431_GAP_ANALYSIS_AND_IMPLEMENTATION_PLAN.md (1)
1-433: Excellent engineering planning document.This gap analysis provides valuable context for the UQ implementation:
- Clear scope definition: "Exhaustive MVP" criteria are well-defined
- Concurrency awareness: Section 2.1.2 correctly identifies the thread-safety risk with mutable dropout state
- Phased approach: The 8-phase plan balances incremental delivery with correctness
- Decision tracking: Open questions and stakeholder decisions are documented
The document will be valuable for onboarding contributors and tracking implementation progress. Consider keeping it updated as phases complete.
src/UncertaintyQuantification/Layers/MCDropoutLayer.cs (4)
62-73: Constructor validation and inverted dropout scaling are correct.The validation
dropoutRate < 0 || dropoutRate >= 1correctly rejects invalid rates. The inverted dropout scaling (1/(1-p)) is the standard approach that avoids rescaling during inference.
85-113: Forward pass logic is correct.The implementation correctly:
- Passes through unchanged when not in training/MC mode
- Applies inverted dropout scaling to kept neurons
- Preserves input shape in the output
For a future optimization, the element-wise loop could be vectorized using SIMD operations, but the current implementation is correct for the MVP.
120-138: Backward pass correctly applies the dropout mask to gradients.The implementation properly:
- Validates that forward pass occurred first
- Applies the same dropout mask to gradients (maintaining the dropout pattern)
- Preserves the original input shape
176-181: Clone creates an independent instance.The clone correctly copies configuration (
_dropoutRate,_mcMode,_initialSeed) and training mode. Note that if_initialSeedis null, the clone gets a freshRandom()instance with a different sequence than the original—this provides diversity but means clones aren't deterministic relative to each other unless seeded.src/Interfaces/IPredictionModelBuilder.cs (1)
357-386: Clean interface additions for uncertainty quantification configuration.The new methods follow the established fluent builder pattern and provide clear separation between:
- General UQ options (
ConfigureUncertaintyQuantification)- Regression calibration data (
xCalibration,yCalibrationasTOutput)- Classification calibration data (
xCalibration,calibrationLabelsasVector<int>)The documentation appropriately emphasizes that calibration data must be independent from training/test data for valid statistical guarantees. All methods are properly implemented in
PredictionModelBuilder.cswith correct mutual exclusivity between regression and classification calibration paths.src/UncertaintyQuantification/Layers/BayesianDenseLayer.cs (2)
363-387: AddKLDivergenceGradients computes gradients w.r.t. KL term correctly.The KL divergence gradient w.r.t. mean is
mean / prior_var(line 372), and w.r.t. log-variance is0.5 * (variance / prior_var - 1)(line 376). These match the analytical derivatives of the Gaussian KL divergence. The scaled gradients are accumulated into the existing gradient accumulators (lines 373, 377). This looks correct.
309-319: The log-variance gradient implementation is correct.The code properly implements the Bayes by Backprop reparameterization gradient:
weightStd = sqrt(exp(logVar))correctly computes standard deviation from log-varianceepsilon = (sampledWeights - mean) / (std + 1e-8)correctly isolates the reparameterization noise with numerical stabilitygradLogVar = gradMean * epsilon * 0.5 * weightStdcorrectly applies the chain rule, multiplying by standard deviation (not variance)The 1e-8 offset appropriately prevents numerical instability. No changes are needed.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/LoRA/Adapters/VBLoRAAdapter.cs (1)
425-475: Critical: Race condition on shared bank reads.UpdateLoRALayerFromBanks reads from the shared _globalBankA and _globalBankB dictionaries (lines 427-428) without acquiring _bankLock, while UpdateBanksFromLoRALayer (line 537) acquires the lock for writes. This creates a read-write race condition that could result in:
- Reading partially updated bank vectors during concurrent training
- Non-deterministic behavior in multi-threaded scenarios
- Potential dictionary access violations if banks are modified concurrently
🔎 Proposed fix to add lock protection for bank reads
private void UpdateLoRALayerFromBanks(LoRALayer<T> loraLayer) { - Matrix<T> bankA = _globalBankA[_bankKey]; - Matrix<T> bankB = _globalBankB[_bankKey]; + Matrix<T> bankA; + Matrix<T> bankB; + + lock (_bankLock) + { + bankA = _globalBankA[_bankKey]; + bankB = _globalBankB[_bankKey]; + } // Build matrix A from selected bank columns Matrix<T> loraA = new Matrix<T>(bankA.Rows, Rank);
🧹 Nitpick comments (6)
tests/AiDotNet.Tests/TestUtilities/TestOptimizers.cs (1)
119-133: Consider pattern matching and handling type mismatch explicitly.Two observations:
- The
ascast with null check can be simplified using pattern matching.- When
_modelis not aNeuralNetworkModel<double>, the method silently returns the unchanged model. This could mask test setup errors where the wrong model type is passed.🔎 Suggested refactor using pattern matching
public OptimizationResult<double, TInput, TOutput> Optimize(OptimizationInputData<double, TInput, TOutput> inputData) { - var nn = _model as NeuralNetworkModel<double>; - if (nn != null) + if (_model is NeuralNetworkModel<double> nn) { var parameters = nn.Network.GetParameters(); for (int i = 0; i < parameters.Length; i++) { parameters[i] = (i + 1) * 0.01; } nn.Network.UpdateParameters(parameters); } + else + { + throw new InvalidOperationException( + $"Expected NeuralNetworkModel<double> but got {_model.GetType().Name}"); + } return new OptimizationResult<double, TInput, TOutput> { BestSolution = _model }; }If the silent no-op behavior is intentional (e.g., for testing mixed model scenarios), please disregard the exception suggestion and keep only the pattern matching simplification.
src/LoRA/Adapters/VBLoRAAdapter.cs (1)
492-499: Consider caching to avoid redundant bank synchronization.UpdateLoRALayerFromBanks is called on every Forward pass (line 495), rebuilding the LoRA matrices from banks even when they haven't changed. This ensures consistency but may impact performance in inference-heavy workloads where banks remain static between parameter updates.
💡 Optional optimization: track bank synchronization state
Consider adding a dirty flag:
+private bool _banksSyncRequired = true; + public override Tensor<T> Forward(Tensor<T> input) { - // Sync LoRA layer with current bank state before forward pass - UpdateLoRALayerFromBanks(_loraLayer); + if (_banksSyncRequired) + { + UpdateLoRALayerFromBanks(_loraLayer); + _banksSyncRequired = false; + } // Use base class forward pass (base layer + LoRA layer) return base.Forward(input); } public override void UpdateParameters(T learningRate) { // Update base class (updates LoRA layer and optionally base layer) base.UpdateParameters(learningRate); // Write updated LoRA parameters back to banks UpdateBanksFromLoRALayer(_loraLayer); + _banksSyncRequired = true; }This avoids rebuilding matrices when banks haven't changed, while maintaining correctness.
tests/AiDotNet.Tests/IntegrationTests/UncertaintyQuantificationFacadeTests.cs (4)
161-181: Clarify or fix the length-mismatch handling.When
candidate.Length != baseline.Length(line 166), the method continues to the next attempt instead of returningtrue. This means if all attempts produce vectors of different lengths, the method returnsfalsedespite detecting differences.If length mismatch should count as a difference, return
trueimmediately. If you intend to skip invalid samples and only test valid-length samples, consider adding a comment to clarify this behavior.🔎 Proposed fix if length mismatch should count as a difference
for (int i = 0; i < attempts; i++) { var candidate = sampleFactory(); if (candidate.Length != baseline.Length) { - continue; + return true; } for (int j = 0; j < baseline.Length; j++) { if (candidate[j] != baseline[j]) { return true; } } }
183-193: Consider tolerance-based comparison for floating-point values.Line 191 uses exact equality (
==) to comparedoublevalues, which can be problematic for floating-point arithmetic due to rounding and precision errors. This may lead to flaky tests.If bit-level determinism is guaranteed (e.g., when using fixed seeds), exact equality is appropriate. Otherwise, consider using a tolerance-based comparison (e.g.,
Assert.InRangeor a helper likeAssert.Equal(expected, actual, precision)).🔎 Proposed tolerance-based comparison
private static void AssertTensorEqual(Tensor<double> left, Tensor<double> right) { Assert.Equal(left.Shape, right.Shape); var leftVector = left.ToVector(); var rightVector = right.ToVector(); Assert.Equal(leftVector.Length, rightVector.Length); for (int i = 0; i < leftVector.Length; i++) { - Assert.Equal(leftVector[i], rightVector[i]); + Assert.Equal(leftVector[i], rightVector[i], precision: 10); // or appropriate tolerance } }Alternatively, if bit-level determinism is guaranteed, add a comment explaining why exact equality is safe.
82-94: Strengthen the sanity check to verify stochasticity.The comment states "Sanity-check: MC mode should yield stochastic predictions," but the code only asserts shape equality (line 94) without verifying that
sample1andsample2actually differ. Consider adding an assertion to confirm the samples are not identical.🔎 Proposed enhancement
var sample1 = result.Predict(x); var sample2 = result.Predict(x); foreach (var layer in injectedMcDropoutLayers) { layer.MonteCarloMode = false; } var sample1Tensor = ConversionsHelper.ConvertToTensor<double>(sample1); var sample2Tensor = ConversionsHelper.ConvertToTensor<double>(sample2); Assert.Equal(sample1Tensor.Shape, sample2Tensor.Shape); + +// Verify samples differ (stochasticity) +var s1Vec = sample1Tensor.ToVector(); +var s2Vec = sample2Tensor.ToVector(); +Assert.True(s1Vec.Where((val, idx) => val != s2Vec[idx]).Any(), "MC samples should differ");
315-354: Consider strengthening the variance assertion.Line 353 checks that at least one variance value is greater than zero using
Assert.Contains. While this validates that some uncertainty is captured, a more robust check could verify that multiple variance values (or a certain percentage) are non-zero to ensure ensemble diversity.🔎 Proposed enhancement
var varianceVector = uq.Variance!.ToVector(); -Assert.Contains(varianceVector, v => v > 0.0); +var nonZeroCount = varianceVector.Count(v => v > 0.0); +Assert.True(nonZeroCount > 0, "At least one variance should be non-zero"); +// Optionally: Assert.True(nonZeroCount >= varianceVector.Length / 2, "Majority of variances should be non-zero for ensemble diversity");
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
docs/UncertaintyQuantification/PR431_TODO.md(1 hunks)src/LinearAlgebra/TensorBase.cs(2 hunks)src/LoRA/Adapters/VBLoRAAdapter.cs(2 hunks)tests/AiDotNet.Tests/IntegrationTests/UncertaintyQuantificationFacadeTests.cs(1 hunks)tests/AiDotNet.Tests/TestUtilities/TestOptimizers.cs(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- docs/UncertaintyQuantification/PR431_TODO.md
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-12-18T08:49:25.295Z
Learnt from: ooples
Repo: ooples/AiDotNet PR: 444
File: src/Interfaces/IPruningMask.cs:1-102
Timestamp: 2025-12-18T08:49:25.295Z
Learning: In the AiDotNet repository, the project-level global using includes AiDotNet.Tensors.LinearAlgebra via AiDotNet.csproj. Therefore, Vector<T>, Matrix<T>, and Tensor<T> are available without per-file using directives. Do not flag missing using directives for these types in any C# files within this project. Apply this guideline broadly to all C# files (not just a single file) to avoid false positives. If a file uses a type from a different namespace not covered by the global using, flag as usual.
Applied to files:
src/LoRA/Adapters/VBLoRAAdapter.cssrc/LinearAlgebra/TensorBase.cstests/AiDotNet.Tests/IntegrationTests/UncertaintyQuantificationFacadeTests.cstests/AiDotNet.Tests/TestUtilities/TestOptimizers.cs
📚 Learning: 2025-12-18T08:49:53.103Z
Learnt from: ooples
Repo: ooples/AiDotNet PR: 444
File: src/Interfaces/IPruningStrategy.cs:1-4
Timestamp: 2025-12-18T08:49:53.103Z
Learning: In this repository, global using directives are declared in AiDotNet.csproj for core namespaces (AiDotNet.Tensors.* and AiDotNet.*) and common system types. When reviewing C# files, assume these global usings are in effect; avoid adding duplicate using statements for these namespaces and for types like Vector<T>, Matrix<T>, Tensor<T>, etc. If a type is not found, verify the global usings or consider adding a file-scoped using if needed. Prefer relying on global usings to reduce boilerplate.
Applied to files:
src/LoRA/Adapters/VBLoRAAdapter.cssrc/LinearAlgebra/TensorBase.cstests/AiDotNet.Tests/IntegrationTests/UncertaintyQuantificationFacadeTests.cstests/AiDotNet.Tests/TestUtilities/TestOptimizers.cs
🧬 Code graph analysis (1)
tests/AiDotNet.Tests/TestUtilities/TestOptimizers.cs (1)
src/PredictionModelBuilder.cs (1)
IOptimizer(427-454)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build All Frameworks
🔇 Additional comments (12)
src/LinearAlgebra/TensorBase.cs (1)
75-100: LGTM! Clean flat indexer implementation.The flat indexer provides efficient direct access to the underlying tensor storage with proper bounds checking. The implementation is correct, well-documented (including the row-major order note), and consistent with the existing multi-dimensional indexer pattern. This addition complements the existing API nicely and will be useful for sequential access patterns.
tests/AiDotNet.Tests/TestUtilities/TestOptimizers.cs (2)
10-59: LGTM!Clean implementation of a passthrough optimizer test double. The no-op implementations for
Reset(),Serialize(), andDeserialize()are appropriate for a test utility, and throwingNotSupportedExceptionfor persistence methods clearly communicates the limited scope of this test double.
61-108: LGTM!The single-step training optimizer provides a useful test double for scenarios requiring a minimal training pass. Letting any training exceptions propagate naturally is appropriate for test utilities.
src/LoRA/Adapters/VBLoRAAdapter.cs (2)
415-418: Verify relevance to PR scope.This initialization-order fix is technically sound and addresses the classic C# pitfall of calling virtual methods from base constructors. However, it appears unrelated to the PR's stated objectives (uncertainty quantification features like Bayesian NNs, calibration, and conformal prediction).
Please confirm whether this change:
- Is an incidental bugfix discovered during UQ development
- Should be in a separate PR for clarity
314-316: Initialization order fix is correct, but verify thread safety.The explicit synchronization call after banks and indices are initialized correctly resolves the virtual-method-from-constructor issue. All required fields (_bankKey, _bankSizeA, _bankSizeB, banks, and indices) are properly initialized before this call.
However, UpdateLoRALayerFromBanks reads from shared banks without acquiring _bankLock, creating a potential race condition with concurrent writes (see separate comment on line 425).
tests/AiDotNet.Tests/IntegrationTests/UncertaintyQuantificationFacadeTests.cs (7)
22-31: LGTM!The test correctly validates that
MCDropoutLayerproduces stochastic outputs whenmcModeis enabled, even withTrainingModeset to false. Using 8 attempts with a 50% dropout rate provides sufficient probability to detect variance.
104-159: LGTM with a caveat on floating-point comparison.The test correctly validates that
PredictWithUncertaintyproduces deterministic results when using a fixedRandomSeed. However, note thatAssertTensorEqualuses exact equality for floating-point values (see separate comment on lines 183-193), which may cause flakiness if bit-level determinism is not guaranteed.
195-250: LGTM!The test thoroughly validates conformal prediction for regression, including:
- Proper setup of training and calibration datasets
- Configuration of confidence level (0.9)
- Verification that predictions fall within the computed intervals
252-313: LGTM!The test comprehensively validates conformal classification with temperature scaling calibration:
- Multi-class classification setup (3 classes)
- Prediction set generation with coverage guarantees
- Per-sample probability normalization after temperature scaling (lines 298-309)
- Presence of uncertainty metrics
399-447: LGTM!The test effectively validates thread safety by executing 12 concurrent
PredictWithUncertaintycalls and verifying that all results are complete and consistent. This provides good coverage for concurrency concerns in the UQ implementation.
44-44: No action needed. All referenced helper optimizers (PassthroughOptimizer,SingleStepTrainOptimizer, andDeterministicNeuralNetworkParameterOptimizer) are properly defined inAiDotNet.Tests.TestUtilities.TestOptimizersand correctly accessible via the existing import statement. The test file will compile without issues.Likely an incorrect or invalid review comment.
375-380: Verify the target format consistency for single-output regression.Lines 375-380 construct
yas aMatrix<double>with shape (3, 1), whereas other regression tests useVector<double>(e.g., lines 215, 334). TheBayesianNeuralNetwork.Train()method explicitly handles 1D targets as the "common ergonomic contract." Align this test with the established pattern unless matrix-shaped outputs are intentionally being tested.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
src/Optimizers/OptimizerBase.cs (1)
983-991: Good improvement to preserve derived optimizer options types during deserialization.The change now deserializes options using the runtime-determined type rather than always using the base type, which prevents loss of derived-class-specific properties during serialization round-trips. This is essential for derived optimizers like
BFGSOptimizer,FTRLOptimizer, etc., which have specialized options classes.However, consider these refinements:
Line 983: Since
Optionsis always initialized to a non-null value in the constructor (line 142), the null-conditional operator?.may be unnecessary. Consider usingOptions.GetType()directly for clarity.Lines 988-991: The silent skip when
optionsis null might hide deserialization failures. Consider throwing an exception instead to surface issues early:if (options == null) { throw new InvalidOperationException("Failed to deserialize optimizer options."); } UpdateOptions(options);🔎 Proposed refinement
// Deserialize options string optionsJson = reader.ReadString(); -Type optionsType = Options?.GetType() ?? typeof(OptimizationAlgorithmOptions<T, TInput, TOutput>); +Type optionsType = Options.GetType(); object? deserializedOptions = JsonConvert.DeserializeObject(optionsJson, optionsType); var options = deserializedOptions as OptimizationAlgorithmOptions<T, TInput, TOutput>; // Update the options -if (options != null) +if (options == null) { - UpdateOptions(options); + throw new InvalidOperationException("Failed to deserialize optimizer options."); } +UpdateOptions(options);src/NeuralNetworks/Layers/DenseLayer.cs (1)
154-159: Consider using ASCII-compatible multiplication symbol.The documentation uses the Unicode '×' character (lines 154, 159). While this renders nicely, it may cause display issues in ASCII-only terminals or older documentation tools. Consider using
*orxfor broader compatibility.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/LoRA/Adapters/VBLoRAAdapter.cs(3 hunks)src/LossFunctions/MeanAbsoluteErrorLoss.cs(1 hunks)src/NeuralNetworks/Layers/DenseLayer.cs(8 hunks)src/Optimizers/LionOptimizer.cs(7 hunks)src/Optimizers/OptimizerBase.cs(2 hunks)tests/AiDotNet.Tests/UnitTests/LossFunctions/SparseCategoricalCrossEntropyLossTests.cs(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-12-18T08:49:25.295Z
Learnt from: ooples
Repo: ooples/AiDotNet PR: 444
File: src/Interfaces/IPruningMask.cs:1-102
Timestamp: 2025-12-18T08:49:25.295Z
Learning: In the AiDotNet repository, the project-level global using includes AiDotNet.Tensors.LinearAlgebra via AiDotNet.csproj. Therefore, Vector<T>, Matrix<T>, and Tensor<T> are available without per-file using directives. Do not flag missing using directives for these types in any C# files within this project. Apply this guideline broadly to all C# files (not just a single file) to avoid false positives. If a file uses a type from a different namespace not covered by the global using, flag as usual.
Applied to files:
tests/AiDotNet.Tests/UnitTests/LossFunctions/SparseCategoricalCrossEntropyLossTests.cssrc/Optimizers/LionOptimizer.cssrc/LossFunctions/MeanAbsoluteErrorLoss.cssrc/LoRA/Adapters/VBLoRAAdapter.cssrc/NeuralNetworks/Layers/DenseLayer.cssrc/Optimizers/OptimizerBase.cs
📚 Learning: 2025-12-18T08:49:53.103Z
Learnt from: ooples
Repo: ooples/AiDotNet PR: 444
File: src/Interfaces/IPruningStrategy.cs:1-4
Timestamp: 2025-12-18T08:49:53.103Z
Learning: In this repository, global using directives are declared in AiDotNet.csproj for core namespaces (AiDotNet.Tensors.* and AiDotNet.*) and common system types. When reviewing C# files, assume these global usings are in effect; avoid adding duplicate using statements for these namespaces and for types like Vector<T>, Matrix<T>, Tensor<T>, etc. If a type is not found, verify the global usings or consider adding a file-scoped using if needed. Prefer relying on global usings to reduce boilerplate.
Applied to files:
tests/AiDotNet.Tests/UnitTests/LossFunctions/SparseCategoricalCrossEntropyLossTests.cssrc/Optimizers/LionOptimizer.cssrc/LossFunctions/MeanAbsoluteErrorLoss.cssrc/LoRA/Adapters/VBLoRAAdapter.cssrc/NeuralNetworks/Layers/DenseLayer.cssrc/Optimizers/OptimizerBase.cs
🧬 Code graph analysis (2)
src/LoRA/Adapters/VBLoRAAdapter.cs (6)
src/LoRA/Adapters/MoRAAdapter.cs (1)
LoRALayer(369-375)src/LoRA/Adapters/DVoRAAdapter.cs (1)
LoRALayer(478-484)src/LoRA/Adapters/LoRAAdapterBase.cs (1)
LoRALayer(177-182)src/LoRA/LoRALayer.cs (2)
LoRALayer(32-577)LoRALayer(155-203)src/LoRA/Adapters/TiedLoRAAdapter.cs (1)
LoRALayer(431-437)src/LoRA/Adapters/VeRAAdapter.cs (1)
LoRALayer(298-305)
src/Optimizers/OptimizerBase.cs (8)
src/Optimizers/CMAESOptimizer.cs (2)
OptimizationAlgorithmOptions(447-450)UpdateOptions(426-436)src/Optimizers/BFGSOptimizer.cs (2)
OptimizationAlgorithmOptions(260-263)UpdateOptions(240-250)src/Optimizers/LBFGSOptimizer.cs (2)
OptimizationAlgorithmOptions(297-300)UpdateOptions(273-283)src/Optimizers/ConjugateGradientOptimizer.cs (3)
OptimizationAlgorithmOptions(262-265)T(168-174)UpdateOptions(241-251)src/Optimizers/DFPOptimizer.cs (2)
OptimizationAlgorithmOptions(276-279)UpdateOptions(255-265)src/Optimizers/AMSGradOptimizer.cs (2)
OptimizationAlgorithmOptions(237-240)UpdateOptions(216-226)src/Optimizers/ProximalGradientDescentOptimizer.cs (2)
OptimizationAlgorithmOptions(362-365)UpdateOptions(331-341)src/Optimizers/FTRLOptimizer.cs (1)
UpdateOptions(245-255)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build All Frameworks
🔇 Additional comments (9)
tests/AiDotNet.Tests/UnitTests/LossFunctions/SparseCategoricalCrossEntropyLossTests.cs (1)
63-63: LGTM! Test assertion corrected to match actual computation.The updated expected value (0.9830564281864164) is mathematically accurate for the average of -log(0.7) and -log(0.2). The previous value was off by approximately 8.3×10⁻⁶.
src/LossFunctions/MeanAbsoluteErrorLoss.cs (1)
47-56: LGTM! Correct implementation of MAE derivative with proper zero-difference handling.The updated derivative calculation correctly implements the sign function for MAE's gradient:
- Returns +1 when predicted > actual
- Returns -1 when predicted < actual
- Returns 0 when predicted == actual (the subgradient at zero)
This properly handles the case where predictions are perfect (difference = 0), which aligns with the commit message fix. The choice of 0 as the subgradient at the discontinuity is standard practice and ensures stable gradient descent.
src/NeuralNetworks/Layers/DenseLayer.cs (4)
368-385: LGTM!The forward pass correctly normalizes input to 2D, performs the linear transformation, applies activation, and restores the original shape when needed. The
squeezeOutputflag properly tracks shape restoration.
456-483: LGTM!The
EnsureRank2BatchFirsthelper correctly handles:
- 1D tensors by reshaping to
[1, N]with squeeze flag- Validation of batch size and tensor length compatibility
- Short-circuit for already-correct 2D shapes
- Reshaping higher-rank tensors to 2D
The validation logic properly catches non-divisible tensor lengths.
420-431: Verify the gradient reshape fallback behavior.The fallback reshape logic (lines 423-430) attempts to fix batch dimension mismatches by reshaping
outputGradient. While this adds flexibility, it could mask bugs where shapes are genuinely incompatible due to upstream errors.Consider adding a debug-mode assertion or warning when this fallback is triggered to aid debugging during development.
214-216: The default activation change from ReLU to Identity does not impact existing code in this repository, as all DenseLayer instantiations explicitly specify their activation functions. This change is safe and appears intentional (possibly for supporting specialized use cases like linear output layers or Bayesian approaches). No action required.src/LoRA/Adapters/VBLoRAAdapter.cs (3)
314-316: LGTM: Constructor synchronization ensures valid initial state.The immediate call to
UpdateLoRALayerFromBanksafter bank and index initialization is good defensive practice. It ensures the LoRA layer is properly synchronized with the selected bank vectors right after construction, enabling safe inspection or parameter access before the first forward pass.
422-425: LGTM: Clear explanation of initialization order constraints.The comment accurately explains why
CreateLoRALayermust return a standard LoRA layer without bank synchronization. Since this virtual method is invoked from the base constructor before VB-LoRA-specific fields are initialized, deferring synchronization to the derived constructor andForward()is the correct approach.This documentation will help future maintainers understand the design decision.
352-365: Document the Bank B initialization strategy choice.Bank B is initialized with small Gaussian noise (stddev = sqrt(1/bankSizeB) * 0.01) rather than zeros (standard LoRA) or uniform distribution (official VBLoRA). This is a design decision that deviates from both conventions:
- Standard LoRA initializes either B to zero and A to random, or vice versa, ensuring the adapter starts with no effect.
- Official VBLoRA uses uniform distribution between -init_vector_bank_bound and init_vector_bank_bound, typically 0.02, not Gaussian.
The Gaussian approach with dynamic scaling based on bankSizeB may be intentional for shared bank learning, but should be explicitly documented at the class level explaining why this diverges from standard VBLoRA guidance and whether it has been validated for training stability.
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 (2)
src/Interpretability/InterpretabilityMetricsHelper.cs (2)
128-144: Breaking change: threshold modified from> 0to>= 0.5without migration path or documentation update.The change from
GreaterThan(predictions[i], Zero)toGreaterThanOrEquals(predictions[i], threshold)withthreshold = 0.5introduces a breaking behavioral change:
Semantic shift: Previously, any positive value was treated as a positive prediction. Now, only predictions ≥ 0.5 are considered positive. For probabilistic predictions in [0, 1], this is standard practice and aligns with calibration workflows (relevant given the PR's UQ/calibration focus). However, this silently changes results for existing callers.
Boundary behavior: Predictions exactly at 0.5 are now classified as positive (previously negative). This edge case affects fairness metrics and downstream analyses.
Documentation mismatch: Line 115 states
"binary: 0 or 1", implying strictly binary inputs, but the new threshold logic suggests predictions are continuous probabilities. Update the documentation to clarify whether this method expects binary labels or probability scores, and document the 0.5 threshold behavior.Consider:
- Adding a release note or migration guide documenting this breaking change.
- Updating XML documentation across all affected methods (lines 115, 149, 198, 247) to clarify the expected input format (binary vs. probabilistic) and threshold semantics.
- If backward compatibility is needed, consider making the threshold configurable (see next comment).
128-291: Add test coverage for threshold behavior across all fairness metrics; verify compatibility with existing callers.These metrics are used by six fairness evaluator classes (ComprehensiveFairnessEvaluator, DisparateImpactBiasDetector, GroupFairnessEvaluator, DemographicParityBiasDetector, EqualOpportunityBiasDetector, BasicFairnessEvaluator). The 0.5 threshold is critical for correctness in fairness analysis. Current implementation has no test coverage for:
- Boundary conditions (predictions exactly at 0.5)
- Consistency of threshold behavior across all four metrics
- Integration tests verifying that callers receive expected results
Add unit tests covering the threshold boundary and integration tests with the dependent bias detectors before merging.
🧹 Nitpick comments (11)
src/Interpretability/FairnessEvaluatorBase.cs (1)
111-113: Good improvement, but update the XML documentation.The change to
ArgumentOutOfRangeExceptionis the correct exception type for range validation and follows .NET best practices. However, the XML documentation at line 84 still mentionsArgumentExceptionfor the invalid index scenario.🔎 Proposed fix for the documentation
- /// <exception cref="ArgumentException">Thrown when sensitiveFeatureIndex is invalid or when actualLabels length doesn't match inputs.</exception> + /// <exception cref="ArgumentOutOfRangeException">Thrown when sensitiveFeatureIndex is out of range.</exception> + /// <exception cref="ArgumentException">Thrown when actualLabels length doesn't match inputs.</exception>src/FeatureSelectors/SelectFromModel.cs (1)
268-281: LGTM! Type-specific tolerance values are appropriate.The helper method correctly returns type-appropriate epsilon values. The runtime type checking pattern is correct for generic methods, and the epsilon values (1e-6 for float, 1e-12 for double) align with the typical precision limits of these types.
Returning zero for other numeric types is a safe fallback that preserves exact threshold behavior for types where floating-point precision isn't a concern.
Optional: Add explanatory comment
While the code is clear, a brief comment explaining why these specific epsilon values are chosen could help future maintainers:
private T GetThresholdTolerance() { + // Return type-specific epsilon for floating-point comparison tolerance. + // These values account for typical precision limits of each type. if (typeof(T) == typeof(float)) { return NumOps.FromDouble(1e-6); }src/Interpretability/DisparateImpactBiasDetector.cs (2)
100-110: Consider caching group indices to avoid redundant computation.The loop iterates over the same groups and recomputes
groupIndicesandgroupPredictionsthat were already computed in the first loop (lines 60-69). For large datasets or many groups, this doubles the work.🔎 Suggested approach
Store the group indices and predictions in a dictionary during the first loop:
// In first loop (around line 60): var groupData = new Dictionary<string, (List<int> Indices, Vector<T> Predictions)>(); foreach (var group in groups) { var groupIndices = InterpretabilityMetricsHelper<T>.GetGroupIndices(sensitiveFeature, group); var groupPredictions = InterpretabilityMetricsHelper<T>.GetSubset(predictions, groupIndices); string groupKey = group?.ToString() ?? "unknown"; groupData[groupKey] = (groupIndices, groupPredictions); // ... rest of logic } // Then reuse in second loop: foreach (var (groupKey, (indices, preds)) in groupData) { var groupActual = InterpretabilityMetricsHelper<T>.GetSubset(actualLabels, indices); // ... }
116-120: Redundant count check.Since
groups.Count >= 2is guaranteed by line 49 (early return otherwise),tprswill always have at least 2 elements. Theif (tprs.Count > 0)check is unnecessary.🔎 Proposed simplification
var tprs = groupTruePositiveRates.Values.Select(v => Convert.ToDouble(v)).ToList(); - if (tprs.Count > 0) - { - result.EqualOpportunityDifference = _numOps.FromDouble(tprs.Max() - tprs.Min()); - } + result.EqualOpportunityDifference = _numOps.FromDouble(tprs.Max() - tprs.Min());tests/AiDotNet.Tests/UnitTests/Interpretability/FairnessEvaluatorTests.cs (1)
143-145: Consider verifying specific metric values for stronger validation.The current assertions only check that metrics are non-zero, which doesn't catch incorrect computations. Given the test data, the expected values are approximately:
- Equal Opportunity: |2/3 - 1/2| ≈ 0.167
- Equalized Odds: max(0.167, 0.5) = 0.5
- Predictive Parity: |1.0 - 0.5| = 0.5
🔎 Proposed stronger assertions
- Assert.NotEqual(0.0, result.EqualOpportunity); // Should have some value - Assert.NotEqual(0.0, result.EqualizedOdds); // Should have some value - Assert.NotEqual(0.0, result.PredictiveParity); // Should have some value + Assert.Equal(0.167, result.EqualOpportunity, 2); // |TPR_group0 - TPR_group1| = |2/3 - 1/2| + Assert.Equal(0.5, result.EqualizedOdds, 2); // max(TPR_diff, FPR_diff) = max(0.167, 0.5) + Assert.Equal(0.5, result.PredictiveParity, 2); // |PPV_group0 - PPV_group1| = |1.0 - 0.5|src/Optimizers/LionOptimizer.cs (1)
380-380: Minor formatting nit: extra blank line.This adds an extra blank line before the
Reset()method. Consider removing for consistency, though this is purely cosmetic.src/MetaLearning/Trainers/MetaLearnerBase.cs (1)
112-117: Duplication exists—consider consolidating validation logic.The specific validation checks for
NumMetaIterations,MetaBatchSize, andInnerSteps(lines 112-117) duplicate constraints already enforced byconfig.IsValid()on line 119. Both the specific checks and theIsValid()implementations inReptileTrainerConfigandMAMLTrainerConfigvalidate these properties with> 0(equivalent to>= 1).While the specific checks improve error messages, they create a maintenance burden: any future changes to validation constraints must be updated in multiple locations. Consider either:
- Removing the specific checks and enhancing
IsValid()to throw detailed exceptions directly, or- Documenting that the specific checks intentionally precede
IsValid()for improved developer experiencesrc/Genetics/ModelIndividual.cs (1)
90-95: Consider invalidating the parameter cache in SetGenes.Line 94 recreates
_innerModelvia the factory, which could produce a model with a different parameter count. However, unlikeSetParameters(line 288), this method doesn't invalidate_parameterCountCache.Consider adding
_parameterCountCache = null;after line 94 to ensure the cached parameter count stays consistent with the new model structure.🔎 Suggested fix
public void SetGenes(ICollection<TGene> genes) { _genes = [.. genes]; // Recreate the model with the new genes _innerModel = _modelFactory(_genes); + _parameterCountCache = null; // invalidate cache when model structure changes }tests/AiDotNet.Tests/UnitTests/MetaLearning/Helpers/SimpleMockModel.cs (1)
93-93: Consider documenting the relationship between OutputFeatureCount and actual output shape.With the
Predictmethod now returning a tensor of shape[batchSize](line 65), theOutputFeatureCount = 1property represents that each sample produces one scalar output. While this is logically consistent, a brief comment on this property could help clarify that the output tensor shape omits the explicit feature dimension for scalar predictions.📝 Suggested documentation enhancement
// IFeatureAware implementation public int InputFeatureCount => 10; -public int OutputFeatureCount => 1; +public int OutputFeatureCount => 1; // Scalar output per sample; Predict returns shape [batch] public string[] FeatureNames { get; set; } = Array.Empty<string>();src/Helpers/FeatureSelectorHelper.cs (1)
339-343: Align with the simpler pattern used in CreateFeatureSubset (line 294).Lines 339-343 use an explicit cast to
IEnumerable<T>when selecting columns, while the similar logic inCreateFeatureSubset(line 294) simply passesVector<T>[]directly. SinceVector<T>implementsIEnumerable<T>, the explicit cast is unnecessary. For consistency and simplicity, use the same pattern asCreateFeatureSubset:var selectedColumns = selectedFeatureIndices - .Select(i => (IEnumerable<T>)matrix.GetColumn(i)) + .Select(i => matrix.GetColumn(i)) .ToArray(); return (TInput)(object)Matrix<T>.FromColumns(selectedColumns);src/Interpretability/InterpretabilityMetricsHelper.cs (1)
134-134: Consider parameterizing the decision threshold for flexibility.The threshold is hardcoded to 0.5 across all four methods (
ComputePositiveRate,ComputeTruePositiveRate,ComputeFalsePositiveRate,ComputePrecision). While 0.5 is a common default for balanced classification, different application domains often require custom thresholds:
- Medical diagnosis: Lower thresholds (e.g., 0.3) to minimize false negatives.
- Fraud detection: Higher thresholds (e.g., 0.7) to minimize false positives.
- Cost-sensitive decisions: Domain-specific trade-offs between precision and recall.
Given the PR's focus on uncertainty quantification and calibration, offering threshold configurability would enhance the utility of these interpretability metrics for reliability analysis and fairness evaluation across diverse use cases.
Proposed refactor: Add optional threshold parameter
Example for
ComputePositiveRate:-public static T ComputePositiveRate(Vector<T> predictions) +public static T ComputePositiveRate(Vector<T> predictions, T? threshold = null) { if (predictions.Length == 0) return _numOps.Zero; int positiveCount = 0; - T threshold = _numOps.FromDouble(0.5); + T thresholdValue = threshold ?? _numOps.FromDouble(0.5); for (int i = 0; i < predictions.Length; i++) { - if (_numOps.GreaterThanOrEquals(predictions[i], threshold)) + if (_numOps.GreaterThanOrEquals(predictions[i], thresholdValue)) { positiveCount++; } } return _numOps.Divide(_numOps.FromDouble(positiveCount), _numOps.FromDouble(predictions.Length)); }Apply similar changes to the other three methods. This maintains backward compatibility (default 0.5) while enabling customization.
Also applies to: 172-172, 221-221, 270-270
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
src/FeatureSelectors/SelectFromModel.cs(2 hunks)src/Genetics/ModelIndividual.cs(1 hunks)src/Helpers/FeatureSelectorHelper.cs(2 hunks)src/Interpretability/BiasDetectorBase.cs(1 hunks)src/Interpretability/DisparateImpactBiasDetector.cs(2 hunks)src/Interpretability/FairnessEvaluatorBase.cs(1 hunks)src/Interpretability/InterpretabilityMetricsHelper.cs(4 hunks)src/MetaLearning/Trainers/MetaLearnerBase.cs(1 hunks)src/NeuralNetworks/Layers/PatchEmbeddingLayer.cs(2 hunks)src/Optimizers/LionOptimizer.cs(7 hunks)tests/AiDotNet.Tests/UnitTests/Interpretability/FairnessEvaluatorTests.cs(1 hunks)tests/AiDotNet.Tests/UnitTests/MetaLearning/Helpers/SimpleMockModel.cs(1 hunks)tests/AiDotNet.Tests/UnitTests/Optimizers/LionOptimizerTests.cs(3 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-12-18T08:49:25.295Z
Learnt from: ooples
Repo: ooples/AiDotNet PR: 444
File: src/Interfaces/IPruningMask.cs:1-102
Timestamp: 2025-12-18T08:49:25.295Z
Learning: In the AiDotNet repository, the project-level global using includes AiDotNet.Tensors.LinearAlgebra via AiDotNet.csproj. Therefore, Vector<T>, Matrix<T>, and Tensor<T> are available without per-file using directives. Do not flag missing using directives for these types in any C# files within this project. Apply this guideline broadly to all C# files (not just a single file) to avoid false positives. If a file uses a type from a different namespace not covered by the global using, flag as usual.
Applied to files:
tests/AiDotNet.Tests/UnitTests/Optimizers/LionOptimizerTests.cssrc/MetaLearning/Trainers/MetaLearnerBase.cssrc/Interpretability/FairnessEvaluatorBase.cssrc/FeatureSelectors/SelectFromModel.cssrc/Interpretability/BiasDetectorBase.cssrc/NeuralNetworks/Layers/PatchEmbeddingLayer.cssrc/Interpretability/InterpretabilityMetricsHelper.cstests/AiDotNet.Tests/UnitTests/MetaLearning/Helpers/SimpleMockModel.cstests/AiDotNet.Tests/UnitTests/Interpretability/FairnessEvaluatorTests.cssrc/Optimizers/LionOptimizer.cssrc/Genetics/ModelIndividual.cssrc/Helpers/FeatureSelectorHelper.cssrc/Interpretability/DisparateImpactBiasDetector.cs
📚 Learning: 2025-12-18T08:49:53.103Z
Learnt from: ooples
Repo: ooples/AiDotNet PR: 444
File: src/Interfaces/IPruningStrategy.cs:1-4
Timestamp: 2025-12-18T08:49:53.103Z
Learning: In this repository, global using directives are declared in AiDotNet.csproj for core namespaces (AiDotNet.Tensors.* and AiDotNet.*) and common system types. When reviewing C# files, assume these global usings are in effect; avoid adding duplicate using statements for these namespaces and for types like Vector<T>, Matrix<T>, Tensor<T>, etc. If a type is not found, verify the global usings or consider adding a file-scoped using if needed. Prefer relying on global usings to reduce boilerplate.
Applied to files:
tests/AiDotNet.Tests/UnitTests/Optimizers/LionOptimizerTests.cssrc/MetaLearning/Trainers/MetaLearnerBase.cssrc/Interpretability/FairnessEvaluatorBase.cssrc/FeatureSelectors/SelectFromModel.cssrc/Interpretability/BiasDetectorBase.cssrc/NeuralNetworks/Layers/PatchEmbeddingLayer.cssrc/Interpretability/InterpretabilityMetricsHelper.cstests/AiDotNet.Tests/UnitTests/MetaLearning/Helpers/SimpleMockModel.cstests/AiDotNet.Tests/UnitTests/Interpretability/FairnessEvaluatorTests.cssrc/Optimizers/LionOptimizer.cssrc/Genetics/ModelIndividual.cssrc/Helpers/FeatureSelectorHelper.cssrc/Interpretability/DisparateImpactBiasDetector.cs
🧬 Code graph analysis (2)
tests/AiDotNet.Tests/UnitTests/Optimizers/LionOptimizerTests.cs (1)
src/Optimizers/LionOptimizer.cs (1)
Vector(265-311)
src/FeatureSelectors/SelectFromModel.cs (1)
src/FeatureSelectors/SequentialFeatureSelector.cs (1)
T(310-342)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build All Frameworks
🔇 Additional comments (14)
src/Interpretability/BiasDetectorBase.cs (2)
106-107: LGTM! Improved error message clarity.The updated error message is more explicit and includes actual length values, which will help with debugging. This is a good improvement over a generic "same length" message.
110-111: LGTM! Consistent and informative error messaging.This error message follows the same clear pattern as the previous validation message, providing specific details about the length mismatch. Both changes enhance the developer experience when debugging validation failures.
src/NeuralNetworks/Layers/PatchEmbeddingLayer.cs (2)
102-105: LGTM! ParameterCount property correctly implemented.The calculation accurately sums projection weights and bias parameters, and it matches the logic in
GetParameters()at line 429. This property provides a convenient API for parameter introspection, which is valuable for model analysis and the uncertainty quantification features added in this PR.
138-141: LGTM! Output shape now correctly initialized.The base constructor now receives the accurate output shape
[num_patches, embedding_dim], which aligns with the actualForward()output at line 245. This change improves the layer's metadata accuracy and supports proper shape inference for downstream components.src/FeatureSelectors/SelectFromModel.cs (1)
244-250: LGTM! Tolerance mechanism handles floating-point precision correctly.The implementation correctly addresses floating-point comparison issues by computing an
effectiveThresholdthat's slightly lower than the original threshold. This ensures features whose importance is numerically very close to the threshold (within type-specific epsilon) are included, preventing spurious exclusions due to rounding errors.The logic is sound:
- Tolerance is subtracted from threshold (not added), making the filter more inclusive
- Type-specific epsilon values (1e-6 for float, 1e-12 for double) are standard and appropriate
- Top-K selection path remains unaffected, as intended
src/Interpretability/DisparateImpactBiasDetector.cs (1)
78-78: LGTM!The
StatisticalParityDifferencecomputation is correct—it represents the range of positive prediction rates across groups, which is the standard definition.tests/AiDotNet.Tests/UnitTests/Interpretability/FairnessEvaluatorTests.cs (1)
133-133: LGTM! The test data change improves test coverage.The change from predicting 0 to predicting 1 at index 6 creates a false positive for group 1, which introduces disparity in fairness metrics and ensures the test validates that
EqualOpportunity,EqualizedOdds, andPredictiveParityare computed correctly.src/Optimizers/LionOptimizer.cs (3)
216-216: Critical bug fix correctly implemented.The previous
GetEffectiveLearningRate()method incorrectly scaled the learning rate by(1 - beta1) * (1 - beta2), which reduced the effective learning rate by ~1000× with typical Lion hyperparameters. This change correctly uses_currentLearningRatedirectly, aligning with the standard Lion algorithm specification.Also applies to: 240-240
276-276: Consistent fix applied to Vector UpdateParameters.Same correct pattern applied here—using the raw learning rate without momentum-based scaling.
Also applies to: 300-300
335-335: Consistent fix applied to Matrix UpdateParameters.Same correct pattern applied to the matrix variant, ensuring all update paths use the proper learning rate.
Also applies to: 364-364
tests/AiDotNet.Tests/UnitTests/Optimizers/LionOptimizerTests.cs (2)
410-454: Test correctly exercises Beta1's effect on momentum interpolation.The refactored test builds momentum with a large positive gradient, then applies a small negative gradient. With Beta1=0.2, more weight is on the current gradient (0.8), so the interpolation trends negative. With Beta1=0.95, more weight is on the accumulated momentum (0.95), so the interpolation stays positive. This properly validates that different Beta1 values produce different update directions via the sign-based mechanism.
456-499: Test correctly validates Beta2's influence on momentum accumulation.The test demonstrates that Beta2 controls how momentum (
m_t) is updated:m_t = beta2 * m_{t-1} + (1 - beta2) * g_t. With Beta2=0.5, momentum adapts quickly to the new negative gradient. With Beta2=0.999, momentum retains more of the previous positive value. This affects the subsequent interpolation (via Beta1) and produces different update behavior.One minor observation: Line 486 uses
Assert.NotEqual(params1, params2)which may rely on Vector's default equality semantics. The subsequent manual loop check (lines 489-498) provides a more reliable verification.src/Genetics/ModelIndividual.cs (1)
51-51: Critical bug fix: genes now properly initialized.Previously, this constructor ignored the
genesparameter and left_genesas an empty list. This would causeGetGenes()to return an empty collection and breakClone()(which iterates_genes), even though the model itself was correctly created with the provided genes.The fix correctly copies the input genes using collection expression syntax, ensuring consistency between the stored genes and the model state.
tests/AiDotNet.Tests/UnitTests/MetaLearning/Helpers/SimpleMockModel.cs (1)
61-65: The output shape and OutputFeatureCount are consistent and well-documented in the code. The comment at lines 62-63 already explains that the method returns "one prediction per sample" in a 1D tensor with shape [batchSize], which aligns with OutputFeatureCount=1 following standard ML conventions. No clarification or changes are needed.
github-actions
Bot
changed the title
This commit fixes one or more commit messages that didn't follow conventional commits format. The original commits have been squashed and their messages updated to comply with the project's standards. Original issues fixed: - Subject line now follows lowercase convention - Type now uses valid conventional commit types Co-Authored-By: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
|
🤖 PR Title Auto-Fixed Your PR title was automatically updated to follow Conventional Commits format. Original title: New title: Detected type: Valid types and their effects:
If the detected type is incorrect, you can manually edit the PR title. |
🤖 Commit Messages Auto-FixedThe commitlint check failed because one or more commit messages didn't follow Conventional Commits format. Action taken — All non-compliant commits have been fixed to follow the conventional commits format. Changes made:
The PR branch has been force-pushed with the fixed commits. If you had local changes, you may need to |
3808b8c to
84f83e0
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
docs/UncertaintyQuantification/PR431_GAP_ANALYSIS_AND_IMPLEMENTATION_PLAN.md (2)
228-233: Improve readability: vary sentence starters in routing policy.Lines 229, 232, and 233 all begin with "Else" or "Else if", creating repetitive rhythm. Consider restructuring for clarity:
1. If user provided a conformal/calibration configuration and a calibration set exists → use conformal for intervals/sets. 2. Otherwise, if model implements an uncertainty-capable interface → use it. 3. Otherwise, if neural network with MC Dropout layers → MC Dropout. 4. Fallback to bootstrap ensemble.This variant avoids repetition while maintaining the decision-tree structure.
216-225: Clarify thread-safety guarantees and performance trade-offs for cloning strategy.Section 4.2 proposes cloning the model for thread-safe concurrent inference, but the document defers performance optimization to Phase 8 (streaming aggregation, allocation reduction). Recommend adding a brief note here:
- Cloning ensures safety but may introduce measurable overhead per call.
- Phase 8 will optimize the sampling loop and memory usage.
- Alternatively, consider whether a per-thread lock (with documented throughput implications) is preferable to cloning for high-concurrency serving.
This helps architects understand the upfront cost of the thread-safety choice.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
docs/UncertaintyQuantification/PR431_GAP_ANALYSIS_AND_IMPLEMENTATION_PLAN.md(1 hunks)docs/UncertaintyQuantification/PR431_TODO.md(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- docs/UncertaintyQuantification/PR431_TODO.md
🧰 Additional context used
🪛 LanguageTool
docs/UncertaintyQuantification/PR431_GAP_ANALYSIS_AND_IMPLEMENTATION_PLAN.md
[style] ~233-~233: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...an be injected safely) → MC Dropout. 4. Else fallback → bootstrap ensemble (generic ...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Codacy Security Scan
- GitHub Check: CodeQL Analysis
- GitHub Check: SonarCloud Analysis
🔇 Additional comments (3)
docs/UncertaintyQuantification/PR431_GAP_ANALYSIS_AND_IMPLEMENTATION_PLAN.md (3)
56-123: Gap analysis is thorough and directly addresses PR #418 requirements.Sections 2.1–2.6 comprehensively identify gaps across facade routing, model coverage, conformal/calibration integration, Bayesian training, and documentation. Each gap is concrete and traced through the implementation plan, reducing ambiguity for handoff to engineering.
235-396: Phased implementation plan clearly maps gaps to concrete deliverables and acceptance criteria.Phases 0–8 provide explicit goals, tasks, tests, and acceptance criteria. Phase 1 (thread-safety foundation) correctly prioritizes concurrency, and the progression from strategy routing → model-agnostic baselines → specialized methods (ensembles, conformal, Bayesian) is sound. Phase 8 (performance) appropriately defers allocation/memory optimization after correctness is proven.
124-198: User-facing output design strikes reasonable balance between richness and API surface.Proposing
UncertaintyPredictionResult<TOutput>withPrediction,Variance,StdDev, andMetrics(dict) avoids API explosion while supporting classification entropy, mutual information, and conformal outputs. Requirement to include metrics keys even when not computable (setting to 0 by default) ensures consistent downstream handling.Backward-compatibility path (section 3.2) appropriately leaves the tuple-return decision to stakeholder review.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
CodeQL found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
|
This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation. |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (10)
src/Evaluation/DefaultModelEvaluator.cs (1)
246-269: Broad exception catch inCalculateModelStatsmay hide useful diagnosticsWrapping the entire stats construction in
try/catch (Exception)and returningModelStats.Empty()is safe but opaque. If this path ever triggers in production, users get no hint why model‑level stats are missing.Consider at least logging the exception (or attaching a reason field inside
ModelStatsif available) before returningEmpty(), so failures inPredictionModelResultconstruction orInputHelper.GetInputSizeare debuggable.src/PredictionModelBuilder.cs (3)
17-19: Partial class + new UQ fields are wired consistently into the supervised build pathMaking
PredictionModelBuilderpartial and adding_uncertaintyQuantificationOptions/_uncertaintyCalibrationDatafields is coherent with the newPredictionModelBuilder.UncertaintyQuantificationpartial. The supervisedBuildAsyncpath correctly:
- Trains the model,
- Constructs
PredictionModelResult,- Then applies UQ options and calibration/deep‑ensemble artifacts.
Note that the meta‑learning and RL build paths intentionally do not propagate these UQ options; if you ever want UQ for those modes, you’ll need parallel wiring there or explicit documentation that UQ is “supervised‑only”.
Also applies to: 55-56, 117-119, 671-712
945-971: Deep ensemble training behavior and optimizer choiceThe deep‑ensemble integration looks sound: you preserve the primary
BestSolution, derive a template from_model.DeepCopy()(or architecture for neural nets), then:
- Perturb initial parameters per member,
- Optionally bootstrap training data per member,
- Train each member independently with fresh optimizer instances.
Two behavioral notes to be aware of:
Ensemble members are always trained with optimizers constructed from the base
optimizer, notfinalOptimizer. In distributed‑training scenarios this means ensemble members won’t be sharded/parallelized. That’s acceptable but might surprise users training very large ensembles on multi‑GPU setups.Knowledge‑distillation builds still use the standard (non‑distillation) optimization data for deep‑ensemble members. If the intent is “KD for the main model, classic supervised training for ensemble members”, this is fine; otherwise, mirroring the KD data path per member would be more consistent.
If these behaviors are intentional, consider a brief XML doc note on
DeepEnsemblein the options to set expectations.Also applies to: 973-974, 1061-1066
3313-3703: MC Dropout injection and ensemble optimizer factory look correct but are strictThe new helper cluster is generally well‑implemented:
TryInjectMonteCarloDropoutLayers:
- Skips injection if any
MCDropoutLayer<T>already exists.- Enforces
0 < MonteCarloDropoutRate < 1with a thrownArgumentException.- Injects dropout immediately after activation layers and avoids touching the final layer set.
- Logs a debug warning when no suitable layers are found.
CreateOptimizerForEnsembleMember:
- Prefers
(model, options)then(model)constructors via reflection.- Special‑cases
NormalOptimizercreation.- Throws a clear
InvalidOperationExceptioninstead of silently falling back when a custom optimizer cannot be re‑instantiated.This strictness is good for surfacing misconfigurations early. Just be aware that enabling Monte Carlo Dropout on a non‑NeuralNetworkModel will now fail fast with an
InvalidOperationException, which is probably what you want given the current docs.src/PredictionModelBuilder.UncertaintyQuantification.cs (2)
70-125: Calibration artifact gating is selective; confirm intended Method restrictions
TryComputeAndAttachUncertaintyCalibrationArtifactsonly runs when:
- UQ is enabled, and
MethodisAuto,ConformalPrediction,LaplaceApproximation, orSwag.That means if a user explicitly selects
MonteCarloDropout,DeepEnsemble, orBayesianNeuralNetwork, they won’t get ECE/temperature/Platt/isotonic/conformal artifacts, even though the calibration code itself is method‑agnostic over probability outputs.If the goal is “calibration + conformal + posterior artifacts are available for any probability‑producing classifier once calibration data is supplied”, you may want to decouple this gating from
Methodand instead drive it purely off:
- Presence of calibration data (
HasLabels/HasTargets),- Per‑feature flags (
EnableTemperatureScaling,EnablePlattScaling, conformal mode, Laplace/SWAG flags).Otherwise, a short remark in XML docs for
UncertaintyQuantificationMethodclarifying that some methods skip calibration artifacts would help avoid surprises.
794-912: Laplace and SWAG posterior fitting are defensively implemented but constrained to Tensor buildersThe Laplace and SWAG routines:
- Require
TInput/TOutputto beTensor<T>and log a clear debug warning otherwise viaTryPreparePosteriorCalibrationTensors*andTryCastTensorInputs.- Use
PosteriorFitMaxSamplesto cap calibration cost.- Catch failures in
ComputeGradients/Clone()and emit debug warnings instead of throwing, which is appropriate for an optional UQ add‑on.- For Laplace, build a diagonal Hessian as
priorPrecision + g²and invert it to get per‑parameter variance.- For SWAG, maintain running means and squared‑means over snapshots to derive diagonal variances, with configurable burn‑in, steps, and snapshot count.
Beyond the regression batch‑alignment nit above, this is a sensible “best‑effort” implementation that degrades gracefully when the model doesn’t support gradients/clone. The Tensor‑only constraint is reasonable given the current API surface, but should be documented in the UQ options so callers know when Laplace/SWAG will be a no‑op.
Also applies to: 914-1045, 1046-1142, 1144-1215
src/Models/Options/UncertaintyQuantificationOptions.cs (1)
150-150: Validate SwagBurnInSteps ≤ SwagNumSteps.The SWAG burn-in phase should not exceed the total number of steps, but no validation enforces
SwagBurnInSteps ≤ SwagNumSteps. This could cause incorrect behavior during posterior fitting.Consider adding a validation method or enforcing the constraint in setters:
public void Validate() { if (SwagBurnInSteps > SwagNumSteps) { throw new InvalidOperationException("SwagBurnInSteps must not exceed SwagNumSteps."); } // ... other validations }Also applies to: 155-155
src/Models/Results/PredictionModelResult.Uncertainty.cs (3)
184-233: Clarify behavior when MonteCarloDropout is selected but no MC layers exist.When
Automethod selection falls through toMonteCarloDropout(line 186) but no MC dropout layers are present (lines 224-233), the method returns a deterministic prediction with zero variance. This silent fallback could be confusing to users who expect uncertainty estimates.Consider logging a warning or adjusting the auto-selection logic to detect the absence of MC layers earlier:
else { var mcDropoutLayers = GetMonteCarloDropoutLayers(Model); if (mcDropoutLayers.Count > 0) { method = UncertaintyQuantificationMethod.MonteCarloDropout; } else { // Log warning: "No uncertainty method available, returning deterministic prediction" method = UncertaintyQuantificationMethod.Auto; // or a sentinel value } }This would make the fallback behavior more explicit and predictable.
634-714: Eliminate code duplication in BuildPredictionSets overloads.The two
BuildPredictionSetsoverloads (lines 634-671 and 673-714) contain nearly identical logic, differing only in threshold retrieval. The fallback logic for empty prediction sets (lines 651-665 and 694-708) is duplicated.🔎 Proposed refactor
Extract the core logic into a shared helper:
private static int[][] BuildPredictionSets(Vector<T> probsFlat, int batch, int classes, T threshold) { return BuildPredictionSetsCore(probsFlat, batch, classes, b => threshold); } private static int[][] BuildPredictionSets(Vector<T> probsFlat, int batch, int classes, Vector<T> thresholdsPerSample) { return BuildPredictionSetsCore(probsFlat, batch, classes, b => b < thresholdsPerSample.Length ? thresholdsPerSample[b] : thresholdsPerSample[thresholdsPerSample.Length - 1]); } private static int[][] BuildPredictionSetsCore(Vector<T> probsFlat, int batch, int classes, Func<int, T> getThreshold) { var numOps = MathHelper.GetNumericOperations<T>(); var sets = new int[batch][]; for (int b = 0; b < batch; b++) { var threshold = getThreshold(b); var indices = new List<int>(); var baseIndex = b * classes; for (int c = 0; c < classes; c++) { if (numOps.GreaterThanOrEquals(probsFlat[baseIndex + c], threshold)) { indices.Add(c); } } if (indices.Count == 0) { // Fallback to best class (extracted once) indices.Add(FindBestClass(probsFlat, baseIndex, classes, numOps)); } sets[b] = indices.ToArray(); } return sets; } private static int FindBestClass(Vector<T> probsFlat, int baseIndex, int classes, INumericOperations<T> numOps) { var best = 0; var bestProb = probsFlat[baseIndex]; for (int c = 1; c < classes; c++) { var p = probsFlat[baseIndex + c]; if (numOps.GreaterThan(p, bestProb)) { bestProb = p; best = c; } } return best; }
904-922: Consider binary search for isotonic calibration lookup.The isotonic calibration evaluation uses linear search (O(n)) through the calibration points. For large calibration sets, binary search would be more efficient (O(log n)).
🔎 Proposed optimization
private static T EvaluateIsotonic(T p, Vector<T> x, Vector<T> y, INumericOperations<T> numOps) { if (x.Length == 0) { return p; } var spanX = x.AsSpan(); var spanY = y.AsSpan(); // Binary search for the first x[i] >= p int left = 0, right = spanX.Length - 1; while (left < right) { int mid = left + (right - left) / 2; if (numOps.LessThan(spanX[mid], p)) { left = mid + 1; } else { right = mid; } } return spanY[left]; }This is optional and only beneficial if calibration sets are large (e.g., > 100 points).
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
src/Enums/ConformalPredictionMode.cs(1 hunks)src/Enums/ProbabilityCalibrationMethod.cs(1 hunks)src/Enums/UncertaintyQuantificationMethod.cs(1 hunks)src/Evaluation/DefaultModelEvaluator.cs(2 hunks)src/Models/NeuralNetworkModel.cs(1 hunks)src/Models/Options/UncertaintyQuantificationOptions.cs(1 hunks)src/Models/Results/PredictionModelResult.Uncertainty.cs(1 hunks)src/Models/Results/UncertaintyCalibrationArtifacts.cs(1 hunks)src/NeuralNetworks/GraphSAGENetwork.cs(4 hunks)src/PredictionModelBuilder.UncertaintyQuantification.cs(1 hunks)src/PredictionModelBuilder.cs(20 hunks)src/TimeSeries/STLDecomposition.cs(1 hunks)tests/AiDotNet.Tests/IntegrationTests/UncertaintyQuantificationFacadeTests.cs(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- tests/AiDotNet.Tests/IntegrationTests/UncertaintyQuantificationFacadeTests.cs
🚧 Files skipped from review as they are similar to previous changes (4)
- src/TimeSeries/STLDecomposition.cs
- src/Enums/ConformalPredictionMode.cs
- src/Enums/ProbabilityCalibrationMethod.cs
- src/Models/NeuralNetworkModel.cs
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-12-18T08:49:25.295Z
Learnt from: ooples
Repo: ooples/AiDotNet PR: 444
File: src/Interfaces/IPruningMask.cs:1-102
Timestamp: 2025-12-18T08:49:25.295Z
Learning: In the AiDotNet repository, the project-level global using includes AiDotNet.Tensors.LinearAlgebra via AiDotNet.csproj. Therefore, Vector<T>, Matrix<T>, and Tensor<T> are available without per-file using directives. Do not flag missing using directives for these types in any C# files within this project. Apply this guideline broadly to all C# files (not just a single file) to avoid false positives. If a file uses a type from a different namespace not covered by the global using, flag as usual.
Applied to files:
src/Evaluation/DefaultModelEvaluator.cssrc/Enums/UncertaintyQuantificationMethod.cssrc/NeuralNetworks/GraphSAGENetwork.cssrc/Models/Results/UncertaintyCalibrationArtifacts.cssrc/PredictionModelBuilder.UncertaintyQuantification.cssrc/PredictionModelBuilder.cssrc/Models/Options/UncertaintyQuantificationOptions.cssrc/Models/Results/PredictionModelResult.Uncertainty.cs
📚 Learning: 2025-12-18T08:49:53.103Z
Learnt from: ooples
Repo: ooples/AiDotNet PR: 444
File: src/Interfaces/IPruningStrategy.cs:1-4
Timestamp: 2025-12-18T08:49:53.103Z
Learning: In this repository, global using directives are declared in AiDotNet.csproj for core namespaces (AiDotNet.Tensors.* and AiDotNet.*) and common system types. When reviewing C# files, assume these global usings are in effect; avoid adding duplicate using statements for these namespaces and for types like Vector<T>, Matrix<T>, Tensor<T>, etc. If a type is not found, verify the global usings or consider adding a file-scoped using if needed. Prefer relying on global usings to reduce boilerplate.
Applied to files:
src/Evaluation/DefaultModelEvaluator.cssrc/Enums/UncertaintyQuantificationMethod.cssrc/NeuralNetworks/GraphSAGENetwork.cssrc/Models/Results/UncertaintyCalibrationArtifacts.cssrc/PredictionModelBuilder.UncertaintyQuantification.cssrc/PredictionModelBuilder.cssrc/Models/Options/UncertaintyQuantificationOptions.cssrc/Models/Results/PredictionModelResult.Uncertainty.cs
📚 Learning: 2025-12-19T19:05:13.598Z
Learnt from: ooples
Repo: ooples/AiDotNet PR: 445
File: src/Interfaces/IPredictionModelBuilder.cs:7-8
Timestamp: 2025-12-19T19:05:13.598Z
Learning: In ooples/AiDotNet, IAggregationStrategy<TModel> is declared in AiDotNet.Interfaces (src/Interfaces/IAggregationStrategy.cs). For src/Interfaces/IPredictionModelBuilder.cs (also in AiDotNet.Interfaces), referencing IAggregationStrategy does not require an additional using. Avoid flagging missing using directives for this case in future reviews.
Applied to files:
src/PredictionModelBuilder.cssrc/Models/Results/PredictionModelResult.Uncertainty.cs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: SonarCloud Analysis
- GitHub Check: CodeQL Analysis
🔇 Additional comments (15)
src/NeuralNetworks/GraphSAGENetwork.cs (2)
6-6: LGTM: Required using directive for RandomHelper.The new using directive correctly provides access to the
RandomHelperclass needed for the seeded random number generator at line 399.
399-399: Excellent: Reproducibility issue resolved.These changes successfully address the reproducibility inconsistency flagged in the previous review. The implementation now ensures deterministic neighbor sampling by:
- Creating a seeded RNG once per training run (line 399)
- Using
RandomHelper.CreateSeededRandomfor robust seeded generation- Propagating the same random instance to
SampleSubgraph(line 413)- Accepting and using the injected random parameter in neighbor sampling (lines 453-454)
This eliminates the previous dependency on the global
ThreadSafeRandomsingleton and provides end-to-end reproducibility for UQ experiments.Also applies to: 413-413, 453-454
src/Enums/UncertaintyQuantificationMethod.cs (1)
3-48: Enum surface and documentation look solidThe enum values and XML docs cleanly cover the intended UQ strategies (dropout, ensembles, BNN, Laplace, SWAG, conformal) without over‑exposing implementation detail. No changes needed here.
src/Evaluation/DefaultModelEvaluator.cs (2)
97-112: Length‑mismatch handling for error/prediction stats is appropriateGuarding
ErrorStatsandPredictionStatsbehindalignedwhile still computing basic stats and preserving raw predictions/actuals is a reasonable way to avoid hard failures on length mismatches. No issues here.
114-159: UncertaintyStats population is safe and minimally invasiveRestricting uncertainty aggregation to
PredictionModelResultwith UQ enabled, and usingTryGetValueplus a safeMeanOf(zero on empty tensors) avoids exceptions while filling only metrics that are actually present. This is a good balance between robustness and observability.src/Models/Results/UncertaintyCalibrationArtifacts.cs (1)
5-45: Internal calibration artifact container is well‑structuredThe flag + payload pattern (e.g.,
HasLaplacePosterior/LaplacePosteriorMean) keeps this internal DTO clear and safe to use, anddefault!on guardedTproperties is appropriate. No changes needed.src/PredictionModelBuilder.UncertaintyQuantification.cs (3)
17-55:ConfigureUncertaintyQuantificationAPI and defaults are reasonableThe builder method cleanly:
- Enables UQ with sensible defaults when
optionsis null.- Honors a caller‑supplied
Enabled = falseto keep UQ disabled.- Stores optional calibration data for later artifact computation.
This matches the fluent style used elsewhere and keeps the public surface focused on a single, composable configuration call.
134-188: Conformal regression/classification quantiles and label handling look mathematically consistent
- Regression:
- Residuals are
|y - ŷ|.- Quantile index uses
Ceil((n + 1) * confidence) / nthenCeil(n * adjusted) - 1, matching standard split‑conformal coverage correction.- Classification:
- Scores are per‑sample true‑label probabilities.
- Threshold quantile uses
(1 - confidenceLevel)so low‑score tails define the cutoff, which is the expected behavior for prediction‑set construction.- The updated handling of invalid labels (skip with debug warning, keep processing, and bail if
validCount == 0) avoids the previous “early abort” trap.This all aligns with the usual conformal formulations; no issues beyond the Method‑level gating mentioned separately.
Also applies to: 190-343
345-403: Temperature scaling, Platt, and isotonic calibration implementations are robustHighlights:
FitTemperatureFromProbabilities:
- Validates/repairs per‑sample probability vectors (clamps negatives, handles near‑zero sums with a uniform fallback, optional renormalization, epsilon floor before
Log).- Works fully in
TviaINumericOperations<T>, then hands clean logits to the temperature scaler.
ApplyTemperatureScalingToProbabilities:
- Recomputes probabilities via a numerically stable softmax (max‑logit subtraction) with a floor epsilon to avoid
Log(0).- Explicitly renormalizes each row to ensure valid distributions.
Platt scaling and isotonic regression:
- Operate in
doublefor the calibration step, returning parameters/vectors cast back toT.- Use standard Newton updates with a regularized Hessian for Platt and PAV for isotonic, which is appropriate.
The defensive clamping/renormalization is important here and looks correct.
Also applies to: 413-462, 489-665
src/Models/Options/UncertaintyQuantificationOptions.cs (1)
5-16: Excellent documentation with beginner-friendly guidance.The XML documentation and beginner-friendly remarks provide clear context for each configuration option. This will help users understand the uncertainty quantification system.
src/Models/Results/PredictionModelResult.Uncertainty.cs (5)
329-429: Deep ensemble implementation looks correct.The deep ensemble prediction correctly:
- Normalizes input once
- Collects predictions from all ensemble members
- Applies probability calibration when appropriate
- Computes mean and variance
- Calculates entropy and mutual information for probability distributions
- Properly denormalizes outputs and variances
1220-1229: Excellent reproducibility design with seeded RNG.The deterministic RNG reset logic (lines 1220-1229) ensures reproducible Monte Carlo sequences when
RandomSeedis provided. The use of prime number offsets (10007, 1009) in the seed formula prevents seed collisions across samples and layers. The explanatory comment clearly documents the intent.
828-867: Platt scaling implementation is correct.The Platt scaling logic properly:
- Validates binary classification requirements
- Clips probabilities to prevent numerical issues with log transforms
- Applies the standard Platt scaling formula: sigmoid(a * logit + b)
- Computes complementary probabilities
1049-1094: Variance computation is correct for predictive uncertainty.The variance computation (lines 1049-1094) uses population variance (dividing by
samples.Countat line 1086) rather than sample variance (n-1). This is correct for estimating the variance of the predictive distribution from Monte Carlo samples, not for estimating the variance of the sample mean.
1162-1181: Probability sum tolerance is reasonable.The probability distribution validation (lines 1162-1181) uses a tolerance of 1e-2 (1%) for checking that probabilities sum to 1.0 (line 1163). This tolerance accommodates floating-point rounding errors while still catching major distribution errors. The choice appears appropriate for real-world usage with various numeric types.
|
This commit implements comprehensive uncertainty quantification capabilities for Phase 3, addressing all requirements specified in Issue #418.
Bayesian Neural Networks
Monte Carlo Dropout
Variational Inference (Bayes by Backprop)
Deep Ensembles
Uncertainty Types
All Bayesian approaches support:
Calibration Methods
Temperature Scaling
Expected Calibration Error (ECE)
Conformal Prediction
Split Conformal Predictor
Conformal Classifier
Testing
Comprehensive unit tests covering:
Resolves #418
User Story / Context
merge-dev2-to-masterSummary
Verification
Copilot Review Loop (Outcome-Based)
Record counts before/after your last push:
Files Modified
Notes