refactor: remove DefaultModelEvaluator and IModelEvaluator - #820
Merged
Merged
Conversation
Remove legacy DefaultModelEvaluator and IModelEvaluator interfaces in favor of the new MetricEvaluationEngine through AiModelResult facade pattern. Changes: - Delete DefaultModelEvaluator.cs and IModelEvaluator.cs - Add AiModelResult.Evaluation.cs with EvaluateFull() and GetDataSetStats() - Remove IModelEvaluator dependency from AutoML classes - Remove IModelEvaluator dependency from Optimizer classes - Remove IModelEvaluator dependency from Genetics classes - Remove IModelEvaluator from OptimizationAlgorithmOptions - Update StepwiseRegression with inline evaluation - Remove ConfigureModelEvaluator from AiModelBuilder - Update tests to use new evaluation pattern Closes #334 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
WalkthroughThis PR removes the IModelEvaluator abstraction and DefaultModelEvaluator, inlines model evaluation into consumers, and adds post-build evaluation APIs on AiModelResult. Constructor signatures and public APIs that accepted IModelEvaluator were removed across AutoML, optimizers, genetic algorithms, and regression components. Changes
Sequence Diagram(s)sequenceDiagram
participant Builder as AiModelBuilder
participant AutoML as AutoML (strategy)
participant Model as AiModelResult
participant Eval as AiModelResult.Evaluation
Builder->>AutoML: Build() (train + produce AiModelResult)
AutoML->>Model: return AiModelResult
Model->>Eval: EvaluateFull(inputData)
Eval-->>Model: ModelEvaluationData (Training/Validation/Test, ModelStats)
Model-->>Builder: evaluation results available post-build
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
Contributor
There was a problem hiding this comment.
Pull request overview
This pull request removes the legacy DefaultModelEvaluator and IModelEvaluator interfaces in favor of a new facade pattern through AiModelResult. The refactoring eliminates dependency injection of model evaluators across the codebase and replaces them with inline evaluation methods.
Changes:
- Deleted
IModelEvaluatorinterface andDefaultModelEvaluatorimplementation - Added new
AiModelResult.Evaluation.cspartial class withEvaluateFull()andGetDataSetStats()facade methods - Replaced model evaluator dependencies with inline evaluation logic in
OptimizerBase,GeneticBase,AutoMLModelBase, andStepwiseRegression - Removed
ConfigureModelEvaluator()fromAiModelBuilderandSetModelEvaluator()from AutoML interfaces - Removed automatic cross-validation execution during
Build()in favor of manual invocation - Updated test code to use the new
EvaluateFull()facade pattern
Reviewed changes
Copilot reviewed 27 out of 27 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Interfaces/IModelEvaluator.cs | Deleted legacy model evaluator interface |
| src/Evaluation/DefaultModelEvaluator.cs | Deleted default implementation with 649 lines of evaluation logic |
| src/Models/Results/AiModelResult.Evaluation.cs | New 466-line partial class providing evaluation facade with comprehensive multi-class support |
| src/Optimizers/OptimizerBase.cs | Added inline evaluation methods (153 lines) to replace evaluator dependency |
| src/Genetics/GeneticBase.cs | Added inline evaluation methods (118 lines) to replace evaluator dependency |
| src/AutoML/AutoMLModelBase.cs | Added inline evaluation methods (279 lines) to replace evaluator dependency |
| src/Regression/StepwiseRegression.cs | Added inline evaluation method (63 lines) and removed evaluator parameter |
| src/Models/Options/OptimizationAlgorithmOptions.cs | Removed ModelEvaluator property and initialization |
| src/Interfaces/IAiModelBuilder.cs | Removed ConfigureModelEvaluator method and updated documentation |
| src/Interfaces/IAutoMLModel.cs | Removed SetModelEvaluator method |
| src/AiModelBuilder.cs | Removed evaluator field, ConfigureModelEvaluator method, and automatic cross-validation execution |
| src/Finance/AutoML/FinancialAutoML.cs | Removed modelEvaluator constructor parameter |
| src/AutoML/*.cs | Removed modelEvaluator parameters from all AutoML class constructors |
| src/Genetics/*.cs | Removed modelEvaluator parameters from all genetic algorithm constructors |
| src/Optimizers/*.cs | Removed modelEvaluator parameters from optimizer constructors |
| tests/AiDotNet.Tests/IntegrationTests/UncertaintyQuantificationFacadeTests.cs | Updated to use new EvaluateFull facade |
| tests/AiDotNet.Tests/IntegrationTests/Evaluation/EvaluationIntegrationTests.cs | Removed DefaultModelEvaluator constructor tests and updated reflection code |
💡 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/Genetics/NonDominatedSortingGeneticAlgorithm.cs (1)
11-21:⚠️ Potential issue | 🟡 MinorValidate objectives before indexing in the base initializer.
objectives[0]is evaluated before the null/Count check, so null or empty inputs throw before your intended validation error.🛠️ Suggested fix
public class NSGAII<T, TInput, TOutput> : StandardGeneticAlgorithm<T, TInput, TOutput> { @@ private readonly List<IFitnessCalculator<T, TInput, TOutput>> _objectives; + private static IFitnessCalculator<T, TInput, TOutput> GetPrimaryObjective( + List<IFitnessCalculator<T, TInput, TOutput>> objectives) + { + if (objectives == null || objectives.Count < 2) + { + throw new ArgumentException("NSGA-II requires at least two objectives", nameof(objectives)); + } + + return objectives[0]; + } + public NSGAII( Func<IFullModel<T, TInput, TOutput>> modelFactory, List<IFitnessCalculator<T, TInput, TOutput>> objectives) - : base(modelFactory, objectives[0]) + : base(modelFactory, GetPrimaryObjective(objectives)) { - if (objectives == null || objectives.Count < 2) - { - throw new ArgumentException("NSGA-II requires at least two objectives", nameof(objectives)); - } - _objectives = objectives; }src/Optimizers/OptimizerBase.cs (1)
344-359:⚠️ Potential issue | 🟠 MajorCache key still ignores selected features → wrong cache hits.
Applying feature selection earlier doesn’t help ifGenerateCacheKeyonly hashes parameters; different feature subsets can collide and reuse incorrect step data.✅ Suggested fix: include selected features in the cache key
- string cacheKey = GenerateCacheKey(solution, inputData); + string cacheKey = GenerateCacheKey(solution, inputData, selectedFeaturesIndices);-protected virtual string GenerateCacheKey(IFullModel<T, TInput, TOutput> solution, OptimizationInputData<T, TInput, TOutput> inputData) +protected virtual string GenerateCacheKey( + IFullModel<T, TInput, TOutput> solution, + OptimizationInputData<T, TInput, TOutput> inputData, + IReadOnlyCollection<int>? selectedFeatures = null) { // Generate a simple cache key based on parameter values var parameters = solution.GetParameters(); var paramHash = parameters.GetHashCode(); - return $"{solution.GetType().Name}_{paramHash}"; + var featuresHash = selectedFeatures is null + ? 0 + : HashCode.Combine(selectedFeatures.Count, string.Join(",", selectedFeatures)); + return $"{solution.GetType().Name}_{paramHash}_{featuresHash}"; }Also applies to: 730-736
🤖 Fix all issues with AI agents
In `@src/AiModelBuilder.cs`:
- Around line 1794-1796: The code currently sets cvResults to null and never
uses _crossValidator so ConfigureCrossValidation() is a no-op; fix by checking
if _crossValidator is configured and either (A) execute it to populate cvResults
(e.g., if (_crossValidator != null) cvResults = await
_crossValidator.RunCrossValidationAsync<T, TInput, TOutput>(...) or call the
CrossValidationEngine entry point with the same inputs used elsewhere), or (B)
if you prefer deferred evaluation, pass the configured _crossValidator instance
into the AiModelResult (set AiModelResult.CrossValidationResult or a new
CrossValidator property) so downstream evaluation can run it; update the
AiModelResult construction site to accept and store the validator or the
computed CrossValidationResult<T,TInput,TOutput> accordingly so
ConfigureCrossValidation() is no longer ignored.
In `@src/Optimizers/OptimizerBase.cs`:
- Around line 441-566: TryGetAlignedVectorsForOptimizer currently only converts
outputs to Vector<T>, losing multiclass metrics when predicted/actual are
matrices/tensors; update TryGetAlignedVectorsForOptimizer to detect
matrix/tensor outputs (using ConversionsHelper.ConvertToMatrix / ConvertToTensor
or equivalent) when PredictionType indicates multiclass, compute argmax across
the class dimension to produce label vectors for both actual and predicted, then
convert those argmax indices to Vector<T> so lengths align; keep the existing
ConvertToVector path and exception handling as a fallback so non-multiclass
cases still work and return false on any conversion mismatch.
🧹 Nitpick comments (1)
src/Optimizers/TabuSearchOptimizer.cs (1)
52-63: Missing XML documentation forfitnessCalculatorparameter.The constructor parameter
fitnessCalculatoron line 62 lacks a corresponding<param>XML documentation entry. All other parameters have documentation.📝 Proposed fix to add missing documentation
/// <param name="model">The model to be optimized.</param> /// <param name="options">Options specific to the Tabu Search algorithm.</param> /// <param name="geneticAlgorithm">The genetic algorithm to use for mutations. If null, a StandardGeneticAlgorithm will be used.</param> +/// <param name="fitnessCalculator">The fitness calculator to use for evaluating solutions. If null, a MeanSquaredErrorFitnessCalculator will be used.</param> /// <param name="engine">The computation engine (CPU or GPU) for vectorized operations.</param>
![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.
The matern-kernel playground example used 'length' but the constructor parameter is named 'lengthScale', causing compilation test failure. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
5 tasks done
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
Remove legacy
DefaultModelEvaluatorandIModelEvaluatorinterfaces in favor of the newMetricEvaluationEnginethroughAiModelResultfacade pattern.DefaultModelEvaluator.csandIModelEvaluator.csAiModelResult.Evaluation.cswithEvaluateFull()andGetDataSetStats()methodsIModelEvaluatordependency from AutoML, Optimizer, and Genetics classesStepwiseRegressionwith inline evaluationConfigureModelEvaluatorfromAiModelBuilderTest plan
DefaultModelEvaluatororIModelEvaluatorin src/Closes #334
🤖 Generated with Claude Code