feat: add comprehensive model evaluation framework - #801
Merged
Merged
Conversation
This commit introduces a production-ready model evaluation framework that exceeds industry standards (scikit-learn, MLflow, W&B, TensorFlow Model Analysis). Key features: - 70+ regression and classification metrics with confidence intervals - 16 cross-validation strategies (K-Fold, Stratified, Time Series, Monte Carlo, etc.) - 10 statistical tests (DeLong, McNemar, Wilcoxon, Friedman, Kruskal-Wallis, etc.) - Specialized engines for calibration, fairness, robustness, and learning curves - All metrics use generic T with INumericOperations for type flexibility - Bootstrap-based confidence interval computation (BCa, percentile) - Comprehensive XML documentation with beginner-friendly remarks Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughAdds a comprehensive Evaluation subsystem: cross-validation abstractions and numerous CV strategies; multiple evaluation engines (cross‑validation, metrics, calibration, fairness, robustness, learning/validation curves, statistical tests); many metric implementations (classification, regression, time‑series); options/enums; and result/report types. All changes are additive public APIs. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User as Client
participant CV as CrossValidationEngine<T>
participant Strat as ICrossValidationStrategy<T>
participant Train as trainFunc
participant Model as TrainedModel
participant Predict as predictFunc
participant Metrics as MetricComputationEngine<T>
rect rgba(200, 200, 255, 0.5)
User->>CV: Execute(strategy, features, targets, trainFunc, predictFunc)
CV->>Strat: Split(dataSize)
loop For each fold
Strat-->>CV: (TrainIndices, ValidationIndices)
CV->>Train: trainFunc(features[train], targets[train])
Train-->>Model: model
CV->>Predict: predictFunc(model, features[val])
Predict-->>CV: predictions
CV->>Metrics: Compute(predictions, targets[val])
Metrics-->>CV: fold metrics
end
CV-->>User: CrossValidationResult<T>
end
sequenceDiagram
autonumber
actor User as Client
participant LCE as LearningCurveEngine<T>
participant CV as CrossValidationEngine<T>
participant Strat as KFoldStrategy<T>
participant Metrics as MetricComputationEngine<T>
rect rgba(200, 255, 200, 0.5)
User->>LCE: Generate(trainSizes, cvFolds, ...)
loop For each train size
LCE->>CV: Execute(Strat(k=cvFolds), subset(features, targets), trainFunc, predictFunc)
CV-->>LCE: CrossValidationResult
LCE->>Metrics: Aggregate fold metrics
Metrics-->>LCE: Means/Stds
end
LCE-->>User: LearningCurveResult<T>
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~180 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
🤖 Fix all issues with AI agents
In `@src/Evaluation/CrossValidation/SlidingWindowStrategy.cs`:
- Around line 45-55: The constructor SlidingWindowStrategy currently assigns
_step = step ?? testSize without validating step; add a guard that if the
optional parameter step.HasValue and step.Value < 1 then throw an
ArgumentException (e.g. "Step must be at least 1.") before assigning _step to
prevent infinite loops where trainStart doesn't advance. Ensure you still
default _step to testSize when step is null.
In `@src/Evaluation/Options/ModelComparisonOptions.cs`:
- Line 155: The enum declaration name contains a hidden soft hyphen (U+00AD) —
`MultipleTestingCorrectionMethod` — which must be replaced with the correct
identifier `MultipleTestingCorrectionMethod`; update the enum declaration and
all references/usages (e.g., any switch, variable, property, or parameter types
that use MultipleTestingCorrectionMethod) to use the cleaned name, ensuring you
only remove the hidden character and preserve casing so the symbol is consistent
across the codebase.
🟠 Major comments (51)
src/Evaluation/Metrics/Classification/CohensKappaMetric.cs-86-93 (1)
86-93:⚠️ Potential issue | 🟠 Major
ciMethodparameter is ignored; implementation doesn't match stated default.The method accepts
ciMethodwith defaultBCaBootstrap, but the actual computation always uses percentile bootstrap (viaBootstrapCI). BCa (Bias-Corrected and accelerated) bootstrap requires additional bias correction and acceleration factor calculations that are not present.This leads to misleading behavior: users requesting different CI methods will all receive percentile bootstrap results, and the default claims BCa but delivers percentile.
🛠️ Suggested approaches
Option A: Rename/document the default to
Percentileif BCa isn't needed:public MetricWithCI<T> ComputeWithCI(ReadOnlySpan<T> predictions, ReadOnlySpan<T> actuals, - ConfidenceIntervalMethod ciMethod = ConfidenceIntervalMethod.BCaBootstrap, + ConfidenceIntervalMethod ciMethod = ConfidenceIntervalMethod.Percentile, double confidenceLevel = 0.95, int bootstrapSamples = 1000, int? randomSeed = null)Option B: Implement proper method dispatch if multiple CI methods are required:
var (lower, upper) = ciMethod switch { ConfidenceIntervalMethod.Percentile => PercentileBootstrapCI(...), ConfidenceIntervalMethod.BCaBootstrap => BCaBootstrapCI(...), _ => throw new NotSupportedException($"CI method {ciMethod} not supported.") };src/Evaluation/CrossValidation/ICrossValidationStrategy.cs-21-77 (1)
21-77: 🛠️ Refactor suggestion | 🟠 MajorAlign interface/type placement with the project’s architectural requirements.
Line 21-77 places
ICrossValidationStrategy<T>andCVFold<T>in the same file undersrc/Evaluation/CrossValidation/. The PR objectives call for interfaces insrc/Interfaces/and one type per file. Consider movingICrossValidationStrategy<T>tosrc/Interfaces(namespaceAiDotNet.Interfaces) and splittingCVFold<T>into its own file to match the required layout.src/Evaluation/Metrics/Classification/HammingLossMetric.cs-53-79 (1)
53-79:⚠️ Potential issue | 🟠 Major
ComputeWithCIignoresciMethodand lacks bootstrap input validation.Line 53-59 always uses percentile BootstrapCI regardless of
ciMethod(default BCa), so the reported method can be incorrect. Also,bootstrapSamples <= 1produces negative indices in Line 77-79. Please validate parameters and either implement or guard unsupported CI methods.🛡️ Suggested input validation
public MetricWithCI<T> ComputeWithCI(ReadOnlySpan<T> predictions, ReadOnlySpan<T> actuals, ConfidenceIntervalMethod ciMethod = ConfidenceIntervalMethod.BCaBootstrap, double confidenceLevel = 0.95, int bootstrapSamples = 1000, int? randomSeed = null) { + if (bootstrapSamples < 2) + throw new ArgumentException("bootstrapSamples must be at least 2.", nameof(bootstrapSamples)); + if (confidenceLevel <= 0 || confidenceLevel >= 1) + throw new ArgumentException("confidenceLevel must be between 0 and 1 (exclusive).", nameof(confidenceLevel)); + var value = Compute(predictions, actuals); var (lower, upper) = BootstrapCI(predictions, actuals, bootstrapSamples, confidenceLevel, randomSeed); return new MetricWithCI<T>(value, lower, upper, confidenceLevel, ciMethod, Name, Direction); }src/Evaluation/Metrics/Classification/InformednessMetric.cs-59-85 (1)
59-85:⚠️ Potential issue | 🟠 MajorValidate CI inputs and align empty-input CI behavior.
Line 59-65 ignores
ciMethod(default BCa) and Line 83-85 can index out of range whenbootstrapSamples <= 1. Also, Line 71 returns (-1, 1) for empty input whileComputereturns 0; consider aligning CI bounds with the computed value or throwing on empty input.🛡️ Suggested validation + empty-input alignment
public MetricWithCI<T> ComputeWithCI(ReadOnlySpan<T> predictions, ReadOnlySpan<T> actuals, ConfidenceIntervalMethod ciMethod = ConfidenceIntervalMethod.BCaBootstrap, double confidenceLevel = 0.95, int bootstrapSamples = 1000, int? randomSeed = null) { + if (bootstrapSamples < 2) + throw new ArgumentException("bootstrapSamples must be at least 2.", nameof(bootstrapSamples)); + if (confidenceLevel <= 0 || confidenceLevel >= 1) + throw new ArgumentException("confidenceLevel must be between 0 and 1 (exclusive).", nameof(confidenceLevel)); + var value = Compute(predictions, actuals); var (lower, upper) = BootstrapCI(predictions, actuals, bootstrapSamples, confidenceLevel, randomSeed); return new MetricWithCI<T>(value, lower, upper, confidenceLevel, ciMethod, Name, Direction); } @@ private (T, T) BootstrapCI(ReadOnlySpan<T> pred, ReadOnlySpan<T> actual, int samples, double conf, int? seed) { int n = pred.Length; - if (n == 0) return (NumOps.FromDouble(-1), NumOps.One); + if (n == 0) return (NumOps.Zero, NumOps.Zero);src/Evaluation/CrossValidation/BootstrapStrategy.cs-63-87 (1)
63-87:⚠️ Potential issue | 🟠 MajorEnsure
Splityields the declaredNumSplits.Line 84-85 skips iterations with no OOB samples; for small datasets (e.g., dataSize=2) this can drop ~50% of folds, so
NumSplitsandDescriptionoverstate what gets produced and downstream aggregation may miscount. Consider resampling until an OOB set exists (or redefineNumSplitsto reflect actual yields).🔧 Suggested fix: resample until OOB exists
- for (int b = 0; b < _numBootstraps; b++) + int generated = 0; + while (generated < _numBootstraps) { // Sample with replacement for training var trainIndices = new int[dataSize]; var selectedSet = new HashSet<int>(); @@ // Skip if no OOB samples (rare but possible) if (oobIndices.Count == 0) continue; yield return (trainIndices, oobIndices.ToArray()); + generated++; }src/Evaluation/CrossValidation/PurgedKFoldStrategy.cs-93-102 (1)
93-102:⚠️ Potential issue | 🟠 MajorDon’t silently drop folds when the purge eliminates all training samples.
Skipping
yield returnreduces the number of folds and can break downstream assumptions. Prefer throwing a clear error (or adjust purgeGap upstream).🛠️ Proposed fix
- if (trainIndices.Count > 0) - yield return (trainIndices.ToArray(), testIndices); + if (trainIndices.Count == 0) + throw new InvalidOperationException("Purged K-Fold produced an empty training set; reduce purgeGap or increase dataSize."); + + yield return (trainIndices.ToArray(), testIndices);src/Evaluation/CrossValidation/PurgedKFoldStrategy.cs-59-66 (1)
59-66:⚠️ Potential issue | 🟠 MajorValidate
timeIndiceslength to prevent out-of-range access.
timeOrderis built from_timeIndices, but fold sizing usesdataSize. If_timeIndices.Length != dataSize, indexing intotimeOrdercan throw. Add a guard before sorting.🛠️ Proposed fix
if (_timeIndices != null) { + if (_timeIndices.Length != dataSize) + throw new ArgumentException("timeIndices length must match dataSize.", nameof(_timeIndices)); // Sort sample indices by their time values var indexedTimes = _timeIndices.Select((t, i) => (time: t, index: i)).OrderBy(x => x.time).ToArray(); timeOrder = indexedTimes.Select(x => x.index).ToArray(); }src/Evaluation/Metrics/Regression/SpearmanCorrelationMetric.cs-44-59 (1)
44-59:⚠️ Potential issue | 🟠 MajorSpearman formula is incorrect with ties — compute Pearson on ranks.
The shortcut
1 - 6Σd²/(n(n²−1))assumes no ties; with tied or constant ranks it can return misleading results (e.g., 1). SinceComputeRanksalready averages ties, finish with Pearson correlation on ranks and guard zero variance.🛠️ Proposed fix
- // Compute Spearman correlation using Pearson on ranks - double sumD2 = 0; - for (int i = 0; i < n; i++) - { - double d = predRanks[i] - actualRanks[i]; - sumD2 += d * d; - } - - // Formula: ρ = 1 - 6Σd²/(n(n²-1)) - double rho = 1 - (6 * sumD2) / (n * ((double)n * n - 1)); + // Compute Spearman correlation using Pearson on ranks + double meanPred = 0, meanActual = 0; + for (int i = 0; i < n; i++) + { + meanPred += predRanks[i]; + meanActual += actualRanks[i]; + } + meanPred /= n; + meanActual /= n; + + double cov = 0, varPred = 0, varActual = 0; + for (int i = 0; i < n; i++) + { + double dp = predRanks[i] - meanPred; + double da = actualRanks[i] - meanActual; + cov += dp * da; + varPred += dp * dp; + varActual += da * da; + } + + if (varPred < 1e-12 || varActual < 1e-12) + return NumOps.Zero; + + double rho = cov / Math.Sqrt(varPred * varActual);src/Evaluation/CrossValidation/StratifiedKFoldStrategy.cs-25-73 (1)
25-73:⚠️ Potential issue | 🟠 MajorRounding labels to
intcan merge distinct classes.
Math.Roundcollapses close labels and can mis-handle large integer values. Stratification should preserve exact class labels or explicitly validate integer labels.🛠️ Proposed fix (group by exact labels)
- private static readonly INumericOperations<T> NumOps = MathHelper.GetNumericOperations<T>(); private readonly int _k; private readonly bool _shuffle; private readonly int? _randomSeed; @@ - var classSamples = new Dictionary<int, List<int>>(); + var classSamples = new Dictionary<T, List<int>>(); for (int i = 0; i < dataSize; i++) { - int classLabel = (int)Math.Round(NumOps.ToDouble(labelsArray[i])); - if (!classSamples.ContainsKey(classLabel)) - classSamples[classLabel] = new List<int>(); - classSamples[classLabel].Add(i); + var classLabel = labelsArray[i]; + if (!classSamples.TryGetValue(classLabel, out var samples)) + { + samples = new List<int>(); + classSamples[classLabel] = samples; + } + samples.Add(i); }src/Evaluation/Metrics/Regression/RMSLEMetric.cs-55-81 (1)
55-81:⚠️ Potential issue | 🟠 MajorCI method is ignored and bootstrap parameters aren’t validated.
ciMethodis accepted but never applied, andbootstrapSamples/confidenceLevelcan produce invalid indices (e.g., samples ≤ 1). Add guards and fail fast for unsupported methods.🛠️ Proposed fix
public MetricWithCI<T> ComputeWithCI(ReadOnlySpan<T> predictions, ReadOnlySpan<T> actuals, ConfidenceIntervalMethod ciMethod = ConfidenceIntervalMethod.BCaBootstrap, double confidenceLevel = 0.95, int bootstrapSamples = 1000, int? randomSeed = null) { + if (ciMethod != ConfidenceIntervalMethod.BCaBootstrap) + throw new NotSupportedException($"CI method {ciMethod} is not supported for {Name}."); var value = Compute(predictions, actuals); var (lower, upper) = BootstrapCI(predictions, actuals, bootstrapSamples, confidenceLevel, randomSeed); return new MetricWithCI<T>(value, lower, upper, confidenceLevel, ciMethod, Name, Direction); } private (T, T) BootstrapCI(ReadOnlySpan<T> pred, ReadOnlySpan<T> actual, int samples, double conf, int? seed) { + if (samples < 2) + throw new ArgumentOutOfRangeException(nameof(samples), "Bootstrap samples must be at least 2."); + if (conf <= 0 || conf >= 1) + throw new ArgumentOutOfRangeException(nameof(conf), "Confidence level must be between 0 and 1."); int n = pred.Length; if (n == 0) return (NumOps.Zero, NumOps.Zero);src/Evaluation/Metrics/Classification/HingeLossMetric.cs-47-53 (1)
47-53:⚠️ Potential issue | 🟠 MajorBinarizing predictions defeats the purpose of hinge loss.
The implementation converts predictions to
{-1, +1}using a 0.5 threshold (Line 50), which means hinge loss can only ever be 0 or 2. Hinge loss is meant to capture margin violations using the raw decision function output (e.g., signed distance from hyperplane), not thresholded class labels.With this implementation:
y == yHat→max(0, 1 - 1) = 0y != yHat→max(0, 1 - (-1)) = 2🐛 Proposed fix to use raw prediction scores
- // If actual is 0/1 encoded, convert to -1/+1 double y = actual <= 0.5 ? -1.0 : 1.0; - // Predictions should ideally be in range [-1, 1] or decision function output - double yHat = pred <= 0.5 ? -1.0 : 1.0; - // Hinge loss: max(0, 1 - y * yHat) - totalLoss += Math.Max(0.0, 1.0 - y * yHat); + // Hinge loss uses raw prediction scores (decision function output) + // pred should be in range approximately [-inf, +inf] or [-1, 1] + totalLoss += Math.Max(0.0, 1.0 - y * pred);Alternatively, document that this metric expects raw decision function scores, not class probabilities.
src/Evaluation/Metrics/Classification/JaccardScoreMetric.cs-21-48 (1)
21-48:⚠️ Potential issue | 🟠 MajorSupportsMultiClass is true but the implementation is binary-only.
The 0.5 thresholding path only handles binary labels; multi-class callers will get incorrect results. Either implement a multi-class Jaccard (macro/micro) or mark it as binary-only.
🛠️ Minimal fix (mark as binary-only)
- public bool SupportsMultiClass => true; + public bool SupportsMultiClass => false;src/Evaluation/Metrics/Classification/MatthewsCorrelationCoefficientMetric.cs-225-270 (1)
225-270:⚠️ Potential issue | 🟠 MajorValidate bootstrapSamples > 0 to avoid index errors.
A non-positive bootstrapSamples produces a zero-length array and will throw at indexing time.
🛠️ Proposed fix
private (T lower, T upper) ComputeBootstrapCI( ReadOnlySpan<T> predictions, ReadOnlySpan<T> actuals, int bootstrapSamples, double confidenceLevel, int? randomSeed) { + if (bootstrapSamples <= 0) + { + throw new ArgumentOutOfRangeException(nameof(bootstrapSamples), "Must be positive."); + } int n = predictions.Length; if (n == 0) { return (NumOps.FromDouble(-1), NumOps.One); }src/Evaluation/Metrics/Classification/MatthewsCorrelationCoefficientMetric.cs-94-162 (1)
94-162:⚠️ Potential issue | 🟠 MajorPrevent unseen prediction labels from being dropped in multi-class MCC.
The class list is built from actuals only, so predictions containing labels not present in actuals are silently ignored, biasing the confusion matrix and MCC. Build the class set from both predictions and actuals and map indices once.
🛠️ Proposed fix
- var classes = new HashSet<double>(); - for (int i = 0; i < actuals.Length; i++) - { - classes.Add(NumOps.ToDouble(actuals[i])); - } + var classes = new HashSet<double>(); + for (int i = 0; i < actuals.Length; i++) + { + classes.Add(NumOps.ToDouble(actuals[i])); + classes.Add(NumOps.ToDouble(predictions[i])); + } - var classList = classes.ToList(); - int k = classList.Count; - var confusionMatrix = new int[k, k]; + var classList = classes.ToList(); + int k = classList.Count; + var classIndex = new Dictionary<double, int>(k); + for (int i = 0; i < k; i++) + { + classIndex[classList[i]] = i; + } + var confusionMatrix = new int[k, k]; - int predIdx = classList.IndexOf(NumOps.ToDouble(predictions[i])); - int actualIdx = classList.IndexOf(NumOps.ToDouble(actuals[i])); - - if (predIdx >= 0 && actualIdx >= 0) - { - confusionMatrix[actualIdx, predIdx]++; - } + int predIdx = classIndex[NumOps.ToDouble(predictions[i])]; + int actualIdx = classIndex[NumOps.ToDouble(actuals[i])]; + confusionMatrix[actualIdx, predIdx]++;src/Evaluation/Metrics/Classification/NPVMetric.cs-34-52 (1)
34-52:⚠️ Potential issue | 🟠 MajorAvoid returning 1 when
TN + FN == 0.
When there are no negative predictions, NPV is undefined; returning 1 overstates performance. Consider returning 0 (or a configurable/NaN outcome).🔧 Suggested fix
- return denominator == 0 ? NumOps.One : NumOps.FromDouble((double)tn / denominator); + return denominator == 0 ? NumOps.Zero : NumOps.FromDouble((double)tn / denominator);src/Evaluation/Metrics/Regression/LogCoshLossMetric.cs-75-101 (1)
75-101:⚠️ Potential issue | 🟠 MajorHonor
ciMethodand validate CI parameters.
ciMethodis ignored and invalidbootstrapSamples/confidenceLevelcan cause index errors. Consider guarding inputs and explicitly handling unsupported CI methods.🔧 Suggested fix
public MetricWithCI<T> ComputeWithCI(ReadOnlySpan<T> predictions, ReadOnlySpan<T> actuals, ConfidenceIntervalMethod ciMethod = ConfidenceIntervalMethod.BCaBootstrap, double confidenceLevel = 0.95, int bootstrapSamples = 1000, int? randomSeed = null) { + if (bootstrapSamples <= 0) + throw new ArgumentOutOfRangeException(nameof(bootstrapSamples), "Must be > 0."); + if (confidenceLevel <= 0 || confidenceLevel >= 1) + throw new ArgumentOutOfRangeException(nameof(confidenceLevel), "Must be in (0, 1)."); + if (ciMethod != ConfidenceIntervalMethod.BCaBootstrap) + throw new NotSupportedException($"CI method '{ciMethod}' is not implemented."); + var value = Compute(predictions, actuals); var (lower, upper) = BootstrapCI(predictions, actuals, bootstrapSamples, confidenceLevel, randomSeed); return new MetricWithCI<T>(value, lower, upper, confidenceLevel, ciMethod, Name, Direction); }src/Evaluation/Engines/ValidationCurveEngine.cs-145-163 (1)
145-163:⚠️ Potential issue | 🟠 MajorOptimal parameter selection assumes “higher is better.”
FindOptimalParameteralways maximizes validation score, which is incorrect for loss/error metrics (e.g., MSE/MAE). Consider using metric metadata (higher-is-better vs. lower-is-better) or exposing an option.src/Evaluation/CrossValidation/BlockedKFoldStrategy.cs-58-70 (1)
58-70:⚠️ Potential issue | 🟠 MajorValidate
dataSizevs. fold count to avoid empty folds.When
dataSize < _nFolds,foldSizebecomes 0 and splits are degenerate. Add a guard like inKFoldStrategy.💡 Proposed fix
public IEnumerable<(int[] TrainIndices, int[] ValidationIndices)> Split(int dataSize, ReadOnlySpan<T> labels = default) { + if (dataSize <= 0) + throw new ArgumentException("Data size must be positive.", nameof(dataSize)); + if (dataSize < _nFolds) + throw new ArgumentException($"Cannot have {_nFolds} folds with only {dataSize} samples.", nameof(dataSize)); + var indices = Enumerable.Range(0, dataSize).ToArray();src/Evaluation/Engines/ValidationCurveEngine.cs-68-76 (1)
68-76:⚠️ Potential issue | 🟠 MajorValidate
cvFoldsandparameterValuesto avoid empty folds and invalid results.
cvFolds > totalSamplesor an emptyparameterValuesarray leads to empty splits and downstream failures. Fail fast with clear exceptions.💡 Proposed fix
int totalSamples = features.GetLength(0); int numFeatures = features.GetLength(1); + if (parameterValues == null || parameterValues.Length == 0) + throw new ArgumentException("At least one parameter value is required.", nameof(parameterValues)); + if (cvFolds < 2 || cvFolds > totalSamples) + throw new ArgumentException($"cvFolds must be between 2 and {totalSamples}.", nameof(cvFolds)); + var result = new ValidationCurveResult<T>src/Evaluation/Engines/ValidationCurveEngine.cs-115-132 (1)
115-132:⚠️ Potential issue | 🟠 MajorFail fast when
metricNameis not produced to avoid empty-score averages.If the metric key is missing,
trainScores/valScoresremain empty and.Average()throws later. UseTryGetValueand throw a clear error.💡 Proposed fix
var trainMetrics = isClassification ? _metricEngine.ComputeClassificationMetrics(trainPreds, trainTargets) : _metricEngine.ComputeRegressionMetrics(trainPreds, trainTargets); - var trainMetric = trainMetrics[metricName]; - if (trainMetric != null) - trainScores.Add(NumOps.ToDouble(trainMetric.Value)); + if (!trainMetrics.TryGetValue(metricName, out var trainMetric) || trainMetric == null) + throw new ArgumentException($"Metric '{metricName}' was not produced.", nameof(metricName)); + trainScores.Add(NumOps.ToDouble(trainMetric.Value)); // Evaluate on validation set var valPreds = predictFunc(model, valFeatures); var valMetrics = isClassification ? _metricEngine.ComputeClassificationMetrics(valPreds, valTargets) : _metricEngine.ComputeRegressionMetrics(valPreds, valTargets); - var valMetric = valMetrics[metricName]; - if (valMetric != null) - valScores.Add(NumOps.ToDouble(valMetric.Value)); + if (!valMetrics.TryGetValue(metricName, out var valMetric) || valMetric == null) + throw new ArgumentException($"Metric '{metricName}' was not produced.", nameof(metricName)); + valScores.Add(NumOps.ToDouble(valMetric.Value));src/Evaluation/Statistics/PairedTTest.cs-48-75 (1)
48-75:⚠️ Potential issue | 🟠 MajorHandle zero-variance differences to avoid NaN/Infinity results.
When all paired differences are identical,
stdDiffbecomes 0 and bothtStatand Cohen’s d divide by zero, producing NaN/Infinity and invalid p-values. Add a short-circuit for this case.💡 Proposed fix
double meanDiff = differences.Average(); double sumSqDiff = differences.Sum(d => (d - meanDiff) * (d - meanDiff)); double stdDiff = Math.Sqrt(sumSqDiff / (n - 1)); + if (stdDiff == 0) + { + double pValueZeroVar = meanDiff == 0 ? 1.0 : 0.0; + return new StatisticalTestResult<T> + { + TestName = Name, + Statistic = NumOps.Zero, + PValue = NumOps.FromDouble(pValueZeroVar), + IsSignificant = pValueZeroVar < alpha, + Alpha = alpha, + DegreesOfFreedom = n - 1, + EffectSize = NumOps.Zero, + Description = "All paired differences are identical; t-statistic undefined." + }; + }src/Evaluation/Statistics/McNemarTest.cs-110-122 (1)
110-122:⚠️ Potential issue | 🟠 MajorCap exact two-tailed p-value at 1.0.
2 * pValuecan exceed 1 (e.g., b=c=1), yielding invalid probabilities. Clamp the result to 1.🛠️ Proposed fix
- return 2 * pValue; // Two-tailed + return Math.Min(1.0, 2 * pValue); // Two-tailedsrc/Evaluation/Statistics/DeLongTest.cs-184-195 (1)
184-195:⚠️ Potential issue | 🟠 MajorGuard covariance for length < 2 to avoid NaN.
With only one positive or negative sample,
x.Length - 1becomes 0 and returns NaN, which propagates into the z‑statistic and p‑value. HandleLength < 2explicitly.🛠️ Proposed fix
- private double Covariance(double[] x, double[] y) + private double Covariance(double[] x, double[] y) { - if (x.Length != y.Length || x.Length == 0) return 0; + if (x.Length != y.Length || x.Length < 2) return 0; double meanX = x.Average(); double meanY = y.Average(); double sum = 0; for (int i = 0; i < x.Length; i++) { sum += (x[i] - meanX) * (y[i] - meanY); } return sum / (x.Length - 1); }src/Evaluation/Metrics/TimeSeries/MASEMetric.cs-34-63 (1)
34-63:⚠️ Potential issue | 🟠 MajorValidate
seasonalPeriodto prevent invalid indexing.If
seasonalPeriodis 0 or negative, the naive-forecast loop can start at a negative index (or collapse to a trivial baseline), causing runtime errors or misleading results.🛠️ Proposed fix
- int period = seasonalPeriod ?? 1; + int period = seasonalPeriod ?? 1; + if (period < 1) + throw new ArgumentException("Seasonal period must be at least 1.", nameof(seasonalPeriod));src/Evaluation/Statistics/KruskalWallisTest.cs-40-48 (1)
40-48:⚠️ Potential issue | 🟠 MajorReject empty groups to avoid divide‑by‑zero.
If any group is empty, the rank sum and variance calculations divide by zero. Add a guard before proceeding.Proposed fix
@@ int k = groups.Length; if (k < 2) throw new ArgumentException("Need at least 2 groups for Kruskal-Wallis test."); + if (groups.Any(g => g.Length == 0)) + throw new ArgumentException("Each group must contain at least one observation.", nameof(groups));src/Evaluation/Engines/RobustnessEngine.cs-1-5 (1)
1-5:⚠️ Potential issue | 🟠 MajorRobustness/degradation assumes higher‑is‑better metrics.
For error metrics (RMSE/MAE), degradation and overall robustness are inverted. Incorporate metric direction (or accept it as a parameter) and normalize the calculations.Proposed fix
@@ -using AiDotNet.Evaluation.Options; +using AiDotNet.Evaluation.Enums; +using AiDotNet.Evaluation.Options; @@ - public RobustnessResult<T> Analyze<TModel>( + public RobustnessResult<T> Analyze<TModel>( T[,] features, T[] targets, Func<TModel, T[,], T[]> predictFunc, TModel model, string metricName = "Accuracy", - bool isClassification = true) + bool isClassification = true, + MetricDirection direction = MetricDirection.HigherIsBetter) { @@ - foreach (var noiseLevel in noiseLevels) + double directionSign = direction == MetricDirection.HigherIsBetter ? 1.0 : -1.0; + foreach (var noiseLevel in noiseLevels) { @@ - result.NoiseDegradation[noiseLevel] = result.BaselineScore - score; + result.NoiseDegradation[noiseLevel] = (result.BaselineScore - score) * directionSign; } @@ - importance[featureIdx] = baselineScore - score; + importance[featureIdx] = (baselineScore - score) * directionSign; } @@ - result.OverallRobustnessScore = ComputeOverallRobustness(result); + result.OverallRobustnessScore = ComputeOverallRobustness(result, direction, result.BaselineScore); @@ - private double ComputeOverallRobustness(RobustnessResult<T> result) + private double ComputeOverallRobustness(RobustnessResult<T> result, MetricDirection direction, double baselineScore) { - double noiseRobustness = 0; + double noiseRobustness = 0; + double baseline = Math.Max(0.001, Math.Abs(baselineScore)); if (result.NoiseRobustness.Count > 0) { - noiseRobustness = result.NoiseRobustness.Values.Average() / Math.Max(0.001, result.BaselineScore); + var avg = result.NoiseRobustness.Values.Average(); + noiseRobustness = direction == MetricDirection.LowerIsBetter + ? baseline / Math.Max(0.001, avg) + : avg / baseline; } @@ - if (result.DropoutRobustness.Count > 0) + if (result.DropoutRobustness.Count > 0) { - dropoutRobustness = result.DropoutRobustness.Values.Average() / Math.Max(0.001, result.BaselineScore); + var avg = result.DropoutRobustness.Values.Average(); + dropoutRobustness = direction == MetricDirection.LowerIsBetter + ? baseline / Math.Max(0.001, avg) + : avg / baseline; }Also applies to: 96-120, 184-238
src/Evaluation/Metrics/Regression/PearsonCorrelationMetric.cs-77-104 (1)
77-104:⚠️ Potential issue | 🟠 Major
ciMethodis ignored inComputeWithCI.
Callers can pass a different CI method but still receive the same percentile bootstrap interval. Either implement the requested method or restrict/rename the option to match actual behavior.Proposed fix (restrict to supported method)
@@ - public MetricWithCI<T> ComputeWithCI(ReadOnlySpan<T> predictions, ReadOnlySpan<T> actuals, - ConfidenceIntervalMethod ciMethod = ConfidenceIntervalMethod.BCaBootstrap, + public MetricWithCI<T> ComputeWithCI(ReadOnlySpan<T> predictions, ReadOnlySpan<T> actuals, + ConfidenceIntervalMethod ciMethod = ConfidenceIntervalMethod.Percentile, double confidenceLevel = 0.95, int bootstrapSamples = 1000, int? randomSeed = null) { + if (ciMethod != ConfidenceIntervalMethod.Percentile) + throw new NotSupportedException($"{ciMethod} is not supported yet for PearsonCorrelationMetric."); var value = Compute(predictions, actuals); var (lower, upper) = BootstrapCI(predictions, actuals, bootstrapSamples, confidenceLevel, randomSeed); return new MetricWithCI<T>(value, lower, upper, confidenceLevel, ciMethod, Name, Direction); }src/Evaluation/Engines/LearningCurveEngine.cs-151-178 (1)
151-178:⚠️ Potential issue | 🟠 MajorBias/variance diagnosis assumes higher‑is‑better metrics.
For error metrics (e.g., RMSE/MAE), the thresholds invert the interpretation. Normalize by metric direction (or accept a direction parameter) before applying the heuristics.Proposed fix
@@ - public LearningCurveResult<T> Generate<TModel>( + public LearningCurveResult<T> Generate<TModel>( T[,] features, T[] targets, Func<T[,], T[], TModel> trainFunc, Func<TModel, T[,], T[]> predictFunc, string metricName = "Accuracy", double[]? trainSizes = null, int cvFolds = 5, - bool isClassification = true) + bool isClassification = true, + MetricDirection direction = MetricDirection.HigherIsBetter) @@ - result.Diagnosis = DiagnoseLearningCurve(result); + result.Diagnosis = DiagnoseLearningCurve(result, direction); @@ - private BiasVarianceDiagnosis DiagnoseLearningCurve(LearningCurveResult<T> curve) + private BiasVarianceDiagnosis DiagnoseLearningCurve(LearningCurveResult<T> curve, MetricDirection direction) { if (curve.TrainScoreMeans.Count < 2) return BiasVarianceDiagnosis.Unknown; - double finalTrainScore = curve.TrainScoreMeans.Last(); - double finalValScore = curve.ValidationScoreMeans.Last(); + double Normalize(double v) => direction == MetricDirection.LowerIsBetter ? -v : v; + double finalTrainScore = Normalize(curve.TrainScoreMeans.Last()); + double finalValScore = Normalize(curve.ValidationScoreMeans.Last());src/Evaluation/Engines/RobustnessEngine.cs-54-95 (1)
54-95:⚠️ Potential issue | 🟠 MajorValidate input sizes and metric lookup before scoring.
A targets/features length mismatch or unknownmetricNamewill currently fail mid‑run. Add upfront validation and useTryGetValue(apply the same pattern for noise/dropout/permutation lookups).Proposed fix
@@ - int n = features.GetLength(0); - int numFeatures = features.GetLength(1); + int n = features.GetLength(0); + int numFeatures = features.GetLength(1); + if (targets.Length != n) + throw new ArgumentException("Targets length must match number of feature rows.", nameof(targets)); + if (n == 0) + throw new ArgumentException("At least one sample is required.", nameof(features)); @@ - var baselineMetric = baselineMetrics[metricName]; - result.BaselineScore = baselineMetric != null ? NumOps.ToDouble(baselineMetric.Value) : 0; + if (!baselineMetrics.TryGetValue(metricName, out var baselineMetric) || baselineMetric is null) + throw new ArgumentException($"Metric '{metricName}' not found.", nameof(metricName)); + result.BaselineScore = NumOps.ToDouble(baselineMetric.Value); @@ - var noisyMetric = noisyMetrics[metricName]; - double score = noisyMetric != null ? NumOps.ToDouble(noisyMetric.Value) : 0; + if (!noisyMetrics.TryGetValue(metricName, out var noisyMetric) || noisyMetric is null) + throw new ArgumentException($"Metric '{metricName}' not found.", nameof(metricName)); + double score = NumOps.ToDouble(noisyMetric.Value);src/Evaluation/Statistics/KruskalWallisTest.cs-141-176 (1)
141-176:⚠️ Potential issue | 🟠 MajorDunn post‑hoc ranks are mapped to the wrong groups.
The current code fillsranksin sorted order but then aggregates ranks by original group order, which misassigns ranks and produces incorrect p‑values. Track rank sums bygroupIdxinstead.Proposed fix
@@ - var ranks = new double[N]; - int idx = 0; - int rankIdx = 0; + var rankSums = new double[k]; + int idx = 0; while (idx < N) { int start = idx; double value = sorted[start].value; while (idx < N && Math.Abs(sorted[idx].value - value) < 1e-10) idx++; double avgRank = (start + 1 + idx) / 2.0; for (int i = start; i < idx; i++) - ranks[rankIdx++] = avgRank; + rankSums[sorted[i].groupIdx] += avgRank; } - // Compute mean ranks per group - var meanRanks = new double[k]; - var groupSizes = new int[k]; - rankIdx = 0; - for (int g = 0; g < k; g++) - { - groupSizes[g] = groups[g].Length; - for (int i = 0; i < groupSizes[g]; i++) - { - meanRanks[g] += ranks[rankIdx++]; - } - meanRanks[g] /= groupSizes[g]; - } + // Compute mean ranks per group + var groupSizes = groups.Select(g => g.Length).ToArray(); + var meanRanks = new double[k]; + for (int g = 0; g < k; g++) + meanRanks[g] = rankSums[g] / groupSizes[g];src/Evaluation/Engines/LearningCurveEngine.cs-58-137 (1)
58-137:⚠️ Potential issue | 🟠 MajorAdd dataset/parameter validation and safe metric lookup in
Generate.
Generateassumes the feature/target lengths match and thatmetricNameexists; mismatches currently fail late andcvFolds ≥ totalSamplescan yield empty validation splits. Validate early and useTryGetValuefor clearer failures.Proposed fix
@@ - int totalSamples = features.GetLength(0); - int numFeatures = features.GetLength(1); + int totalSamples = features.GetLength(0); + int numFeatures = features.GetLength(1); + if (targets.Length != totalSamples) + throw new ArgumentException("Targets length must match number of feature rows.", nameof(targets)); + if (totalSamples < 2) + throw new ArgumentException("At least 2 samples are required.", nameof(features)); + if (cvFolds < 2 || cvFolds >= totalSamples) + throw new ArgumentOutOfRangeException(nameof(cvFolds), "cvFolds must be between 2 and totalSamples - 1."); @@ - var trainMetric = trainMetrics[metricName]; - if (trainMetric != null) - trainScores.Add(NumOps.ToDouble(trainMetric.Value)); + if (!trainMetrics.TryGetValue(metricName, out var trainMetric) || trainMetric is null) + throw new ArgumentException($"Metric '{metricName}' not found.", nameof(metricName)); + trainScores.Add(NumOps.ToDouble(trainMetric.Value)); @@ - var valMetric = valMetrics[metricName]; - if (valMetric != null) - valScores.Add(NumOps.ToDouble(valMetric.Value)); + if (!valMetrics.TryGetValue(metricName, out var valMetric) || valMetric is null) + throw new ArgumentException($"Metric '{metricName}' not found.", nameof(metricName)); + valScores.Add(NumOps.ToDouble(valMetric.Value));src/Evaluation/Metrics/Classification/SpecificityMetric.cs-22-65 (1)
22-65:⚠️ Potential issue | 🟠 MajorSupportsMultiClass is misleading with a one-vs-rest implementation.
Line 34 advertises multiclass support, but Compute only evaluates a single positive label. In multiclass evaluation this will silently score just one label and can mislead. Either implement macro-averaged specificity (one-vs-rest per class) or mark it as binary-only.
🔧 Minimal fix if multiclass isn’t supported
- public bool SupportsMultiClass => true; + public bool SupportsMultiClass => false;src/Evaluation/Metrics/Regression/R2ScoreMetric.cs-59-60 (1)
59-60:⚠️ Potential issue | 🟠 MajorR² should not return 1 for constant actuals unless predictions are perfect.
Currently any constant target returns 1, even with poor predictions, which overstates model quality.
✅ Suggested fix
- if (Math.Abs(ssTot) < 1e-10) return NumOps.One; // All actuals are the same + if (Math.Abs(ssTot) < 1e-10) + { + return Math.Abs(ssRes) < 1e-10 ? NumOps.One : NumOps.Zero; + }src/Evaluation/Metrics/Regression/MeanSquaredLogErrorMetric.cs-64-81 (1)
64-81:⚠️ Potential issue | 🟠 MajorGuard against invalid CI parameters to prevent index errors.
If
samples <= 0orconfis outside (0,1), percentile indexing can underflow/overflow. Validate early and fail fast with a clear error.🔧 Proposed fix
private (T, T) BootstrapCI(ReadOnlySpan<T> pred, ReadOnlySpan<T> actual, int samples, double conf, int? seed) { + if (samples <= 0) + throw new ArgumentOutOfRangeException(nameof(samples), "Bootstrap samples must be > 0."); + if (conf <= 0 || conf >= 1) + throw new ArgumentOutOfRangeException(nameof(conf), "Confidence level must be between 0 and 1 (exclusive)."); int n = pred.Length; if (n == 0) return (NumOps.Zero, NumOps.Zero);src/Evaluation/Metrics/Regression/MeanSquaredLogErrorMetric.cs-40-49 (1)
40-49:⚠️ Potential issue | 🟠 MajorAvoid clamping negatives in MSLE; validate inputs instead.
MSLE is undefined for negative targets/predictions; clamping to zero silently changes the metric and can mask data issues. Prefer rejecting negatives (or explicitly documenting a shift) so results match the definition.
🔧 Proposed fix
- double actual = Math.Max(0, NumOps.ToDouble(actuals[i])); - double pred = Math.Max(0, NumOps.ToDouble(predictions[i])); + double actual = NumOps.ToDouble(actuals[i]); + double pred = NumOps.ToDouble(predictions[i]); + if (actual < 0 || pred < 0) + throw new ArgumentException("MSLE requires non-negative predictions and actuals.");src/Evaluation/Statistics/BootstrapTest.cs-43-47 (1)
43-47:⚠️ Potential issue | 🟠 MajorValidate
numBootstraps > 0.
Non‑positive values can makeComputeCIindex into empty arrays and produce meaningless p‑values.🛠️ Proposed fix
public BootstrapTest(int numBootstraps = 10000, int? randomSeed = null) { - _numBootstraps = numBootstraps; + if (numBootstraps <= 0) + throw new ArgumentOutOfRangeException(nameof(numBootstraps), "numBootstraps must be > 0."); + _numBootstraps = numBootstraps; _randomSeed = randomSeed; }src/Evaluation/Statistics/BootstrapTest.cs-133-159 (1)
133-159:⚠️ Potential issue | 🟠 MajorComputeCI lacks input/CI bounds validation.
Mismatched lengths or empty arrays will throw (Random.Next(0)/ index errors), and invalidconfidenceLevelcan produce incorrect bounds.🛠️ Proposed fix
public (double Lower, double Upper) ComputeCI(T[] scores1, T[] scores2, double confidenceLevel = 0.95) { + if (scores1.Length != scores2.Length) + throw new ArgumentException("Score arrays must have the same length."); + if (confidenceLevel <= 0 || confidenceLevel >= 1) + throw new ArgumentOutOfRangeException(nameof(confidenceLevel), "confidenceLevel must be between 0 and 1."); int n = scores1.Length; + if (n == 0) return (0, 0); var random = _randomSeed.HasValue ? RandomHelper.CreateSeededRandom(_randomSeed.Value) : new Random();src/Evaluation/Metrics/Regression/MAPEMetric.cs-60-86 (1)
60-86:⚠️ Potential issue | 🟠 MajorciMethod is ignored and CI inputs are unchecked.
ComputeWithCIalways returns a percentile interval while defaulting toBCaBootstrap, so the reported method can be wrong. Also,bootstrapSamples <= 0orconfidenceLeveloutside (0,1) will cause indexing errors. Please validate inputs and either honorciMethodor reject unsupported methods.🛠️ Proposed input validation
public MetricWithCI<T> ComputeWithCI(ReadOnlySpan<T> predictions, ReadOnlySpan<T> actuals, ConfidenceIntervalMethod ciMethod = ConfidenceIntervalMethod.BCaBootstrap, double confidenceLevel = 0.95, int bootstrapSamples = 1000, int? randomSeed = null) { + if (bootstrapSamples <= 0) + throw new ArgumentOutOfRangeException(nameof(bootstrapSamples), "bootstrapSamples must be > 0."); + if (confidenceLevel <= 0 || confidenceLevel >= 1) + throw new ArgumentOutOfRangeException(nameof(confidenceLevel), "confidenceLevel must be between 0 and 1."); var value = Compute(predictions, actuals); var (lower, upper) = BootstrapCI(predictions, actuals, bootstrapSamples, confidenceLevel, randomSeed); return new MetricWithCI<T>(value, lower, upper, confidenceLevel, ciMethod, Name, Direction); }src/Evaluation/Metrics/Classification/AUCROCMetric.cs-41-116 (1)
41-116:⚠️ Potential issue | 🟠 MajorValidate multi‑class probability shape and
numClasses.
ComputeMultiClassAUCindexesprobs[i * numClasses + c]without verifying that the buffer matchesactuals.Length * numClasses, which can throw or silently misalign data. Also, guard againstnumClasses < 2.🛠️ Proposed fix
public T Compute(ReadOnlySpan<T> probabilities, ReadOnlySpan<T> actuals, int numClasses = 2) { - if (probabilities.Length == 0 || actuals.Length == 0) return NumOps.FromDouble(0.5); + if (numClasses < 2) + throw new ArgumentOutOfRangeException(nameof(numClasses), "numClasses must be >= 2."); + + int expectedLength = numClasses == 2 ? actuals.Length : actuals.Length * numClasses; + if (probabilities.Length != expectedLength) + throw new ArgumentException("Probabilities length must match actuals.Length (binary) or actuals.Length * numClasses (multi-class)."); + if (expectedLength == 0) return NumOps.FromDouble(0.5); if (numClasses == 2) { if (probabilities.Length != actuals.Length) throw new ArgumentException("For binary, probabilities and actuals must have same length."); return ComputeBinaryAUC(probabilities, actuals); }src/Evaluation/Statistics/BootstrapTest.cs-55-127 (1)
55-127:⚠️ Potential issue | 🟠 MajorPaired test permutation breaks pairing; p‑values are incorrect.
IsPairedis true, but the null distribution is built by pooling and permuting labels, which is unpaired. For paired tests, permute/sign‑flip within pairs and compute effect size from paired differences.🛠️ Proposed fix (paired sign‑flip)
- // Compute observed difference - double observedDiff = ComputeMeanDifference(scores1, scores2); - - // Compute differences under null hypothesis (permutation approach) - var pooled = new double[n * 2]; - for (int i = 0; i < n; i++) - { - pooled[i] = NumOps.ToDouble(scores1[i]); - pooled[n + i] = NumOps.ToDouble(scores2[i]); - } - - int moreExtreme = 0; - for (int b = 0; b < _numBootstraps; b++) - { - // Permute labels - var perm = Enumerable.Range(0, n * 2).OrderBy(_ => random.Next()).ToArray(); - double mean1 = 0, mean2 = 0; - for (int i = 0; i < n; i++) - { - mean1 += pooled[perm[i]]; - mean2 += pooled[perm[n + i]]; - } - mean1 /= n; - mean2 /= n; - double permDiff = mean1 - mean2; - - if (Math.Abs(permDiff) >= Math.Abs(observedDiff)) - moreExtreme++; - } + // Compute observed paired differences + double observedDiff = ComputeMeanDifference(scores1, scores2); + var diffs = new double[n]; + for (int i = 0; i < n; i++) + diffs[i] = NumOps.ToDouble(scores1[i]) - NumOps.ToDouble(scores2[i]); + + int moreExtreme = 0; + for (int b = 0; b < _numBootstraps; b++) + { + double sum = 0; + for (int i = 0; i < n; i++) + sum += (random.Next(2) == 0 ? diffs[i] : -diffs[i]); + double permDiff = sum / n; + if (Math.Abs(permDiff) >= Math.Abs(observedDiff)) + moreExtreme++; + } @@ - // Compute effect size (Cohen's d) - double std1 = ComputeStd(scores1); - double std2 = ComputeStd(scores2); - double pooledStd = Math.Sqrt((std1 * std1 + std2 * std2) / 2); - double cohensD = pooledStd > 1e-10 ? observedDiff / pooledStd : 0; + // Compute effect size (paired Cohen's dz) + double stdDiff = ComputeStd(diffs); + double cohensDz = stdDiff > 1e-10 ? observedDiff / stdDiff : 0; @@ - EffectSize = NumOps.FromDouble(cohensD), + EffectSize = NumOps.FromDouble(cohensDz), Interpretation = pValue < 0.05 - ? $"Significant difference (Δ={observedDiff:F4}, Cohen's d={cohensD:F2})" + ? $"Significant difference (Δ={observedDiff:F4}, Cohen's d={cohensDz:F2})" : $"No significant difference (Δ={observedDiff:F4})", Description = $"Bootstrap permutation test with {_numBootstraps} resamples." }; } + + private double ComputeStd(double[] values) + { + int n = values.Length; + if (n < 2) return 0; + double mean = values.Average(); + double sumSq = values.Sum(v => (v - mean) * (v - mean)); + return Math.Sqrt(sumSq / (n - 1)); + }src/Evaluation/Statistics/NemenyiPostHocTest.cs-51-58 (1)
51-58:⚠️ Potential issue | 🟠 MajorCritical value lookup is incorrect for k=11.
The validation allows
kup to 11, butQValuesonly contains rows for k=2 through k=10 (indices 0-8). When k=11, line 58 caps the index at 8, silently using k=10's critical values instead.Either add k=11 critical values to the table or restrict the upper bound:
🔧 Option 1: Restrict to k ≤ 10
- if (k < 2 || k > 11) - throw new ArgumentException("Nemenyi test supports 2-11 algorithms."); + if (k < 2 || k > 10) + throw new ArgumentException("Nemenyi test supports 2-10 algorithms.");🔧 Option 2: Add k=11 row to QValues
private static readonly double[,] QValues = new double[,] { { 1.960, 2.241, 2.638 }, // k=2 { 2.343, 2.571, 2.913 }, // k=3 { 2.569, 2.773, 3.080 }, // k=4 { 2.728, 2.919, 3.203 }, // k=5 { 2.850, 3.031, 3.299 }, // k=6 { 2.949, 3.124, 3.379 }, // k=7 { 3.031, 3.200, 3.447 }, // k=8 { 3.102, 3.266, 3.505 }, // k=9 { 3.164, 3.324, 3.557 }, // k=10 + { 3.219, 3.376, 3.603 }, // k=11 (verify values from Demsar 2006) };src/Evaluation/Metrics/Classification/FBetaScoreMetric.cs-40-45 (1)
40-45:⚠️ Potential issue | 🟠 MajorValidate beta > 0 to avoid NaN/invalid results.
beta <= 0can trigger division by zero (e.g., recall = 0 with beta = 0) and yield NaNs.🛡️ Suggested validation
public FBetaScoreMetric(double beta = 1.0, T? positiveLabel = default, AveragingMethod averaging = AveragingMethod.Binary) { + if (double.IsNaN(beta) || double.IsInfinity(beta) || beta <= 0) + throw new ArgumentOutOfRangeException(nameof(beta), "beta must be a finite value greater than 0."); _beta = beta; _positiveLabel = positiveLabel ?? NumOps.One; _averaging = averaging; }src/Evaluation/Metrics/Regression/AdjustedR2Metric.cs-31-76 (1)
31-76:⚠️ Potential issue | 🟠 MajorHandle zero-variance actuals correctly.
If
ssTotis ~0 and predictions are not perfect, returning 1 is misleading; it should only be 1 whenssResis ~0.🛠️ Suggested fix
- if (Math.Abs(ssTot) < 1e-10) return NumOps.One; + if (Math.Abs(ssTot) < 1e-10) + return Math.Abs(ssRes) < 1e-10 ? NumOps.One : NumOps.Zero;src/Evaluation/Metrics/Classification/FBetaScoreMetric.cs-56-117 (1)
56-117:⚠️ Potential issue | 🟠 MajorInclude predicted-only classes to avoid inflated F-beta.
Using classes from
actualsonly ignores labels that appear only in predictions, which can overstate micro/macro F-beta.✅ Proposed fix
- var classes = GetClasses(actuals); + var classes = GetClasses(predictions, actuals); @@ - private static HashSet<double> GetClasses(ReadOnlySpan<T> vals) + private static HashSet<double> GetClasses(ReadOnlySpan<T> predictions, ReadOnlySpan<T> actuals) { var c = new HashSet<double>(); - for (int i = 0; i < vals.Length; i++) c.Add(NumOps.ToDouble(vals[i])); + for (int i = 0; i < predictions.Length; i++) c.Add(NumOps.ToDouble(predictions[i])); + for (int i = 0; i < actuals.Length; i++) c.Add(NumOps.ToDouble(actuals[i])); return c; }src/Evaluation/Metrics/Classification/F1ScoreMetric.cs-98-201 (1)
98-201:⚠️ Potential issue | 🟠 MajorInclude predicted-only classes to avoid inflated F1.
Using classes from
actualsonly ignores labels that appear only in predictions, which can overstate micro/macro F1. Use the union of predicted and actual classes.✅ Proposed fix
- var classes = GetUniqueClasses(actuals); + var classes = GetUniqueClasses(predictions, actuals); @@ - private static HashSet<double> GetUniqueClasses(ReadOnlySpan<T> values) + private static HashSet<double> GetUniqueClasses(ReadOnlySpan<T> predictions, ReadOnlySpan<T> actuals) { var classes = new HashSet<double>(); - for (int i = 0; i < values.Length; i++) - { - classes.Add(NumOps.ToDouble(values[i])); - } + for (int i = 0; i < predictions.Length; i++) + { + classes.Add(NumOps.ToDouble(predictions[i])); + } + for (int i = 0; i < actuals.Length; i++) + { + classes.Add(NumOps.ToDouble(actuals[i])); + } return classes; }src/Evaluation/Engines/CrossValidationEngine.cs-134-143 (1)
134-143:⚠️ Potential issue | 🟠 MajorHorizon is ignored when selecting predictions/actuals.
For
horizon > 1,pred[0]andseries[startIdx + lookback]correspond to horizon‑1, not the requested horizon. This misaligns targets and predictions.🛠️ Suggested fix
var pred = predictFunc(model, window); - predictions[i] = pred[0]; // First prediction in horizon - actuals[i] = series[startIdx + lookback]; // Actual value at horizon + if (pred.Length < horizon) + throw new ArgumentException("predictFunc must return at least 'horizon' values."); + predictions[i] = pred[horizon - 1]; + actuals[i] = series[startIdx + lookback + horizon - 1];src/Evaluation/Results/Core/MetricCollection.cs-73-98 (1)
73-98:⚠️ Potential issue | 🟠 MajorAdd() silently overwrites despite docs; category index can become inconsistent.
The method claims to throw on duplicate names, but it overwrites and leaves the old category entry intact. This can corrupt
_categoryIndex.🛠️ Suggested fix (enforce uniqueness)
public void Add(MetricWithCI<T> metric) { if (string.IsNullOrEmpty(metric.Name)) { throw new ArgumentException("Metric must have a name.", nameof(metric)); } + + if (_metrics.ContainsKey(metric.Name)) + { + throw new ArgumentException($"Metric '{metric.Name}' already exists.", nameof(metric)); + } _metrics[metric.Name] = metric;src/Evaluation/Engines/CrossValidationEngine.cs-326-340 (1)
326-340:⚠️ Potential issue | 🟠 MajorAvoid returning
default!for missing metrics.
default!violates the “avoid default(T)” requirement and hides errors when a metric is missing. Prefer throwing or returningNumOps.Zeroexplicitly.🛠️ Suggested fix (fail fast)
public T GetMeanMetric(string metricName) { var metric = AggregatedMetrics[metricName]; - return metric != null ? metric.Value : default!; + if (metric == null) + throw new KeyNotFoundException($"Metric '{metricName}' not found."); + return metric.Value; } public T GetStdMetric(string metricName) { var metric = AggregatedMetrics[metricName]; - if (metric == null || metric.StandardDeviation == null) - return default!; + if (metric == null || metric.StandardDeviation == null) + throw new KeyNotFoundException($"Metric '{metricName}' not found or has no std."); return metric.StandardDeviation; }src/Evaluation/Metrics/Classification/AUCPRMetric.cs-80-106 (1)
80-106:⚠️ Potential issue | 🟠 MajorAdd parameter validation for CI inputs to avoid invalid indices.
bootstrapSamples <= 1orconfidenceLeveloutside(0,1)can lead to negative/overflowed percentile indices inBootstrapCI.🛠️ Suggested fix
public MetricWithCI<T> ComputeWithCI(ReadOnlySpan<T> predictions, ReadOnlySpan<T> actuals, ConfidenceIntervalMethod ciMethod = ConfidenceIntervalMethod.BCaBootstrap, double confidenceLevel = 0.95, int bootstrapSamples = 1000, int? randomSeed = null) { + if (bootstrapSamples <= 1) + throw new ArgumentOutOfRangeException(nameof(bootstrapSamples), "bootstrapSamples must be > 1."); + if (confidenceLevel <= 0 || confidenceLevel >= 1) + throw new ArgumentOutOfRangeException(nameof(confidenceLevel), "confidenceLevel must be between 0 and 1."); + var value = Compute(predictions, actuals); var (lower, upper) = BootstrapCI(predictions, actuals, bootstrapSamples, confidenceLevel, randomSeed); return new MetricWithCI<T>(value, lower, upper, confidenceLevel, ciMethod, Name, Direction); }src/Evaluation/Engines/CrossValidationEngine.cs-121-128 (1)
121-128:⚠️ Potential issue | 🟠 MajorTime‑series training ignores non‑prefix indices (leakage risk).
trainSeriesis built as a prefix up totrainIndices.Max(), which silently includes samples not intrainIndicesif the strategy returns non‑contiguous windows. Either validate the prefix assumption or build the training series from the provided indices.🛠️ Suggested fix (validate contiguous prefix)
foreach (var (trainIndices, valIndices) in strategy.Split(numSamples)) { // For time series, indices refer to starting positions of windows - var trainSeries = new T[trainIndices.Max() + lookback + horizon]; + var orderedTrain = trainIndices.OrderBy(i => i).ToArray(); + int maxTrainIdx = orderedTrain[^1]; + if (!orderedTrain.SequenceEqual(Enumerable.Range(0, maxTrainIdx + 1))) + throw new ArgumentException("Time-series strategies must return contiguous prefix indices."); + var trainSeries = new T[maxTrainIdx + lookback + horizon]; Array.Copy(series, trainSeries, Math.Min(series.Length, trainSeries.Length));src/Evaluation/Metrics/Classification/PrecisionMetric.cs-93-177 (1)
93-177:⚠️ Potential issue | 🟠 MajorPrecision ignores predicted‑only classes in micro/macro modes.
GetUniqueClasses(actuals)drops classes that appear only in predictions, so false positives for those classes are never counted and precision is inflated (common in folds missing a class). Build the class set from the union of predictions and actuals.🛠️ Suggested fix
- var classes = GetUniqueClasses(actuals); + var classes = GetUniqueClasses(predictions, actuals); ... - private static HashSet<double> GetUniqueClasses(ReadOnlySpan<T> values) + private static HashSet<double> GetUniqueClasses(ReadOnlySpan<T> values1, ReadOnlySpan<T> values2) { var classes = new HashSet<double>(); - for (int i = 0; i < values.Length; i++) + for (int i = 0; i < values1.Length; i++) { - classes.Add(NumOps.ToDouble(values[i])); + classes.Add(NumOps.ToDouble(values1[i])); } + for (int i = 0; i < values2.Length; i++) + { + classes.Add(NumOps.ToDouble(values2[i])); + } return classes; }
Contributor
There was a problem hiding this comment.
Pull request overview
This pull request implements a comprehensive model evaluation framework addressing Issue #334, adding 70+ metrics across regression, classification, and time series domains, along with 16 cross-validation strategies.
Changes:
- Added 25+ regression metrics (MSE, RMSE, MAE, R², MAPE variants, Huber/LogCosh loss, quantile metrics, etc.)
- Added 30+ classification metrics (accuracy, precision, recall, F1, AUC-ROC/PR, likelihood ratios, etc.)
- Added 4 time series metrics (MASE, SMAPE, Theil-U, WAPE)
- Implemented 13 cross-validation strategies (K-Fold, Stratified, Time Series, Bootstrap, Monte Carlo, etc.)
- Added MetricDirection and AveragingMethod enums with extensive documentation
Reviewed changes
Copilot reviewed 130 out of 130 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| Regression metrics (25 files) | Complete set of error metrics with bootstrap CI support |
| Classification metrics (30 files) | Comprehensive binary classification metrics |
| Time series metrics (4 files) | Specialized forecasting accuracy metrics |
| CV strategies (13 files) | Flexible cross-validation implementations |
| Enums (2 files) | Well-documented enumerations for metric configuration |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![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.
- Change default CI method from BCaBootstrap to PercentileBootstrap across all metrics - Add parameter validation for bootstrapSamples (>= 2) and confidenceLevel (0 < x < 1) - Fix NemenyiPostHocTest to reject k > 10 (critical values unavailable) - Fix StratifiedKFoldStrategy label rounding that merged distinct classes - Fix BootstrapTest paired permutation test to preserve pairing structure - Fix CrossValidationEngine time-series training leakage and horizon handling - Fix HingeLossMetric to use raw decision values instead of binarizing - Fix MetricCollection.Add() to throw on duplicates, add AddOrUpdate() - Add timeIndices length validation in PurgedKFoldStrategy - Add higherIsBetter parameter to curve engines for metric direction - Various other minor fixes and improvements Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 20
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🤖 Fix all issues with AI agents
In `@src/Evaluation/Engines/RobustnessEngine.cs`:
- Around line 237-253: ComputeOverallRobustness currently divides Average by
BaselineScore which flips meaning for lower-is-better metrics; update
ComputeOverallRobustness to consult the metric direction (e.g., a higherIsBetter
flag on RobustnessResult<T> or pass it in) and normalize each perturbation score
using a stable epsilon (Math.Max(epsilon, result.BaselineScore)). For
higherIsBetter use scoreRatio = average / baseline; for lowerIs-better use
scoreRatio = baseline / average so that a higher ratio always indicates better
robustness, then combine noise and dropout ratios (e.g., average the two) to
produce the final robustness. Ensure you reference
RobustnessResult<T>.BaselineScore, RobustnessResult<T>.NoiseRobustness,
RobustnessResult<T>.DropoutRobustness and the ComputeOverallRobustness method
when making the change.
- Around line 55-134: The Analyze<TModel> method uses a hardcoded default
metricName ("Accuracy") and then indexes
baselineMetrics/noisyMetrics/droppedMetrics by metricName without validation;
fix by choosing a sensible default based on isClassification (e.g., "Accuracy"
for classification, "RMSE" or "MSE" for regression) when metricName is null or
empty, and immediately validate that baselineMetrics contains the requested
metricName after calling _metricEngine.ComputeClassificationMetrics or
_metricEngine.ComputeRegressionMetrics; if the metric key is missing throw an
ArgumentException with a clear message. Apply the same validation before reading
noisyMetrics[metricName] and droppedMetrics[metricName] (symbols:
Analyze<TModel>, metricName, isClassification, baselineMetrics, noisyMetrics,
droppedMetrics) so the method fails fast with a clear error instead of causing
downstream key or null errors.
In `@src/Evaluation/Engines/ValidationCurveEngine.cs`:
- Around line 58-77: The Generate<TModel> method lacks input-shape validation
which can cause null refs, index errors, or empty/out-of-range folds; add checks
at the start of ValidationCurveEngine.Generate to validate that features and
targets are not null, that features has two dimensions (GetLength(0) > 0 and
GetLength(1) > 0), that targets.Length equals features.GetLength(0), and that
cvFolds <= totalSamples (and >=2 as already checked); throw
ArgumentNullException or ArgumentException with clear parameter names (features,
targets, cvFolds) when these conditions fail so downstream indexing in Generate,
trainFunc and predictFunc cannot produce out-of-range errors.
- Around line 122-145: The code currently assumes metricName produced values and
calls trainScores.Average()/valScores.Average(), which will throw if the metric
was missing; update the block after the fold to guard against empty score lists:
check trainScores.Any() and valScores.Any() and if either is empty, throw a
clear exception (e.g. InvalidOperationException or ArgumentException)
referencing metricName, or alternatively set the corresponding mean/std to
double.NaN to preserve result shape; make the change near the code that
populates result.ParameterValues, result.TrainScoreMeans, result.TrainScoreStds,
result.ValidationScoreMeans, and result.ValidationScoreStds so the behavior is
deterministic when metricName is unknown.
In `@src/Evaluation/Metrics/Classification/BalancedErrorRateMetric.cs`:
- Around line 59-70: ComputeWithCI currently always computes percentile
bootstrapped intervals by calling BootstrapCI but returns the caller's ciMethod;
update ComputeWithCI to switch on the ciMethod parameter (e.g.,
ConfidenceIntervalMethod.PercentileBootstrap,
ConfidenceIntervalMethod.BcaBootstrap, etc.), calling the appropriate CI routine
for each supported method (call BootstrapCI for PercentileBootstrap, call the
BCa implementation for BcaBootstrap, etc.), and if a requested method is
unsupported throw an ArgumentException; ensure the computed (lower, upper) used
to construct the MetricWithCI<T> match the chosen method so Name/Direction
remain unchanged.
In `@src/Evaluation/Metrics/Classification/BrierScoreMetric.cs`:
- Around line 40-53: The Compute method currently returns zero on an empty
probabilities span and doesn't validate multi-class shapes; update Compute to
only return NumOps.Zero when both probabilities and actuals are empty, otherwise
throw ArgumentException if probabilities is empty but actuals is not; validate
that numClasses >= 2 and for the multi-class branch ensure probabilities.Length
== actuals.Length * numClasses (throw ArgumentException if not) before calling
ComputeMultiClassBrier; keep the existing binary checks for ComputeBinaryBrier
but ensure numClasses == 2 is enforced consistently.
- Around line 70-85: The denominator n * numClasses in ComputeMultiClassBrier
can overflow as both are ints; change the division to use a double denominator
by casting one operand to double (e.g., (double)n * numClasses) when computing
sum / (n * numClasses) so the multiplication is done in double precision and
then call NumOps.FromDouble on the resulting double; update the return
expression in ComputeMultiClassBrier accordingly.
In `@src/Evaluation/Metrics/Classification/FalseNegativeRateMetric.cs`:
- Around line 55-66: ComputeWithCI currently ignores the ciMethod parameter and
always calls BootstrapCI; update ComputeWithCI to branch on the ciMethod (the
ConfidenceIntervalMethod enum) and call the proper CI routine (e.g., BootstrapCI
for PercentileBootstrap/BootstrapBCa variations or other methods if
implemented), passing through bootstrapSamples, confidenceLevel and randomSeed,
and return MetricWithCI<T> constructed from the chosen method’s (lower, upper);
if a requested ciMethod is not implemented, throw a clear NotSupportedException
referencing ComputeWithCI and the unsupported ConfidenceIntervalMethod to avoid
silently mislabeling results.
In `@src/Evaluation/Metrics/Classification/FalsePositiveRateMetric.cs`:
- Around line 55-67: The ComputeWithCI method currently ignores the ciMethod
parameter and always calls BootstrapCI; update ComputeWithCI to switch on the
ciMethod (e.g., case ConfidenceIntervalMethod.PercentileBootstrap: use
BootstrapCI(predictions, actuals, ...); case other supported methods: call their
corresponding CI helper; default: throw NotSupportedException or
ArgumentException for unsupported methods) and only pass the declared ciMethod
into the MetricWithCI<T> constructor when the computed CI corresponds to that
method; ensure any new CI helpers are named clearly (e.g., BootstrapCI,
WilsonCI, etc.) and used in the switch so the reported ciMethod matches the
actual computation.
In `@src/Evaluation/Metrics/Classification/HingeLossMetric.cs`:
- Around line 37-70: The Compute method in HingeLossMetric currently
auto-converts predictions in [0,1] to [-1,1] (the pred -> yHat conversion),
which can corrupt legitimate raw decision scores; remove the heuristic that
checks "if (pred >= 0.0 && pred <= 1.0) yHat = 2.0 * pred - 1.0", use pred
directly as the decision value (yHat = pred), and update the inline comment to
state that predictions must be raw decision-function outputs (-/+ margin), not
probabilities; also update any constructor/docs/tests referencing this behavior
to require callers to perform probability->margin conversion themselves or
enable an explicit conversion flag if you prefer option 2 later.
In `@src/Evaluation/Metrics/Regression/ExplainedVarianceMetric.cs`:
- Around line 29-61: The Compute method currently returns 1 when varActuals is
~0 regardless of residuals; change the final logic to check both variances:
compute varActuals and varResiduals (already computed as sums of squared diffs)
and if Math.Abs(varActuals) < 1e-10 then return NumOps.One only when
Math.Abs(varResiduals) < 1e-10, otherwise return NumOps.Zero; otherwise keep
returning NumOps.FromDouble(1 - varResiduals / varActuals). Reference: method
Compute, variables varActuals and varResiduals.
In `@src/Evaluation/Metrics/Regression/HuberLossMetric.cs`:
- Around line 44-47: The constructor currently silently clamps delta using
Math.Max; instead validate and reject non-positive values: in the
HuberLossMetric(double delta = 1.0) constructor check if delta <= 0 and throw an
ArgumentOutOfRangeException (include "delta" as the parameter name and a short
message), otherwise assign _delta = delta; reference the HuberLossMetric
constructor and the _delta field when making this change.
In `@src/Evaluation/Metrics/Regression/LogCoshLossMetric.cs`:
- Around line 75-87: The ComputeWithCI method currently ignores the ciMethod
parameter and always uses percentile bootstrap; update ComputeWithCI to respect
ciMethod by switching on the ConfidenceIntervalMethod parameter: if ciMethod ==
ConfidenceIntervalMethod.PercentileBootstrap call BootstrapCI(predictions,
actuals, bootstrapSamples, confidenceLevel, randomSeed) as before, for any other
methods either call their respective CI helper (e.g., a NormalApproximationCI or
JackknifeCI function if implemented) or throw a NotSupportedException mentioning
the unsupported ciMethod; ensure the returned MetricWithCI still uses the
provided ciMethod in its constructor.
In `@src/Evaluation/Metrics/Regression/NormalizedMSEMetric.cs`:
- Around line 35-63: The Compute method currently returns NumOps.Zero when
variance < 1e-10 which hides cases where MSE>0; modify Compute (method Compute
in NormalizedMSEMetric) to check when variance < eps (use the existing 1e-10)
and then: if mse <= eps return NumOps.Zero (treat as both near-zero), else throw
a clear InvalidOperationException (e.g., "Normalized MSE undefined: target
variance is ~0 but prediction error is non-zero") so callers are notified that
NMSE is undefined rather than reporting 0; keep references to mse, variance, and
NumOps.Zero/FromDouble as needed.
In `@src/Evaluation/Metrics/Regression/RelativeAbsoluteErrorMetric.cs`:
- Around line 58-59: The current check in RelativeAbsoluteErrorMetric that
returns NumOps.Zero when sat < 1e-10 can hide real errors; change the guard to
check both sat and sae: if sat < 1e-10 and sae < 1e-10 then return NumOps.Zero,
otherwise when sat < 1e-10 but sae is not ≈0 return a sentinel via
NumOps.FromDouble(double.PositiveInfinity) (or alternatively throw an
InvalidOperationException) so callers can detect the
zero-variance-but-nonzero-error case; update the block that currently reads "if
(sat < 1e-10) return NumOps.Zero; return NumOps.FromDouble(sae / sat);" to
implement this dual check using the sae and sat symbols and
NumOps.FromDouble/NumOps.Zero constructors.
In `@src/Evaluation/Metrics/Regression/RMSLEMetric.cs`:
- Around line 40-49: The loop in RMSLEMetric (where sumSquaredLogError is
computed over predictions and actuals) currently clamps negative values using
Math.Max(0, ...), which hides bad inputs; instead, validate that
NumOps.ToDouble(actuals[i]) and NumOps.ToDouble(predictions[i]) are >= 0 and
throw a clear exception (e.g., ArgumentException) identifying the offending
index and value when a negative is found; remove the Math.Max clamping and
ensure the exception message references the method/class (RMSLEMetric) and the
array (actuals/predictions) so callers can detect and fix invalid inputs.
In `@src/Evaluation/Statistics/DeLongTest.cs`:
- Around line 132-146: ComputeAUC currently multiplies posIndices.Count *
negIndices.Count as ints which can overflow; change the denominator calculation
in ComputeAUC to perform the multiplication in a wider numeric type or double
(e.g., cast one operand to double or use (long) before converting) so the
division uses a non-overflowing double denominator (refer to ComputeAUC,
posIndices.Count and negIndices.Count).
In `@src/Evaluation/Statistics/KruskalWallisTest.cs`:
- Around line 103-109: The computations for H, the tie correction and the Dunn
variance use N (an int) in multiplications/divisions which can overflow for
large samples; change the arithmetic to use floating-point intermediates by
casting N to double (or store a double/long temp like double nD = (double)N) and
use nD in expressions for H, tieCorrection and the Dunn variance calculation in
KruskalWallisTest (references: H, tieCorrection, N, sumRankSquaredOverN,
tieGroups and the Dunn variance computation). Ensure all divisors and powers
(e.g. N*(N+1), N*N*N - N) are computed as doubles so the division is
floating-point and no integer overflow occurs.
- Around line 151-178: The rank assignment currently writes ranks in sorted
order into the ranks[] array but later aggregates them in pooled/group order
causing misalignment; change the logic in the ranking block (working with
pooled, sorted, ranks) to assign avgRank into ranks at the original pooled index
(use the pooled index stored in sorted items, similar to how Test does it) so
that ranks[originalPooledIndex] = avgRank, then keep the group aggregation in
meanRanks and groupSizes as-is to compute correct mean ranks per group (refer to
pooled, sorted, ranks, groups, meanRanks).
In `@src/Evaluation/Statistics/NemenyiPostHocTest.cs`:
- Around line 46-52: In NemenyiPostHocTest<T>.Test(T[][] samples, double alpha =
0.05) add input validation: throw ArgumentNullException if samples is null,
throw ArgumentException if samples.Length == 0 or samples[0] is null or
samples[0].Length == 0, verify all inner arrays have the same length and throw
ArgumentException if any row length differs (to avoid jagged array issues), and
validate alpha is one of the supported values (0.01, 0.05, 0.10) throwing
ArgumentOutOfRangeException or ArgumentException otherwise; place these checks
before computing k and n so subsequent uses (k, n and any divisions) are safe
and error messages clearly reference Test and the offending parameter.
🟡 Minor comments (12)
src/Evaluation/Metrics/Regression/PearsonCorrelationMetric.cs-77-89 (1)
77-89:⚠️ Potential issue | 🟡 Minor
ciMethodparameter is accepted but ignored in CI computation.The method accepts
ConfidenceIntervalMethod ciMethodbutBootstrapCIalways uses percentile bootstrap regardless of the value passed. If BCa bootstrap or other methods should behave differently, the implementation needs to branch onciMethod. Otherwise, users may be misled when passingConfidenceIntervalMethod.BCaBootstrap.If only percentile bootstrap is intentionally supported for this metric, consider either:
- Removing the parameter and hardcoding the method in the
MetricWithCIconstructor, or- Validating that only supported methods are passed and throwing for unsupported ones.
src/Evaluation/Statistics/NemenyiPostHocTest.cs-61-61 (1)
61-61:⚠️ Potential issue | 🟡 MinorFix potential integer overflow pattern flagged by static analysis.
The expression
k * (k + 1)performs integer multiplication before the division. While the current k≤10 constraint prevents actual overflow, the pattern should be fixed to satisfy static analysis and prevent future issues if the constraint changes.🔧 Proposed fix
- double cd = q * Math.Sqrt(k * (k + 1) / (6.0 * n)); + double cd = q * Math.Sqrt((double)k * (k + 1) / (6.0 * n));src/Evaluation/Metrics/Regression/RelativeSquaredErrorMetric.cs-62-74 (1)
62-74:⚠️ Potential issue | 🟡 MinorThe
ciMethodparameter is not used in the actual CI computation.The method accepts
ConfidenceIntervalMethod ciMethodbut always performs percentile bootstrap regardless of the value. The parameter is only passed through toMetricWithCIfor metadata, which could mislead callers expecting different CI computation strategies (e.g., BCa bootstrap).Either:
- Implement the different CI methods based on the parameter, or
- Remove the parameter and hardcode
PercentileBootstrap, or- Document that only percentile bootstrap is currently supported and throw
NotSupportedExceptionfor other methods.Option 3: Add validation for unsupported CI methods
public MetricWithCI<T> ComputeWithCI(ReadOnlySpan<T> predictions, ReadOnlySpan<T> actuals, ConfidenceIntervalMethod ciMethod = ConfidenceIntervalMethod.PercentileBootstrap, double confidenceLevel = 0.95, int bootstrapSamples = 1000, int? randomSeed = null) { + if (ciMethod != ConfidenceIntervalMethod.PercentileBootstrap) + throw new NotSupportedException($"Only {nameof(ConfidenceIntervalMethod.PercentileBootstrap)} is currently supported."); if (bootstrapSamples < 2) throw new ArgumentOutOfRangeException(nameof(bootstrapSamples), "Bootstrap samples must be at least 2.");src/Evaluation/Statistics/McNemarTest.cs-42-48 (1)
42-48:⚠️ Potential issue | 🟡 MinorValidate null inputs and alpha range (Line 42).
Current code will throw a less helpful NRE on null inputs and accepts invalid alpha values (<=0 or >=1). Consider explicit guards for API hygiene.
✅ Proposed fix
public StatisticalTestResult<T> Test(T[] correctA, T[] correctB, double alpha = 0.05) { + if (correctA is null) throw new ArgumentNullException(nameof(correctA)); + if (correctB is null) throw new ArgumentNullException(nameof(correctB)); + if (alpha <= 0 || alpha >= 1) + throw new ArgumentOutOfRangeException(nameof(alpha), "Alpha must be between 0 and 1 (exclusive)."); if (correctA.Length != correctB.Length) throw new ArgumentException("Samples must have the same length."); if (correctA.Length < 10) throw new ArgumentException("Need at least 10 observations for McNemar's test.");src/Evaluation/Statistics/KruskalWallisTest.cs-137-141 (1)
137-141:⚠️ Potential issue | 🟡 MinorAdd basic input validation to DunnPostHoc.
DunnPostHocskips the same guards asTest; empty groups or fewer than 2 groups will produce invalid results or division by zero.✅ Suggested fix
public Dictionary<(int, int), double> DunnPostHoc(T[][] groups) { + if (groups == null || groups.Length < 2) + throw new ArgumentException("Need at least 2 groups for Dunn post-hoc test.", nameof(groups)); + if (groups.Any(g => g.Length == 0)) + throw new ArgumentException("Each group must contain at least one observation.", nameof(groups)); int k = groups.Length; int N = groups.Sum(g => g.Length); var results = new Dictionary<(int, int), double>();src/Evaluation/Metrics/Classification/OptimizedPrecisionMetric.cs-30-61 (1)
30-61:⚠️ Potential issue | 🟡 Minor
MinValuedoesn’t match possible negative outputs.
accuracy - penaltycan go below 0 (e.g., low prevalence with penalty = 1). Either clamp the output to 0 or lowerMinValue(and docs) to reflect the true range.🛠️ Proposed fix (align metadata with formula)
- public T? MinValue => NumOps.Zero; + public T? MinValue => NumOps.FromDouble(-1.0);src/Evaluation/Metrics/Classification/LogLossMetric.cs-141-150 (1)
141-150:⚠️ Potential issue | 🟡 MinorSilent skip of out-of-range class indices may mask data errors.
When
trueClassis outside[0, numClasses), the sample is silently ignored. This could hide misconfigured data (e.g., wrongnumClassesparameter or malformed labels). Consider logging a warning or throwing an exception for invalid class indices.🛡️ Proposed fix to validate class indices
for (int i = 0; i < numSamples; i++) { int trueClass = (int)Math.Round(NumOps.ToDouble(actuals[i])); - if (trueClass >= 0 && trueClass < numClasses) - { - double p = Clamp(NumOps.ToDouble(probabilities[i * numClasses + trueClass]), _epsilon, 1 - _epsilon); - loss -= Math.Log(p); - } + if (trueClass < 0 || trueClass >= numClasses) + { + throw new ArgumentException($"Actual class index {trueClass} at position {i} is out of range [0, {numClasses})."); + } + + double p = Clamp(NumOps.ToDouble(probabilities[i * numClasses + trueClass]), _epsilon, 1 - _epsilon); + loss -= Math.Log(p); }src/Evaluation/Engines/LearningCurveEngine.cs-100-112 (1)
100-112:⚠️ Potential issue | 🟡 MinorValidation set fallback may cause train/validation overlap.
When
trainSize == totalSamples, the fallback at lines 107-112 splits the training indices in half for validation. However, sincetrainIndicesis reassigned to half its size, the model is effectively trained on only half the data even though the user requested 100% training size. This behavior should be documented or reconsidered.src/Evaluation/Metrics/Classification/AccuracyMetric.cs-129-131 (1)
129-131:⚠️ Potential issue | 🟡 MinorPotential integer overflow in Wilson score calculation.
The expressions
2 * nand4 * nperform integer multiplication before being used in double division. For very large arrays (n approachingint.MaxValue / 2), this could overflow. Castntodoubleto ensure floating-point arithmetic throughout.🔧 Proposed fix to prevent overflow
- double denominator = 1 + z2 / n; - double center = (p + z2 / (2 * n)) / denominator; - double margin = z * Math.Sqrt((p * (1 - p) + z2 / (4 * n)) / n) / denominator; + double nDouble = n; + double denominator = 1 + z2 / nDouble; + double center = (p + z2 / (2 * nDouble)) / denominator; + double margin = z * Math.Sqrt((p * (1 - p) + z2 / (4 * nDouble)) / nDouble) / denominator;src/Evaluation/Metrics/Classification/MatthewsCorrelationCoefficientMetric.cs-112-145 (1)
112-145:⚠️ Potential issue | 🟡 MinorUse epsilon comparison for floating-point denominator check.
The static analysis correctly flags that
denominator == 0(line 139) is an exact equality check on a floating-point value. Due to floating-point arithmetic, the productd1 * d2 * d3 * d4might not be exactly zero even when one factor is zero afterMath.Sqrt. Use an epsilon comparison instead.🛡️ Proposed fix
- if (denominator == 0) + if (denominator < 1e-15) { return NumOps.Zero; }src/Evaluation/Metrics/Classification/MatthewsCorrelationCoefficientMetric.cs-147-211 (1)
147-211:⚠️ Potential issue | 🟡 MinorUse epsilon comparison for floating-point denominator check.
Same issue as in
ComputeBinaryMCC- line 205 uses exact equalitydenominator == 0on a floating-point value.🛡️ Proposed fix
- if (denominator == 0) + if (denominator < 1e-15) { return NumOps.Zero; }src/Evaluation/Metrics/Classification/DiagnosticOddsRatioMetric.cs-42-68 (1)
42-68:⚠️ Potential issue | 🟡 MinorHardcoded 0.5 threshold inconsistent with other classification metrics.
This metric uses a hardcoded
>= 0.5threshold for binary classification (lines 51-52), while other metrics in this PR (F1Score, FBetaScore, MCC) use a configurablepositiveLabelwith exact matching. This inconsistency could lead to unexpected behavior when:
- Labels are not 0/1 (e.g., -1/+1)
- Users expect consistent behavior across metrics
Consider adding a
positiveLabelconstructor parameter for consistency.♻️ Suggested fix
public class DiagnosticOddsRatioMetric<T> : IClassificationMetric<T> { private static readonly INumericOperations<T> NumOps = MathHelper.GetNumericOperations<T>(); + private readonly T _positiveLabel; + + public DiagnosticOddsRatioMetric(T? positiveLabel = default) + { + _positiveLabel = positiveLabel ?? NumOps.One; + } // ... in Compute method: + double posVal = NumOps.ToDouble(_positiveLabel); for (int i = 0; i < predictions.Length; i++) { - bool pred = NumOps.ToDouble(predictions[i]) >= 0.5; - bool actual = NumOps.ToDouble(actuals[i]) >= 0.5; + bool pred = Math.Abs(NumOps.ToDouble(predictions[i]) - posVal) < 1e-10; + bool actual = Math.Abs(NumOps.ToDouble(actuals[i]) - posVal) < 1e-10;
🧹 Nitpick comments (28)
src/Evaluation/Metrics/Regression/PearsonCorrelationMetric.cs (1)
91-109: Consider improving readability with one statement per line.The bootstrap logic is correct, but lines 97 and 100-101 compress multiple statements onto single lines, making the code harder to read and debug.
♻️ Suggested formatting improvement
private (T, T) BootstrapCI(ReadOnlySpan<T> pred, ReadOnlySpan<T> actual, int samples, double conf, int? seed) { int n = pred.Length; if (n < 2) return (NumOps.FromDouble(-1), NumOps.One); var random = seed.HasValue ? RandomHelper.CreateSeededRandom(seed.Value) : new Random(); var values = new double[samples]; - var predArr = pred.ToArray(); var actArr = actual.ToArray(); + var predArr = pred.ToArray(); + var actArr = actual.ToArray(); for (int b = 0; b < samples; b++) { - var sp = new T[n]; var sa = new T[n]; - for (int i = 0; i < n; i++) { int idx = random.Next(n); sp[i] = predArr[idx]; sa[i] = actArr[idx]; } + var sp = new T[n]; + var sa = new T[n]; + for (int i = 0; i < n; i++) + { + int idx = random.Next(n); + sp[i] = predArr[idx]; + sa[i] = actArr[idx]; + } values[b] = NumOps.ToDouble(Compute(sp, sa)); } Array.Sort(values);src/Evaluation/Metrics/Classification/NegativeLikelihoodRatioMetric.cs (1)
63-75: TheciMethodparameter is accepted but not used to vary the computation.The
ConfidenceIntervalMethod ciMethodparameter is passed toMetricWithCIfor metadata purposes, but the actual CI calculation inBootstrapCIalways uses percentile bootstrap regardless of the method specified. If BCa or other methods are intended to be supported, the implementation should branch onciMethod.If only percentile bootstrap is supported for now, consider either:
- Removing the
ciMethodparameter and hardcodingPercentileBootstrapin the result, or- Validating that
ciMethod == ConfidenceIntervalMethod.PercentileBootstrapand throwingNotSupportedExceptionfor other methods.src/Evaluation/Statistics/NemenyiPostHocTest.cs (1)
132-132: Consider whether the generic type parameterTis necessary.The
NemenyiResult<T>class doesn't use the type parameterTanywhere—all properties are concrete types (double,bool,int). This may be intentional for API consistency with other result types in the framework, but if not, removing it would simplify the API.src/Evaluation/Metrics/Regression/RelativeSquaredErrorMetric.cs (2)
58-59: Consider returning a sentinel value or throwing when SST ≈ 0.When SST is near zero (constant target values), returning
0implies a perfect model, which is misleading if SSE > 0. Consider:
- Returning
NaNorPositiveInfinityto signal an undefined/degenerate case.- Throwing an exception with a descriptive message.
- At minimum, document this behavior in the XML remarks.
The hardcoded
1e-10threshold may also behave unexpectedly for very small-scale data.
89-93: Percentile index calculation may produce degenerate intervals with small sample sizes.With
bootstrapSamples = 2andconfidenceLevel = 0.95:
lo = (int)(0.025 * 2) = 0hi = (int)(0.975 * 2) - 1 = 1 - 1 = 0Both indices are 0, resulting in
lower == upper, which gives a zero-width confidence interval. Consider either:
- Increasing the minimum
bootstrapSamplesvalidation (e.g., >= 100 for meaningful CIs), or- Using a more robust percentile interpolation method.
Increase minimum bootstrap samples
if (bootstrapSamples < 2) - throw new ArgumentOutOfRangeException(nameof(bootstrapSamples), "Bootstrap samples must be at least 2."); + throw new ArgumentOutOfRangeException(nameof(bootstrapSamples), "Bootstrap samples must be at least 100 for meaningful confidence intervals.");src/Evaluation/Metrics/Classification/HammingLossMetric.cs (3)
8-21: Clarify documentation regarding multi-label support.The documentation states Hamming Loss is "the complement of accuracy (1 - accuracy)" and is "particularly useful for multi-label classification." However, these statements are partially inconsistent:
- Hamming Loss = 1 - accuracy only holds for single-label classification.
- For true multi-label classification, Hamming Loss is computed per label across all samples, not as a simple 1-to-1 comparison.
The current implementation handles single-label/multi-class scenarios (comparing one predicted class vs one actual class per sample). Consider updating line 20 to clarify: "Useful for binary and multi-class single-label classification" or expand the implementation to support multi-label input formats.
35-51: Update comment and consider validating actuals are discrete labels.
Line 44: The comment "For binary" is misleading since
SupportsMultiClass => true. The logic correctly handles multi-class single-label classification.Line 46: Rounding
actualsis unusual—ground truth labels should already be discrete integers (0, 1, 2, ...). If actuals are passed as non-integers, it may indicate upstream data issues. Consider either:
- Documenting that this metric accepts soft/probabilistic labels for both inputs, or
- Validating that actuals are already integers and only rounding predictions.
✏️ Suggested comment fix
- // For binary: compare rounded values + // Compare class labels (rounded for probabilistic predictions)
67-85: Consider improving readability and percentile index calculation.
Lines 73, 76-77: The dense formatting reduces readability. Breaking these into separate lines would improve maintainability.
Lines 82-83: The percentile index calculation has a minor off-by-one nuance. For the lower bound,
(int)(alpha / 2 * samples)truncates down, which is correct. However, for exact percentile positioning,Math.FloorandMath.Ceiling(or interpolation) are sometimes used. This is acceptable for bootstrap approximation but worth noting.✨ Suggested readability improvement
- var predArr = pred.ToArray(); var actArr = actual.ToArray(); + var predArr = pred.ToArray(); + var actArr = actual.ToArray(); for (int b = 0; b < samples; b++) { - var sp = new T[n]; var sa = new T[n]; - for (int i = 0; i < n; i++) { int idx = random.Next(n); sp[i] = predArr[idx]; sa[i] = actArr[idx]; } + var sp = new T[n]; + var sa = new T[n]; + for (int i = 0; i < n; i++) + { + int idx = random.Next(n); + sp[i] = predArr[idx]; + sa[i] = actArr[idx]; + } values[b] = NumOps.ToDouble(Compute(sp, sa)); }src/Evaluation/Metrics/Regression/SpearmanCorrelationMetric.cs (1)
124-142: Minor percentile index inconsistency.The lower bound calculation at Line 139 produces index 25 for 1000 samples at 95% CI, but the 2.5th percentile is conventionally at index 24 (0-indexed). The upper bound calculation subtracts 1 but the lower doesn't. While this has negligible practical impact with typical sample sizes, it's inconsistent.
♻️ Optional fix for consistent percentile indexing
Array.Sort(values); double alpha = 1 - conf; - int lo = Math.Max(0, (int)(alpha / 2 * samples)); - int hi = Math.Min(samples - 1, (int)((1 - alpha / 2) * samples) - 1); + int lo = Math.Max(0, (int)Math.Ceiling(alpha / 2 * samples) - 1); + int hi = Math.Min(samples - 1, (int)Math.Floor((1 - alpha / 2) * samples) - 1); return (NumOps.FromDouble(values[lo]), NumOps.FromDouble(values[hi]));src/Evaluation/Metrics/Classification/PrevalenceThresholdMetric.cs (1)
86-91: Consider splitting multi-statement lines for readability.The compressed formatting makes debugging and reviewing harder. While functional, splitting these would improve maintainability:
♻️ Suggested formatting improvement
- var predArr = pred.ToArray(); var actArr = actual.ToArray(); + var predArr = pred.ToArray(); + var actArr = actual.ToArray(); for (int b = 0; b < samples; b++) { - var sp = new T[n]; var sa = new T[n]; - for (int i = 0; i < n; i++) { int idx = random.Next(n); sp[i] = predArr[idx]; sa[i] = actArr[idx]; } + var sp = new T[n]; + var sa = new T[n]; + for (int i = 0; i < n; i++) + { + int idx = random.Next(n); + sp[i] = predArr[idx]; + sa[i] = actArr[idx]; + } values[b] = NumOps.ToDouble(Compute(sp, sa)); }src/Evaluation/Statistics/PairedTTest.cs (2)
53-70: Minor: Effect size representation in zero-variance case.When all differences are identical but non-zero (stdDiff ≈ 0, meanDiff ≠ 0), Cohen's d is mathematically undefined (would be infinite). Setting
EffectSizetoNumOps.Zeromay be misleading since it suggests no effect when there's actually perfect consistency in the difference.Consider using
double.PositiveInfinityordouble.NaNfor the non-zero mean case, or document this behavior in the Description field.Suggested enhancement
// If mean difference is also zero, samples are identical (not significant) // If mean difference is non-zero but variance is zero, it's highly significant double pValueZeroVar = Math.Abs(meanDiff) < 1e-12 ? 1.0 : 0.0; + double effectSizeZeroVar = Math.Abs(meanDiff) < 1e-12 ? 0.0 : double.PositiveInfinity; return new StatisticalTestResult<T> { TestName = Name, Statistic = NumOps.Zero, PValue = NumOps.FromDouble(pValueZeroVar), IsSignificant = pValueZeroVar < alpha, Alpha = alpha, DegreesOfFreedom = n - 1, - EffectSize = NumOps.Zero, + EffectSize = NumOps.FromDouble(effectSizeZeroVar), Description = "All paired differences are identical; t-statistic undefined." };
188-199: Nit: Comment describes Stirling's but implementation is Lanczos approximation.The implementation uses the Lanczos approximation (with coefficients for g=5), which is more accurate than Stirling's approximation. Consider updating the comment for accuracy.
Suggested fix
private static double LogGamma(double x) { - // Stirling's approximation for log(Gamma(x)) + // Lanczos approximation for log(Gamma(x)) double[] coef = { 76.18009173, -86.50532033, 24.01409822, -1.231739516, 0.00120858003, -0.00000536382 };src/Evaluation/Statistics/BootstrapTest.cs (1)
140-141: Consider consistent edge-case handling withTest()method.
Test()returns early whenn < 2, butComputeCI()only checks forn == 0. Withn == 1, the bootstrap will resample the same single pair repeatedly, yielding a degenerate confidence interval whereLower == Upper. Consider aligning the guard condition for consistency.Suggested fix
int n = scores1.Length; - if (n == 0) return (0, 0); + if (n < 2) return (0, 0);src/Evaluation/Statistics/KruskalWallisTest.cs (1)
52-81: Avoid O(N²) lookups when assigning ranks.
pooled.IndexOf(sorted[i])inside the tie loop is quadratic. Carry the pooled index to assign ranks in O(N).♻️ Suggested refactor
- var pooled = new List<(double value, int groupIdx, int sampleIdx)>(); + var pooled = new List<(double value, int groupIdx, int sampleIdx, int pooledIdx)>(); + int pooledIdx = 0; for (int g = 0; g < k; g++) { for (int i = 0; i < groups[g].Length; i++) { - pooled.Add((NumOps.ToDouble(groups[g][i]), g, i)); + pooled.Add((NumOps.ToDouble(groups[g][i]), g, i, pooledIdx++)); } } ... double avgRank = (start + 1 + idx) / 2.0; for (int i = start; i < idx; i++) { - int originalIdx = pooled.IndexOf(sorted[i]); - ranks[originalIdx] = avgRank; + ranks[sorted[i].pooledIdx] = avgRank; }src/Evaluation/Metrics/Classification/LogLossMetric.cs (1)
81-86: Consider usingMath.Clampinstead of custom implementation..NET Core 2.0+ and .NET Standard 2.1+ provide
Math.Clamp(value, min, max)which is more idiomatic.♻️ Suggested refactor
- private static double Clamp(double value, double min, double max) - { - if (value < min) return min; - if (value > max) return max; - return value; - } + private static double Clamp(double value, double min, double max) => Math.Clamp(value, min, max);src/Evaluation/Metrics/Regression/PoissonDevianceMetric.cs (1)
90-108: Consider improving readability with proper line breaks.The compressed formatting on lines 96, 99-100 makes the code harder to read and maintain. This is a minor style concern.
♻️ Suggested formatting improvement
private (T, T) BootstrapCI(ReadOnlySpan<T> pred, ReadOnlySpan<T> actual, int samples, double conf, int? seed) { int n = pred.Length; if (n == 0) return (NumOps.Zero, NumOps.Zero); var random = seed.HasValue ? RandomHelper.CreateSeededRandom(seed.Value) : new Random(); var values = new double[samples]; - var predArr = pred.ToArray(); var actArr = actual.ToArray(); + var predArr = pred.ToArray(); + var actArr = actual.ToArray(); + for (int b = 0; b < samples; b++) { - var sp = new T[n]; var sa = new T[n]; - for (int i = 0; i < n; i++) { int idx = random.Next(n); sp[i] = predArr[idx]; sa[i] = actArr[idx]; } + var sp = new T[n]; + var sa = new T[n]; + for (int i = 0; i < n; i++) + { + int idx = random.Next(n); + sp[i] = predArr[idx]; + sa[i] = actArr[idx]; + } values[b] = NumOps.ToDouble(Compute(sp, sa)); }src/Evaluation/Metrics/Classification/SpecificityMetric.cs (1)
36-65: Edge case: returning 1.0 when no actual negatives may be misleading.When
actualNegatives == 0, returningNumOps.Oneimplies perfect specificity, but there were no negatives to evaluate. Consider returningNaNor documenting this behavior explicitly. However, returning 1.0 is a common convention (no mistakes were made on an empty set), so this is acceptable if documented.The current behavior is defensible but worth documenting in the XML remarks:
/// <para><b>Edge case:</b> Returns 1.0 when there are no actual negatives in the data.</para>src/Evaluation/Metrics/Classification/PositiveLikelihoodRatioMetric.cs (1)
37-41: Consider returning NaN or throwing for empty input instead of 1.Returning
NumOps.One(LR+ = 1) for empty input suggests "no diagnostic value," which may silently hide data issues. Other metrics in this PR returnZerofor empty inputs. Consider aligning with a consistent strategy or throwing an exception.src/Evaluation/Engines/MetricComputationEngine.cs (3)
185-188: Silent exception swallowing hides metric computation failures.The empty catch blocks make it impossible to diagnose why specific metrics fail. Consider logging the exception or collecting failed metric names for diagnostic purposes.
♻️ Proposed improvement to track failed metrics
- catch (Exception) - { - // Skip metrics that fail (e.g., due to data issues) - } + catch (Exception ex) + { + // Optionally track failures for diagnostics + // Consider: collection.AddFailure(kvp.Key, ex.Message); + }Alternatively, add an optional callback or flag in
EvaluationOptions<T>to control whether failures should be logged or collected.
28-28: Unused field:NumOpsis declared but never referenced.The static
NumOpsfield is initialized but not used anywhere in this class. Consider removing it to avoid confusion.🧹 Proposed fix
public class MetricComputationEngine<T> { - private static readonly INumericOperations<T> NumOps = MathHelper.GetNumericOperations<T>(); - private readonly EvaluationOptions<T> _options;
128-131: Registration silently overwrites existing metrics.Using dictionary indexer assignment means registering a metric with a duplicate name silently replaces the existing one. This could hide configuration bugs. Consider throwing or warning on duplicate registration, similar to how
MetricCollection.Add()was updated per the PR description.src/Evaluation/Metrics/Regression/MeanAbsolutePercentageErrorMetric.cs (1)
47-59:validCountis redundant—it always equalspredictions.Length.The
validCountvariable is incremented unconditionally on every iteration, making it identical topredictions.Length. The early return on line 45 already handles the empty case, so the final ternary check is unnecessary.♻️ Proposed simplification
double sum = 0; - int validCount = 0; for (int i = 0; i < predictions.Length; i++) { double actual = NumOps.ToDouble(actuals[i]); double pred = NumOps.ToDouble(predictions[i]); double denominator = Math.Max(Math.Abs(actual), _epsilon); sum += Math.Abs(actual - pred) / denominator; - validCount++; } - return validCount > 0 ? NumOps.FromDouble(100.0 * sum / validCount) : NumOps.Zero; + return NumOps.FromDouble(100.0 * sum / predictions.Length);src/Evaluation/Engines/CrossValidationEngine.cs (1)
205-212: T-value approximation is rough for very small samples.The formula
2.0 + 4.0/nis a reasonable approximation for moderate sample sizes but diverges significantly from actual t-distribution critical values whennis very small (e.g., for n=2, this yields 4.0 vs. actual ~12.7). Consider documenting this limitation or using a lookup table for small n.src/Evaluation/Engines/LearningCurveEngine.cs (1)
153-203: Hardcoded thresholds assume normalized metrics.The diagnosis heuristics use absolute thresholds (0.9, 0.7, 0.1, 0.3) that assume metrics are normalized to [0, 1]. For unbounded metrics like MSE or RMSE, these thresholds won't produce meaningful diagnoses. Consider making thresholds configurable via
LearningCurveOptionsor documenting that diagnosis is only reliable for bounded metrics.src/Evaluation/Metrics/Classification/PrecisionMetric.cs (1)
96-167: Micro precision can be computed in one pass.Micro precision equals overall accuracy, so the nested class loop can be simplified to O(n).
♻️ Proposed refactor
- if (_averaging == AveragingMethod.Micro) - { - return ComputeMicroPrecision(predictions, actuals, classes); - } + if (_averaging == AveragingMethod.Micro) + { + return ComputeMicroPrecision(predictions, actuals); + } @@ - private T ComputeMicroPrecision(ReadOnlySpan<T> predictions, ReadOnlySpan<T> actuals, HashSet<double> classes) + private T ComputeMicroPrecision(ReadOnlySpan<T> predictions, ReadOnlySpan<T> actuals) { - int totalTruePositives = 0; - int totalPredictedPositives = 0; - - foreach (var cls in classes) - { - for (int i = 0; i < predictions.Length; i++) - { - if (Math.Abs(NumOps.ToDouble(predictions[i]) - cls) < 1e-10) - { - totalPredictedPositives++; - if (Math.Abs(NumOps.ToDouble(actuals[i]) - cls) < 1e-10) - { - totalTruePositives++; - } - } - } - } - - return totalPredictedPositives > 0 ? NumOps.FromDouble((double)totalTruePositives / totalPredictedPositives) : NumOps.Zero; + int correct = 0; + for (int i = 0; i < predictions.Length; i++) + { + if (Math.Abs(NumOps.ToDouble(predictions[i]) - NumOps.ToDouble(actuals[i])) < 1e-10) + { + correct++; + } + } + return predictions.Length > 0 ? NumOps.FromDouble((double)correct / predictions.Length) : NumOps.Zero; }src/Evaluation/Metrics/Classification/FBetaScoreMetric.cs (1)
143-162: Minor inconsistency in bootstrap CI index calculation.The index calculation here uses a slightly different formula than other metric files in this PR (e.g.,
BalancedAccuracyMetricusesMath.Floor/Math.Ceilingwith clamping). While both approaches are valid for percentile bootstrap, consider aligning the implementation for consistency and maintainability across the codebase.Current (this file):
int lo = Math.Max(0, (int)(alpha / 2 * samples)); int hi = Math.Min(samples - 1, (int)((1 - alpha / 2) * samples) - 1);Other files use:
int lowerIdx = (int)Math.Floor(alpha / 2 * bootstrapSamples); int upperIdx = (int)Math.Ceiling((1 - alpha / 2) * bootstrapSamples) - 1; // then clamp bothsrc/Evaluation/Metrics/Classification/F1ScoreMetric.cs (1)
163-218: Consider reusing FBetaScoreMetric internally.
F1ScoreMetricis functionally equivalent toFBetaScoreMetricwithbeta=1. To reduce code duplication and maintenance burden, consider delegating toFBetaScoreMetricinternally.♻️ Example refactor approach
public class F1ScoreMetric<T> : IClassificationMetric<T> { private readonly FBetaScoreMetric<T> _inner; public F1ScoreMetric(T? positiveLabel = default, AveragingMethod averaging = AveragingMethod.Binary) { _inner = new FBetaScoreMetric<T>(beta: 1.0, positiveLabel, averaging); } public T Compute(ReadOnlySpan<T> predictions, ReadOnlySpan<T> actuals) => _inner.Compute(predictions, actuals); // ... delegate other methods similarly }src/Evaluation/Metrics/IMetric.cs (1)
106-162: Consider adding ComputeWithCI to IRankingMetric and ITimeSeriesMetric.
IRankingMetric<T>andITimeSeriesMetric<T>lack theComputeWithCImethod that other metric interfaces provide. If this is intentional (e.g., CI computation is less common for these metric types), consider documenting this design decision. Otherwise, adding CI support would provide a consistent API across all metric types.
…dom() Replace all 75 usages of `new Random()` with `RandomHelper.CreateSecureRandom()` in the Evaluation module to address cryptographic security concerns. Files affected: - 36 classification metrics - 26 regression metrics - 8 cross-validation strategies - 3 evaluation engines - 2 statistics tests Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 130 out of 130 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
ooples
added a commit
that referenced
this pull request
Jul 18, 2026
Brings the #801/#803 GPU kernels into the dependency: weight-only dequant-GEMM (all 6 backends), fused-epilogue GEMM, and paged-attention kernels + DevicePagedKVCache. At 0.114.0 these did not exist, so the process-wide GPU engine (set by the facade's auto-detect) could not use them; the general model forward can now dispatch to the 0.115.x GPU kernels. NOTE: this does NOT yet wire the serving batcher's paged-attention path onto the Tensors GPU DevicePagedKVCache — that path still uses AiDotNet's own CPU PagedAttentionKernel and is the next piece of GPU-serving work. Full serving suite green on 0.115.1 (267 tests); the earlier concurrent-forward concern does not reproduce. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
ooples
added a commit
that referenced
this pull request
Jul 18, 2026
Brings the #801/#803 GPU kernels into the dependency: weight-only dequant-GEMM (all 6 backends), fused-epilogue GEMM, and paged-attention kernels + DevicePagedKVCache. At 0.114.0 these did not exist, so the process-wide GPU engine (set by the facade's auto-detect) could not use them; the general model forward can now dispatch to the 0.115.x GPU kernels. NOTE: this does NOT yet wire the serving batcher's paged-attention path onto the Tensors GPU DevicePagedKVCache — that path still uses AiDotNet's own CPU PagedAttentionKernel and is the next piece of GPU-serving work. Full serving suite green on 0.115.1 (267 tests); the earlier concurrent-forward concern does not reproduce. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
ooples
added a commit
that referenced
this pull request
Jul 22, 2026
…code, prefix sharing, speculation) (#1888) * feat(serving): openAI-compatible API, streaming generation, and serving benchmark Add an OpenAI-compatible surface to AiDotNet.Serving plus a backend-agnostic serving benchmark so AiDotNet can be measured apples-to-apples against vLLM/TGI. Serving (src/AiDotNet.Serving): - /v1/chat/completions, /v1/completions, /v1/models (OpenAiController): SSE streaming + non-streaming, usage accounting, stop strings, OpenAI error shape - ITextGenerationService.GenerateStream: true incremental token streaming with temperature/top-p/top-k sampling (SampleToken/LastPositionLogits helpers) - ITokenizerRegistry: per-model text<->token-id bridge; LoadModelRequest gains TokenizerPath (registered on load, removed on unload) - ChatTemplate: neutral tokenizer-agnostic default; SpeculativeDecodingRequest gains TopP/TopK Benchmarks (benchmarks/): - AiDotNet.Serving.Benchmarks (adnbench): backend-agnostic load tester (OpenAI SSE + native token-id); TTFT/TPOT/ITL/E2E/throughput/goodput; Poisson arrivals - AiDotNet.Serving.DevHost: dev-only host wiring the real OpenAI controller + engine with a synthetic model (no auth/DB) for end-to-end measurement Verified: builds clean (0 errors); dev-host run = 100/100 requests through /v1 chat SSE. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(serving): add min-p sampling to the live generation path Salvaged from the (now-abandoned) #1881 engine the one sampling knob the existing serving path lacked: min-p (drop tokens below minP x the top token's probability, applied before top-p). Added to SpeculativeDecodingRequest + the OpenAI request DTOs (min_p) and threaded through TextGenerationService.SampleToken. Everything else in #1881 duplicated existing infra (ContinuousBatcher/PagedKVCache/ SpeculativeDecoder/RadixAttention) and is not carried over. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * feat(serving): route streaming generation through a shared per-model ContinuousBatcher The streaming path bypassed the batcher entirely (per-request IGenerationSession), so concurrent requests never batched together. Now TextGenerationService holds one shared, continuously-running ContinuousBatcher per (model, numeric type); streaming requests are enqueued into it and drained via the request's OnTokenGenerated callback (push -> pull bridge over a BlockingCollection). Concurrent HTTP requests to a model now share one in-flight batch — the actual throughput win. - The batcher owns sampling; threaded TopP/TopK + added MinP (min-p) into GenerationRequest<T> and ContinuousBatcher.SampleFromLogits (previously only Temperature reached the batcher; min-p was missing everywhere). - TextGenerationService is IDisposable and disposes its batchers. Non-stream GenerateTyped still uses a per-request batcher — routing it through the shared instance + flowing InferenceOptimizationConfig into the batcher config are the next increments. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * Revert "feat(serving): route streaming generation through a shared per-model ContinuousBatcher" This reverts commit 4bbd8dca10093cec2e0205fbba06db200cc4bd0f. * feat(serving): continuousBatcher paged incremental decode (Phase 1a of unified engine) Adds the fast path to the continuous batcher: a context-aware constructor taking the optimized model + a shared PagedKVCache, and paged prefill/decode that call model.PredictWithContext(tokens, ctx(sequenceId, position)) so each sequence decodes incrementally against its own paged KV (O(1)/step) instead of the stateless full-context recompute. Sequences are isolated by SequenceId in the shared cache; blocks are freed on completion. Speculation on the paged path falls back to paged single-token decode for now (Phase 2). The stateless Func path is preserved as the capability fallback for models that cannot build the paged path. This is the core of merging the two half-engines (batcher scheduling + session's paged KV). Not yet routed to the live OpenAI path (next phase) so behavior is unchanged: 40 ContinuousBatcher unit tests + 12 IncrementalGenerationEndToEnd tests stay green; main build 0 errors. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(serving): remove null-forgiving operators from paged batcher path Replace _pagedCache! / _incrementalModel! with pattern-matched non-null locals (is not { } x -> guard), per the strict no-null-forgiving-operator rule. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * feat(serving): per-sequence seeded RNG + greedy + min-p in batcher sampler (Phase 1b) Makes the batcher's sampling deterministic and feature-complete so it can replace the session path without regressing: - Per-sequence RNG seeded from GenerationRequest.Seed (else cryptographically secure), persisted per sequence and freed on completion — replaces the shared ThreadSafeRandom that made batched sampling non-deterministic. - Greedy branch (temperature <= 0 -> argmax) — the sampler previously divided by temperature with no guard (broke temp=0 / greedy). - min-p (GenerationRequest.MinP + ApplyMinP, before top-p) restored on the batcher sampler. SampleFromLogits now takes the SequenceState (for its RNG + request). No null-forgiving operators. 40 batcher unit + 12 incremental tests green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(serving): prove paged batcher == session path greedy output (Phase 1c) Expose the wrapper's incremental model + shared PagedKVCache (internal accessors) so ONE continuous-batching engine can drive the same model + cache the GenerationSession path uses, and add equivalence tests proving they are interchangeable before the live path routes through the batcher: - greedy output is byte-for-byte identical to the session path - two concurrent sequences in one batcher (shared cache) each match their sequential greedy reference — isolation holds under continuous batching - seeded sampling (temperature>0) is reproducible run-to-run No null-forgiving operators (nullable accessors narrowed via `is null` guards / Assert.Fail). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * feat(serving): radixAttention prefix sharing in the paged batcher (Phase 1d) Extract the prefix registry (previously private to ServableModelWrapper) into a core RadixPrefixCache<T> keyed on the shared PagedKVCache, and wire it into ContinuousBatcher's paged prefill so ONE engine reuses prompt prefixes copy-on-write: - new prompts fork the longest registered STRICT prefix and forward only the suffix - base sequence ids are minted from a disjoint negative range, so a prefix base can never collide with a live (positive) sequence id in the shared cache - LastPrefillReusedPrefixTokens exposes the reuse count for tests/telemetry The suffix after a fork is forwarded ONE token at a time (the proven prefix-transparent path). A single multi-token forward at a non-zero offset over forked KV is NOT yet validated (cached-attention position handling for offset multi-token inputs — the batched-prefill / ComputeBatchedAttention work) and diverges, so it is intentionally deferred to Phase 2. Test proves prefix sharing is transparent (identical greedy output with vs without reuse) on ONE model+cache, and that the fork actually happened (reuse == 4). No null-forgiving operators. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * perf(serving): batched suffix prefill after prefix fork (Phase 1d follow-up) A dedicated diagnostic confirmed a multi-token forward at a non-zero offset over forked KV is byte-for-byte equivalent to the per-token forward (CachedMultiHeadAttention position handling is correct) — so the earlier divergence was a two-wrapper nondeterminism artifact in the test, not a kernel bug. Restore the SINGLE batched forward of the (cached..promptLen) suffix from the forked position instead of forwarding it one token at a time: exact, and the batched-prefill throughput win we need to exceed vLLM. Prefix-equivalence + incremental tests stay green (16/16). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * feat(serving): greedy-exact prompt-lookup speculation on the paged batcher path Wire draft-model-free speculative decoding into the batcher's paged decode (previously it fell back to single-token decode). Each greedy decode step: forward the last token for the target's greedy next token, draft up to SpeculationDepth tokens by matching the trailing n-gram against the running stream, verify them in ONE multi-token forward over the paged KV, accept the longest greedy-matching prefix, roll the paged KV back over rejected drafts (TruncateSequence), and emit the correction. Emitted tokens are byte-for-byte identical to plain greedy decode (verified) — speculation only changes how many forwards it takes. Acceptance rate is tracked and surfaced via SpeculationAcceptanceRate. Prompt-lookup is exact only for greedy, so it engages only when Temperature <= 0; sampled requests decode normally. Also add per-request EosTokenId to GenerationRequest (a shared batcher serves requests with different stop tokens; AccountForToken now honors it, falling back to the batcher default). Test proves greedy == spec and acceptance > 0 at the batcher level. No null-forgiving operators. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * feat(serving): route all generation through ONE shared batcher + delete session path (Phase 1e) TextGenerationService.Generate/GenerateStream now submit to a single shared ContinuousBatcher per model (paged KV, RadixAttention prefix sharing, greedy-exact prompt-lookup speculation, per-request temperature sampling), driven by a synchronous cooperative driver (Step() under an engine lock; whoever holds it advances the whole batch, others spin — concurrent requests co-batch, forwards stay serialized, no background thread). Sampling params (temperature/top-p/top-k/min-p/eos/speculation-depth/seed) travel per request; temperature 0 = greedy, >0 = sampling (matches OpenAI/vLLM; the old non-streaming path was secretly greedy-only). Fixes the prefill nondeterminism that this exposed: sequence-collapsing models (Flatten head, SupportsBatchedPrefill=false) must prefill/decode one token at a time, else a shape-dependent head re-fits its weights with an unseeded RNG. ContinuousBatcherConfig.SupportsBatchedPrefill gates this. The GenerationSession path is now production-dead and removed entirely (unreleased, no shim): IGenerationSession/GenerationSession/BeginGeneration, the wrapper prefix registry, and the dead GenerateIncremental/SpeculativeDecodeLoop. The 3 session-API tests are superseded by the batcher equivalence tests; the greedy reference is re-targeted to a direct model forward. Serving suite 14 tests green + stable 3x; 26 batcher unit tests green; net10.0 + net471 build clean. No null-forgiving operators. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * feat(serving): batched paged decode — one forward serves all active sequences (Phase 2) True continuous batching: instead of one model forward per decoding sequence, the batcher now serves the whole decode batch in ONE forward. - InferenceForwardContext gains a BATCHED mode (per-row SequenceIds[]/Positions[]): batch row b belongs to sequence SequenceIds[b] at Positions[b]. - PagedCachedMultiHeadAttention.ComputePagedAttention generalizes its per-token loop to iterate the batch dimension, each row using its own sequence id + position over the shared paged KV cache (replacing the batchSize==1 restriction). - ContinuousBatcher.Step() collects the decoding sequences and, on the paged path with speculation off and >1 sequence, runs RunBatchedPagedDecodeStep: a [batch,1] input + batched context, one forward, per-row sampling. Speculation / single-sequence keep the per-sequence path. - SampleFromLogits takes a batchIndex to select each row's logits. Per-row output is byte-for-byte identical to per-sequence decode (proven at the model level and via a 3-sequence co-batch test that also asserts LastBatchedDecodeCount >= 2). Works for sequence-collapsing (Flatten) models too, since decode is always seqLen==1 per row. net10.0 + net471 build clean; 9 incremental + 7 equivalence + 26 batcher unit tests green. No null-forgiving operators. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * feat(serving): batched paged prefill — prefill all new sequences in one padded forward (Phase 2) Extends batched decode to the prefill side: when >1 new sequence is scheduled on the paged path, the batcher prefills them ALL in ONE right-padded forward instead of one forward per prompt. - InferenceForwardContext.RowLengths carries each row's valid token count. PagedCachedMultiHeadAttention processes only rowLengths[b] tokens per row; the padded tail writes no KV and its output stays zero. - ContinuousBatcher.RunBatchedPagedPrefill pads prompts to the max length, forwards once, and samples each sequence's first token from its own last real position (SampleFromLogits lastPositionOverride). Prefix reuse is skipped for the batched group (uniform start position 0); allocation failure rolls back and falls back to per-sequence prefill. Per-row output is identical to per-sequence prefill — covered by ConcurrentRequests (lengths 3,2) and PagedBatcher_MultiSequence (lengths 3,2,4), both asserting equality with sequential references. net10.0 + net471 clean; 16 serving + 26 batcher unit tests green. No null-forgiving operators. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * feat(serving): chunked prefill — interleave a long prefill with ongoing decode Opt-in ContinuousBatcherConfig.MaxPrefillChunkTokens (default 0 = whole prompt). When > 0, a long prompt is prefilled in chunks across successive steps instead of one large forward, so it does not block ongoing decode: - SequenceState.PrefillCursor tracks how many prompt tokens are cached so far (starts at the reused prefix length). - RunPagedPrefill forwards the next MaxPrefillChunkTokens-sized chunk from the cursor (paged suffix forward at the cursor position), emitting the first token only when the cursor reaches the prompt end; otherwise it stays Prefilling for the next step. - RunPrefill marks complete via the cursor, not unconditionally. - Batched (whole-prompt) prefill is used only when chunking is off (alternative strategies). Sequence-collapsing models prefill per-token in one step (unchanged). Chunked (2-token) prefill is byte-identical to whole-prompt prefill (new test). net10.0 + net471 clean; 17 serving + 26 batcher unit tests green. No null-forgiving operators. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * feat(serving): batched speculative decode for same-draft-length groups When speculation is on and multiple sequences decode together, the batcher now co-batches their speculative forwards instead of running each sequence's greedy + verify passes separately: - RunPagedDecodeBatch drafts each generating sequence (prompt-lookup) and partitions them: no-draft / ineligible sequences decode via one batched forward; drafted sequences are grouped by draft length. - RunBatchedPagedSpeculativeStep serves each same-draft-length group with ONE batched greedy forward ([B,1]) + ONE batched verify forward ([B,K]), then per-row accept/roll-back/emit. - The accept/roll-back/emit logic (steps 4-6) is factored into EmitSpeculativeRow, shared by the per-sequence and batched paths; ArgMaxAtBatchPosition indexes a batched [B,S,vocab] verify tensor. Output stays byte-for-byte identical to per-sequence speculation (== plain greedy). New test: two identical prompts co-batch their verify, match greedy references, and accept drafts (rate > 0). Single-sequence speculation and the non-speculative batched path are unchanged. net10.0 + net471 clean; 18 serving + 26 batcher unit tests green. No null-forgiving operators. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * perf(serving): drive the shared batcher from ONE background loop (fixes concurrency collapse) The benchmark exposed that the cooperative synchronous driver collapsed throughput under load: each request thread competed to drive Step() and spun/busy-looped while another drove, stealing CPU from the model forward's internal parallelism. Output throughput went DOWN with concurrency (c1 1085 -> c8 268 tok/s). Switch to the standard serving design: the shared ContinuousBatcher runs ONE background loop (AutoStart) that is the sole thread running forwards; request threads just await their result (RunGeneration) or block-consume tokens off a BlockingCollection the loop fills (StreamGeneration). No engine lock, no spin. This was safe to adopt once the real cause of the earlier background-loop "flakiness" was fixed (the Flatten per-token-prefill bug) — the loop is a single forward thread, so forwards stay serialized and per-request output is deterministic (RNG seeded per request; batched rows isolated by sequence id). Result: throughput now SCALES with concurrency — c1 568 -> c8 2257 tok/s (~4x), vs the old collapse. Serving suite deterministic 3x (18 tests, incl the Flatten-model RepeatedRequests/ConcurrentRequests); 26 batcher unit tests green; net10.0 + net471 clean. Also: DevHost serves a small real per-position transformer LM by default (was a synthetic Func that routed the stateless path) so the benchmark actually drives the paged batched engine; DEVHOST_SYNTHETIC=1 restores the pure-overhead synthetic forward. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * perf(serving): wake the batcher loop on request arrival (single-stream latency) The background run loop idled via Task.Delay(IdleSleepMs=10ms), so a request arriving right after the loop went idle waited up to 10ms for pickup — the main single-stream latency cost of moving to a background loop. Add a SemaphoreSlim the loop waits on when idle and GenerateAsync releases on enqueue, so the loop wakes immediately (falling back to the idle interval as a periodic re-check). Over-signaling is harmless (a spurious no-op Step). Disposed with the batcher; cancellation still breaks the wait. Single-stream output throughput 568 -> 866 tok/s; concurrency-8 2257 -> 2902 tok/s (peak). Throughput still scales with concurrency (c1 866 -> c8 2902, ~3.4x). 18 serving + 26 batcher tests green; net10.0 + net471 clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(serving): don't cross-sequence batch prefill for sequence-collapsing models RunBatchedPagedPrefill pads variable-length prompts to a common length, but a Flatten-head model collapses the padded width into its head input, so the batched forward diverged from a single-prompt forward (ConcurrentRequests, Flatten model). Gate the batched-prefill path on SupportsBatchedPrefill (the single-sequence prefill path already did this); sequence-collapsing models prefill per sequence. Latent bug that Tensors 0.114.0 tolerated by chance; a numerics change exposed it. No null-forgiving operators. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * feat(serving): flow facade InferenceOptimizationConfig into the paged batcher A facade-built generative model is an AiModelResult<T, Tensor, Tensor> that wraps an inner NeuralNetwork and carries the user's ConfigureInferenceOptimizations settings. ServableModelWrapper.FromModel previously failed the ctor's `model is NeuralNetworkBase` check for such a facade, so it skipped the paged incremental path entirely and the batcher used hardcoded serving defaults. FromModel now detects the AiModelResult facade, unwraps its inner network (whose Predict is the raw token->logits forward, without the facade's I/O normalization), and threads GetInferenceOptimizationConfigForServing() through. EnsureBatcher derives EnableSpeculativeDecoding, SpeculationDepth, SpeculationPolicy, UseTreeSpeculation, and scheduler MaxBatchSize from that config with serving-side fallbacks for raw (non-facade) models. Adds InferenceOptimizationConfig.Clone() so the wrapper can force paged-path-essential overrides without mutating the user's config. Test FromModel_FacadeResult_UnwrapsInnerNetwork_AndHonorsInferenceConfig proves a facade model serves through the paged path honoring a distinctive config (SpeculationDepth=7, EnableSpeculativeDecoding=false, MaxBatchSize=13). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(licensing): let internal-operation scope suppress save trial counting EnforceBeforeSave deliberately ignored the InternalOperation() scope (unlike EnforceBeforeLoad, EnforceBeforeSerialize, and EnforceBeforeDeserialize, which all honor it). Server-side infrastructure — the federated coordinator's per-round global-model checkpoint (FederatedCoordinatorService.cs:582), serving controllers, registry writers — wraps its internal SaveModel calls in InternalOperation to declare "not a user-driven save", but the save counted anyway. A long-running federated run therefore exhausted the 10-op trial against its own checkpoints and threw LicenseRequiredException mid-training. EnforceBeforeSave now short-circuits on _internalOperationDepth, symmetric with EnforceBeforeLoad. InternalOperation is an internal API not reachable from user code, so user-facing SaveModel (depth 0) still counts against the trial — the end-user licensing contract is unchanged. Updates the three guard tests that pinned the old asymmetry to the new symmetric contract, keeping the depth-0 assertions that prove user saves still count. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * feat(serving): structured/guided decoding — token-constraint masking in the sampler Adds the foundation for structured output (JSON / regex / grammar / choice-constrained generation), matching vLLM/TGI/SGLang guided decoding. A per-sequence ITokenConstraint masks the logits before sampling so the model can only emit tokens that keep the output within the required format, while still choosing freely among valid tokens. - ITokenConstraint: ApplyMask(Span<float>) sets forbidden tokens to -inf, Accept(token) advances state, IsComplete reports an accepting/terminal state. Tokenizer-agnostic (speaks token ids), so it composes with any registered tokenizer and every backend. - TokenFsmConstraint: a compiled token-level DFA doing the masking + advance, with FromSequence (exact token sequence) and FromChoices (enum/choice via a shared-prefix trie) factories. Higher-level regex/JSON-schema compilers will lower to this. - ContinuousBatcher.SampleFromLogits applies the mask after logit extraction (composes with temperature/min-p/top-p/top-k) and advances the constraint through a single exit point. Speculative decoding is force-disabled for constrained requests (the greedy draft/verify path bypasses the mask), overriding even ForceOn. - GenerationRequest.Constraint carries the per-request constraint. Tests: FSM masking/advance (sequence + shared-prefix choices) and an end-to-end batcher test proving a constrained request emits only permitted tokens even when the model always prefers a forbidden token (with speculation enabled in config, proving the gate). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * feat(serving): regex-guided decoding — RegexTokenConstraint (regex -> token DFA) Compiles a regular expression to a character-level Thompson NFA and runs a lazy on-the-fly DFA over it to decide, at each step, which vocabulary tokens keep the output on a path that can still complete a full match. This is regex-guided decoding (outlines/xgrammar style) and the lowering target for JSON-schema and grammar constraints. Supported syntax: literals, ., character classes [a-z0-9_] with ranges and negation [^...], shorthands \d \w \s (and negated), escapes, quantifiers * + ? and bounded {n} {n,} {n,m}, alternation |, and (non-capturing) grouping. Tokenizer-agnostic: constructed with the vocabulary's token->string pieces, so it judges whole multi-character tokens by feasibility and works on every backend. Per-DFA-state allowed-token masks are memoized. Tests: exhaustive acceptance comparison against System.Text.RegularExpressions.Regex over a small alphabet (10 patterns incl. the overlapping-prefix alternation trap (ab|a)b and bounded repetition), plus targeted prefix-feasibility (ISO-date shape forbids letters at digit slots) and multi-character-token masking (true|false). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * feat(serving): jSON-schema-guided decoding — JsonSchemaConstraint Compiles a JSON Schema (subset) to a regex and reuses RegexTokenConstraint so a model can be forced to emit compact JSON conforming to the schema — the mechanism behind OpenAI-compatible response_format: { type: "json_schema" }. Supported: object with ordered properties (all required, declared order), scalar types string/integer/number/boolean/null, string enum and pattern, array of a scalar item type, and a top-level enum. AnyJsonObject(maxDepth) provides bounded-nesting "any valid JSON" for the json_object mode (JSON is context-free, so a finite regex bounds nesting depth). Tests: valid compact JSON accepted; malformed/mistyped rejected (wrong property order, missing required key, unquoted string, integer-as-string, non-integer); string enum + boolean; array of integers (trailing comma / non-integer rejected); bounded any-object; invalid schema throws. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * feat(serving): wire OpenAI response_format to guided decoding end-to-end Exposes structured output through the OpenAI-compatible API. `response_format` on /v1/chat/completions and /v1/completions is compiled to an ITokenConstraint and flows through the engine so the model is forced to conform: - { "type": "json_object" } -> any valid (bounded-depth) compact JSON object - { "type": "json_schema", "json_schema": {..} } -> JSON matching the schema - { "type": "regex", "regex": "..." } -> AiDotNet extension: output matches the regex - { "type": "text" } / absent -> unconstrained (default) - StructuredOutputFactory parses response_format and builds the constraint over the model's tokenizer vocabulary (token id -> decoded piece, cached per tokenizer). Malformed response_format -> 400. - SpeculativeDecodingRequest.Constraint carries it; BuildGenerationRequest threads it into the engine request. - ContinuousBatcher stops a sequence the moment its constraint reaches a complete valid instance (guided decoding shouldn't depend on the model emitting EOS to terminate a closed JSON object). - Fix a pre-existing bug: the full-context fallback path built its GenerationRequest inline and dropped EOS/top-p/top-k/min-p/seed/speculation — now it uses BuildGenerationRequest like the incremental path, so those (and the new Constraint) are honored on both paths. Tests: factory parsing (all response_format kinds + malformed -> throw) and an end-to-end flow proving a constraint travels through TextGenerationService into the batcher and stops exactly when complete (not at MaxNewTokens), with speculation requested (proving the gate). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(serving): prove guided-decoding constraint holds on the incremental paged path The earlier structured-output end-to-end test drove the stateless full-context fallback. This adds a test on the PRODUCTION path — the KV-cached incremental paged batcher — asserting a token constraint forces the exact output sequence there too and stops the instant the constraint completes (with speculation requested, proving the gate on that path). Full serving suite green (264 tests). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * feat(serving): openAI logit_bias support (per-token biasing/banning) Adds the OpenAI logit_bias parameter (token id -> additive bias): positive values make a token more likely, large negatives (e.g. -100) effectively ban it. Applied in the sampler before any structured-output mask so the two compose (a banned token stays banned; a biased-up token can still be vetoed by a format constraint). Wired end-to-end: GenerationRequest.LogitBias + SpeculativeDecodingRequest.LogitBias + BuildGenerationRequest, the logit_bias field on chat/completions DTOs, and controller parsing (integer token-id keys -> float bias; malformed -> 400). Test: a large negative bias bans the model's preferred token and a large positive bias forces another to win. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * feat(serving): openAI frequency_penalty and presence_penalty Implements the two OpenAI repetition-control parameters (the pre-existing GenerationRequest .RepetitionPenalty field was declared but never applied — dead). In the sampler each token's logit is reduced by presence_penalty (once if it has appeared) + frequency_penalty x (times it has appeared in the text so far), applied before logit_bias and the structured-output mask so all three compose. Wired GenerationRequest -> SpeculativeDecodingRequest -> BuildGenerationRequest, the frequency_penalty/presence_penalty fields on the chat/completions DTOs, and controller mapping. Test: with the model narrowly preferring one token, a frequency penalty breaks the greedy repetition loop and forces token variety. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * feat(serving): engine-level per-token log-probabilities (OpenAI logprobs core) Adds log-probability recording to the continuous-batching sampler. When a request sets IncludeLogProbs, each decode step records the chosen token's log-probability plus the top-N alternatives (TopLogProbs), computed as log(softmax(logits / effective-temperature)) from the post-penalty/bias/mask logits BEFORE top-p/top-k/min-p truncation (those are sampling filters, not the model's probability). Results surface on GenerationResult.LogProbs. Types: TokenLogProb + PositionLogProbs. Masked (-inf) tokens are excluded from the top-K. Greedy and sampling branches share the same distribution via a single Finalize exit point. Test: ranked logits -> chosen token is the argmax, its logprob is negative and matches the analytic value, and top_logprobs are the correct tokens in descending order. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * feat(serving): return logprobs/top_logprobs through the OpenAI API Wires the engine-level log-probabilities to the OpenAI-compatible surface. chat/completions accepts logprobs (bool) + top_logprobs (0-20); the non-streaming response includes the standard choices[].logprobs.content[] structure ({ token, logprob, top_logprobs[] }) with token ids decoded to strings via the model tokenizer. - SpeculativeDecodingRequest.Logprobs/TopLogprobs -> BuildGenerationRequest; response carries the per-token PositionLogProbs; both the incremental and full-context paths populate it. - The controller's non-streaming path uses the batch Generate() when logprobs are requested (the streaming Collect path doesn't surface them) and builds the OpenAI logprobs object. - Speculative decoding is disabled when logprobs are requested: the greedy draft/verify path emits accepted tokens via argmax and bypasses the per-token sampler, so their distributions (and thus logprobs) are never computed. Mirrors the structured-output gate. Test: logprobs flow through TextGenerationService — one entry per generated token, chosen token is the argmax, logprob negative, top_logprobs the correct tokens/count. Full serving suite green (265). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * feat(serving): openAI n (multiple chat completions) The non-streaming /v1/chat/completions path now honors n, generating n independent completions of the same prompt and returning them as choices[0..n-1]. At temperature > 0 each draws from its own RNG so they differ; at temperature 0 (greedy) they are deterministically identical, as expected. Usage.completion_tokens sums across all choices. Test: a controller-level test (constructed directly, no HTTP host) with n=3 returns three indexed choices, each with a message. Full serving suite green (266). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * feat(serving): openAI function calling (tools) via guided decoding Implements OpenAI tools / function calling by reusing the structured-decoding engine: each tool's JSON-schema parameters is compiled to a regex and the model is constrained to emit {"name":"<fn>","arguments":<args-matching-that-tool's-schema>}. Multiple tools become an alternation; tool_choice selects a specific function ("required"/"auto" allow any, "none" disables). - ToolConstraintFactory builds the constraint (reusing StructuredOutputFactory's cached vocab and JsonSchemaConstraint.CompileToRegex) and parses the completed JSON back into choices[].message .tool_calls[] with finish_reason "tool_calls" (content null). - DTOs: tools + tool_choice on the request; tool_calls on the message; ToolDefinition / FunctionDefinition / ToolCall / FunctionCallOut. - Constrained requests (tools or logprobs) run through the batch generation path, which reliably drives a constrained generation to completion (the streaming Collect path is for free-form output). Test: an enum-constrained tool schema makes the whole tool-call JSON deterministic; the controller returns a valid set_status tool call with {"status":"on"} arguments regardless of the model's logits. Full serving suite green (267). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * feat(lora): merge LoRA adapters into a base model for serving (multi-LoRA slice 1) Adds LoRAAdapterMerger.MergeInPlace, which bakes every mergeable ILoRAAdapter layer in a network into a plain layer (reusing the existing DenseLoRAAdapter.MergeToOriginalLayer, W' = W + scaling·B·A). This is the standard way to deploy a LoRA-fine-tuned model as its own servable variant: merge, then register/serve it through the existing ServableModelWrapper.FromModel path. Adapters whose type does not support merging are left in place (they still compute correctly). This is slice 1 of multi-LoRA serving (merged variants — one model per adapter, no batcher changes). Slice 2 (shared-base S-LoRA: one base + per-request adapter swap via the existing SLoRAAdapter) is the follow-up. Tests: merging a non-trivially-adapted model preserves its output exactly (merged inference == adapter inference) and removes all adapter layers; a model with no adapters is a no-op. net10.0 + net471 clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * feat(serving): shared-base multi-LoRA (S-LoRA) per-request adapter selection Multi-LoRA serving slice 2: one shared base model serves many LoRA adapters, selected per request, without reloading the model. Reuses the existing MultiLoRAAdapter machinery. - LoRAAdapterSelection.SelectTask/HasMultiLoRAAdapters: the shared "switch every adapter layer to task X" primitive. AiModelResult's inference-session path is refactored to call it too, so the selection logic lives in ONE place (was duplicated inline). - Batcher: GenerationRequest.AdapterId flows through; before each of a sequence's forwards the batcher switches the shared model's adapter (ApplyAdapter). Sequences that request an adapter decode/prefill per-sequence rather than through a co-batched forward (the shared model's active adapter is global, and the single-loop driver makes sequential per-sequence selection race-free). No overhead when no model has adapters (gated on a cached _hasMultiLoRA). - OpenAI: the model field "baseModel@adapter" selects a per-request adapter (SplitModelAndAdapter); SpeculativeDecodingRequest.AdapterId -> BuildGenerationRequest. Test: a shared base with two distinct adapters + a zero base task — selecting task A vs B vs base produces three different outputs, proving the shared-base swap. net10.0 + net471 clean; 267 serving tests green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * feat(serving): unbounded-depth JSON grammar constraint (pushdown automaton) Adds JsonGrammarConstraint — a character-level pushdown automaton that constrains output to any well-formed JSON value of arbitrary nesting depth. JSON is context-free, so bounded regex can only express finite nesting; this PDA (a stack of open containers + strict scalar sub-states for strings/numbers/keywords) handles unbounded depth, matching xgrammar-class guided decoding. response_format: { type: "json_object" } now uses this instead of the finite bounded-depth regex, so deeply nested JSON is permitted. Tokenizer-agnostic; per token it tests whether appending keeps the output a viable JSON prefix, EOS allowed once a complete top-level value is read. Tests: exhaustive acceptance vs the strict System.Text.Json parser over ~16k strings (Newtonsoft is too lenient — it accepts "00" etc. — so it can't validate a strict grammar); depth-20 nested arrays + nested objects (beyond any fixed regex depth); malformed rejection (trailing comma, missing value, unquoted key, unterminated); true/false/null keywords; per-step structural enforcement. net10.0 + net471 clean; serving suite green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * chore(deps): bump AiDotNet.Tensors 0.114.0 -> 0.115.1 Brings the #801/#803 GPU kernels into the dependency: weight-only dequant-GEMM (all 6 backends), fused-epilogue GEMM, and paged-attention kernels + DevicePagedKVCache. At 0.114.0 these did not exist, so the process-wide GPU engine (set by the facade's auto-detect) could not use them; the general model forward can now dispatch to the 0.115.x GPU kernels. NOTE: this does NOT yet wire the serving batcher's paged-attention path onto the Tensors GPU DevicePagedKVCache — that path still uses AiDotNet's own CPU PagedAttentionKernel and is the next piece of GPU-serving work. Full serving suite green on 0.115.1 (267 tests); the earlier concurrent-forward concern does not reproduce. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * refactor(serving): make OpenAI DTO wire-parsing helpers internal The ResolveMaxTokens/ResolveStop/TextContent/PromptText helpers on the OpenAI request DTOs are controller-side parsing convenience, not a customization surface; the DTOs themselves stay public. Addresses a CodeRabbit maintainability comment (#1888). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(serving): address clear-cut CodeRabbit comments (benchmarks + tests) - Metrics: report goodput as unavailable (null) unless a request has BOTH TTFT and TPOT, instead of counting missing measurements as SLA passes. - Benchmarks README: OpenAI serving route now exists; drop stale 'once it exposes' / native-only 'today' wording. - StructuredOutputServingTests: await Task.Yield() so the xUnit timeout engages. - IncrementalGenerationEndToEndTests: dispose the wrapper that starts a shared background batcher (using var). - ModelPersistenceGuardTests: assert EXACTLY one recorded trial operation. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(serving): close two request-controlled security holes - ModelsController: canonicalize and boundary-check the user-supplied TokenizerPath against the configured model directory (mirroring the model-path check) so it cannot read arbitrary local files. A path that escapes the directory skips tokenizer registration (non-fatal; the model still loads and serves via the native token-ID endpoint). - RegexTokenConstraint: cap request-supplied regex quantifier repetitions at 1000 and reject larger/overflowing bounds with ArgumentException (HTTP 400), preventing a{2147483647}-style NFA-materialization memory exhaustion. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(serving): constraint stops on terminal state, not first accepting state - ITokenConstraint: replace IsComplete with IsTerminal ('no valid non-EOS continuation remains'). The batcher now stops guided decoding only when the grammar is exhausted, not the instant a state is merely accepting. Extendable matches (\d+, a complete value that could still take whitespace) end when the model emits EOS, which the mask permits in accepting states — so output is no longer truncated to its shortest valid prefix. Updated all three constraint impls (DFA/NFA/PDA) and ContinuousBatcher. - RegexTokenConstraint: capture the atom span in a local so a nested ParseAtom (group contents) can't clobber it; (ab){2} now repeats the whole group rather than cloning 'b)'. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(serving): harden structured-output constraint building - StructuredOutputFactory.Build: wrap regex/json_schema compilation so parser FormatException/OverflowException normalize to ArgumentException, honoring the documented 400 contract instead of surfacing a 500. - GetVocabStrings: narrow the catch-all to expected decode exceptions and log the offending token id via InferenceDiagnostics, rather than silently swallowing every exception (which hid real tokenizer defects). - JsonSchemaConstraint: replace the loose JSON string grammar ([^"\]|\.) with an RFC 8259-correct one — exclude raw control chars U+0000..U+001F and accept only valid escapes (\" \ \/ \b \f \n \r \t or \uXXXX). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(serving): log EOS-token resolution failures instead of swallowing * fix(inference): fFN/dense weight quantization honors selected FP8/NF4 format The DenseLayer/FFN quantization path hardcoded INT8, so selecting WeightOnlyFP8 or WeightOnlyNF4 quantized attention correctly but silently downgraded every FFN/dense weight to INT8 — inconsistent with the user-selected mode. Extract the shared WeightOnlyProjection engine (quantize + dequant-matmul for INT8/FP8/NF4) used by both QuantizedAttentionLayer and QuantizedDenseLayer so every weight-only-quantized layer honors the same format. Adds a regression test asserting FFN dense layers report the selected format. * feat(serving): prometheus /metrics endpoint (vLLM/TGI-parity observability) Adds a native Prometheus text-exposition endpoint at /metrics so a standard Prometheus/Grafana stack scrapes AiDotNet serving out of the box, matching vLLM/TGI. Rendered natively (no third-party exporter dependency, avoiding the beta-only OpenTelemetry.Exporter.Prometheus.AspNetCore package): - request counters (by outcome), prompt/generation token counters - request-duration, time-to-first-token, and time-per-output-token histograms recorded from the OpenAI chat/completions paths (streaming TTFT + TPOT) - live batcher gauges (throughput, batch size, queue depth, utilization, latency percentiles) folded in from IRequestBatcher.GetPerformanceMetrics() MetricsController is [AllowAnonymous] (standard for Prometheus scrapers). Tested via 4 renderer unit tests + an end-to-end /metrics scrape integration test. * feat(serving): user-providable custom draft model for speculative decoding Make IDraftModel<T>, DraftResult<T>, and NeuralDraftModel<T> public so users can supply their own draft model (a contract users implement must not be internal). Wire a supplied draft through the generative ServableModelWrapper ctor into the paged continuous-batching decode: the greedy-exact speculative path now drafts via the custom IDraftModel when one is provided (falling back to the built-in prompt-lookup n-gram draft otherwise). Greedy-exactness is preserved because every drafted token is still verified against the target model's argmax. Adds a test proving a user-supplied draft is actually invoked on the paged serving path; the full paged-equivalence suite (incl. default prompt-lookup speculation) stays green. * feat(serving): sliding-window attention on the paged path (Mistral-style SWA) Sliding-window attention was wired for the contiguous KV cache but PagedKVCache (the serving path) had none, so a Mistral/Mixtral-style model served through the paged batcher attended over the full context. Add WindowSize to PagedAttentionConfig and bound both paged attention kernels (ComputeAttention + ComputeTiledPagedAttention) to the most recent WindowSize keys, with the softmax normalized over the window. InferenceOptimizer flows the facade's UseSlidingWindowKVCache/KVCacheWindowSize into the paged kernel. WindowSize=0 keeps full causal attention (unchanged). Theory test proves both kernel paths attend only to the window; the full 41-test paged-attention suite stays green. * feat(serving): tensor-parallel inference runner + sharded==unsharded equivalence Add TensorParallelInference.ForwardInProcess: runs a tensor-parallel forward across N ranks (one task per rank over a shared InMemoryCommunicationBackend) so column-parallel gather / row-parallel all-reduce rendezvous and combine into the output every rank sees — the serving mechanism for running a TP-sharded model, reusing the existing Megatron ColumnParallelLinear / RowParallelLinear primitives. Adds an equivalence test proving a sharded Megatron MLP block (ColumnParallel up + ReLU -> RowParallel down all-reduce) at world-size 2 and 4 is bit-identical to the un-sharded (world-size 1) forward when seeded from the same full weights. This validates tensor-parallel serving without needing N physical GPUs (shard-count invariance == correctness). * feat(serving): tensor-parallel multi-head attention + equivalence Add TensorParallelAttention: Megatron-style attention-TP for serving — Q/K/V column-parallel (each rank owns a head group), per-rank local scaled-dot-product attention over its heads (no cross-rank comm inside attention), output projection row-parallel (one all-reduce sums the per-rank head contributions). Completes the TP transformer block (the partitioner previously did the MLP pair only; attention was explicitly not done). Equivalence test proves the sharded attention at world-size 2 and 4 (and causal) is bit-identical to the un-sharded forward when seeded from the same full Q/K/V/O weights. * feat(serving): full tensor-parallel transformer block + equivalence Add TensorParallelTransformerBlock: a complete drop-in Megatron-style TP decoder block for serving (pre-LN GPT style) — h + Attn(LN1(h)) then h + MLP(LN2(h)), composing the proven TensorParallelAttention with the column/row-parallel MLP pair. LayerNorms and residuals are replicated per rank; each attention/MLP runs one all-reduce. Equivalence test proves the sharded block at world-size 2 and 4 is bit-identical to the un-sharded forward. This is the stackable unit for a tensor-parallel transformer served across GPUs. * fix(serving): structured-output constraints fail closed on dead-ends Previously the constraint ApplyMask safety-nets opened the EOS token when a state had no valid continuation, faking a successful stop and emitting output that violates the constraint. Now each constraint throws StructuredOutputConstraintException on a genuine non-accepting dead-end, and the batcher catches it PER-SEQUENCE (in every decode path: stateless, paged, and batched-paged), failing just that sequence with StopReason.Error and a message while the rest of the batch keeps running. Added SequenceState.ErrorMessage and GenerationResult.Error so the reason surfaces to the consumer. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(serving): wire chunked-prefill + speculative-method from facade to the batcher Wiring audit gap: EnsureBatcher never flowed SpeculativeMethod (so Eagle/Medusa tree speculation was inert through the facade) and there was no facade knob for chunked prefill (so MaxPrefillChunkTokens stayed 0 = disabled no matter what). Add MaxPrefillChunkTokens to InferenceOptimizationConfig and flow both SpeculativeMethod and MaxPrefillChunkTokens from the facade config into ContinuousBatcherConfig, so a facade-built model actually gets the chunked prefill and speculative method the user configured. * fix(serving): restore ITokenConstraint.IsComplete (accepting) alongside IsTerminal The #33 rename dropped IsComplete entirely, breaking AiDotNet.Tests (the constraint unit tests query IsComplete as the accepting/'complete valid instance' predicate, distinct from IsTerminal's 'no non-EOS continuation remains'). Restore IsComplete on the interface and all three impls; the batcher still stops on IsTerminal, so the anti-truncation fix is preserved. This is CodeRabbit's 'expose terminality separately' option. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(serving): openAI chat correctness — fresh constraint per choice, reject unsupported streaming - Build a FRESH SpeculativeDecodingRequest (and its stateful structured-output / tool constraint) for every 'n' choice; a shared constraint stayed terminal after the first choice and corrupted later ones. - Reject streaming with n>1 (400) instead of silently returning one choice. - Reject streaming tool calls (400) instead of streaming the raw constrained JSON as assistant content and finishing with 'stop' rather than tool_calls. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * feat(facade): configureDraftModel wires a custom draft model through to serving The custom IDraftModel the batcher consumes had no facade path (built but not wired). Add IAiModelBuilder.ConfigureDraftModel(IDraftModel<T>) (on the interface + AiModelBuilder), which stores the live draft and switches DraftModelType to Custom. It flows builder -> AiModelResultOptions -> AiModelResult (JsonIgnore, live object) -> ServableModelWrapper.FromModel -> the generative wrapper ctor -> the continuous-batching engine's draft override, so a facade-built model served in-process actually uses the user's draft for speculative decoding. Follows the existing Configure* builder convention (not With*). * fix(serving): propagate engine failures, buffer stop-prefixes, trim logprobs at stop - CollectWithLogProbs now surfaces a generation-engine failure (resp.Error, non- cancellation) as HTTP 500 via GenerationFailedException instead of returning a truncated 200; client cancellation stays an OperationCanceledException. - Streaming (chat + completions) holds back the longest suffix that could begin a stop string (PendingStopPrefixLen), so a stop spanning tokens never leaks its prefix; the tail is flushed if no stop matches. - The batch logprobs path trims the reported token count AND log-probs to the same decoded stop boundary as the text, keeping usage/logprobs internally consistent. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(serving): enforce per-request generation limits at the HTTP boundary Add ServingLimitsOptions (MaxCompletionTokens/MaxN/MaxContextTokens; nullable with industry-standard defaults 4096/16/32768, bound from the 'ServingLimits' config section). OpenAiController now rejects (400) a chat/completions request whose n, max_tokens, or prompt+max_tokens context exceeds the configured limit instead of silently clamping (dropped the Math.Clamp(n,1,128)). Limits stay active with zero config via the defaults. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * feat(serving): compositePagedKVCache for tensor-parallel per-rank KV (paged-TP 1/7) Make PagedKVCache's scheduler-facing methods (allocate/extend/free/fork/truncate/length/ capacity/block-table/stats/dispose) virtual, and add CompositePagedKVCache: a PagedKVCache facade over N per-rank paged caches whose allocate/extend/free/truncate fan out to every rank in lockstep (all-or-nothing allocation with rollback), answering length/block-table/count from rank 0. This lets the continuous-batching scheduler manage tensor-parallel per-rank KV as one cache; per-rank attention still reads/writes its own concrete cache directly. First step of full paged tensor-parallel serving. Fan-out unit test added; paged-equivalence suite unaffected (virtual is non-breaking). * feat(serving): tensorParallelPagedAttention over per-rank paged KV (paged-TP 3/7) Per-rank paged tensor-parallel attention: ColumnParallel Q/K/V over the rank's LOCAL heads, writes this step's K/V into the rank's PagedKVCache, attends its local-head queries against its cached K/V (causal, O(1) new KV per token), and RowParallel O-projection all-reduces the per-rank head contributions into the full output. Plain scaled-dot-product (no RoPE/ALiBi/quant) so it is numerically transparent under sharding. Equivalence test: paged prefill+decode over a sharded per-rank cache at world size 2 and 4 is bit-identical to the un-sharded (world size 1) run. * refactor(nn): make PredictWithContext virtual (paged-TP model override seam) Prerequisite for a tensor-parallel paged serving model that overrides the context-aware forward to run N ranks. Non-breaking (adds virtual). * feat(serving): tensorParallelPagedModel — full paged tensor-parallel serving model (paged-TP 4/7) A tensor-parallel transformer LM served over per-rank paged KV caches: attention heads and the FFN hidden dim are partitioned across worldSize ranks (each rank keeps its own PagedKVCache for its head-group), and the output/down projections sum each rank's partial (the all-reduce). Overrides PredictWithContext for the continuous-batching engine. The forward runs ranks SEQUENTIALLY and reduces partials in fixed rank order, so the sharded result is bit-deterministic and identical to the un-sharded model (no dependence on the process-global compute engine / autodiff tape, which are not thread-safe; a real multi-GPU deployment runs the ranks in parallel on separate devices — same math). Equivalence test: paged prefill+decode at world size 2 and 4 matches world size 1 (deterministic, 3x). * test(serving): end-to-end paged tensor-parallel serving through the batcher (paged-TP 7/7 core) Drives greedy generation through the real ContinuousBatcher over a TensorParallelPagedModel + a CompositePagedKVCache fanning to the per-rank paged caches; world size 2 and 4 produce the same generated tokens as world size 1. Wires the whole stack end to end: scheduler -> composite per-rank KV allocation -> tensor-parallel paged decode. * fix(serving): streaming speculation-off, path-consistent token/logprob normalization, no …constraint-retry - Streaming forces SpeculationDepth=0 (bursty accepted drafts distorted TPOT). - NormalizeGeneration applied on BOTH the incremental and full-context paths: strip trailing EOS, cap to MaxNewTokens, and trim log-probs to the same final token count, so the response no longer depends on which path served it. - When incremental generation faults WITH an active structured-output constraint, fail rather than falling back with the already-advanced (stateful) constraint. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(serving): eOS terminator excluded from generated tokens on both paths Generate_StopsAtEosToken asserted the old full-context-fallback behavior (EOS token kept in GeneratedTokens). The normalization fix excludes the terminating EOS on both paths (matching the incremental path and OpenAI usage semantics), so a model emitting EOS first yields zero content tokens. Update the assertion to the corrected, path-consistent contract. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(serving): fallback streaming through the shared sampler; route tree-speculation to the …configurable batcher - The stateless streaming fallback now drives a per-request continuous-batching engine (streamed via OnTokenGenerated) instead of a hand-written sampler loop, so structured constraints, logit_bias, penalties, and stop handling are applied identically to the incremental path. Removed the now-dead SampleToken / LastPositionLogits / ArgMax helpers. - Requests opting into tree speculation are routed to the per-request batcher (whose config honors TreeBranchFactor/MaxTreeDepth) rather than the shared engine, which has a single fixed config and cannot honor per-request tree knobs. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(serving): concurrency/lifecycle hardening (batcher disposal, speculation flag, stream faults, …guard rollback) - ServableModelWrapper: synchronize batcher creation with disposal under the same lock (EnsureBatcher rejects after dispose; Dispose takes _batcherInitLock) so a request never races a half-built/disposed batcher; honor the constructor's EnableSpeculativeDecoding when no facade config (was force-enabled); StreamGeneration now captures a generation fault and rethrows it after draining, instead of ending the stream as a silent truncated success. - ModelPersistenceGuard.InternalOperation: roll back the ambient depth increment if acquiring the tensor-side scope throws, so a failed acquisition can't leave the logical context permanently in save/load bypass mode. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(serving): batcher lifecycle + speculation-vs-controls + logprob correctness - GenerateAsync/Dispose gated by a shared _lifecycleLock: a request is either fully enqueued before disposal or rejected with ObjectDisposedException, never inserted after the run loop/signal are torn down (which returned a never- completing task). - Disable speculation when a sequence uses frequency/presence penalties or logit_bias: the draft/verify path selects by raw argmax and bypasses SampleFromLogits, so those controls must force plain sampled decode. - RecordLogProbs now guarantees the emitted token appears in TopLogProbs even when sampling chose a token outside the top-K, per the PositionLogProbs contract. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(serving): real tool_choice semantics + require complete tool-call envelope - Parse tool_choice explicitly: none/auto (and the omitted default) do NOT force a tool call (auto preserves free text — a hard grammar can't express optional calls); 'required' forces any tool; a {function:{name}} object forces that tool; unknown strings / malformed objects are rejected with 400 instead of silently forcing. - The tool-call schema now marks name AND arguments required, so the constraint cannot accept {} or an object missing arguments; Parse returns null (rather than substituting {}) when arguments is absent. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(serving): honor the OpenAI seed for reproducible sampling Both /v1/chat/completions and /v1/completions deserialized 'seed' but never used it. Add SpeculativeDecodingRequest.Seed, populate it from the request in the controller, and use it (in preference to the RequestId-derived fallback) as the per-sequence RNG seed in BuildGenerationRequest — so seed now produces reproducible output instead of being a silent no-op. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * feat(serving): redesign speculative-decoding facade API (ConfigureSpeculativeDecoding) Replace the unilateral ConfigureDraftModel(IDraftModel) builder method with a discussed, beginner-friendly ConfigureSpeculativeDecoding surface, and move speculation tuning off the flat InferenceOptimizationConfig into a dedicated options object. Public API: - New public SpeculativeDecodingOptions (Enabled, SpeculationDepth, SpeculationPolicy, SpeculativeMethod, UseTreeSpeculation) with For-Beginners docs. - InferenceOptimizationConfig.SpeculativeDecoding replaces the flat EnableSpeculativeDecoding / SpeculationDepth / SpeculationPolicy / SpeculativeMethod / UseTreeSpeculation properties (breaking, agreed). Clone() deep-copies the sub-object; Validate() checks the nested depth. - Removed the DraftModelType enum entirely; the draft is now chosen by passing an IDraftModel<T>. - Made NGramDraftModel<T> public (alongside already-public IDraftModel/DraftResult/NeuralDraftModel). - IAiModelBuilder.ConfigureSpeculativeDecoding overloads: (IDraftModel<T>, options?), (IFullModel<T,TInput,TOutput>, options?) wrapping a token model as a NeuralDraftModel, and (SpeculativeDecodingOptions) tuning-only over the zero-cost N-gram draft. Calling any overload opts into speculation. Wiring: - InferenceOptimizer defaults to the N-gram draft and honors a SetCustomDraftModel override; AiModelResult flows the facade draft into the in-session optimizer. - ServableModelWrapper.EnsureBatcher reads facade.SpeculativeDecoding.*; the live draft still threads builder -> options -> result -> wrapper -> continuous batcher. - YAML: the nested speculativeDecoding: section binds via YamlDotNet reflection (stays wired). Tests updated to the new surface (serving + inference + config + yaml). src builds clean on net10.0 and net471; 32 serving tests green (facade config-flow, custom draft, paged-TP). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(serving/bench): cLI validation, no silent greedy substitution, goodput reason, tokenizer failure …boundary - BenchmarkOptions.Parse validates all downstream invariants (token counts, warmup, timeout, vocab, finite temperature/SLAs, backend, mode, base URL) and rejects aidotnet-native + greedy up front. - AiDotNetNativeBackend passes the requested temperature through unchanged instead of silently substituting 1.0 for greedy (which invalidated backend comparisons). - Metrics: goodput 'n/a' reason is now neutral (null also occurs for duration<=0). - ModelsController: tokenizer path canonicalization moved inside the warning-only boundary, so a Path.GetFullPath failure no longer 500s after the model is loaded. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(serving): terminality requires acceptance; mask unknown tokens; correct ApplyMask contract doc - IsTerminal (TokenFsm/Regex/JsonGrammar) now requires BOTH an accepting state AND no valid non-EOS continuation. A non-accepting dead-end is no longer reported as a (successful) terminal stop; it stays an error handled by the fail-closed ApplyMask throw. - JsonGrammarConstraint.ApplyMask masks model token ids beyond the tokenizer vocabulary instead of indexing past _tokenText (which threw and failed the request repeatedly). - ITokenConstraint.ApplyMask doc no longer claims sampling 'can never dead-end'; it documents the StructuredOutputConstraintException fail-closed behavior. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(serving): total regex compilation budget + ITokenConstraint implementer/migration doc - RegexNfa caps total pattern length (100k chars) and total automaton states (1M), throwing ArgumentException (=> HTTP 400) before materialization. Capping a single quantifier is insufficient: a long pattern or many valid quantifiers could still allocate an unbounded NFA from request-controlled response_format.regex. - ITokenConstraint documents that IsComplete + IsTerminal are both required of implementers (new interface) with the reference relationship, addressing the compatibility/migration note. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(serving): o(n) stop-boundary token mapping + reject malformed base@adapter ids - TokensCoveringPrefix accumulates per-token decoded lengths once (O(n)) instead of re-decoding every growing prefix (O(n^2) formatting/allocation). - SplitModelAndAdapter rejects malformed multi-LoRA identifiers (more than one '@', or an empty base/adapter) with a 400 rather than silently mis-parsing them. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(serving): validate all sampling controls; accept zero-valued greedy/no-speculation SpeculativeDecodingRequest.Validate now checks TopP (0,1], TopK>=0, MinP [0,1], frequency/presence penalties (finite, [-2,2]), TopLogprobs [0,20], and finite logit-bias values — previously unvalidated, so direct callers could push invalid ranges into sampler math. It also now ACCEPTS Temperature 0 (greedy) and NumDraftTokens 0 (speculation off) as valid rather than rejecting them, matching the documented zero-valued controls. Updated the error-branch test accordingly. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(serving): harden NeuralDraftModel, reject negative prefill chunk, document ServingDraftModel - NeuralDraftModel validates positive vocabSize/maxDraftTokens in the ctor and, in GenerateDraft, non-null input, non-negative draft count, positive temperature, and that the forward delegate returns exactly VocabSize logits — the newly public paths can no longer crash or emit invalid tokens on bad input. - InferenceOptimizationConfig.Validate rejects a negative MaxPrefillChunkTokens (domain is 0 or positive) instead of silently treating it as disabled. - AiModelResultOptions.ServingDraftModel gains the required <value> and [For Beginners] documentation. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(serving): strengthen JSON grammar/schema constraint tests - MatchesJsonDocument_Exhaustively (renamed from ...Newtonsoft; the oracle is System.Text.Json.JsonDocument): assert the exact exhaustive-case count (11110 = 10-char alphabet over lengths 1..4) instead of a loose '> 1000'. - AcceptsDeeplyNestedJson: build the depth-10 nested object programmatically (the literal only reached depth 4). - AnyJsonObject_BoundedNesting: use a fresh constraint per case and assert both the a…
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Implements Issue #334 - a production-ready model evaluation framework that exceeds industry standards (scikit-learn, MLflow, W&B, TensorFlow Model Analysis).
Features Added
Metrics (70+)
Cross-Validation Strategies (16)
Statistical Tests (10)
Engines
Key Design Decisions
TwithINumericOperations<T>for type flexibilityComputeWithCI()methodRelated Issue
Closes #334
Test plan
🤖 Generated with Claude Code