Skip to content

Fix issue 324 and create FeatureSelectorBase class - #340

Merged
ooples merged 6 commits into
masterfrom
claude/issue-324-feature-selector-011CUsZeY7ucaZnfkWxSrJjt
Nov 7, 2025
Merged

ooples merged 6 commits into
masterfrom
claude/issue-324-feature-selector-011CUsZeY7ucaZnfkWxSrJjt

Conversation

@ooples

@ooples ooples commented Nov 7, 2025

Copy link
Copy Markdown
Owner

This commit implements all three phases of the advanced feature selection system:

Phase 1: UnivariateFeatureSelector (Advanced Filter Methods)

  • Statistical testing-based feature ranking
  • Supports three scoring functions:
    • Chi-Squared: For categorical features and targets
    • ANOVA F-value: For continuous features with categorical targets
    • Mutual Information: For any combination of feature/target types
  • Selects top K features based on univariate statistical relationships
  • Includes comprehensive unit tests with 13 test cases

Phase 2: SequentialFeatureSelector (Advanced Wrapper Methods)

  • Iterative feature selection using model performance evaluation
  • Supports two directions:
    • Forward Selection: Incrementally adds best-performing features
    • Backward Elimination: Iteratively removes worst-performing features
  • Integrates with any IFullModel for performance evaluation
  • Flexible scoring function support for different metrics
  • Includes comprehensive unit tests with 11 test cases

Phase 3: SelectFromModel (Embedded Methods)

  • Importance-based feature selection from trained models
  • Extracts feature importances from models implementing IFeatureImportance
  • Supports multiple threshold strategies:
    • Mean: Keep features above average importance
    • Median: Keep top 50% of features
    • Custom threshold: Explicit importance cutoff
    • Top K: Select exactly K most important features
  • Works with tree-based models (Gini importance) and L1-regularized linear models
  • Includes comprehensive unit tests with 16 test cases

Core Architecture Components

FeatureSelectorBase

  • Abstract base class providing common functionality
  • Implements IFeatureSelector<T, TInput>
  • Encapsulates:
    • Numeric operations handling (INumericOperations)
    • Higher-dimensional tensor feature extraction strategies
    • Common helper methods for feature vector extraction
  • Template method pattern for feature selection logic

Supporting Enums

  • UnivariateScoringFunction: ChiSquared, FValue, MutualInformation
  • SequentialFeatureSelectionDirection: Forward, Backward
  • ImportanceThresholdStrategy: Mean, Median

Key Design Principles

  • Generic type safety with INumericOperations (no hardcoded numeric types)
  • Inheritance pattern: Interface → Base class → Concrete implementations
  • Seamless integration with existing PredictionModelBuilder
  • Beginner-friendly defaults with research-backed justifications
  • Comprehensive documentation with "For Beginners" sections

Testing

  • Total of 40 unit tests across all three feature selectors
  • Tests cover:
    • Basic functionality and happy paths
    • Edge cases (single class, zero importances, etc.)
    • Error conditions (null inputs, mismatched dimensions)
    • Multiple numeric types (double and float)
    • Different feature selection strategies and configurations

All architectural requirements from issue #324 have been satisfied.

User Story / Context

  • Reference: [US-XXX] (if applicable)
  • Base branch: merge-dev2-to-master

Summary

  • What changed and why (scoped strictly to the user story / PR intent)

Verification

  • Builds succeed (scoped to changed projects)
  • Unit tests pass locally
  • Code coverage >= 90% for touched code
  • Codecov upload succeeded (if token configured)
  • TFM verification (net46, net6.0, net8.0) passes (if packaging)
  • No unresolved Copilot comments on HEAD

Copilot Review Loop (Outcome-Based)

Record counts before/after your last push:

  • Comments on HEAD BEFORE: [N]
  • Comments on HEAD AFTER (60s): [M]
  • Final HEAD SHA: [sha]

Files Modified

  • List files changed (must align with scope)

Notes

  • Any follow-ups, caveats, or migration details

This commit implements all three phases of the advanced feature selection system:

## Phase 1: UnivariateFeatureSelector (Advanced Filter Methods)
- Statistical testing-based feature ranking
- Supports three scoring functions:
  * Chi-Squared: For categorical features and targets
  * ANOVA F-value: For continuous features with categorical targets
  * Mutual Information: For any combination of feature/target types
- Selects top K features based on univariate statistical relationships
- Includes comprehensive unit tests with 13 test cases

## Phase 2: SequentialFeatureSelector (Advanced Wrapper Methods)
- Iterative feature selection using model performance evaluation
- Supports two directions:
  * Forward Selection: Incrementally adds best-performing features
  * Backward Elimination: Iteratively removes worst-performing features
- Integrates with any IFullModel for performance evaluation
- Flexible scoring function support for different metrics
- Includes comprehensive unit tests with 11 test cases

## Phase 3: SelectFromModel (Embedded Methods)
- Importance-based feature selection from trained models
- Extracts feature importances from models implementing IFeatureImportance
- Supports multiple threshold strategies:
  * Mean: Keep features above average importance
  * Median: Keep top 50% of features
  * Custom threshold: Explicit importance cutoff
  * Top K: Select exactly K most important features
- Works with tree-based models (Gini importance) and L1-regularized linear models
- Includes comprehensive unit tests with 16 test cases

## Core Architecture Components

### FeatureSelectorBase
- Abstract base class providing common functionality
- Implements IFeatureSelector<T, TInput>
- Encapsulates:
  * Numeric operations handling (INumericOperations<T>)
  * Higher-dimensional tensor feature extraction strategies
  * Common helper methods for feature vector extraction
- Template method pattern for feature selection logic

### Supporting Enums
- UnivariateScoringFunction: ChiSquared, FValue, MutualInformation
- SequentialFeatureSelectionDirection: Forward, Backward
- ImportanceThresholdStrategy: Mean, Median

## Key Design Principles
- Generic type safety with INumericOperations<T> (no hardcoded numeric types)
- Inheritance pattern: Interface → Base class → Concrete implementations
- Seamless integration with existing PredictionModelBuilder
- Beginner-friendly defaults with research-backed justifications
- Comprehensive documentation with "For Beginners" sections

## Testing
- Total of 40 unit tests across all three feature selectors
- Tests cover:
  * Basic functionality and happy paths
  * Edge cases (single class, zero importances, etc.)
  * Error conditions (null inputs, mismatched dimensions)
  * Multiple numeric types (double and float)
  * Different feature selection strategies and configurations

All architectural requirements from issue #324 have been satisfied.
Copilot AI review requested due to automatic review settings November 7, 2025 00:29
@coderabbitai

coderabbitai Bot commented Nov 7, 2025 •

Copy link
Copy Markdown
Contributor

Note

Other AI code review bot(s) detected

CodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review.

Summary by CodeRabbit

  • New Features

    • Added three new feature selection strategies: model-based feature importance selection, sequential forward/backward feature selection, and univariate statistical scoring methods.
  • Tests

    • Comprehensive test coverage added for new feature selection methods.

Walkthrough

Introduces a FeatureSelectorBase and three new enums; adds SelectFromModel, SequentialFeatureSelector, and UnivariateFeatureSelector; refactors existing selectors to inherit the base and return selected feature indices; and adds comprehensive unit tests and mocks for the new selectors.

Changes

Cohort / File(s) Summary
New Enums
src/Enums/ImportanceThresholdStrategy.cs, src/Enums/SequentialFeatureSelectionDirection.cs, src/Enums/UnivariateScoringFunction.cs
Added three public enums (ImportanceThresholdStrategy, SequentialFeatureSelectionDirection, UnivariateScoringFunction) with XML documentation to configure thresholding, sequential direction, and univariate scoring.
Feature Selector Base
src/FeatureSelectors/FeatureSelectorBase.cs
New abstract generic base class implementing IFeatureSelector<T, TInput>; centralizes numeric ops, extraction strategy, and provides public SelectFeatures that delegates to protected abstract List<int> SelectFeatureIndices(...).
Refactored Selectors
src/FeatureSelectors/CorrelationFeatureSelector.cs, src/FeatureSelectors/NoFeatureSelector.cs, src/FeatureSelectors/RecursiveFeatureElimination.cs, src/FeatureSelectors/VarianceThresholdFeatureSelector.cs
Migrated from implementing IFeatureSelector to inheriting FeatureSelectorBase; changed subclass entry to protected override List<int> SelectFeatureIndices(...) and adapted internal numeric/extraction calls to base helpers.
New Feature Selectors
src/FeatureSelectors/SelectFromModel.cs, src/FeatureSelectors/SequentialFeatureSelector.cs, src/FeatureSelectors/UnivariateFeatureSelector.cs
Added SelectFromModel (model-importance selection with mean/median/custom/top-K), SequentialFeatureSelector (forward/backward greedy selection using cloned models and scoring), and UnivariateFeatureSelector (ChiSquared, FValue, MutualInformation scoring).
Tests & Mocks
tests/UnitTests/FeatureSelectors/SelectFromModelTests.cs, tests/UnitTests/FeatureSelectors/SequentialFeatureSelectorTests.cs, tests/UnitTests/FeatureSelectors/UnivariateFeatureSelectorTests.cs
Added unit tests and mock models (double/float variants) covering thresholds, feature-name parsing, forward/backward flows, scoring functions, edge cases, and constructor validation.

Sequence Diagram(s)

sequenceDiagram
    autonumber
    participant User
    participant Public as Selector (public)
    participant Base as FeatureSelectorBase
    participant Input as InputHelper
    participant Sub as Subclass (SelectFeatureIndices)
    participant Helper as FeatureSelectorHelper

    User->>Public: SelectFeatures(allFeatures)
    activate Public
    Public->>Base: SelectFeatures(allFeatures)
    activate Base
    Base->>Input: determine numSamples & numFeatures
    Input-->>Base: (numSamples, numFeatures)
    Base->>Sub: SelectFeatureIndices(allFeatures, numSamples, numFeatures)
    activate Sub
    Note right of Sub: Subclass logic varies:\n- SelectFromModel: parse importances, threshold/top-K\n- Sequential: clone/train/evaluate subsets\n- Univariate: compute per-feature scores
    Sub-->>Base: List<int> selectedIndices
    deactivate Sub
    Base->>Helper: build filtered TInput from indices
    Helper-->>Base: filteredData (TInput)
    Base-->>Public: filteredData
    deactivate Base
    Public-->>User: filteredData
    deactivate Public
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

  • Focus areas:
    • src/FeatureSelectors/FeatureSelectorBase.cs — template method, numeric ops wiring, extraction strategy interactions.
    • src/FeatureSelectors/SelectFromModel.cs — feature-name parsing, importance sorting, median calculation, numeric generics.
    • src/FeatureSelectors/SequentialFeatureSelector.cs — clone/train/evaluate loop, termination criteria, scoring stability and exception handling.
    • src/FeatureSelectors/UnivariateFeatureSelector.cs — statistical implementations (ANOVA F, chi-squared, mutual information) and edge-case guards.
    • Refactored selectors — ensure preserved semantics where public surface changed from returning data to returning indices; check callers/tests.

Possibly related issues

Poem

🐰
I hopped through matrices, counted each score,
Mean or median, I knocked on new doors,
Base to guide paws, selectors in tune,
Mock models cheered under the testing moon,
A carrot for code — features found soon!

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 40.59% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title 'Fix issue 324 and create FeatureSelectorBase class' accurately describes the main changes: resolving issue 324 and introducing the FeatureSelectorBase architectural component.
Description check ✅ Passed The description is comprehensive and directly related to the changeset, detailing all three phases of feature selection (UnivariateFeatureSelector, SequentialFeatureSelector, SelectFromModel), the FeatureSelectorBase architecture, supporting enums, and extensive testing coverage.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch claude/issue-324-feature-selector-011CUsZeY7ucaZnfkWxSrJjt

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between bb5cc86 and 3e47950.

📒 Files selected for processing (4)
  • src/Enums/ImportanceThresholdStrategy.cs (1 hunks)
  • src/FeatureSelectors/RecursiveFeatureElimination.cs (4 hunks)
  • src/FeatureSelectors/SelectFromModel.cs (1 hunks)
  • src/FeatureSelectors/UnivariateFeatureSelector.cs (1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
src/FeatureSelectors/UnivariateFeatureSelector.cs (1)
src/FeatureSelectors/FeatureSelectorBase.cs (5)
  • TInput (91-102)
  • FeatureSelectorBase (18-146)
  • FeatureSelectorBase (63-70)
  • Vector (137-145)
  • List (123-123)
src/FeatureSelectors/RecursiveFeatureElimination.cs (3)
src/FeatureSelectors/SelectFromModel.cs (3)
  • T (386-408)
  • List (189-264)
  • List (283-323)
src/FeatureSelectors/NoFeatureSelector.cs (1)
  • List (43-47)
src/FeatureSelectors/VarianceThresholdFeatureSelector.cs (1)
  • List (85-108)
src/FeatureSelectors/SelectFromModel.cs (3)
src/FeatureSelectors/FeatureSelectorBase.cs (4)
  • TInput (91-102)
  • FeatureSelectorBase (18-146)
  • FeatureSelectorBase (63-70)
  • List (123-123)
tests/UnitTests/FeatureSelectors/SelectFromModelTests.cs (2)
  • Dictionary (21-24)
  • Dictionary (427-430)
tests/UnitTests/FeatureSelectors/SequentialFeatureSelectorTests.cs (2)
  • Dictionary (67-67)
  • Dictionary (408-408)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: Build All Frameworks
🔇 Additional comments (2)
src/FeatureSelectors/RecursiveFeatureElimination.cs (1)

128-157: Critical issue from previous review has been resolved.

The elimination logic now correctly:

  • Continues trimming until the desired number of features remain
  • Sorts importances in descending order (highest importance first)
  • Removes the least important feature each iteration
  • Returns the surviving feature indices rather than the eliminated ones

Great work addressing the previous concern!

src/FeatureSelectors/SelectFromModel.cs (1)

211-220: Top-K mode correctly bypasses threshold filtering.

The previous concern about top-K mode applying unwanted threshold filters has been resolved. This branch now correctly takes the top K features by importance without any threshold-based filtering, ensuring exactly K features are selected (or fewer if K exceeds the total number of features).


Comment @coderabbitai help to get the list of available commands and usage tips.

…lectorBase

This commit refactors all existing feature selectors to inherit from the new
FeatureSelectorBase class, ensuring consistent architecture across the codebase.

## Changes Made

### VarianceThresholdFeatureSelector
- Now extends FeatureSelectorBase<T, TInput> instead of implementing IFeatureSelector directly
- Removed duplicate fields (_numOps, _higherDimensionStrategy, _dimensionWeights)
- Uses protected properties from base class (NumOps, HigherDimensionStrategy, DimensionWeights)
- Implements SelectFeatureIndices() instead of SelectFeatures()
- Uses base class ExtractFeatureVector() helper method

### CorrelationFeatureSelector
- Now extends FeatureSelectorBase<T, TInput> instead of implementing IFeatureSelector directly
- Removed duplicate fields and uses base class properties
- Implements SelectFeatureIndices() instead of SelectFeatures()
- Simplified implementation using base class helpers

### RecursiveFeatureElimination
- Now extends FeatureSelectorBase<T, TInput> instead of implementing IFeatureSelector directly
- Removed duplicate fields and uses base class NumOps property
- Implements SelectFeatureIndices() instead of SelectFeatures()
- Maintains all existing RFE-specific logic and functionality

### NoFeatureSelector
- Now extends FeatureSelectorBase<T, TInput> instead of implementing IFeatureSelector directly
- Implements SelectFeatureIndices() to return all feature indices
- Maintains pass-through behavior while following new architecture pattern

## Benefits

- **Code Reuse**: Eliminates duplicate code across feature selectors
- **Consistency**: All feature selectors follow the same architectural pattern
- **Maintainability**: Common functionality centralized in base class
- **Type Safety**: Leverages base class numeric operations handling
- **Extensibility**: Easier to add new feature selectors following the pattern

All existing functionality is preserved with no breaking changes to the public API.
The SelectFeatures() method is still available through the base class implementation.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR introduces three new feature selection methods to the AiDotNet library: UnivariateFeatureSelector, SequentialFeatureSelector, and SelectFromModel. These implementations provide different strategies for dimensionality reduction and feature selection in machine learning workflows.

Key Changes

  • Added univariate feature selection using statistical tests (Chi-Squared, F-value, Mutual Information)
  • Implemented sequential feature selection with forward and backward strategies
  • Created model-based feature selection using feature importance scores
  • Added comprehensive unit test coverage for all three selectors

Reviewed Changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 10 comments.

Show a summary per file
File Description
src/FeatureSelectors/UnivariateFeatureSelector.cs Implements statistical test-based feature ranking and selection
src/FeatureSelectors/SequentialFeatureSelector.cs Implements wrapper-based forward/backward feature selection
src/FeatureSelectors/SelectFromModel.cs Implements embedded feature selection using model importance scores
src/FeatureSelectors/FeatureSelectorBase.cs Provides common functionality for all feature selectors
src/Enums/UnivariateScoringFunction.cs Defines statistical scoring functions for univariate selection
src/Enums/SequentialFeatureSelectionDirection.cs Defines forward/backward selection directions
src/Enums/ImportanceThresholdStrategy.cs Defines threshold strategies for importance-based selection
tests/UnitTests/FeatureSelectors/UnivariateFeatureSelectorTests.cs Comprehensive test coverage for univariate selector
tests/UnitTests/FeatureSelectors/SequentialFeatureSelectorTests.cs Comprehensive test coverage for sequential selector
tests/UnitTests/FeatureSelectors/SelectFromModelTests.cs Comprehensive test coverage for model-based selector

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/FeatureSelectors/SelectFromModel.cs
Comment thread src/FeatureSelectors/UnivariateFeatureSelector.cs Outdated
Comment thread src/FeatureSelectors/UnivariateFeatureSelector.cs Outdated
Comment thread src/FeatureSelectors/UnivariateFeatureSelector.cs Outdated
Comment thread src/FeatureSelectors/SelectFromModel.cs Outdated
Comment thread src/FeatureSelectors/SequentialFeatureSelector.cs Outdated
Comment thread src/FeatureSelectors/UnivariateFeatureSelector.cs Outdated
Comment thread src/FeatureSelectors/UnivariateFeatureSelector.cs Outdated
Comment thread src/FeatureSelectors/UnivariateFeatureSelector.cs Outdated
Comment thread src/FeatureSelectors/SelectFromModel.cs Outdated
- Replace NumOps.Compare with GreaterThan/LessThan comparisons in SelectFromModel and UnivariateFeatureSelector
- Combine nested if statements in SelectFromModel for cleaner logic
- Replace generic catch clauses with specific exception handling (ArgumentException, InvalidOperationException)
- Add explicit Where() filters to eliminate implicit filtering in foreach loops
- Convert if/else to ternary operator in SelectFromModel median calculation
- Add exception logging in SequentialFeatureSelector for better diagnostics

Addresses 10 CodeRabbit code quality suggestions on PR #340

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 6

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 06b5236 and 1144b52.

📒 Files selected for processing (14)
  • src/Enums/ImportanceThresholdStrategy.cs (1 hunks)
  • src/Enums/SequentialFeatureSelectionDirection.cs (1 hunks)
  • src/Enums/UnivariateScoringFunction.cs (1 hunks)
  • src/FeatureSelectors/CorrelationFeatureSelector.cs (3 hunks)
  • src/FeatureSelectors/FeatureSelectorBase.cs (1 hunks)
  • src/FeatureSelectors/NoFeatureSelector.cs (2 hunks)
  • src/FeatureSelectors/RecursiveFeatureElimination.cs (4 hunks)
  • src/FeatureSelectors/SelectFromModel.cs (1 hunks)
  • src/FeatureSelectors/SequentialFeatureSelector.cs (1 hunks)
  • src/FeatureSelectors/UnivariateFeatureSelector.cs (1 hunks)
  • src/FeatureSelectors/VarianceThresholdFeatureSelector.cs (4 hunks)
  • tests/UnitTests/FeatureSelectors/SelectFromModelTests.cs (1 hunks)
  • tests/UnitTests/FeatureSelectors/SequentialFeatureSelectorTests.cs (1 hunks)
  • tests/UnitTests/FeatureSelectors/UnivariateFeatureSelectorTests.cs (1 hunks)
🧰 Additional context used
🧬 Code graph analysis (11)
src/FeatureSelectors/RecursiveFeatureElimination.cs (3)
src/FeatureSelectors/CorrelationFeatureSelector.cs (1)
  • List (93-124)
src/FeatureSelectors/NoFeatureSelector.cs (1)
  • List (43-47)
src/FeatureSelectors/VarianceThresholdFeatureSelector.cs (1)
  • List (85-108)
src/FeatureSelectors/UnivariateFeatureSelector.cs (1)
src/FeatureSelectors/FeatureSelectorBase.cs (5)
  • TInput (91-102)
  • FeatureSelectorBase (18-146)
  • FeatureSelectorBase (63-70)
  • Vector (137-145)
  • List (123-123)
tests/UnitTests/FeatureSelectors/SelectFromModelTests.cs (1)
src/FeatureSelectors/SelectFromModel.cs (4)
  • SelectFromModel (30-394)
  • SelectFromModel (102-113)
  • SelectFromModel (132-143)
  • SelectFromModel (161-171)
src/FeatureSelectors/VarianceThresholdFeatureSelector.cs (3)
src/FeatureSelectors/CorrelationFeatureSelector.cs (1)
  • List (93-124)
src/FeatureSelectors/NoFeatureSelector.cs (1)
  • List (43-47)
src/FeatureSelectors/RecursiveFeatureElimination.cs (1)
  • List (113-160)
src/FeatureSelectors/FeatureSelectorBase.cs (2)
src/Helpers/MathHelper.cs (2)
  • INumericOperations (33-61)
  • MathHelper (16-987)
src/Helpers/InputHelper.cs (3)
  • InputHelper (6-707)
  • GetBatchSize (13-21)
  • GetInputSize (28-40)
src/FeatureSelectors/SequentialFeatureSelector.cs (2)
src/FeatureSelectors/FeatureSelectorBase.cs (4)
  • TInput (91-102)
  • FeatureSelectorBase (18-146)
  • FeatureSelectorBase (63-70)
  • List (123-123)
tests/UnitTests/FeatureSelectors/SequentialFeatureSelectorTests.cs (4)
  • IFullModel (54-57)
  • IFullModel (373-376)
  • Train (19-23)
  • Train (347-347)
tests/UnitTests/FeatureSelectors/UnivariateFeatureSelectorTests.cs (2)
src/FeatureSelectors/FeatureSelectorBase.cs (1)
  • Vector (137-145)
src/FeatureSelectors/UnivariateFeatureSelector.cs (2)
  • UnivariateFeatureSelector (26-382)
  • UnivariateFeatureSelector (82-92)
src/FeatureSelectors/SelectFromModel.cs (2)
src/FeatureSelectors/FeatureSelectorBase.cs (4)
  • TInput (91-102)
  • FeatureSelectorBase (18-146)
  • FeatureSelectorBase (63-70)
  • List (123-123)
tests/UnitTests/FeatureSelectors/SelectFromModelTests.cs (2)
  • Dictionary (21-24)
  • Dictionary (427-430)
tests/UnitTests/FeatureSelectors/SequentialFeatureSelectorTests.cs (2)
src/FeatureSelectors/FeatureSelectorBase.cs (1)
  • Vector (137-145)
src/FeatureSelectors/SequentialFeatureSelector.cs (2)
  • SequentialFeatureSelector (30-343)
  • SequentialFeatureSelector (122-136)
src/FeatureSelectors/CorrelationFeatureSelector.cs (3)
src/FeatureSelectors/NoFeatureSelector.cs (1)
  • List (43-47)
src/FeatureSelectors/RecursiveFeatureElimination.cs (1)
  • List (113-160)
src/FeatureSelectors/VarianceThresholdFeatureSelector.cs (1)
  • List (85-108)
src/FeatureSelectors/NoFeatureSelector.cs (3)
src/FeatureSelectors/CorrelationFeatureSelector.cs (1)
  • List (93-124)
src/FeatureSelectors/RecursiveFeatureElimination.cs (1)
  • List (113-160)
src/FeatureSelectors/VarianceThresholdFeatureSelector.cs (1)
  • List (85-108)
🪛 GitHub Actions: Build
tests/UnitTests/FeatureSelectors/SequentialFeatureSelectorTests.cs

[error] 5-5: dotnet build failed. CS0234: The type or namespace name 'Metadata' does not exist in the namespace 'AiDotNet' (are you missing an assembly reference?).

🪛 GitHub Check: Build All Frameworks
tests/UnitTests/FeatureSelectors/SequentialFeatureSelectorTests.cs

[failure] 14-14:
'SimpleMockModel' does not implement interface member 'IParameterizable<double, Matrix, Vector>.ParameterCount'


[failure] 14-14:
'SimpleMockModel' does not implement interface member 'IParameterizable<double, Matrix, Vector>.WithParameters(Vector)'


[failure] 14-14:
'SimpleMockModel' does not implement interface member 'IParameterizable<double, Matrix, Vector>.SetParameters(Vector)'


[failure] 14-14:
'SimpleMockModel' does not implement interface member 'IParameterizable<double, Matrix, Vector>.GetParameters()'. 'SimpleMockModel.GetParameters()' cannot implement 'IParameterizable<double, Matrix, Vector>.GetParameters()' because it does not have the matching return type of 'Vector'.


[failure] 14-14:
'SimpleMockModel' does not implement interface member 'IModelSerializer.Deserialize(byte[])'


[failure] 14-14:
'SimpleMockModel' does not implement interface member 'IModelSerializer.Serialize()'


[failure] 14-14:
'SimpleMockModel' does not implement interface member 'IModel<Matrix, Vector, ModelMetadata>.GetModelMetadata()'. 'SimpleMockModel.GetModelMetadata()' cannot implement 'IModel<Matrix, Vector, ModelMetadata>.GetModelMetadata()' because it does not have the matching return type of 'ModelMetadata'.


[failure] 364-364:
The type or namespace name 'ModelMetadata<>' could not be found (are you missing a using directive or an assembly reference?)


[failure] 41-41:
The type or namespace name 'ModelMetadata<>' could not be found (are you missing a using directive or an assembly reference?)


[failure] 5-5:
The type or namespace name 'Metadata' does not exist in the namespace 'AiDotNet' (are you missing an assembly reference?)

Comment thread src/Enums/ImportanceThresholdStrategy.cs
Comment thread src/FeatureSelectors/RecursiveFeatureElimination.cs Outdated
Comment thread src/FeatureSelectors/SelectFromModel.cs Outdated
Comment thread src/FeatureSelectors/UnivariateFeatureSelector.cs Outdated
Comment thread tests/UnitTests/FeatureSelectors/SelectFromModelTests.cs
Comment thread tests/UnitTests/FeatureSelectors/SequentialFeatureSelectorTests.cs
ooples and others added 2 commits November 6, 2025 22:03
- Fix using statement (AiDotNet.Metadata -> AiDotNet.Models)
- Add missing IModelSerializer methods (Serialize, Deserialize)
- Fix IParameterizable methods to use Vector<T> instead of Dictionary
- Add WithParameters method and ParameterCount property
- Add DeepCopy method for ICloneable interface
- Replace RowCount/ColumnCount with Rows/Columns for Matrix properties

These test files were added in this PR and need to match the current IFullModel interface requirements.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (4)
tests/UnitTests/FeatureSelectors/UnivariateFeatureSelectorTests.cs (1)

249-249: Weak assertion: verify feature index selection rather than feature values.

The assertion checks whether the first selected feature's value is 1.0 or 5.0, but this doesn't confirm which feature index was selected (column 0 with highly discriminative values vs. columns 1-2 with low discrimination). The test could pass even if the wrong feature is selected if values happen to match.

Replace with an assertion that verifies the selected feature index directly:

-// First feature should be selected (has highest variance between classes)
-Assert.True(Math.Abs(result[0, 0] - 1.0) < 0.01 || Math.Abs(result[0, 0] - 5.0) < 0.01);
+// Verify first feature (column 0) was selected by checking it has the highest between-class variance
+// Column 0 values: [1.0, 2.0, 9.0, 10.0] have range 9.0
+// Columns 1-2 have much smaller ranges (~0.3), so column 0 should be selected
+Assert.True(Math.Abs(result[0, 0] - features[0, 0]) < 0.01, 
+    "Expected first feature (column 0) to be selected due to highest F-value");

Alternatively, capture and assert on the selected feature indices before the filtering occurs by testing the selector's internal state or by comparing the result matrix columns against the original feature columns.

tests/UnitTests/FeatureSelectors/SequentialFeatureSelectorTests.cs (3)

16-22: Remove unused training data storage.

The Train() method stores _trainedData and _trainedTarget (lines 16-17, 21-22) but Predict() never uses them—it simply applies a fixed threshold (sum > 10) regardless of training. This creates dead code and misleading semantics.

Apply this diff to remove the unused fields:

-private Matrix<double>? _trainedData;
-private Vector<double>? _trainedTarget;
-
 public void Train(Matrix<double> input, Vector<double> expectedOutput)
 {
-    _trainedData = input;
-    _trainedTarget = expectedOutput;
+    // Mock training - no-op for testing
 }

286-323: Test name claims to verify different results but doesn't.

The test is named SelectFeatures_ForwardAndBackward_ProduceDifferentResults, but the assertions (lines 320-322) only check that both selectors return 2 features—they don't verify that the selected features actually differ. The comment "Results may differ depending on the selection process" acknowledges this without asserting it, making the test name misleading.

Either rename the test to reflect what it actually validates, or add an assertion to verify the difference:

Option 1: Rename to reflect actual behavior

-public void SelectFeatures_ForwardAndBackward_ProduceDifferentResults()
+public void SelectFeatures_ForwardAndBackward_BothSelectCorrectCount()

Option 2: Add assertion to verify difference

 // Assert - Both should select 2 features
 Assert.Equal(2, forwardResult.Columns);
 Assert.Equal(2, backwardResult.Columns);
-// Results may differ depending on the selection process
+// Verify that forward and backward may select different features
+// (In this specific case with the mock model, they should differ)
+bool columnsDiffer = false;
+for (int i = 0; i < forwardResult.Rows; i++)
+{
+    for (int j = 0; j < 2; j++)
+    {
+        if (Math.Abs(forwardResult[i, j] - backwardResult[i, j]) > 0.01)
+        {
+            columnsDiffer = true;
+            break;
+        }
+    }
+    if (columnsDiffer) break;
+}
+// Note: Depending on the data, forward/backward might select the same features
+// This test primarily ensures both directions work without errors

366-420: Inconsistent mock implementation: Train() is empty in float variant.

SimpleMockModelFloat has an empty Train() method (line 368), while SimpleMockModel stores training data (even though unused). For consistency and to avoid confusion, either both should be empty or both should store data.

Add a comment to clarify the no-op behavior:

 public void Train(Matrix<float> input, Vector<float> expectedOutput) 
-{ }
+{
+    // Mock training - no-op for testing
+}

Or mirror the double version's structure (storing unused data) for consistency, though removing storage from both (as suggested in the earlier comment) is preferable.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 1144b52 and bb5cc86.

📒 Files selected for processing (3)
  • tests/UnitTests/FeatureSelectors/SelectFromModelTests.cs (1 hunks)
  • tests/UnitTests/FeatureSelectors/SequentialFeatureSelectorTests.cs (1 hunks)
  • tests/UnitTests/FeatureSelectors/UnivariateFeatureSelectorTests.cs (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • tests/UnitTests/FeatureSelectors/SelectFromModelTests.cs
🧰 Additional context used
🧬 Code graph analysis (2)
tests/UnitTests/FeatureSelectors/SequentialFeatureSelectorTests.cs (2)
src/FeatureSelectors/FeatureSelectorBase.cs (1)
  • Vector (137-145)
src/FeatureSelectors/SequentialFeatureSelector.cs (2)
  • SequentialFeatureSelector (30-343)
  • SequentialFeatureSelector (122-136)
tests/UnitTests/FeatureSelectors/UnivariateFeatureSelectorTests.cs (2)
src/FeatureSelectors/FeatureSelectorBase.cs (1)
  • Vector (137-145)
src/FeatureSelectors/UnivariateFeatureSelector.cs (2)
  • UnivariateFeatureSelector (26-382)
  • UnivariateFeatureSelector (82-92)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: Build All Frameworks
🔇 Additional comments (5)
tests/UnitTests/FeatureSelectors/UnivariateFeatureSelectorTests.cs (2)

10-222: Excellent test coverage for core functionality and edge cases.

The test suite comprehensively validates UnivariateFeatureSelector across:

  • All three scoring functions (FValue, MutualInformation, ChiSquared)
  • Default k behavior (50% of features)
  • Boundary conditions (k > feature count, single class)
  • Error handling (target length mismatch, null target)
  • Type generics (float support)

The tests are well-structured with clear arrange-act-assert patterns and descriptive names.


252-278: LGTM! Multi-class scenario validation.

Correctly validates 3-class classification with 6 samples, confirming the selector handles multi-class targets beyond binary classification.

tests/UnitTests/FeatureSelectors/SequentialFeatureSelectorTests.cs (3)

14-79: Past review concerns addressed: interface implementation now complete.

SimpleMockModel now implements all required IFullModel members including IParameterizable (ParameterCount, GetParameters, SetParameters, WithParameters), IModelSerializer (Serialize, Deserialize, SaveModel, LoadModel), IFeatureAware, IFeatureImportance, and ICloneable. GetModelMetadata() correctly returns ModelMetadata.


243-284: LGTM! Comprehensive null argument validation.

The three constructor validation tests correctly verify that ArgumentNullException is thrown for null model, null target, and null scoring function, ensuring defensive programming in the SequentialFeatureSelector constructor.


99-241: Well-structured tests for sequential selection behavior.

The test suite effectively validates:

  • Forward vs. backward selection modes
  • Default feature count (50%)
  • Single-feature selection
  • Boundary case (requested features > total)

Tests follow clear naming conventions and arrange-act-assert structure, making the behavior expectations explicit.

- ImportanceThresholdStrategy.cs: Remove misleading Custom strategy from documentation,
  clarify that custom thresholds are available via constructor overload
- RecursiveFeatureElimination.cs: Fix critical logic error returning eliminated features
  instead of kept features, rename variables for clarity
- SelectFromModel.cs: Add explicit top-K mode detection to guarantee exactly K features
  when using k constructor, use NumOps.Zero for proper initialization
- UnivariateFeatureSelector.cs: Add validation to throw ArgumentException for k<=0
  instead of silently coercing to 1
- All fixes verified with build passing 0 errors

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
@ooples
ooples merged commit 7367d59 into master Nov 7, 2025
5 checks passed
@ooples
ooples deleted the claude/issue-324-feature-selector-011CUsZeY7ucaZnfkWxSrJjt branch November 7, 2025 03:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants