test: add comprehensive classification integration tests - #614
Conversation
Add 211 integration tests covering the entire Classification module: - NaiveBayesIntegrationTests (29 tests): Gaussian, Multinomial, Bernoulli, Complement - KNeighborsIntegrationTests (26 tests): distance metrics, weighting, edge cases - DecisionTreeIntegrationTests (26 tests): Gini/Entropy, depth control, pruning - DiscriminantAnalysisIntegrationTests (25 tests): LDA, QDA, regularization - LinearClassifierIntegrationTests (24 tests): Perceptron, Logistic, SGD - SVMIntegrationTests (28 tests): kernels, soft margins, multi-class - EnsembleClassifierIntegrationTests (24 tests): AdaBoost, GradientBoosting, RandomForest - MetaClassifierIntegrationTests (29 tests): OneVsRest, Voting, Stacking, Bagging Fix serialization bugs discovered by tests: - GaussianNaiveBayes: serialize means, variances, LogPriors, ClassCounts - KNeighborsClassifier: serialize training data and KNN options - DecisionTreeClassifier: serialize tree structure and feature importances 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
Warning Rate limit exceeded@ooples has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 7 minutes and 21 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughAdds JSON-based serialization/deserialization support to three classifiers (GaussianNaiveBayes, KNeighborsClassifier, DecisionTreeClassifier) and introduces comprehensive integration tests across the Classification module to validate mathematical correctness, edge cases, and behavioral properties. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…ctorclassifier - Add 7 tests for CategoricalNaiveBayes covering basic classification, probability sums, Laplace smoothing, multiclass, clone, and error handling - Add 13 tests for NuSupportVectorClassifier covering nu parameter validation, edge cases, decision function, probabilities, RBF/polynomial kernels, clone, error handling, metadata, and numerical stability - Total classification integration tests now at 230, all passing 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR adds 211 comprehensive integration tests across the Classification module and fixes 3 serialization bugs in GaussianNaiveBayes, KNeighborsClassifier, and DecisionTreeClassifier.
Summary: The PR significantly improves test coverage for classification algorithms, covering various scenarios including edge cases, mathematical correctness, and serialization. The tests are well-structured and verify behavior without blindly trusting implementation details.
Key Changes:
- Added 8 integration test files covering all major classifier types
- Implemented serialization support for DecisionTreeClassifier
- Fixed serialization bugs in 3 classifiers
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| SVMIntegrationTests.cs | Tests SVM and LinearSVC with various kernels, regularization, and edge cases (28 tests) |
| NaiveBayesIntegrationTests.cs | Tests Gaussian, Multinomial, Bernoulli, Complement, and Categorical NB variants (29 tests) |
| MetaClassifierIntegrationTests.cs | Tests OneVsRest, OneVsOne, Voting, Bagging, Stacking, and multi-label classifiers (29 tests) |
| LinearClassifierIntegrationTests.cs | Tests Perceptron, Ridge, SGD, and Passive-Aggressive classifiers (24 tests) |
| KNeighborsIntegrationTests.cs | Tests KNN with various distance metrics and weighting schemes (26 tests) |
| EnsembleClassifierIntegrationTests.cs | Tests RandomForest, AdaBoost, GradientBoosting, and ExtraTrees (24 tests) |
| DiscriminantAnalysisIntegrationTests.cs | Tests LDA and QDA with regularization and edge cases (25 tests) |
| DecisionTreeIntegrationTests.cs | Tests decision tree splitting, constraints, and serialization (26 tests) |
| DecisionTreeClassifier.cs | Added Serialize/Deserialize methods with recursive tree structure handling |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (17)
tests/AiDotNet.Tests/IntegrationTests/Classification/EnsembleClassifierIntegrationTests.cs (3)
329-333: Redundant and confusing assertion logic.The
||condition is unnecessary. Ifprobs[i, 0] >= 0is true, the assertion passes. If it's false (meaningprobs[i, 0] < 0), then you check>= -Tolerance. This is logically equivalent to just checking>= -Tolerancein all cases.🔎 Suggested simplification
- Assert.True(probs[i, 0] >= 0 || probs[i, 0] >= -Tolerance, + Assert.True(probs[i, 0] >= -Tolerance, $"Probability at ({i}, 0) should be non-negative, got {probs[i, 0]}"); - Assert.True(probs[i, 1] >= 0 || probs[i, 1] >= -Tolerance, + Assert.True(probs[i, 1] >= -Tolerance, $"Probability at ({i}, 1) should be non-negative, got {probs[i, 1]}");
423-470: Test name doesn't reflect actual behavior being tested.The test
ExtraTrees_MoreRandomThanRandomForestdoesn't verify that ExtraTrees is "more random" than RandomForest. It only confirms both classifiers achieve reasonable accuracy on the same dataset. Consider renaming to something likeExtraTrees_AndRandomForest_BothClassifyCorrectlyor adding assertions that actually compare randomness characteristics (e.g., variance in predictions across different seeds).
474-549: Consider adding error handling tests for GradientBoosting.For consistency with RandomForest and AdaBoost, consider adding
GradientBoosting_ThrowsOnMismatchedDimensionsandGradientBoosting_PredictBeforeTrain_Throwstests. The same applies to ExtraTrees.tests/AiDotNet.Tests/IntegrationTests/Classification/MetaClassifierIntegrationTests.cs (5)
91-117: Minor:f3is computed but not used in label determination.The variable
f3(line 100) is set as a feature but isn't used in any label logic. This is fine since it adds feature diversity, but if intentional, a brief comment clarifying its purpose would help.
463-505: Consider strengthening the weight influence test.The test verifies both weighted and equal-weighted classifiers produce valid predictions without crashing, but doesn't assert that weights actually affect the outcome. Since both classifiers are identical
GaussianNaiveBayes, the predictions would be the same regardless of weights. Consider using different base classifiers or verifying soft voting probabilities differ.
858-902: Test name vs. actual assertion mismatch.The test
StackingClassifier_CrossValidation_ReducesOverfittingdoesn't actually verify that CV reduces overfitting. It only checks that both configurations produce valid predictions. To verify the claim, consider comparing train vs. holdout accuracy or using a more complex dataset where overfitting is measurable.
1458-1474: Consider extending mismatched X/Y test coverage.The test covers OneVsRest, OneVsOne, and Bagging, but VotingClassifier, StackingClassifier, ClassifierChain, and MultiOutputClassifier are not included. If these classifiers should also validate input dimensions, consider extending the test.
🔎 Suggested extension
var bagging = new BaggingClassifier<double>(() => new GaussianNaiveBayes<double>()); + var estimators = new List<IClassifier<double>> { new GaussianNaiveBayes<double>() }; + var voting = new VotingClassifier<double>(estimators); + var stacking = new StackingClassifier<double>(estimators, () => new GaussianNaiveBayes<double>()); // Act & Assert Assert.Throws<ArgumentException>(() => ovr.Train(x, y)); Assert.Throws<ArgumentException>(() => ovo.Train(x, y)); Assert.Throws<ArgumentException>(() => bagging.Train(x, y)); + Assert.Throws<ArgumentException>(() => voting.Train(x, y)); + Assert.Throws<ArgumentException>(() => stacking.Train(x, y));
1600-1629: Test name doesn't match assertion.
Bagging_VsSingle_ReducesVarianceonly verifies both produce valid predictions. Variance reduction would require multiple runs with different data samples or measuring prediction stability. Consider renaming toBagging_VsSingle_BothProduceValidPredictionsor adding variance measurement.tests/AiDotNet.Tests/IntegrationTests/Classification/SVMIntegrationTests.cs (2)
162-176: Test only validates positive decisions; consider adding negative decision validation.The test verifies that positive decision values lead to class 1 predictions but doesn't explicitly verify that negative decisions lead to class 0 predictions. Adding an
elsebranch would make the test more comprehensive.Suggested enhancement
// Positive class is the last one (class 1) if (decision > 0) { Assert.True(Math.Abs(prediction - 1) < 0.01, $"Positive decision ({decision}) should predict class 1, got {prediction}"); } + else if (decision < 0) + { + Assert.True(Math.Abs(prediction - 0) < 0.01, + $"Negative decision ({decision}) should predict class 0, got {prediction}"); + } }
564-567: Same pattern: trivially true assertions.These assertions will always pass. Consider verifying that the models achieve at least baseline accuracy or that they produce different behavior for different gamma values.
Suggested fix
- // Just verify both trained without error - Assert.True(correctLow >= 0, $"Low gamma model should train"); - Assert.True(correctHigh >= 0, $"High gamma model should train"); + // Verify both trained and produce valid predictions + Assert.True(correctLow + correctHigh > 0, "At least one model should classify some samples correctly"); + // High gamma typically memorizes training data better + Assert.True(correctHigh >= 6, $"High gamma model should fit training data well. Got {correctHigh}/12");tests/AiDotNet.Tests/IntegrationTests/Classification/DiscriminantAnalysisIntegrationTests.cs (2)
453-498: Test name doesn't match assertion logic.The test is named
LDA_vs_QDA_DifferentCovariance_QDABetter, implying QDA should outperform LDA on this data. However, the assertions only verify both achieve>= 15/20accuracy independently—they don't compare LDA vs QDA performance.Either rename the test to
LDA_vs_QDA_DifferentCovariance_BothClassifyWell, or add an assertion that actually validates QDA performs at least as well as LDA:Proposed fix
// Both should do well on this well-separated data - Assert.True(ldaCorrect >= 15, $"LDA should classify most correctly, got {ldaCorrect}/20"); - Assert.True(qdaCorrect >= 15, $"QDA should classify most correctly, got {qdaCorrect}/20"); + Assert.True(ldaCorrect >= 15, $"LDA should classify most correctly, got {ldaCorrect}/20"); + Assert.True(qdaCorrect >= 15, $"QDA should classify most correctly, got {qdaCorrect}/20"); + Assert.True(qdaCorrect >= ldaCorrect, + $"QDA should perform at least as well as LDA on heterogeneous covariance data. LDA={ldaCorrect}, QDA={qdaCorrect}");
687-716: Missing symmetric test:QDA_Clone_IsIndependent.There's
LDA_Clone_IsIndependent(lines 718-755) but no correspondingQDA_Clone_IsIndependenttest. For consistency and complete coverage, consider adding a parallel test for QDA to verify that clones remain independent after retraining the original model.Would you like me to generate the
QDA_Clone_IsIndependenttest following the same pattern as the LDA version?src/Classification/NaiveBayes/GaussianNaiveBayes.cs (2)
420-431: Missing array length validation before reconstruction.When deserializing the
_meansmatrix, the code doesn't verify thatmeansArray.Lengthmatchesrows * colsbefore iterating. If the serialized data is corrupted or truncated, this could cause anIndexOutOfRangeException.🔎 Proposed fix
if (meansToken is not null) { var meansArray = meansToken.ToObject<double[]>() ?? Array.Empty<double>(); int rows = modelDataObj["MeansRows"]?.ToObject<int>() ?? 0; int cols = modelDataObj["MeansCols"]?.ToObject<int>() ?? 0; - if (rows > 0 && cols > 0) + if (rows > 0 && cols > 0 && meansArray.Length == rows * cols) { _means = new Matrix<T>(rows, cols); int idx = 0;
438-454: Same array length validation missing for variances matrix.Apply the same validation to prevent potential
IndexOutOfRangeExceptionwhen deserializing the_variancesmatrix.🔎 Proposed fix
if (variancesToken is not null) { var variancesArray = variancesToken.ToObject<double[]>() ?? Array.Empty<double>(); int rows = modelDataObj["VariancesRows"]?.ToObject<int>() ?? 0; int cols = modelDataObj["VariancesCols"]?.ToObject<int>() ?? 0; - if (rows > 0 && cols > 0) + if (rows > 0 && cols > 0 && variancesArray.Length == rows * cols) { _variances = new Matrix<T>(rows, cols);src/Classification/Neighbors/KNeighborsClassifier.cs (1)
566-585: Missing array length validation for _xTrain matrix reconstruction.Similar to GaussianNaiveBayes, the deserialization should validate that
xTrainArray.Length == rows * colsbefore iterating to prevent potentialIndexOutOfRangeExceptionfrom corrupted data.🔎 Proposed fix
if (xTrainToken is not null) { var xTrainArray = xTrainToken.ToObject<double[]>() ?? Array.Empty<double>(); int rows = modelDataObj["XTrainRows"]?.ToObject<int>() ?? 0; int cols = modelDataObj["XTrainCols"]?.ToObject<int>() ?? 0; - if (rows > 0 && cols > 0) + if (rows > 0 && cols > 0 && xTrainArray.Length == rows * cols) { _xTrain = new Matrix<T>(rows, cols);tests/AiDotNet.Tests/IntegrationTests/Classification/LinearClassifierIntegrationTests.cs (2)
622-663: Test doesn't verify that C affects aggressiveness.The test name claims
DifferentC_AffectsAggressiveness, but lines 661-662 only assert that both models have weights, not that the different C values produce different behavior.🔎 Proposed enhancement to verify C's effect
var weightsLow = paLow.GetParameters(); var weightsHigh = paHigh.GetParameters(); - // Assert: Both should train, higher C may have larger weights + // Assert: Both should train Assert.True(weightsLow.Length > 0); Assert.True(weightsHigh.Length > 0); + + // Higher C (more aggressive) typically produces larger weight updates + // Verify the models produce different predictions on at least some points + var predsLow = paLow.Predict(x); + var predsHigh = paHigh.Predict(x); + + int differenceCount = 0; + for (int i = 0; i < x.Rows; i++) + { + if (Math.Abs(predsLow[i] - predsHigh[i]) > 0.01) differenceCount++; + } + + // Models with very different C should behave differently (at least during training) + // Note: On converged solutions they may end up similar + Assert.True(differenceCount >= 0, + $"Different C values should affect model behavior. Models differed on {differenceCount}/10 predictions");Alternatively, rename the test to reflect what it verifies:
PassiveAggressive_DifferentC_BothTrain
840-863: Test is too permissive to verify consistent behavior.Lines 854-862 accept both throwing an exception and not throwing. This means the test will always pass and won't catch inconsistent behavior across classifiers or detect future regressions.
Consider either:
- Verify that all classifiers throw consistently:
- // Act & Assert: Should throw or handle gracefully - // (Actual behavior depends on implementation) - try - { - var pred = perceptron.Predict(testPoint); - // If no exception, just verify it didn't crash - } - catch (Exception) - { - // Expected behavior - } + // Act & Assert: All classifiers should throw InvalidOperationException before training + Assert.Throws<InvalidOperationException>(() => perceptron.Predict(testPoint)); + Assert.Throws<InvalidOperationException>(() => ridge.Predict(testPoint)); + Assert.Throws<InvalidOperationException>(() => sgd.Predict(testPoint)); + Assert.Throws<InvalidOperationException>(() => pa.Predict(testPoint));
- Or verify that none throw and return a valid (possibly default) result:
- // Act & Assert: Should throw or handle gracefully - // (Actual behavior depends on implementation) - try - { - var pred = perceptron.Predict(testPoint); - // If no exception, just verify it didn't crash - } - catch (Exception) - { - // Expected behavior - } + // Act & Assert: Classifiers should handle predict before train gracefully + var predPerceptron = perceptron.Predict(testPoint); + var predRidge = ridge.Predict(testPoint); + var predSgd = sgd.Predict(testPoint); + var predPa = pa.Predict(testPoint); + + // Verify predictions are valid (not NaN/Infinity) + Assert.False(double.IsNaN(predPerceptron[0])); + Assert.False(double.IsNaN(predRidge[0])); + Assert.False(double.IsNaN(predSgd[0])); + Assert.False(double.IsNaN(predPa[0]));The current implementation doesn't provide value as a regression test.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
src/Classification/NaiveBayes/GaussianNaiveBayes.cssrc/Classification/Neighbors/KNeighborsClassifier.cssrc/Classification/Trees/DecisionTreeClassifier.cstests/AiDotNet.Tests/IntegrationTests/Classification/DecisionTreeIntegrationTests.cstests/AiDotNet.Tests/IntegrationTests/Classification/DiscriminantAnalysisIntegrationTests.cstests/AiDotNet.Tests/IntegrationTests/Classification/EnsembleClassifierIntegrationTests.cstests/AiDotNet.Tests/IntegrationTests/Classification/KNeighborsIntegrationTests.cstests/AiDotNet.Tests/IntegrationTests/Classification/LinearClassifierIntegrationTests.cstests/AiDotNet.Tests/IntegrationTests/Classification/MetaClassifierIntegrationTests.cstests/AiDotNet.Tests/IntegrationTests/Classification/NaiveBayesIntegrationTests.cstests/AiDotNet.Tests/IntegrationTests/Classification/SVMIntegrationTests.cs
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-12-18T08:49:25.295Z
Learnt from: ooples
Repo: ooples/AiDotNet PR: 444
File: src/Interfaces/IPruningMask.cs:1-102
Timestamp: 2025-12-18T08:49:25.295Z
Learning: In the AiDotNet repository, the project-level global using includes AiDotNet.Tensors.LinearAlgebra via AiDotNet.csproj. Therefore, Vector<T>, Matrix<T>, and Tensor<T> are available without per-file using directives. Do not flag missing using directives for these types in any C# files within this project. Apply this guideline broadly to all C# files (not just a single file) to avoid false positives. If a file uses a type from a different namespace not covered by the global using, flag as usual.
Applied to files:
src/Classification/Neighbors/KNeighborsClassifier.cstests/AiDotNet.Tests/IntegrationTests/Classification/DecisionTreeIntegrationTests.cstests/AiDotNet.Tests/IntegrationTests/Classification/KNeighborsIntegrationTests.cssrc/Classification/Trees/DecisionTreeClassifier.cstests/AiDotNet.Tests/IntegrationTests/Classification/SVMIntegrationTests.cssrc/Classification/NaiveBayes/GaussianNaiveBayes.cstests/AiDotNet.Tests/IntegrationTests/Classification/EnsembleClassifierIntegrationTests.cstests/AiDotNet.Tests/IntegrationTests/Classification/NaiveBayesIntegrationTests.cstests/AiDotNet.Tests/IntegrationTests/Classification/DiscriminantAnalysisIntegrationTests.cstests/AiDotNet.Tests/IntegrationTests/Classification/LinearClassifierIntegrationTests.cstests/AiDotNet.Tests/IntegrationTests/Classification/MetaClassifierIntegrationTests.cs
📚 Learning: 2025-12-18T08:49:53.103Z
Learnt from: ooples
Repo: ooples/AiDotNet PR: 444
File: src/Interfaces/IPruningStrategy.cs:1-4
Timestamp: 2025-12-18T08:49:53.103Z
Learning: In this repository, global using directives are declared in AiDotNet.csproj for core namespaces (AiDotNet.Tensors.* and AiDotNet.*) and common system types. When reviewing C# files, assume these global usings are in effect; avoid adding duplicate using statements for these namespaces and for types like Vector<T>, Matrix<T>, Tensor<T>, etc. If a type is not found, verify the global usings or consider adding a file-scoped using if needed. Prefer relying on global usings to reduce boilerplate.
Applied to files:
src/Classification/Neighbors/KNeighborsClassifier.cstests/AiDotNet.Tests/IntegrationTests/Classification/DecisionTreeIntegrationTests.cstests/AiDotNet.Tests/IntegrationTests/Classification/KNeighborsIntegrationTests.cssrc/Classification/Trees/DecisionTreeClassifier.cstests/AiDotNet.Tests/IntegrationTests/Classification/SVMIntegrationTests.cssrc/Classification/NaiveBayes/GaussianNaiveBayes.cstests/AiDotNet.Tests/IntegrationTests/Classification/EnsembleClassifierIntegrationTests.cstests/AiDotNet.Tests/IntegrationTests/Classification/NaiveBayesIntegrationTests.cstests/AiDotNet.Tests/IntegrationTests/Classification/DiscriminantAnalysisIntegrationTests.cstests/AiDotNet.Tests/IntegrationTests/Classification/LinearClassifierIntegrationTests.cstests/AiDotNet.Tests/IntegrationTests/Classification/MetaClassifierIntegrationTests.cs
🔇 Additional comments (37)
tests/AiDotNet.Tests/IntegrationTests/Classification/EnsembleClassifierIntegrationTests.cs (7)
1-14: LGTM!The file structure, imports, and test class setup follow standard conventions for integration tests.
16-213: LGTM!The RandomForest core tests are well-structured with appropriate coverage of key behaviors: ensemble improvement, bootstrap sampling, feature constraints, OOB scoring, and probability validity. The use of fixed random seeds ensures reproducibility.
551-621: LGTM!Clone tests properly verify that cloned models produce identical predictions. Optionally, consider adding independence verification (e.g., retraining the clone on different data and confirming the original's predictions are unchanged).
623-673: LGTM!Error handling tests properly validate that classifiers throw appropriate exceptions for invalid inputs and improper usage patterns.
675-753: LGTM!Multiclass tests appropriately validate 3-class classification behavior, including probability validation for RandomForest.
755-796: LGTM!Feature importance test validates the correct length and non-negativity constraints. The test data is designed with feature 0 being discriminative, which could optionally be used to verify it has the highest importance.
798-861: LGTM!Metadata tests properly validate that
GetModelMetadataexposes the expected configuration values for both RandomForest and AdaBoost classifiers.tests/AiDotNet.Tests/IntegrationTests/Classification/MetaClassifierIntegrationTests.cs (8)
1-17: LGTM - Well-structured test class setup.The imports, namespace, and constants are appropriate. Using
1e-6tolerance for floating-point comparisons and a fixed random seed ensures reproducibility.
123-143: LGTM - Good test for OvR classifier count verification.The test validates that predictions are within valid class labels. Consider adding a comment noting that the internal classifier count (k=3) could be verified via metadata if exposed.
275-318: LGTM - Comprehensive OvO voting mechanism test.The test correctly creates a 4-class problem (6 pairwise classifiers). The inline data generation for this specific test case is appropriate since it tests a different class count than the default helpers.
627-673: LGTM - Good bootstrap diversity verification.The test correctly uses different random seeds and verifies that probability distributions differ, confirming that bootstrap sampling creates different base models.
1049-1079: LGTM - Good ClassifierChain dependency capture test.The test correctly uses multi-label data where label 2 depends on labels 0 and 1, validating that the chain can capture label dependencies through augmented features.
1255-1294: LGTM - Well-designed independence test.Creating labels based on independent features (
x[i, 0] > 0andx[i, 1] > 0) and verifying each achieves reasonable accuracy validates that the MultiOutputClassifier trains each label independently.
1480-1504: LGTM - Good log probability consistency verification.The test correctly accounts for numerical stability by using
Math.Max(probs[i, c], 1e-15)to match how implementations typically handle near-zero probabilities when computing log values.
10-15: Well-organized comprehensive test suite.The test file provides solid coverage of meta classifiers with clear region organization. The tests align with the PR objectives of validating mathematical correctness and not trusting implementation details. Minor improvements noted above would strengthen some specific test assertions.
tests/AiDotNet.Tests/IntegrationTests/Classification/SVMIntegrationTests.cs (7)
809-816: Good parameter validation test coverage.The Nu parameter validation tests properly verify boundary conditions (0.0, negative, >1.0) and use
Assert.Throwscorrectly. This is a good pattern for testing input validation.
818-842: Good edge case coverage for valid Nu boundary values.Testing both
nu = 1.0andnu = 0.01validates that the valid boundary values work correctly, complementing the invalid value tests above.
203-214: Good probability validation pattern.The test correctly validates that probabilities sum to 1 (within tolerance) and that each probability is within [0, 1]. This pattern is consistently applied across all SVM variants in this file.
649-697: Comprehensive error handling test coverage.The error handling tests properly cover both dimension mismatch and predict-before-train scenarios for all three SVM variants. Using specific exception types (
ArgumentException,InvalidOperationException) is the correct approach.
703-768: Good numerical stability test coverage.Testing both large (1e5) and small (1e-5) feature values with appropriate assertions for NaN and Infinity is a solid approach to verifying numerical stability.
1048-1078: Good metadata exposure validation.The test verifies that Nu-SVC exposes the
NuandRhoparameters in its metadata and validates the expected Nu value (0.3). This ensures the model's configuration is accessible for inspection.
1-15: Well-structured integration test file.The file is well-organized with clear regions, consistent tolerance usage, and follows the Arrange-Act-Assert pattern throughout. The comprehensive coverage of SVC, LinearSVC, and NuSVC variants with tests for predictions, probabilities, decision functions, cloning, error handling, and numerical stability aligns well with the PR objectives for comprehensive classification integration tests.
tests/AiDotNet.Tests/IntegrationTests/Classification/DiscriminantAnalysisIntegrationTests.cs (2)
1-13: Well-structured test file with comprehensive coverage.The test organization using regions, consistent naming conventions, descriptive failure messages, and coverage of core functionality, edge cases, numerical stability, and multiclass scenarios is excellent. The use of seeded
Random(42)for reproducibility is a good practice.
654-756: Consider adding serialization round-trip tests if serialization is supported.The PR objectives mention "verify serialization preserves model behavior," but this file only includes
Clonetests. WhileClonevalidates state copying, it doesn't exercise the JSON serialization/deserialization path that was fixed for other classifiers (GaussianNaiveBayes, KNeighborsClassifier, DecisionTreeClassifier).If LDA/QDA support
Serialize/Deserializemethods, consider adding tests like:[Fact] public void LDA_Serialization_PreservesPredictions() { // Train, serialize to JSON, deserialize, verify predictions match }src/Classification/NaiveBayes/GaussianNaiveBayes.cs (1)
288-353: Serialization implementation looks good overall.The
Serialize()method correctly captures all necessary model state including base properties,LogPriors,ClassCounts, and the learned_meansand_variancesmatrices. The row-major flattening with separate dimension storage is a reasonable approach for matrix serialization.src/Classification/Trees/DecisionTreeClassifier.cs (3)
716-748: Recursive serialization may cause stack overflow on extremely deep trees.The
SerializeNodemethod uses recursion to serialize the tree. While decision trees typically have moderate depth, trees withMaxDepthset very high or unlimited could potentially cause aStackOverflowException. This is likely acceptable for typical use cases but worth noting.Consider whether this is a concern for your expected use cases. If very deep trees are possible, an iterative approach using an explicit stack could be safer.
750-809: LGTM for the main deserialization flow.The
Deserializemethod properly validatesmodelMetadataandmodelDataObj, reconstructs base properties, feature importances, and delegates tree reconstruction toDeserializeNode. The null checks and type validations are appropriate.
814-857: DeserializeNode implementation is well-structured.The method correctly handles null input, uses defensive defaults for missing properties, and properly checks
JTokenType.Objectbefore attempting to deserialize child nodes. The recursive reconstruction mirrors the serialization logic appropriately.tests/AiDotNet.Tests/IntegrationTests/Classification/KNeighborsIntegrationTests.cs (2)
1-14: Comprehensive test suite for KNeighborsClassifier.The test class provides thorough coverage including basic predictions, probability validation, multiple distance metrics, weighting schemes, and edge cases. The organization with
#regionblocks makes navigation easy.
615-644: Good edge case coverage for exact match with distance weighting.This test verifies the important edge case where a query point exactly matches a training point. When using distance weighting, the matching point should receive very high weight. The assertion
probs[0, 0] > 0.99correctly validates this behavior.src/Classification/Neighbors/KNeighborsClassifier.cs (1)
463-517: KNN serialization correctly captures all necessary state.The
Serialize()method properly stores the KNN-specific options (NNeighbors, Metric, Weights, P, Algorithm, LeafSize) alongside the training data. Since KNN is a lazy learner that stores all training data, serializing_xTrainand_yTrainis essential for preserving model behavior.tests/AiDotNet.Tests/IntegrationTests/Classification/DecisionTreeIntegrationTests.cs (2)
1-14: Well-structured test suite for DecisionTreeClassifier.The test class provides comprehensive coverage of the decision tree classifier including training, predictions, probability outputs, tree constraints, feature importances, different impurity criteria, and lifecycle operations. The tests validate both correctness and structural properties.
478-498: Good validation of pure node optimization.This test correctly verifies that when all samples belong to the same class, the decision tree should create exactly one leaf node without any splits. The assertions
Assert.Equal(1, dt.LeafCount)andAssert.Equal(1, dt.NodeCount)properly validate this optimization.tests/AiDotNet.Tests/IntegrationTests/Classification/NaiveBayesIntegrationTests.cs (2)
199-230: Excellent numerical stability test for log probabilities.The
GaussianNB_PredictLogProbabilities_NumericallyStabletest uses extreme values (ranging up to 100000+) to verify that log probability calculations don't produce NaN or Infinity. This directly addresses the PR objective of ensuring numerical stability.
1-14: Comprehensive Naive Bayes integration test suite.The test class thoroughly covers all Naive Bayes variants (Gaussian, Multinomial, Bernoulli, Complement, Categorical) with tests for mathematical correctness, probability invariants, edge cases, and model persistence. The test organization with regions makes it easy to navigate.
tests/AiDotNet.Tests/IntegrationTests/Classification/LinearClassifierIntegrationTests.cs (2)
1-944: Comprehensive test coverage with good structure.This test suite provides solid coverage of linear classifiers with well-organized tests covering:
- ✓ Core functionality and convergence behavior for all four classifier types
- ✓ Different loss functions (hinge, log, squared hinge) for SGD
- ✓ All three Passive-Aggressive variants (PA, PA-I, PA-II)
- ✓ Regularization effects (L1, L2, different alpha values)
- ✓ Edge cases (multiclass errors, dimension mismatches, centered data, intercept handling)
- ✓ Numerical stability (large feature values, collinear features)
- ✓ Clone consistency across all classifiers
- ✓ Metadata verification for PassiveAggressive
The test organization with regions and descriptive test names makes the suite easy to navigate and understand.
1-944: Verify whether serialization support and tests are required for linear classifiers.This test file contains no serialization or deserialization tests for Perceptron, Ridge, SGD, or PassiveAggressive classifiers. If these classifiers support serialization (e.g., via
Serialize()andDeserialize()methods), consider adding round-trip tests to verify that deserialized models produce identical predictions.
- L2 regularization test now asserts regularized weights are smaller - L1 sparsity test now asserts at least 3 near-zero weights - RBF vs Linear test now asserts RBF performs at least as well as linear - LinearSVC Higher C test now asserts reasonable accuracy (>= 5/10) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Summary
Test Coverage
Bug Fixes
GaussianNaiveBayes Serialization
_meansmatrix (class feature means)_variancesmatrix (class feature variances)LogPriors(class log priors)ClassCounts(samples per class)KNeighborsClassifier Serialization
_xTrainmatrix (training features)_yTrainvector (training labels)DecisionTreeClassifier Serialization
FeatureImportancesvectorTest plan
Closes #612
🤖 Generated with Claude Code