feat: add comprehensive feature selection module with 432 implementations - #799
Merged
Merged
Conversation
Migrates CorrelationFeatureSelector functionality to new preprocessing pattern: - Extends TransformerBase<T, Matrix<T>, Matrix<T>> - Implements Fit/Transform API - Adds correlation matrix computation and storage - Adds GetSupportMask() and GetCorrelationsWithSelected() utilities - Full For Beginners documentation on all methods Part of preprocessing migration for issue #324
Migrates LpNormNormalizer functionality to new preprocessing pattern: - Scales features (columns) by dividing by Lp-norm - Supports arbitrary p values (L1, L2, L-infinity) - Supports inverse transform - Full For Beginners documentation Note: This is column-wise normalization, different from row-wise Normalizer Part of preprocessing migration for issue #324
Reorganized src/Preprocessing/FeatureSelection/ into: - Filter/Univariate: VarianceThreshold, SelectKBest, SelectPercentile, etc. - Filter/Correlation: CorrelationSelector - Wrapper: RFE, RFECV, SequentialFeatureSelector - Embedded: SelectFromModel - Helpers: StatisticalTestHelper Updated namespaces and using statements accordingly. Part of preprocessing migration for issue #324
- Update AiModelResult, AiModelResultOptions to use PreprocessingInfo - Update DefaultModelEvaluator to use PreprocessingInfo - Update GeneticAlgorithmRegression, SymbolicRegression to use PreprocessingPipeline - Update QuantumNeuralNetwork to use PreprocessingPipeline - Delete old Normalizers, FeatureSelectors, DataProcessor directories - Delete old interfaces (INormalizer, IFeatureSelector, IDataPreprocessor) - Delete old NormalizationInfo, NormalizerFactory - Update test files to use new PreprocessingInfo pattern - Delete obsolete test files that tested old APIs This completes the migration to sklearn-style preprocessing pipeline. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…ions Add extensive feature selection methods covering critical and high-priority algorithms across multiple categories: Filter Methods: - Chi-squared, Gini impurity, entropy-based selection - ANOVA F-test, information gain, gain ratio - Variance threshold, correlation-based selection - Symmetric uncertainty, Fisher score, ReliefF Embedded Methods: - Lasso, Ridge, Elastic Net regularization - Tree-based importance (Random Forest, Gradient Boosting) - Permutation importance, drop-column importance - SCAD, MCP penalized regression Wrapper Methods: - Forward/backward selection, RFE - Exhaustive search, genetic algorithms - Particle swarm optimization Hybrid Methods: - MI + Correlation combined scoring - Two-stage selection pipelines - Ensemble combination methods Metaheuristic Methods: - Genetic algorithm, PSO, differential evolution - Firefly algorithm, harmony search - Artificial bee colony, ant colony optimization - Simulated annealing, grey wolf optimizer Incremental/Streaming Methods: - Adaptive feature selector - Online feature selection - Streaming feature selector Domain-Specific Methods: - Genomic, text, and image feature selectors - Bioinformatics methods (MRMR, SAM, Boruta) All implementations follow TransformerBase<T, Matrix<T>, Matrix<T>> pattern with proper documentation and beginner-friendly explanations. 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
WalkthroughReplaces legacy normalization/feature‑selection plumbing with a unified PreprocessingPipeline/PreprocessingInfo model; removes many old normalizer/feature‑selector interfaces and classes; adds ~70+ Transformer-based feature‑selection implementations and a planning catalog; updates AiModelBuilder, AiModelResult, evaluator, and related consumers to use the new pipeline. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant AiModelBuilder
participant AiModel as Model
participant PreprocessingPipeline as "Preprocessing\nPipeline (PreprocessingInfo)"
participant Evaluator
User->>AiModelBuilder: Build(modelConfig, preprocessingPipeline?)
AiModelBuilder->>AiModel: Create AiModelResult (attach PreprocessingInfo if provided)
User->>AiModel: Predict(rawInput)
alt PreprocessingInfo configured and fitted
AiModel->>PreprocessingPipeline: TransformFeatures(rawInput)
PreprocessingPipeline-->>AiModel: transformedInput
AiModel->>AiModel: Predict(transformedInput)
AiModel->>PreprocessingPipeline: InverseTransformPredictions(predictions)
PreprocessingPipeline-->>AiModel: finalPredictions
else
AiModel->>AiModel: Predict(rawInput)
end
User->>Evaluator: EvaluateModel(input, PreprocessingInfo?)
Evaluator->>AiModel: Request predictions/statistics
AiModel-->>Evaluator: Predictions + PreprocessingInfo (if any)
Evaluator-->>User: ModelStats (uses PreprocessingInfo when provided)
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
…l, etc.) Add 18 new feature selection methods across 12 new categories: Bayesian Methods: - ARDSelector: Automatic Relevance Determination for sparse selection - BayesianFeatureSelector: Posterior probability-based selection - SpikeAndSlabSelector: Spike-and-slab prior for variable selection Causal Methods: - MarkovBlanketSelector: Identifies Markov blanket of target - PCAlgorithmSelector: PC algorithm for causal discovery Clustering Methods: - ClusteringBasedFS: K-means feature clustering for redundancy removal - HierarchicalFS: Agglomerative clustering for feature grouping Constraint-based: - ConstraintBasedSelector: Respects mandatory/forbidden features and groups Fuzzy Methods: - FuzzyRoughSetSelector: Fuzzy rough set theory for uncertain data Graph-based: - GraphBasedFS: Uses PageRank/centrality on feature correlation graphs Kernel Methods: - KernelPCASelector: Kernel PCA-based non-linear importance - KernelFisherSelector: Kernel Fisher discriminant analysis Multi-objective: - NSGAIISelector: NSGA-II for accuracy vs feature count trade-off Neural Network-based: - AutoencoderSelector: Bottleneck autoencoder importance - AttentionBasedSelector: Learned attention weights for selection Robust Methods: - RobustFeatureSelector: Outlier-resistant rank-based selection Rough Set Methods: - RoughSetSelector: Reduct-based selection using indiscernibility Transfer Learning: - TransferLearningSelector: Combines source and target domain importance All methods follow TransformerBase<T, Matrix<T>, Matrix<T>> pattern with beginner-friendly documentation. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
![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: 16
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/NeuralNetworks/QuantumNeuralNetwork.cs (1)
403-414:⚠️ Potential issue | 🟠 MajorEnsure preprocessed values are non‑negative before
Sqrt.
PrepareQuantumStatenow uses the preprocessing pipeline, which can output negative values (e.g., standardization). That will yield NaNs when computing amplitudes.🛡️ Proposed fix
for (int i = 0; i < flatInput.Length; i++) { - var amplitude = NumOps.Sqrt(flatInput[i]); + var value = flatInput[i]; + if (NumOps.LessThan(value, NumOps.Zero)) + throw new ArgumentException("Preprocessed input must be non-negative for quantum state preparation.", nameof(input)); + var amplitude = NumOps.Sqrt(value); quantumState[i] = new Complex<T>(amplitude, NumOps.Zero); }src/AiModelBuilder.cs (1)
1306-1312:⚠️ Potential issue | 🟠 MajorStreaming training advertises preprocessing it never applies.
In
BuildStreamingSupervisedAsyncthere is noFit/Transformon_preprocessingPipeline, yet Line 1310 attachesPreprocessingInfo. If a pipeline is configured, the model trains on raw inputs while inference will transform inputs (and the pipeline may be unfitted), causing incorrect results or runtime errors. Either apply preprocessing during streaming (with partial-fit support) or keepPreprocessingInfonull / throw when a pipeline is configured.🛠️ Suggested fix (avoid advertising preprocessing before it's applied)
- PreprocessingInfo = _preprocessingPipeline is not null ? new PreprocessingInfo<T, TInput, TOutput>(_preprocessingPipeline) : null, + PreprocessingInfo = null,
🤖 Fix all issues with AI agents
In `@src/Preprocessing/FeatureSelection/Bioinformatics/SAM.cs`:
- Around line 86-97: The current split uses a fixed threshold (threshold=0.5)
and assigns indices to class0/class1 by comparing NumOps.ToDouble(target[i]),
which fails for binary labels that aren't in [0,1]; instead, determine the two
unique label values from target (map the first unique value to class0 and the
second to class1), iterate target and compare equality against those two values
to populate class0 and class1, and throw a clear exception if there are not
exactly two distinct labels; update any downstream assumptions that used the
threshold-based split accordingly.
- Around line 127-145: The code computes s0 from sortedSE (using _s0Percentile)
and then divides by (standardErrors[j] + s0) in the d-statistic loop; if all
standardErrors are zero s0 becomes zero and causes divide-by-zero/NaN. Fix by
flooring s0 to a small positive epsilon (e.g., s0 = Math.Max(s0, 1e-8) or set s0
to a tiny constant when sortedSE yields 0) before using it in the denominator so
that (standardErrors[j] + s0) is always positive; update the logic around s0
(computed from sortedSE/_s0Percentile) and ensure _dStatistics calculation uses
the floored s0.
In `@src/Preprocessing/FeatureSelection/Bioinformatics/SignificanceAnalysis.cs`:
- Around line 135-150: The SAM score computation can divide by zero if s0 is
zero/NaN; after computing s0 in SignificanceAnalysis.cs (the line setting s0
from pooledStds.OrderBy(...).Skip(...).First()), add a guard: if
double.IsNaN(s0) or Math.Abs(s0) == 0, replace s0 with a small positive epsilon
(e.g. 1e-8 or derived from pooledStds magnitude); also guard pooledStds[j] when
computing _samScores[j] by treating NaN/zero pooledStds entries as at least
epsilon so the denominator pooledStds[j] + s0 is never zero or NaN. Ensure you
reference and update the s0 variable and the denominator calculation that
assigns _samScores[j].
- Around line 79-88: The current split by threshold (NumOps.ToDouble(target[i])
< 0.5) in SignificanceAnalysis collapses non {0,1} labels; change logic to
compute the distinct labels from the target array (e.g., collect unique values
from target), ensure there are exactly two distinct labels (throw/return error
if not), then build class0Idx and class1Idx by comparing each target entry for
equality to the two distinct label values (use the actual label values, not
0.5). Update any variable names (target, class0Idx, class1Idx) accordingly and
ensure downstream code expects those two explicit label values.
- Around line 43-58: Add validation in the SignificanceAnalysis constructor to
guard against invalid nPermutations and fdrThreshold: ensure nPermutations is >
0 (throw ArgumentOutOfRangeException/ArgumentException if <= 0) and ensure
fdrThreshold is within a valid probability range (e.g., 0 < fdrThreshold <= 1 or
0 <= fdrThreshold <= 1 per project convention—throw ArgumentOutOfRangeException
if outside). Update the constructor that sets _nPermutations and _fdrThreshold
to perform these checks before assigning fields so downstream FDR/q-value
calculations cannot divide by zero or use nonsensical thresholds.
In `@src/Preprocessing/FeatureSelection/Bioinformatics/VolcanoPlotSelector.cs`:
- Around line 174-195: The IncompleteBeta implementation is missing
normalization by the Beta(a,b) term, so modify IncompleteBeta (used by
TwoTailedTTest) to return the regularized incomplete beta: compute the
normalization factor Beta(a,b)=exp(lgamma(a)+lgamma(b)-lgamma(a+b)) (or use an
existing LogGamma/regularized beta utility) and divide the current integrand
term bt*BetaCF(...) by Beta(a,b) (or equivalently multiply by
exp(lgamma(a+b)-lgamma(a)-lgamma(b))); ensure both branches that return bt *
BetaCF(...) apply this normalization so TwoTailedTTest yields correct p-values.
In `@src/Preprocessing/FeatureSelection/CostSensitive/CostSensitiveFS.cs`:
- Around line 24-26: The Fit overload that accepts featureCosts creates a
temporary CostSensitiveFS and only copies scores/indices, leaving
this._nInputFeatures at 0 and this._featureCosts null; update the overload in
class CostSensitiveFS so it persists the effective input feature count and the
resolved featureCosts into the current instance (_nInputFeatures and
_featureCosts) before delegating to the primary Fit logic (or call the main Fit
from this after setting those fields), ensuring GetSupportMask(), FeatureCosts
and GetTotalCost() operate on the correct state when costs are supplied or
defaulted.
In `@src/Preprocessing/FeatureSelection/Dimensionality/TruncatedSVDSelection.cs`:
- Around line 78-90: The loop computing singular values currently calls
ComputeEigenvalue twice and passes possibly-negative values to Math.Sqrt,
causing NaNs; modify the loop in TruncatedSVDSelection.cs to call
ComputeEigenvalue once (store it in a local variable, e.g., eigenvalue), clamp
that eigenvalue to be >= 0 before taking Math.Sqrt to set singularValues[k], and
then reuse the same clamped eigenvalue when deflating deflatedXtX; ensure you
still populate loadings[k, j] from the eigenvector returned by PowerIteration
and only perform the deflation using the stored eigenvalue.
In `@src/Preprocessing/FeatureSelection/DomainSpecific/GenomicFeatureSelector.cs`:
- Around line 72-112: The Fit method currently can divide by zero when data.Rows
or data.Columns is zero and can index past the end when _variancePercentile ==
1; add guards at the start of Fit to validate n = data.Rows and p = data.Columns
(throw ArgumentException with a clear message if n == 0 or p == 0), and ensure
later variance calculation assumes n > 0; when computing varianceThreshold,
compute an index safely (e.g., int idx = Math.Min((int)(p *
_variancePercentile), p - 1)) and use that index into sortedVariances so
_variancePercentile == 1 maps to the last element; reference the Fit method,
local variables n and p, field _variances, and variable
varianceThreshold/_variancePercentile when making these changes.
In `@src/Preprocessing/FeatureSelection/DomainSpecific/ImageFeatureSelector.cs`:
- Around line 43-61: The constructor ImageFeatureSelector currently doesn't
validate numChannels which can be zero or negative and cause divide/modulo by
zero in GetSpatialPosition; add a check in the ImageFeatureSelector constructor
to ensure numChannels is >= 1 (throw ArgumentException or
ArgumentOutOfRangeException with a clear message and parameter name) and ensure
the private field _numChannels is only set after validation so
GetSpatialPosition (and any other spatial indexing) cannot encounter
division/modulo by zero.
- Around line 70-81: The Fit method can pass an empty dataset to
ComputeBaseScores which divides by n and causes a divide-by-zero; update Fit (in
ImageFeatureSelector.cs) to explicitly check for data.Rows == 0 (and/or
target.Length == 0) before computing scores and throw a clear ArgumentException
(or InvalidOperationException) indicating the dataset/target is empty; ensure
this check occurs before setting _nInputFeatures and before calling
ComputeBaseScores so ComputeBaseScores is never invoked with n == 0.
In `@src/Preprocessing/FeatureSelection/DomainSpecific/TextFeatureSelector.cs`:
- Around line 70-90: In Fit (the public void Fit(Matrix<T> data, Vector<T>
target) method) add an early guard to reject empty datasets before computing
_documentFrequencies: check if data.Rows == 0 (and/or target.Length == 0) and
throw a clear ArgumentException (e.g., "Data must contain at least one row.") so
the subsequent loop that sets _documentFrequencies[j] = (double)docCount / n
cannot divide by zero; place this check alongside the existing target/data
length validation so the document frequency computation is only reached when n >
0.
In `@src/Preprocessing/FeatureSelection/Embedded/ElasticNetFS.cs`:
- Around line 118-119: ElasticNetFS computes l1Penalty and l2Penalty without
scaling by the sample count, causing inconsistent behavior versus
ElasticNetSelector/ElasticNetFeatureSelection; modify the l1Penalty and
l2Penalty calculations in ElasticNetFS to multiply by the number of samples (the
same n used in the other classes) so they read _alpha * _l1Ratio * n and _alpha
* (1 - _l1Ratio) * n (use the local sample-count variable in scope, e.g., n or
sampleCount) to align the penalty scaling with ElasticNetSelector and
ElasticNetFeatureSelection.
In `@src/Preprocessing/FeatureSelection/Embedded/GroupLassoSelector.cs`:
- Around line 75-77: Validate user-provided _groups in the GroupLassoSelector
code path where groups and nGroups are computed: if _groups is null keep the
current default, otherwise check that _groups is non-empty, every group contains
at least one index, every index value x satisfies 0 <= x < p, and no index
appears in more than one group (detect duplicates by flattening groups and
checking counts); when a validation fails throw an ArgumentException with a
clear message identifying the problem so callers know whether it’s out-of-range,
empty group list, empty group, or overlapping indices.
In `@src/Preprocessing/FeatureSelection/Embedded/MCPSelector.cs`:
- Around line 169-184: The MCPThreshold method incorrectly applies one formula
across all regions; update MCPThreshold(z, lambda, gamma) to explicitly handle
three cases: if Math.Abs(z) <= lambda return 0; else if Math.Abs(z) <= gamma *
lambda return Math.Sign(z) * (Math.Abs(z) - lambda) * gamma / (gamma - 1); else
return z. Ensure you reference MCPThreshold and use the existing parameters z,
lambda, gamma and Math.Sign/Math.Abs to implement the corrected piecewise
proximal operator.
In `@src/Preprocessing/FeatureSelection/Embedded/SCADSelector.cs`:
- Around line 170-190: In SCADThreshold(double z, double lambda, double a) the
soft-threshold region boundary is incorrect; change the conditional that checks
the soft-threshold region from using lambda to using 2 * lambda (i.e., replace
the current absZ <= lambda check with absZ < 2 * lambda) so the regions become:
soft threshold for |z| < 2λ, transition for 2λ ≤ |z| ≤ aλ, and no shrinkage for
|z| > aλ; keep the rest of the logic (Math.Sign(z) * Math.Max(0, absZ - lambda),
the transition formula using (a - 1) * z - Math.Sign(z) * a * lambda over (a -
2), and return z) unchanged.
🟡 Minor comments (31)
src/Preprocessing/FeatureSelection/Embedded/RidgeSelector.cs-156-157 (1)
156-157:⚠️ Potential issue | 🟡 MinorSilent handling of near-singular matrices may produce incorrect coefficients.
When the pivot element is near zero (line 156), the code continues without proper handling. Similarly, during back-substitution (line 176), skipping the division leaves
x[i]with an incorrect value.While
alpha > 0ensures the matrix is positive definite,alpha = 0is permitted by the constructor and could lead to singularity with collinear features. Consider either:
- Throwing an exception when singularity is detected, or
- Requiring
alpha > 0in the constructor.🛡️ Option 1: Throw on singularity
if (Math.Abs(augmented[col, col]) < 1e-10) - continue; + throw new InvalidOperationException( + $"Matrix is singular or near-singular at column {col}. Consider increasing alpha.");🛡️ Option 2: Require alpha > 0
- if (alpha < 0) - throw new ArgumentException("Alpha must be non-negative.", nameof(alpha)); + if (alpha <= 0) + throw new ArgumentException("Alpha must be positive.", nameof(alpha));Also applies to: 176-177
src/Preprocessing/FeatureSelection/Embedded/RidgeSelector.cs-54-61 (1)
54-61:⚠️ Potential issue | 🟡 MinorAdd validation for empty data.
If
datahas zero rows, division bynon lines 73, 80, and 87 will produceNaNor infinity. Consider adding early validation.🛡️ Proposed fix
public void Fit(Matrix<T> data, Vector<T> target) { if (data.Rows != target.Length) throw new ArgumentException("Target length must match rows in data."); + if (data.Rows == 0) + throw new ArgumentException("Data must have at least one row.", nameof(data)); + if (data.Columns == 0) + throw new ArgumentException("Data must have at least one column.", nameof(data)); _nInputFeatures = data.Columns;src/Preprocessing/FeatureSelection/Embedded/RidgeSelector.cs-222-233 (1)
222-233:⚠️ Potential issue | 🟡 MinorInconsistent behavior when not fitted, and silent filtering of invalid indices.
Line 224: Returns an empty array when not fitted, but
GetSupportMask()throwsInvalidOperationException. Consider being consistent.Line 230: The
.Where(i => i < inputFeatureNames.Length)silently drops indices ifinputFeatureNamesis shorter than expected. This could hide bugs where the caller passes an incorrect names array, resulting in fewer output names than selected features.♻️ Proposed fix for consistency and explicit validation
public override string[] GetFeatureNamesOut(string[]? inputFeatureNames = null) { - if (_selectedIndices is null) return Array.Empty<string>(); + if (_selectedIndices is null) + throw new InvalidOperationException("RidgeSelector has not been fitted."); if (inputFeatureNames is null) return _selectedIndices.Select(i => $"Feature{i}").ToArray(); + if (inputFeatureNames.Length < _nInputFeatures) + throw new ArgumentException( + $"Expected at least {_nInputFeatures} feature names, but got {inputFeatureNames.Length}.", + nameof(inputFeatureNames)); + return _selectedIndices - .Where(i => i < inputFeatureNames.Length) .Select(i => inputFeatureNames[i]) .ToArray(); }src/Preprocessing/FeatureSelection/Embedded/GradientBoostingImportance.cs-270-281 (1)
270-281:⚠️ Potential issue | 🟡 MinorInconsistent behavior when not fitted.
GetSupportMask()throwsInvalidOperationExceptionwhen not fitted, butGetFeatureNamesOut()silently returns an empty array (line 272). Consider throwing for consistency, or document this intentional difference.Also, the
.Where(i => i < inputFeatureNames.Length)filter silently drops indices if the input array is too short, which could produce fewer output names than selected features.🛠️ Proposed fix for consistency
public override string[] GetFeatureNamesOut(string[]? inputFeatureNames = null) { - if (_selectedIndices is null) return []; + if (_selectedIndices is null) + throw new InvalidOperationException("GradientBoostingImportance has not been fitted."); if (inputFeatureNames is null) return _selectedIndices.Select(i => $"Feature{i}").ToArray(); + if (inputFeatureNames.Length < _nInputFeatures) + throw new ArgumentException( + $"Expected at least {_nInputFeatures} feature names, but got {inputFeatureNames.Length}.", + nameof(inputFeatureNames)); + return _selectedIndices - .Where(i => i < inputFeatureNames.Length) .Select(i => inputFeatureNames[i]) .ToArray(); }src/Preprocessing/FeatureSelection/Embedded/GradientBoostingImportance.cs-41-58 (1)
41-58:⚠️ Potential issue | 🟡 MinorAdd validation for
nEstimators,maxDepth, andlearningRate.The constructor validates
nFeaturesToSelectbut not the other parameters. IfnEstimators <= 0, the boosting loop never runs and importances remain zero. IfmaxDepth <= 0, all nodes become leaves immediately with no splits (no importance accumulated). IflearningRate <= 0, the model won't converge properly.🛡️ Proposed fix to add parameter validation
public GradientBoostingImportance( int nFeaturesToSelect = 10, int nEstimators = 100, int maxDepth = 3, double learningRate = 0.1, int? randomState = null, int[]? columnIndices = null) : base(columnIndices) { if (nFeaturesToSelect < 1) throw new ArgumentException("Number of features must be at least 1.", nameof(nFeaturesToSelect)); + if (nEstimators < 1) + throw new ArgumentException("Number of estimators must be at least 1.", nameof(nEstimators)); + if (maxDepth < 1) + throw new ArgumentException("Max depth must be at least 1.", nameof(maxDepth)); + if (learningRate <= 0) + throw new ArgumentException("Learning rate must be positive.", nameof(learningRate)); _nFeaturesToSelect = nFeaturesToSelect;src/Preprocessing/FeatureSelection/Embedded/SCAD.cs-41-62 (1)
41-62:⚠️ Potential issue | 🟡 MinorAdd validation for
maxIterationsandtoleranceparameters.The constructor validates
nFeaturesToSelect,lambda, anda, butmaxIterationsandtoleranceare not validated. A non-positivemaxIterationswould skip optimization entirely, and a non-positivetolerancecould cause unexpected convergence behavior.🛡️ Proposed fix to add validation
if (a <= 2) throw new ArgumentException("Parameter 'a' must be greater than 2.", nameof(a)); + if (maxIterations < 1) + throw new ArgumentException("Max iterations must be at least 1.", nameof(maxIterations)); + if (tolerance <= 0) + throw new ArgumentException("Tolerance must be positive.", nameof(tolerance)); _nFeaturesToSelect = nFeaturesToSelect;src/Preprocessing/FeatureSelection/Embedded/SCAD.cs-261-284 (1)
261-284:⚠️ Potential issue | 🟡 MinorInconsistent fitted-state handling and silent index filtering.
Two issues:
GetSupportMask()throws if not fitted, butGetFeatureNamesOut()returns an empty array. Consider consistent behavior across both methods.Line 281 silently filters out indices when
inputFeatureNamesis shorter than expected. This could return fewer names than selected features, which may confuse users expecting a 1:1 mapping.🔧 Proposed fix for consistency
public override string[] GetFeatureNamesOut(string[]? inputFeatureNames = null) { - if (_selectedIndices is null) return []; + if (_selectedIndices is null) + throw new InvalidOperationException("SCAD has not been fitted."); if (inputFeatureNames is null) return _selectedIndices.Select(i => $"Feature{i}").ToArray(); + if (inputFeatureNames.Length < _nInputFeatures) + throw new ArgumentException( + $"Expected at least {_nInputFeatures} feature names but got {inputFeatureNames.Length}.", + nameof(inputFeatureNames)); + return _selectedIndices - .Where(i => i < inputFeatureNames.Length) .Select(i => inputFeatureNames[i]) .ToArray(); }src/Preprocessing/FeatureSelection/Dimensionality/PCABasedSelection.cs-78-84 (1)
78-84:⚠️ Potential issue | 🟡 MinorDivision by zero when data has only one sample.
When
n == 1, the denominator(n - 1)becomes zero, causing a division-by-zero exception. While single-sample datasets are uncommon for PCA, the method should fail gracefully.🛡️ Proposed fix to add validation
protected override void FitCore(Matrix<T> data) { _nInputFeatures = data.Columns; int n = data.Rows; int p = data.Columns; + + if (n < 2) + throw new ArgumentException("PCA-based selection requires at least 2 samples.", nameof(data)); // Center the datasrc/Preprocessing/FeatureSelection/Dimensionality/PCABasedSelection.cs-207-218 (1)
207-218:⚠️ Potential issue | 🟡 MinorInconsistent behavior when not fitted, and silent index filtering.
Two concerns:
Line 209: Returns an empty array when not fitted, whereas
GetSupportMask()andTransformCore()throwInvalidOperationException. Consider throwing for consistency.Lines 214-217: Silently filters indices where
i >= inputFeatureNames.Length. If the caller provides a mismatched array, this silently returns fewer names than expected, potentially hiding bugs.🛡️ Proposed fix for consistent error handling
public override string[] GetFeatureNamesOut(string[]? inputFeatureNames = null) { - if (_selectedIndices is null) return []; + if (_selectedIndices is null) + throw new InvalidOperationException("PCABasedSelection has not been fitted."); if (inputFeatureNames is null) return _selectedIndices.Select(i => $"Feature{i}").ToArray(); + if (inputFeatureNames.Length < _nInputFeatures) + throw new ArgumentException( + $"Expected at least {_nInputFeatures} feature names, but got {inputFeatureNames.Length}.", + nameof(inputFeatureNames)); + return _selectedIndices - .Where(i => i < inputFeatureNames.Length) .Select(i => inputFeatureNames[i]) .ToArray(); }src/Preprocessing/FeatureSelection/Bioinformatics/SAM.cs-53-64 (1)
53-64:⚠️ Potential issue | 🟡 MinorValidate
s0Percentilebounds to avoid out-of-range index.If
s0Percentileis outside [0, 1],s0Indexcan go negative or exceedp-1, causing runtime failures at Line 129.🔧 Proposed fix
if (nPermutations < 10) throw new ArgumentException("Number of permutations must be at least 10.", nameof(nPermutations)); if (fdrThreshold <= 0 || fdrThreshold >= 1) throw new ArgumentException("FDR threshold must be between 0 and 1.", nameof(fdrThreshold)); + if (s0Percentile < 0 || s0Percentile > 1) + throw new ArgumentException("s0Percentile must be between 0 and 1.", nameof(s0Percentile));src/Preprocessing/FeatureSelection/Embedded/RandomForestImportance.cs-41-60 (1)
41-60:⚠️ Potential issue | 🟡 MinorAdd validation for
maxDepthandminSamplesLeafparameters.The constructor validates
nFeaturesToSelectandnTreesbut notmaxDepthorminSamplesLeaf. SettingmaxDepth <= 0would prevent any splits (sincedepth >= _maxDepthis checked at line 139), andminSamplesLeaf < 1could cause unexpected behavior.🛡️ Proposed fix to add validation
if (nTrees < 1) throw new ArgumentException("Number of trees must be at least 1.", nameof(nTrees)); + if (maxDepth < 1) + throw new ArgumentException("Max depth must be at least 1.", nameof(maxDepth)); + if (minSamplesLeaf < 1) + throw new ArgumentException("Min samples per leaf must be at least 1.", nameof(minSamplesLeaf)); _nFeaturesToSelect = nFeaturesToSelect;src/Preprocessing/FeatureSelection/Embedded/RandomForestImportance.cs-109-109 (1)
109-109:⚠️ Potential issue | 🟡 MinorUnused
randparameter passed toBuildTree.The
randparameter is passed toBuildTreebut never used within the method. Either remove the parameter or use it (e.g., for random tie-breaking or per-node feature re-sampling, which is standard Random Forest behavior).♻️ Option 1: Remove unused parameter
- var treeImportances = BuildTree(X, y, sampleIndices, featureSubset, 0, rand); + var treeImportances = BuildTree(X, y, sampleIndices, featureSubset, 0);And update the method signature at line 134:
- private double[] BuildTree(double[,] X, double[] y, int[] indices, int[] features, int depth, Random rand) + private double[] BuildTree(double[,] X, double[] y, int[] indices, int[] features, int depth)src/Preprocessing/FeatureSelection/Embedded/RandomForestImportance.cs-244-255 (1)
244-255:⚠️ Potential issue | 🟡 MinorSilent filtering may mask dimension mismatches.
Line 252 filters out selected indices that exceed
inputFeatureNames.Length. This silently produces a shorter result if the input names don't match the original feature count, potentially masking bugs where the model was fit on different data.Consider throwing or logging a warning when
inputFeatureNames.Length != _nInputFeatures.🛡️ Proposed fix to validate input feature names length
public override string[] GetFeatureNamesOut(string[]? inputFeatureNames = null) { if (_selectedIndices is null) return Array.Empty<string>(); if (inputFeatureNames is null) return _selectedIndices.Select(i => $"Feature{i}").ToArray(); + if (inputFeatureNames.Length != _nInputFeatures) + throw new ArgumentException( + $"Expected {_nInputFeatures} feature names but received {inputFeatureNames.Length}.", + nameof(inputFeatureNames)); + - return _selectedIndices - .Where(i => i < inputFeatureNames.Length) - .Select(i => inputFeatureNames[i]) - .ToArray(); + return _selectedIndices.Select(i => inputFeatureNames[i]).ToArray(); }src/Preprocessing/FeatureSelection/Embedded/MCP.cs-41-62 (1)
41-62:⚠️ Potential issue | 🟡 MinorValidate
maxIterationsandtoleranceto avoid silent no-ops.If
maxIterations <= 0the optimizer never runs; iftolerance <= 0convergence checks become meaningless. Guard these like the other parameters.🧩 Suggested validation
if (gamma <= 1) throw new ArgumentException("Gamma must be greater than 1.", nameof(gamma)); + if (maxIterations < 1) + throw new ArgumentException("Max iterations must be at least 1.", nameof(maxIterations)); + if (tolerance <= 0) + throw new ArgumentException("Tolerance must be positive.", nameof(tolerance)); _nFeaturesToSelect = nFeaturesToSelect;src/Preprocessing/FeatureSelection/Dimensionality/TruncatedSVDSelection.cs-194-204 (1)
194-204:⚠️ Potential issue | 🟡 MinorAvoid silently dropping feature names when input list is shorter.
Currently, shorterinputFeatureNamesproduces fewer names than selected features. Prefer explicit validation to prevent silent mismatch.✅ Suggested fix
public override string[] GetFeatureNamesOut(string[]? inputFeatureNames = null) { if (_selectedIndices is null) return []; if (inputFeatureNames is null) return _selectedIndices.Select(i => $"Feature{i}").ToArray(); + if (inputFeatureNames.Length < _nInputFeatures) + throw new ArgumentException($"Expected at least {_nInputFeatures} feature names.", nameof(inputFeatureNames)); return _selectedIndices - .Where(i => i < inputFeatureNames.Length) .Select(i => inputFeatureNames[i]) .ToArray(); }src/Preprocessing/FeatureSelection/Dimensionality/TruncatedSVDSelection.cs-161-172 (1)
161-172:⚠️ Potential issue | 🟡 MinorValidate input feature count before indexing.
IfTransformCorereceives data with a different column count than fit, this will throw an index error. A clear guard improves reliability.✅ Suggested fix
protected override Matrix<T> TransformCore(Matrix<T> data) { if (_selectedIndices is null) throw new InvalidOperationException("TruncatedSVDSelection has not been fitted."); + if (data.Columns != _nInputFeatures) + throw new ArgumentException($"Expected {_nInputFeatures} features, got {data.Columns}.", nameof(data)); int numRows = data.Rows; int numCols = _selectedIndices.Length; var result = new T[numRows, numCols];src/Preprocessing/FeatureSelection/Embedded/PermutationImportance.cs-246-256 (1)
246-256:⚠️ Potential issue | 🟡 MinorFail fast when not fitted in
GetFeatureNamesOut
GetSupportMask()throws when not fitted, butGetFeatureNamesOut()silently returns an empty array. This can hide misuse and produce confusing downstream behavior. Consider throwing for consistency.🛡️ Suggested change
- if (_selectedIndices is null) return []; + if (_selectedIndices is null) + throw new InvalidOperationException("PermutationImportance has not been fitted.");src/Preprocessing/FeatureSelection/Bioinformatics/FoldChangeSelector.cs-25-52 (1)
25-52:⚠️ Potential issue | 🟡 Minor_logTransform has no effect on selection.
The parameter is stored but never used; selection always ranks by abs(log2 fold change), so callers can’t disable log-based scoring. Either remove the parameter or use it to drive ranking.
💡 Suggested fix (use _logTransform to drive scoring)
- var candidateFeatures = new List<(int Index, double AbsFC)>(); + var candidateFeatures = new List<(int Index, double Score)>(); for (int j = 0; j < p; j++) { if (_foldChanges[j] >= _minFoldChange) - candidateFeatures.Add((j, Math.Abs(_log2FoldChanges[j]))); + { + double score = _logTransform ? Math.Abs(_log2FoldChanges[j]) : _foldChanges[j]; + candidateFeatures.Add((j, score)); + } } if (candidateFeatures.Count >= _nFeaturesToSelect) { _selectedIndices = candidateFeatures - .OrderByDescending(x => x.AbsFC) + .OrderByDescending(x => x.Score) .Take(_nFeaturesToSelect) .Select(x => x.Index) .OrderBy(x => x) .ToArray(); } else { // Fall back to top by fold change _selectedIndices = _foldChanges - .Select((fc, idx) => (FC: fc, Index: idx)) - .OrderByDescending(x => x.FC) + .Select((fc, idx) => (Score: _logTransform ? Math.Abs(_log2FoldChanges[idx]) : fc, Index: idx)) + .OrderByDescending(x => x.Score) .Take(_nFeaturesToSelect) .Select(x => x.Index) .OrderBy(x => x) .ToArray(); }Also applies to: 119-144
src/Preprocessing/FeatureSelection/Embedded/TreeBasedImportance.cs-106-116 (1)
106-116:⚠️ Potential issue | 🟡 MinorVariance calculation may produce negative values.
The formula
sumSq / _nTrees - _importances[j] * _importances[j]at line 115 can produce small negative values due to floating-point errors when variance is near zero. This would causeMath.Sqrtto returnNaN.Proposed fix
- _importanceStds[j] = Math.Sqrt(sumSq / _nTrees - _importances[j] * _importances[j]); + double variance = sumSq / _nTrees - _importances[j] * _importances[j]; + _importanceStds[j] = Math.Sqrt(Math.Max(0, variance));src/Preprocessing/FeatureSelection/Embedded/DropColumnImportance.cs-196-200 (1)
196-200:⚠️ Potential issue | 🟡 MinorPotential correlation formula issue in ComputeR2Score.
The correlation calculation at line 199 appears to have an extra
nTrainfactor under the square root. For Pearson correlation,sxyandsxxare already sums over all samples, so the denominator should beMath.Sqrt(sxx * yVar * nTrain * nTrain)or equivalently the formula should use sample covariance/variance instead of sums.Current:
sxy / Math.Sqrt(sxx * yVar * nTrain)
Expected for correlation:sxy / Math.Sqrt(sxx * syy)wheresyy = yVar * nTrainThis may cause the correlation values to be scaled incorrectly, though feature ranking could still work since all features are equally affected.
Proposed fix
- correlations[j] = (sxx > 1e-10 && yVar > 1e-10) ? sxy / Math.Sqrt(sxx * yVar * nTrain) : 0; + double syy = yVar * nTrain; + correlations[j] = (sxx > 1e-10 && syy > 1e-10) ? sxy / Math.Sqrt(sxx * syy) : 0;src/Preprocessing/FeatureSelection/Embedded/ElasticNetSelector.cs-50-51 (1)
50-51:⚠️ Potential issue | 🟡 MinorInconsistent validation:
alpha >= 0here vsalpha > 0in ElasticNetFS.
ElasticNetSelectorallowsalpha = 0(no regularization), whileElasticNetFSrequiresalpha > 0. This API inconsistency may confuse users.Consider aligning the validation across all three Elastic Net implementations.
src/Preprocessing/FeatureSelection/Embedded/BorutaSelector.cs-98-101 (1)
98-101:⚠️ Potential issue | 🟡 MinorIncorrect initialization of
maxShadowbiases against negative importances.Initializing
maxShadow = 0means that if all shadow importances are negative (possible with some importance functions), no real feature will register a "hit" even if it outperforms all shadows.Proposed fix
// Find max shadow importance - double maxShadow = 0; + double maxShadow = double.NegativeInfinity; for (int j = p; j < p * 2; j++) maxShadow = Math.Max(maxShadow, importances[j]);src/Preprocessing/FeatureSelection/Embedded/BorutaSelector.cs-43-55 (1)
43-55:⚠️ Potential issue | 🟡 MinorMissing constructor validation and unused parameter.
- No validation for
nIterationsandnTrees- values <= 0 would cause issues_percentileis stored but never used in the implementationSuggested validation
public BorutaSelector( int nIterations = 100, int nTrees = 50, double percentile = 100.0, int? randomState = null, int[]? columnIndices = null) : base(columnIndices) { + if (nIterations < 1) + throw new ArgumentException("Number of iterations must be at least 1.", nameof(nIterations)); + if (nTrees < 1) + throw new ArgumentException("Number of trees must be at least 1.", nameof(nTrees)); + _nIterations = nIterations; _nTrees = nTrees; - _percentile = percentile; + _percentile = percentile; // TODO: implement percentile-based selection or remove parameter _randomState = randomState; }src/Preprocessing/FeatureSelection/Bioinformatics/Boruta.cs-232-247 (1)
232-247:⚠️ Potential issue | 🟡 MinorNormalQuantile approximation incorrect for p < 0.5.
The Abramowitz-Stegun rational approximation used here is designed for the upper tail. When
p < 0.5,Math.Log(1 - p)produces a negative value andMath.Sqrt(-2 * Math.Log(1 - p))will produce NaN since the argument becomes negative.For
BinomialThreshold, this is called with1 - alpha(typically 0.95), so it works. However, the method is fragile if reused elsewhere.Suggested fix for robustness
private double NormalQuantile(double p) { - // Approximate inverse normal CDF if (p <= 0) return double.NegativeInfinity; if (p >= 1) return double.PositiveInfinity; + + // Use symmetry for lower tail + if (p < 0.5) + return -NormalQuantile(1 - p); double t = Math.Sqrt(-2 * Math.Log(1 - p));src/Preprocessing/FeatureSelection/Embedded/TreeBasedFS.cs-127-130 (1)
127-130:⚠️ Potential issue | 🟡 MinorEdge case: No thresholds evaluated when feature has exactly 2 distinct values.
When
values.Count == 2,Skip(1).Take(values.Count - 2)results inTake(0), meaning no thresholds are evaluated for that feature. This prevents binary features from ever contributing to splits.Consider using
Skip(1)alone orTake(values.Count - 1)to include at least one threshold:Proposed fix
- foreach (double threshold in values.Skip(1).Take(values.Count - 2)) + foreach (double threshold in values.Skip(1))src/Preprocessing/FeatureSelection/CostSensitive/BudgetConstrainedFS.cs-23-24 (1)
23-24:⚠️ Potential issue | 🟡 MinorPersist and validate feature costs used for budgeting.
When
Fit(..., featureCosts)is used,_featureCostsnever updates, soFeatureCostscan remain null even though costs were provided. Also, negative/NaN/∞ costs can invalidate the budget constraint (e.g., negative costs decrease_usedBudget). Persist the effective costs and validate they’re finite and non‑negative.🛠️ Suggested fix
- private readonly double[]? _featureCosts; + private double[]? _featureCosts; @@ var costs = _featureCosts ?? CreateUnitCosts(p); if (costs.Length != p) throw new ArgumentException("Feature costs length must match number of features."); + for (int j = 0; j < p; j++) + { + if (double.IsNaN(costs[j]) || double.IsInfinity(costs[j]) || costs[j] < 0) + throw new ArgumentException("Feature costs must be finite and non-negative."); + } + _featureCosts = costs; @@ public void Fit(Matrix<T> data, Vector<T> target, double[] featureCosts) { if (featureCosts.Length != data.Columns) throw new ArgumentException("Feature costs must match number of columns."); - - _nInputFeatures = data.Columns; - int n = data.Rows; - int p = data.Columns; - - // Compute relevance scores - _relevanceScores = ComputeRelevanceScores(data, target, n, p); - - // Compute value per cost - _valuePerCost = new double[p]; - for (int j = 0; j < p; j++) - { - _valuePerCost[j] = featureCosts[j] > 1e-10 - ? _relevanceScores[j] / featureCosts[j] - : _relevanceScores[j] * 1000; - } - - // Greedy selection within budget - var selected = new List<int>(); - _usedBudget = 0; - - var sortedFeatures = Enumerable.Range(0, p) - .OrderByDescending(j => _valuePerCost[j]) - .ToList(); - - foreach (int j in sortedFeatures) - { - if (_usedBudget + featureCosts[j] <= _budget) - { - selected.Add(j); - _usedBudget += featureCosts[j]; - } - } - - _selectedIndices = selected.OrderBy(x => x).ToArray(); - - IsFitted = true; + _featureCosts = featureCosts; + Fit(data, target); }Also applies to: 33-33, 59-104, 106-147
docs/feature-selection-methods-catalog.md-103-111 (1)
103-111:⚠️ Potential issue | 🟡 MinorEscape the pipe in
H(Y|X)to keep the table columns aligned.Line 109 uses
H(Y|X), which creates an extra column in Markdown tables (MD056). Escaping the pipe (or wrapping in code) fixes rendering.🔧 Proposed fix
- | Conditional entropy | H(Y|X) | High | + | Conditional entropy | H(Y\|X) | High |src/Preprocessing/FeatureSelection/DomainSpecific/TextFeatureSelector.cs-51-56 (1)
51-56:⚠️ Potential issue | 🟡 MinorAdd an ordering check for min/max document frequency.
minDocumentFrequency > maxDocumentFrequencyis an invalid configuration and silently yields empty candidates.🔧 Proposed fix
if (maxDocumentFrequency < 0 || maxDocumentFrequency > 1) throw new ArgumentException("Max document frequency must be between 0 and 1.", nameof(maxDocumentFrequency)); + if (minDocumentFrequency > maxDocumentFrequency) + throw new ArgumentException("Min document frequency must be <= max document frequency.", nameof(minDocumentFrequency));src/Evaluation/DefaultModelEvaluator.cs-559-560 (1)
559-560:⚠️ Potential issue | 🟡 MinorDoc wording still says “normalized.”
The parameter now represents preprocessing (not just normalization). Consider updating the XML doc for clarity.📄 Suggested doc tweak
- /// <param name="preprocessingInfo">Information about how the data was normalized.</param> + /// <param name="preprocessingInfo">Information about how the data was preprocessed.</param>src/Preprocessing/FeatureSelection/Bioinformatics/MRMR.cs-68-171 (1)
68-171:⚠️ Potential issue | 🟡 MinorGuard against empty datasets.
With
data.Rows == 0, relevance stays zero and selection becomes arbitrary. Prefer failing fast.🔧 Suggested fix
public void Fit(Matrix<T> data, Vector<T> target) { if (data.Rows != target.Length) throw new ArgumentException("Target length must match rows in data."); + if (data.Rows == 0) + throw new ArgumentException("Data must contain at least one row.", nameof(data));src/Preprocessing/FeatureSelection/Bioinformatics/MRMR.cs-250-264 (1)
250-264:⚠️ Potential issue | 🟡 MinorValidate input shape before indexing selected columns.
If
Transformis called on data with a different column count than fit-time, this can index the wrong columns or throw.🔧 Suggested fix
protected override Matrix<T> TransformCore(Matrix<T> data) { if (_selectedIndices is null) throw new InvalidOperationException("MRMR has not been fitted."); + if (data.Columns != _nInputFeatures) + throw new ArgumentException("Input feature count does not match the fitted data.", nameof(data)); int numRows = data.Rows;
Contributor
There was a problem hiding this comment.
Actionable comments posted: 17
🤖 Fix all issues with AI agents
In `@src/Preprocessing/FeatureSelection/Bayesian/ARDSelector.cs`:
- Around line 150-151: In ARDSelector where beta is computed from gammaSum and
residualSum (beta = (n - gammaSum) / (residualSum + 1e-10)), guard against
gammaSum >= n by clamping beta to a small positive floor: compute the raw value
then replace beta with Math.Max(rawBeta, minBeta) (e.g., minBeta = 1e-10 or
configurable) so beta never becomes zero or negative and subsequent iterations
remain valid; reference variables: gammaSum, beta, n, residualSum and
class/method ARDSelector where this update occurs.
In `@src/Preprocessing/FeatureSelection/Bayesian/BayesianFeatureSelector.cs`:
- Around line 121-126: The likelihoodRatio computation can overflow: instead of
calling Math.Exp(tStatistic * tStatistic / 2) directly, compute the exponent
(e.g., var exp = tStatistic * tStatistic / 2), clamp it to a safe maximum (like
Math.Min(exp, 50) as done in SpikeAndSlabSelector), then call
Math.Exp(clampedExp) to produce likelihoodRatio; recalc posteriorOdds using
_priorProbability and handle any remaining infinite odds by capping to a large
finite value before computing _posteriorProbabilities[j] to avoid NaN.
In `@src/Preprocessing/FeatureSelection/Bayesian/SpikeAndSlabSelector.cs`:
- Around line 39-60: The constructor SpikeAndSlabSelector currently lacks
validation for priorInclusionProbability and nIterations; add checks in the
constructor to ensure priorInclusionProbability is strictly between 0 and 1
(throw ArgumentException with nameof(priorInclusionProbability) if not) to avoid
division-by-zero or degenerate odds, and ensure nIterations is at least 1 (throw
ArgumentException with nameof(nIterations) if not); reference the fields
_priorInclusionProbability and _nIterations when adding these validations so
invalid values are rejected early.
In `@src/Preprocessing/FeatureSelection/Causal/MarkovBlanketSelector.cs`:
- Around line 37-49: The MarkovBlanketSelector constructor does not validate
nBins allowing nBins <= 0 which will cause invalid discretization/negative bin
indices; update the MarkovBlanketSelector constructor to check that nBins is
greater than 0 and throw an ArgumentException (or ArgumentOutOfRangeException)
with a clear message and parameter name when the check fails, ensuring the field
_nBins is only set after validation.
- Around line 108-115: When the Markov blanket is empty the fallback selects up
to 5 features ignoring the MaxFeatures limit; update the fallback in the block
that sets _selectedIndices so it respects MaxFeatures by replacing the
Take(Math.Min(5, p)) with a nested Math.Min that also includes MaxFeatures
(e.g., Take(Math.Min(MaxFeatures, Math.Min(5, p)))), keeping the same
OrderByDescending on _relevanceScores and final OrderBy(x => x) to produce the
sorted index array.
- Around line 197-249: In ComputeConditionalMI, totalMI is being accumulated
over conditioning features/groups but never normalized, so MI scales with the
size of conditionSet; update the logic to average rather than sum by dividing
totalMI by the number of conditioning features considered (use the existing
count or conditionSet.Count) before returning (or adjust weighting to intended
normalization), ensuring you still fallback to ComputeMutualInformation when
count == 0; reference totalMI, count, conditionSet and ComputeConditionalMI when
making the change.
In `@src/Preprocessing/FeatureSelection/Causal/PCAlgorithmSelector.cs`:
- Around line 235-271: ComputeResiduals currently performs sequential univariate
regressions (order-dependent) instead of a single multivariate OLS; replace this
by constructing the design matrix Z of all conditioning predictors for the
target (including a column of ones for the intercept), solve the normal
equations β = (Z'Z)^{-1} Z' y (implement a stable solver such as Cholesky or
Gaussian elimination in a helper like SolveOLS), then compute residuals as y - Z
* β; update references to ComputeResiduals to call SolveOLS and remove the
iterative per-predictor subtraction so residuals reflect simultaneous regression
of all predictors.
- Around line 323-334: GetFeatureNamesOut currently returns an empty array when
the selector isn't fitted (_selectedIndices is null) and silently drops
out-of-range indices from inputFeatureNames; make it consistent with
GetSupportMask by throwing InvalidOperationException when _selectedIndices is
null, and when inputFeatureNames is provided validate that all indices in
_selectedIndices are < inputFeatureNames.Length, throwing an ArgumentException
(or similar) if any index is out-of-range, otherwise map indices to names;
reference GetFeatureNamesOut, GetSupportMask, and _selectedIndices when locating
the change.
- Around line 175-181: In TestConditionalIndependence, prevent numerical
instability in the Fisher z-transformation by (1) computing df = n -
condSet.Count - 3 and if df <= 0 return false (treat as dependent) to avoid sqrt
of non-positive and invalid inference, and (2) clamp the partialCorr returned by
ComputePartialCorrelation into a safe open interval (e.g. -1 + eps .. 1 - eps
with eps = 1e-10) or add eps to both (1+partialCorr) and (1-partialCorr) before
taking Math.Log to avoid Log(0)/division by zero; then compute z = Math.Sqrt(df)
* 0.5 * Math.Log((1+pc)/(1-pc)) and pValue via NormalCDF, keeping the existing
comparison to _alpha. Ensure these changes are made inside
TestConditionalIndependence and that ComputePartialCorrelation and NormalCDF
names are used unchanged.
In `@src/Preprocessing/FeatureSelection/Clustering/ClusteringBasedFS.cs`:
- Around line 79-86: The code divides by (n - 1) when computing variance in the
FitCore loop (assigning _featureScores[j]) and similarly in
ComputeFeatureCorrelations, which causes division by zero for n == 1; add a
guard so when n <= 1 you use a safe fallback (e.g., set variance/correlation to
0 or use population variance by dividing by n when n == 1) and only divide by (n
- 1) for n > 1, updating both the variance computation in FitCore (where
_featureScores[j] is set) and the analogous computation inside
ComputeFeatureCorrelations.
- Around line 38-49: The constructor ClusteringBasedFS currently validates
nClusters but not maxIterations; add validation in the ClusteringBasedFS(...)
constructor to ensure maxIterations > 0 and throw an ArgumentException (use
nameof(maxIterations)) when it's not, then only assign _maxIterations after
validation so the k-means loop won't be skipped by non-positive values.
- Around line 257-268: GetFeatureNamesOut currently silently returns an empty
array when not fitted and drops out-of-range indices when inputFeatureNames
length mismatches; align it with GetSupportMask by throwing when
_selectedIndices is null (e.g., InvalidOperationException) and validate
inputFeatureNames by checking if any index in _selectedIndices >=
inputFeatureNames.Length, then throw an ArgumentException (or log and throw)
describing the mismatch and listing the offending indices; keep the rest of the
mapping logic (Select(i => inputFeatureNames[i]) and default generated names)
intact.
In `@src/Preprocessing/FeatureSelection/Clustering/HierarchicalFS.cs`:
- Around line 49-105: In FitCore (and ensure the same guard is present in
ComputeFeatureDistances) add an early validation that n (data.Rows) is at least
2 and throw or return a clear error if not to avoid division by zero when
computing variance/correlation; also validate that _nFeaturesToSelect is between
1 and p (data.Columns) and if it's larger than p clamp it to p or throw an
ArgumentException so clustering/AgglomerativeClustering receives a valid k;
ensure you do not perform the variance loop or divide by (n - 1) when n < 2 and
set _selectedIndices/_featureScores to sensible defaults before
returning/throwing so downstream code sees a consistent state.
- Around line 221-234: TransformCore currently assumes input has enough columns
and will throw IndexOutOfRangeException if not; add an explicit shape check at
the start of TransformCore (before computing numRows/numCols) that verifies
data.Columns is greater than the maximum index in _selectedIndices (e.g.,
data.Columns > _selectedIndices.Max()) and throw a clear ArgumentException or
InvalidOperationException with a message like "Input matrix has too few columns
for fitted selected indices" when the check fails; reference TransformCore and
_selectedIndices to locate where to add the check and ensure it runs before the
element-wise copy and before constructing the result Matrix<T>.
In `@src/Preprocessing/FeatureSelection/Constraint/ConstraintBasedSelector.cs`:
- Around line 129-138: Validate indices before adding a feature group: when
iterating the discovered group (variable group from _featureGroups), filter out
any indices that are negative or >= the number of columns in the data (the same
bound checks used by TransformCore) and exclude them from groupToAdd and
forbidden checks; only count and add indices that pass this validation and still
respect _nFeaturesToSelect and existing selected/forbidden sets (update
groupToAdd generation to use the validated index set).
- Around line 189-200: GetFeatureNamesOut currently returns an empty array when
_selectedIndices is null and silently filters out-of-range indices, which is
inconsistent with GetSupportMask/TransformCore; update GetFeatureNamesOut to
throw InvalidOperationException if _selectedIndices is null (like
GetSupportMask/TransformCore), and when inputFeatureNames is provided validate
that every index in _selectedIndices is within inputFeatureNames.Length and
throw an ArgumentException (or InvalidOperationException) if any index is out of
range instead of filtering; retain the existing fallback of generating
"Feature{i}" names only when inputFeatureNames is null.
- Around line 38-53: In the ConstraintBasedSelector constructor add validation
to detect contradictory or impossible constraints: check for any overlap between
mandatoryFeatures and forbiddenFeatures (e.g., any element present in both) and
throw an ArgumentException naming the conflict; also verify
mandatoryFeatures.Length <= nFeaturesToSelect and throw an ArgumentException if
it exceeds _nFeaturesToSelect; perform null checks before accessing arrays and
assign to _mandatoryFeatures/_forbiddenFeatures/_nFeaturesToSelect only after
validations so the object cannot be created in an invalid state.
🧹 Nitpick comments (11)
src/Preprocessing/FeatureSelection/Bayesian/SpikeAndSlabSelector.cs (1)
184-195: Silent filtering may produce fewer output names than selected features.When
inputFeatureNames.Length < _nInputFeatures, indices beyond the array bounds are silently filtered out (line 192), causing the returned array to have fewer elements than_selectedIndices.Length. Consider throwing an exception or returning placeholder names for out-of-bounds indices to maintain consistency.♻️ Alternative: validate or pad missing names
public override string[] GetFeatureNamesOut(string[]? inputFeatureNames = null) { if (_selectedIndices is null) return []; if (inputFeatureNames is null) return _selectedIndices.Select(i => $"Feature{i}").ToArray(); return _selectedIndices - .Where(i => i < inputFeatureNames.Length) - .Select(i => inputFeatureNames[i]) + .Select(i => i < inputFeatureNames.Length ? inputFeatureNames[i] : $"Feature{i}") .ToArray(); }src/Preprocessing/FeatureSelection/Bayesian/ARDSelector.cs (2)
41-56: Missing validation foralphaThreshold,maxIterations, andtolerance.These parameters should be validated:
alphaThresholdshould be positive,maxIterationsshould be at least 1, andtoleranceshould be non-negative.🛡️ Proposed validation additions
public ARDSelector( int nFeaturesToSelect = 10, double alphaThreshold = 1e8, int maxIterations = 300, double tolerance = 1e-4, int[]? columnIndices = null) : base(columnIndices) { if (nFeaturesToSelect < 1) throw new ArgumentException("Number of features must be at least 1.", nameof(nFeaturesToSelect)); + if (alphaThreshold <= 0) + throw new ArgumentException("Alpha threshold must be positive.", nameof(alphaThreshold)); + if (maxIterations < 1) + throw new ArgumentException("Max iterations must be at least 1.", nameof(maxIterations)); + if (tolerance < 0) + throw new ArgumentException("Tolerance must be non-negative.", nameof(tolerance)); _nFeaturesToSelect = nFeaturesToSelect;
225-236: Same silent filtering issue as other selectors.The
Where(i => i < inputFeatureNames.Length)filter silently drops indices, potentially returning fewer names than selected features. Consider using the same fix pattern suggested forSpikeAndSlabSelectorfor consistency across all Bayesian selectors.src/Preprocessing/FeatureSelection/Bayesian/BayesianFeatureSelector.cs (3)
27-27:_thresholdfield is stored but never used.The
thresholdconstructor parameter is assigned to_threshold(line 53) but never referenced in any method. Either remove the unused parameter/field or implement the intended threshold-based selection logic.Also applies to: 42-42, 53-53
78-79: Unused variablespositiveCountandnegativeCount.These variables are computed but never used in the algorithm. Remove them to reduce confusion.
♻️ Remove dead code
for (int i = 0; i < n; i++) targetArray[i] = NumOps.ToDouble(target[i]); - double positiveCount = targetArray.Count(y => y > 0.5); - double negativeCount = n - positiveCount; - for (int j = 0; j < p; j++)
178-189: Same silent filtering pattern as other selectors.For consistency, apply the same fix across all three Bayesian selectors.
src/Preprocessing/FeatureSelection/Clustering/ClusteringBasedFS.cs (2)
33-35: Consider defensive copies for exposed arrays.Properties
ClusterAssignments,FeatureScores, andSelectedIndicesreturn internal arrays directly. External code could inadvertently mutate internal state. Consider returning copies or usingReadOnlySpan<T>/IReadOnlyList<T>if immutability guarantees are desired.♻️ Example using array copies
- public int[]? ClusterAssignments => _clusterAssignments; - public double[]? FeatureScores => _featureScores; - public int[]? SelectedIndices => _selectedIndices; + public int[]? ClusterAssignments => _clusterAssignments?.ToArray(); + public double[]? FeatureScores => _featureScores?.ToArray(); + public int[]? SelectedIndices => _selectedIndices?.ToArray();
156-158: Consider using standardRandominstead of cryptographic RNG for k-means initialization.
RandomHelper.CreateSecureRandom()provides cryptographically secure randomness, which is unnecessary for k-means centroid initialization and may have performance overhead. A standardRandominstance would suffice for this non-security-sensitive use case.src/Preprocessing/FeatureSelection/Constraint/ConstraintBasedSelector.cs (1)
34-35: Consider defensive copies for exposed arrays.
FeatureScoresandSelectedIndicesreturn internal arrays directly, allowing callers to mutate the selector's state. Consider returning clones orReadOnlySpan<T>/IReadOnlyList<T>.🛡️ Suggested defensive copy pattern
- public double[]? FeatureScores => _featureScores; - public int[]? SelectedIndices => _selectedIndices; + public double[]? FeatureScores => _featureScores?.ToArray(); + public int[]? SelectedIndices => _selectedIndices?.ToArray();src/Preprocessing/FeatureSelection/Causal/PCAlgorithmSelector.cs (2)
40-55: Consider validatingmaxConditioningSetSizeparameter.The constructor validates
maxFeaturesandalpha, butmaxConditioningSetSizeis not validated. A negative value would cause the PC algorithm loop (line 91) to skip entirely, potentially leading to unexpected behavior where no edges are pruned.Proposed validation
if (alpha <= 0 || alpha >= 1) throw new ArgumentException("Alpha must be between 0 and 1.", nameof(alpha)); + if (maxConditioningSetSize < 0) + throw new ArgumentException("Max conditioning set size cannot be negative.", nameof(maxConditioningSetSize)); _maxFeatures = maxFeatures;
158-173: Minor allocation overhead in combination generation.The recursive
items.Skip(i + 1).ToList()creates a new list for each recursive call. For typical PC algorithm usage with small conditioning sets (default max 3), this is acceptable. If larger conditioning sets are anticipated, consider using index-based iteration to avoid allocations.
Add extensive feature selection methods across multiple categories: **New categories added:** - Boundary: MarginSelector, DecisionBoundarySelector - Complexity: LempelZivSelector, HurstExponentSelector, FractalDimensionSelector, LyapunovSelector - Distribution: GaussianitySelector, UniformitySelector, ModalitySelector - Entropy: DifferentialEntropySelector, PermutationEntropySelector, ApproximateEntropySelector, SampleEntropySelector - Gain: InformationGainRatioSelector, GiniImpuritySelector - Moment: BimodalitySelector, SkewnessSelector, KurtosisSelector, CoefOfVariationSelector - Nonlinear: MutualInformationSelector, MaximalInformationCoefficientSelector, DistanceCorrelationSelector, HilbertSchmidtSelector - Quantile: QuantileRatioSelector, QuantileDispersionSelector - Range: IQRSelector, MADSelector, PercentileRangeSelector - Sparsity: L1RegularizationSelector, ElasticNetSelector, GroupLassoSelector **Key improvements:** - All selectors follow consistent TransformerBase pattern - Proper 'new' keyword for FitTransform on unsupervised selectors - Fixed net471 compatibility (FirstOrDefault usage) - Comprehensive documentation with "For Beginners" explanations Total: 419 feature selection methods covering statistical, information-theoretic, complexity, entropy, distribution, and regularization-based approaches. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Statistical methods: - TwoWayANOVASelector - Two-factor ANOVA for factorial designs - MANOVASelector - Multivariate ANOVA with Wilks' Lambda - ANCOVASelector - ANOVA with covariate adjustment - HotellingT2Selector - Multivariate t-test generalization Causal methods: - MMPCSelector - Max-Min Parents and Children for Markov blanket Neural methods: - GradCAMSelector - Gradient-weighted class activation mapping - ScoreCAMSelector - Gradient-free activation mapping - AttentionFlowSelector - Attention flow propagation analysis Privacy methods: - DifferentialPrivacySelector - Privacy-preserving with Laplace noise - FederatedFSSelector - Federated learning simulation Fairness methods: - FairFeatureSelector - Balance predictive power with fairness Multi-Armed Bandit methods: - UCB1Selector - Upper Confidence Bound exploration/exploitation - ThompsonSamplingSelector - Bayesian bandit with Beta posteriors Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In `@src/Preprocessing/FeatureSelection/Bandit/ThompsonSamplingSelector.cs`:
- Around line 214-219: The SampleStandardNormal(Random rand) method can produce
Math.Log(0) when rand.NextDouble() returns 0; update the function to guard u1
against zero by replacing the raw u1 with a clipped value (e.g., double eps =
1e-12 or double.Epsilon) or loop until u1 > 0 before computing Math.Log(u1),
keeping the rest of the Box–Muller implementation unchanged; reference the
SampleStandardNormal method and the local variables u1/u2/random to locate where
to apply the clamp or retry.
- Around line 227-241: TransformCore currently assumes input has columns for
every _selectedIndices entry and will throw IndexOutOfRangeException if not; at
the start of TransformCore (before using data[i, _selectedIndices[j]]), validate
that every index in _selectedIndices is within data.Columns (e.g. check
data.Columns > _selectedIndices.Max() or loop check for any idx >= data.Columns)
and throw a clear ArgumentException/InvalidOperationException (mentioning the
mismatch and required column count) if the check fails; keep the existing
behavior of throwing when _selectedIndices is null.
In `@src/Preprocessing/FeatureSelection/Bandit/UCB1Selector.cs`:
- Around line 137-139: The UCB1 selection loop currently starts at iter = p so
no UCB iterations run when _nIterations <= p; update the loop in UCB1Selector
(the "UCB1 selection loop") to run _nIterations iterations after the initial p
pulls—e.g. change the loop bound so it iterates from p to p + _nIterations (use
for (int iter = p; iter < p + _nIterations; iter++)) or alternatively validate
in Fit/constructor that _nIterations > p and throw/adjust accordingly; reference
variables p and _nIterations when making the change.
- Around line 225-236: GetFeatureNamesOut currently returns an empty array when
_selectedIndices is null and silently drops out-of-range indices from
inputFeatureNames, causing inconsistent behavior with
GetSupportMask()/TransformCore and potential mismatches; change
GetFeatureNamesOut (and use of _selectedIndices) to throw the same
NotFitted/InvalidOperation exception as GetSupportMask()/TransformCore when
_selectedIndices is null, and instead of filtering out-of-range indices
silently, validate that every index in _selectedIndices is <
inputFeatureNames.Length and throw a clear ArgumentException (or similar) if any
index is out of range so callers get a deterministic error rather than a
silently truncated list.
In `@src/Preprocessing/FeatureSelection/Causal/MMPCSelector.cs`:
- Around line 92-129: The code in MMPCSelector.cs incorrectly treats _alpha as a
raw association threshold (used in the bestMinAssoc < _alpha check) instead of a
significance level for conditional independence tests; either rename _alpha to
make intent explicit (e.g., minAssociationThreshold) and update
constructor/field docs and all references (including the forward-selection loop
and any external callers) or replace the simple magnitude comparison with a
proper statistical test that computes a p-value from ComputePartialAssociation
(or a new TestConditionalIndependence method) and compare p-value < _alpha;
update variable names, XML docs, and method signatures (e.g.,
ComputePartialAssociation -> returns test statistic or p-value) consistently to
reflect the chosen approach.
- Around line 270-281: GetFeatureNamesOut currently returns an empty array when
_selectedIndices is null and silently drops indices if inputFeatureNames is too
short; make it consistent with GetSupportMask by throwing an
InvalidOperationException when _selectedIndices is null, and instead of
filtering out out-of-range indices, validate that every index in
_selectedIndices is < inputFeatureNames.Length and throw a descriptive
ArgumentException (or IndexOutOfRangeException) if not; locate and update the
method GetFeatureNamesOut and reference the field _selectedIndices when adding
these checks so callers are notified of not-fitted state and mismatched
inputFeatureNames rather than getting a silently truncated result.
🧹 Nitpick comments (3)
src/Preprocessing/FeatureSelection/Bandit/ThompsonSamplingSelector.cs (1)
260-271: Inconsistent behavior when not fitted.
GetSupportMask()throwsInvalidOperationExceptionwhen not fitted, butGetFeatureNamesOut()returns an empty array. Consider making the behavior consistent for better API predictability.♻️ Proposed fix for consistent behavior
public override string[] GetFeatureNamesOut(string[]? inputFeatureNames = null) { - if (_selectedIndices is null) return []; + if (_selectedIndices is null) + throw new InvalidOperationException("ThompsonSamplingSelector has not been fitted."); if (inputFeatureNames is null) return _selectedIndices.Select(i => $"Feature{i}").ToArray(); return _selectedIndices - .Where(i => i < inputFeatureNames.Length) .Select(i => inputFeatureNames[i]) .ToArray(); }src/Preprocessing/FeatureSelection/Bandit/UCB1Selector.cs (1)
38-41: Properties expose mutable internal arrays, allowing external mutation.These properties return direct references to internal arrays. External code could modify them, corrupting the selector's state (e.g.,
selector.SelectedIndices[0] = -1).♻️ Return defensive copies or read-only wrappers
- public double[]? UCBScores => _ucbScores; - public double[]? EstimatedRewards => _estimatedRewards; - public int[]? PullCounts => _pullCounts; - public int[]? SelectedIndices => _selectedIndices; + public IReadOnlyList<double>? UCBScores => _ucbScores; + public IReadOnlyList<double>? EstimatedRewards => _estimatedRewards; + public IReadOnlyList<int>? PullCounts => _pullCounts; + public IReadOnlyList<int>? SelectedIndices => _selectedIndices;src/Preprocessing/FeatureSelection/Causal/MMPCSelector.cs (1)
131-163: Backward phase uses pairwise conditioning instead of full conditioning set.The backward phase tests conditional independence by iterating through each other candidate individually (Lines 144-155) rather than conditioning on all other candidates simultaneously. Standard MMPC would compute: "Is feature j independent of Y given all other candidates?"
Current approach may retain features that would be pruned under full multivariate conditioning. If this is intentional for computational efficiency, consider documenting this simplification in the class remarks.
ooples
changed the title
- Fix binary class split to use actual labels instead of fixed 0.5 threshold (SAM, SignificanceAnalysis) - Add guards against division by zero and NaN (s0 fudge factor, eigenvalues, variance calculations) - Add proper Beta function normalization for p-value calculations (VolcanoPlotSelector) - Fix penalty scaling consistency (ElasticNetFS) - Add validation for constructor parameters (nBins, maxIterations, priorInclusionProbability, etc.) - Add bounds validation for feature group indices and constraint conflicts - Fix MCP and SCAD proximal operators with correct threshold boundaries - Replace sequential univariate regression with proper multivariate OLS (PCAlgorithmSelector) - Fix Fisher z-transformation numerical instability - Normalize conditional MI calculation (MarkovBlanketSelector) - Fix UCB1 selection loop to run additional iterations after initial pulls - Add column/shape validation in Transform methods - Make GetFeatureNamesOut consistent with GetSupportMask (throw when not fitted) - Fix Box-Muller transform to avoid log(0) (ThompsonSamplingSelector) - Rename alpha to minAssociationThreshold for semantic clarity (MMPCSelector) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 Fix all issues with AI agents
In `@src/Preprocessing/FeatureSelection/Bayesian/ARDSelector.cs`:
- Around line 41-56: The ARDSelector constructor currently only validates
nFeaturesToSelect; add argument validation for alphaThreshold, maxIterations,
and tolerance in the ARDSelector(...) constructor: require alphaThreshold > 0
(throw ArgumentOutOfRangeException(nameof(alphaThreshold))), require
maxIterations >= 1 (throw ArgumentOutOfRangeException(nameof(maxIterations))),
and require tolerance > 0 (and optionally < 1, throw
ArgumentOutOfRangeException(nameof(tolerance))). Assign to the existing fields
(_alphaThreshold, _maxIterations, _tolerance) only after these checks to prevent
invalid configuration.
In `@src/Preprocessing/FeatureSelection/Bayesian/BayesianFeatureSelector.cs`:
- Around line 39-54: The constructor stores a threshold but it is never
used—update the class so the supplied threshold is validated (ensure 0 <
threshold < 1 and throw ArgumentException otherwise) and applied during
selection: in the feature-selection method (locate methods like Select,
SelectFeatures, ScoreAndSelect or where _nFeaturesToSelect and _threshold are
used around lines ~130-135) first filter candidate features by their
posterior/probability score >= _threshold, then from those passing the threshold
select up to _nFeaturesToSelect (fall back to selecting top-N only from
threshold-passing features); ensure the selection logic respects both the
threshold and the max-count (_nFeaturesToSelect).
In `@src/Preprocessing/FeatureSelection/Bayesian/SpikeAndSlabSelector.cs`:
- Around line 98-128: The Gibbs loop in SpikeAndSlabSelector.cs can produce
divide-by-zero/NaN when n<2 or xjNorm≈0; modify the sampling in the for-loop
over j to (1) ensure residualVariance uses a safeDenominator = Math.Max(1, n -
1) or early-return when n<2, (2) clamp xjNorm with a small epsilon (e.g. xjNorm
= Math.Max(xjNorm, 1e-12)) before computing betaHat and any divisions, (3)
protect logBFSlab/logBFSpike computations by adding epsilons inside denominators
and Math.Log arguments, (4) after computing logBF and posteriorOdds, check for
double.IsFinite and cap exponent inputs (e.g. Math.Min/Max) and if values are
not finite set posterior inclusion gamma[j] to 0 (or prior) to avoid
contaminating downstream probabilities. Ensure you reference and update
variables residualVariance, xjNorm, betaHat, logBFSlab, logBFSpike,
posteriorOdds and gamma[j].
In `@src/Preprocessing/FeatureSelection/Bioinformatics/SAM.cs`:
- Around line 44-65: The constructor for SAM should validate s0Percentile to
ensure it's within [0, 1] to prevent downstream s0Index calculations from going
out of range; add a check in the SAM(...) constructor that throws an
ArgumentException (or ArgumentOutOfRangeException) if s0Percentile is < 0 or >
1, and keep assigning _s0Percentile only after this validation so any code that
computes s0Index or uses _s0Percentile (referenced by _s0Percentile and the
SAM(...) constructor) will not produce an IndexOutOfRangeException.
In `@src/Preprocessing/FeatureSelection/Bioinformatics/VolcanoPlotSelector.cs`:
- Around line 41-54: The constructor VolcanoPlotSelector currently doesn't
validate minFoldChange or pValueThreshold; add input validation in the
VolcanoPlotSelector(...) constructor to ensure minFoldChange > 0 and 0 <
pValueThreshold <= 1 (throw ArgumentException/ArgumentOutOfRangeException with
clear parameter name if violated) before assigning to _minFoldChange and
_pValueThreshold so downstream log-threshold and filtering calculations cannot
produce NaN/∞ or meaningless results.
- Around line 71-85: The current split into class0/class1 uses a fixed 0.5
threshold on NumOps.ToDouble(target[i]) which fails for label sets like {1,2} or
{-1,1}; update the logic in VolcanoPlotSelector (the loop that populates class0
and class1 using n and target) to identify the two distinct label values present
(e.g., find the two unique values in target), ensure there are exactly two
classes, then assign indices to class0/class1 by equality comparison against
those two labels (rather than using 0.5); keep the existing check that each
class has at least 2 samples and throw the same ArgumentException if not.
In `@src/Preprocessing/FeatureSelection/Clustering/ClusteringBasedFS.cs`:
- Around line 226-240: ClusteringBasedFS.TransformCore lacks input-shape
validation and will IndexOutOfRange when passed a matrix with fewer columns than
the fitted features; modify TransformCore (in class ClusteringBasedFS, method
TransformCore) to verify that data.Columns is greater than the highest selected
index (e.g. compute maxIndex = _selectedIndices.Max()) or that data.Columns >=
_selectedIndices.Length/expected fitted columns and if the check fails throw a
clear ArgumentException (or InvalidOperationException consistent with
HierarchicalFS) with a message like "Input data has fewer columns than data used
to fit ClusteringBasedFS."; perform this check before the row/column copy loop.
In `@src/Preprocessing/FeatureSelection/Clustering/HierarchicalFS.cs`:
- Around line 260-271: GetFeatureNamesOut currently returns an empty array when
_selectedIndices is null and silently skips out-of-range indices, which is
inconsistent with ClusteringBasedFS.GetFeatureNamesOut and GetSupportMask;
change GetFeatureNamesOut to mirror ClusteringBasedFS behavior by throwing
InvalidOperationException when _selectedIndices is null (not fitted) and
validate that inputFeatureNames (when provided) has length >=
max(_selectedIndices)+1, throwing ArgumentException/InvalidOperationException
for out-of-range inputs rather than filtering them; update references in this
class (GetFeatureNamesOut, _selectedIndices) so behavior matches GetSupportMask
and ClusteringBasedFS.GetFeatureNamesOut.
In `@src/Preprocessing/FeatureSelection/Constraint/ConstraintBasedSelector.cs`:
- Around line 83-115: The current scoring converts inputs to double arrays and
uses System.Math, violating the INumericOperations<T> requirement; change
ConstraintBasedSelector so you do not cast all data to double early: keep values
as the generic numeric type (use the existing NumOps/INumericOperations<T>
helpers to accumulate sums, compute means, compute sxx/syy/sxy with
NumOps.Add/Multiply/Subtract/Divide/etc.), compute mean and variance/covariance
using NumOps, and only convert the final correlation value to double when
assigning to _featureScores[j]; update the code paths that create X and y
(replace double[,] X and double[] y) and the loops that compute xMean, yMean,
sxy, sxx, syy to use NumOps methods instead of Math so numeric behavior remains
type‑safe for non‑double T.
- Around line 117-132: Validate that all indices in _mandatoryFeatures and
_forbiddenFeatures are within [0, p-1] after p is computed instead of silently
ignoring them: check each element of _mandatoryFeatures and _forbiddenFeatures
(used when building selected and forbidden) and if any index is <0 or >=p throw
an ArgumentOutOfRangeException (or a clear custom exception) mentioning the
offending index and the collection name; remove the silent bounds checks in the
foreach that builds selected and rely on the prior validation so candidates and
the OrderByDescending on _featureScores operate on guaranteed-valid indices.
🧹 Nitpick comments (8)
src/Preprocessing/FeatureSelection/Causal/MarkovBlanketSelector.cs (2)
37-52: Previous issues addressed; constructor validation looks good.The constructor now properly validates both
maxFeaturesandnBins. However,thresholdis not validated and could accept negative values, which would make the conditional MI check at line 92 (conditionalMI > _threshold) behave unexpectedly (always pass for non-negative MI).Consider adding validation:
🛡️ Optional fix
if (nBins < 1) throw new ArgumentException("nBins must be at least 1.", nameof(nBins)); + if (threshold < 0) + throw new ArgumentException("Threshold must be non-negative.", nameof(threshold));
298-309: Inconsistent not-fitted handling and silent index filtering.
GetSupportMask()throwsInvalidOperationExceptionwhen not fitted, butGetFeatureNamesOut()returns an empty array (line 300). Additionally, the.Where(i => i < inputFeatureNames.Length)filter silently drops out-of-range indices.For consistency with other selectors in this PR (MMPCSelector, PCAlgorithmSelector) that were updated to throw, consider aligning this method:
🔧 Proposed fix
public override string[] GetFeatureNamesOut(string[]? inputFeatureNames = null) { - if (_selectedIndices is null) return []; + if (_selectedIndices is null) + throw new InvalidOperationException("MarkovBlanketSelector has not been fitted."); if (inputFeatureNames is null) return _selectedIndices.Select(i => $"Feature{i}").ToArray(); + if (inputFeatureNames.Length < _nInputFeatures) + throw new ArgumentException( + $"Expected at least {_nInputFeatures} feature names, got {inputFeatureNames.Length}.", + nameof(inputFeatureNames)); + - return _selectedIndices - .Where(i => i < inputFeatureNames.Length) - .Select(i => inputFeatureNames[i]) - .ToArray(); + return _selectedIndices.Select(i => inputFeatureNames[i]).ToArray(); }src/Preprocessing/FeatureSelection/Causal/PCAlgorithmSelector.cs (1)
40-55: Consider validatingmaxConditioningSetSize.The constructor validates
maxFeaturesandalphabut notmaxConditioningSetSize. A negative value would cause the PC algorithm loop (line 91) to execute only once withcondSize = 0, which may not be the intended behavior.🛡️ Optional fix
if (alpha <= 0 || alpha >= 1) throw new ArgumentException("Alpha must be between 0 and 1.", nameof(alpha)); + if (maxConditioningSetSize < 0) + throw new ArgumentException("Max conditioning set size must be non-negative.", nameof(maxConditioningSetSize));src/Preprocessing/FeatureSelection/CostSensitive/CostSensitiveFS.cs (2)
216-222: Inconsistent error handling: consider throwing when not fitted.
GetSupportMask()throwsInvalidOperationExceptionwhen the selector is not fitted, butGetTotalCost()silently returns0. This inconsistency could mask usage errors where a caller expects a meaningful cost value.♻️ Suggested fix for consistency
public double GetTotalCost() { - if (_selectedIndices is null || _featureCosts is null) - return 0; + if (_selectedIndices is null) + throw new InvalidOperationException("CostSensitiveFS has not been fitted."); + if (_featureCosts is null) + return 0; // Uniform costs were used internally return _selectedIndices.Sum(j => _featureCosts[j]); }
224-235: Silent truncation may hide input mismatches.Two concerns:
Inconsistent unfitted behavior: Returns
[]when not fitted (line 226), whereasGetSupportMask()throws. Consider throwing for consistency.Silent index filtering (lines 231-234): If
inputFeatureNameshas fewer elements than the maximum selected index, those indices are silently dropped. This could cause the returned array to have fewer elements than_selectedIndices.Length, creating a mismatch between feature names and transformed columns.♻️ Suggested fix
public override string[] GetFeatureNamesOut(string[]? inputFeatureNames = null) { - if (_selectedIndices is null) return []; + if (_selectedIndices is null) + throw new InvalidOperationException("CostSensitiveFS has not been fitted."); if (inputFeatureNames is null) return _selectedIndices.Select(i => $"Feature{i}").ToArray(); + if (inputFeatureNames.Length < _nInputFeatures) + throw new ArgumentException( + $"Expected at least {_nInputFeatures} feature names, but got {inputFeatureNames.Length}.", + nameof(inputFeatureNames)); + return _selectedIndices - .Where(i => i < inputFeatureNames.Length) .Select(i => inputFeatureNames[i]) .ToArray(); }src/Preprocessing/FeatureSelection/Clustering/HierarchicalFS.cs (1)
36-47: Consider validating thelinkageparameter.The
linkageparameter accepts any string and silently falls back to "average" linkage in the switch default case (lines 202-203). Consider validating the input and throwing for unsupported values to avoid silent behavior changes.♻️ Proposed fix
public HierarchicalFS( int nFeaturesToSelect = 10, string linkage = "average", int[]? columnIndices = null) : base(columnIndices) { if (nFeaturesToSelect < 1) throw new ArgumentException("Number of features must be at least 1.", nameof(nFeaturesToSelect)); + var validLinkages = new[] { "single", "complete", "average" }; + if (!validLinkages.Contains(linkage.ToLowerInvariant())) + throw new ArgumentException($"Linkage must be one of: {string.Join(", ", validLinkages)}.", nameof(linkage)); + _nFeaturesToSelect = nFeaturesToSelect; _linkage = linkage.ToLowerInvariant(); }src/Preprocessing/FeatureSelection/Bandit/ThompsonSamplingSelector.cs (2)
181-186: Latent issue: potential zero result when shape < 1.The
shape < 1branch can return 0 ifrand.NextDouble()returns 0, sinceMath.Pow(0, 1.0/shape) = 0. While this branch is never executed in the current implementation (alpha and beta are always ≥ 1), it could cause division-by-zero inSampleBetaif the method is reused with smaller shape values in the future.🛡️ Proposed guard against zero
if (shape < 1) { // Use Gamma(shape+1) / U^(1/shape) - double u = rand.NextDouble(); + double u; + do + { + u = rand.NextDouble(); + } while (u <= double.Epsilon); return SampleGamma(shape + 1, rand) * Math.Pow(u, 1.0 / shape); }
270-281: Inconsistent error handling for unfitted state and mismatched input.Two concerns:
Unfitted state: Line 272 returns an empty array when not fitted, but
GetSupportMask()andTransformCore()throwInvalidOperationException. This inconsistency can confuse users.Silent filtering: Lines 277-280 silently drop indices when
inputFeatureNamesis shorter than_nInputFeatures. This differs fromTransformCorewhich throws on dimension mismatch, potentially masking caller errors.♻️ Proposed fix for consistent behavior
public override string[] GetFeatureNamesOut(string[]? inputFeatureNames = null) { - if (_selectedIndices is null) return []; + if (_selectedIndices is null) + throw new InvalidOperationException("ThompsonSamplingSelector has not been fitted."); if (inputFeatureNames is null) return _selectedIndices.Select(i => $"Feature{i}").ToArray(); - return _selectedIndices - .Where(i => i < inputFeatureNames.Length) - .Select(i => inputFeatureNames[i]) - .ToArray(); + if (inputFeatureNames.Length < _nInputFeatures) + throw new ArgumentException( + $"Input feature names has {inputFeatureNames.Length} elements but selector was fitted on {_nInputFeatures} features.", + nameof(inputFeatureNames)); + + return _selectedIndices.Select(i => inputFeatureNames[i]).ToArray(); }
- ARDSelector: validate alphaThreshold, maxIterations, tolerance - BayesianFeatureSelector: apply threshold in selection, validate threshold range - SpikeAndSlabSelector: guard against division-by-zero and NaNs in Bayes factor - SAM: validate s0Percentile is in [0, 1] - VolcanoPlotSelector: validate minFoldChange/pValueThreshold, use actual labels - ClusteringBasedFS: validate input shape in TransformCore - HierarchicalFS: consistent error handling in GetFeatureNamesOut - ConstraintBasedSelector: validate mandatory/forbidden feature indices Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Fix all issues with AI agents
In `@src/Preprocessing/FeatureSelection/Bayesian/ARDSelector.cs`:
- Around line 70-77: In Fit(Matrix<T> data, Vector<T> target) add validation to
ensure data.Rows > 0 and data.Columns > 0 and that target.Length equals
data.Rows (retain existing check), then set _nInputFeatures = data.Columns and
enforce nFeaturesToSelect <= data.Columns by throwing an ArgumentException when
nFeaturesToSelect is greater; this prevents operating on empty inputs or
requesting more features than available and keeps the existing variable usage
(Fit, _nInputFeatures, nFeaturesToSelect, data.Rows, data.Columns,
target.Length).
- Around line 198-212: TransformCore currently indexes data using
_selectedIndices without checking that the input has enough columns, causing
IndexOutOfRangeException for mismatched input; update TransformCore (and related
callers if necessary) to validate the input's column count before indexing by
ensuring every index in _selectedIndices is < data.Columns (or that data.Columns
matches the expected training column count if you store it), and throw a clear
InvalidOperationException describing the mismatch if validation fails so callers
know why transformation cannot proceed.
- Around line 231-241: GetFeatureNamesOut currently returns an empty array when
the selector isn’t fitted and silently drops out-of-range names; change it to
throw instead: if _selectedIndices is null throw an InvalidOperationException
(mentioning that the selector must be fitted) and if inputFeatureNames is
provided but does not contain entries for all indices in _selectedIndices (e.g.,
any i >= inputFeatureNames.Length) throw an ArgumentException (or
ArgumentOutOfRangeException) describing the mismatch; keep the existing behavior
of mapping indices to names when valid (using _selectedIndices and
inputFeatureNames) but do not silently return [] or drop names.
In `@src/Preprocessing/FeatureSelection/Bayesian/SpikeAndSlabSelector.cs`:
- Around line 168-180: TransformCore currently assumes incoming data has the
same number of columns as whatever was used when fitting, which can cause
out-of-range indexing into _selectedIndices; before copying values, validate the
input shape by ensuring data.Columns (or equivalent property) is large enough
for all indices in _selectedIndices (e.g., check that _selectedIndices.Max() <
data.Columns) and throw a clear InvalidOperationException like "Input has X
columns but selector expects at least Y" if the check fails; add this guard at
the start of TransformCore in SpikeAndSlabSelector (before the
numRows/numCols/result allocation) so you fail fast with a friendly message
rather than causing an index exception.
In `@src/Preprocessing/FeatureSelection/Bioinformatics/VolcanoPlotSelector.cs`:
- Around line 127-131: The current log2 fold change calculation can yield NaN
when mean0 or mean1 are non‑positive; update the computation in
VolcanoPlotSelector so you first guard/normalize the means: if mean0 or mean1
are <= 0, compute a signed fold change by taking the ratio of their absolute
values plus eps and multiplying the log2 by Math.Sign(mean1 - mean0); otherwise
compute the usual log2((mean1+eps)/(mean0+eps)). Assign the result to
_log2FoldChanges[j] and ensure eps is applied to the absolute values to avoid
division by zero.
In `@src/Preprocessing/FeatureSelection/Clustering/ClusteringBasedFS.cs`:
- Around line 53-58: FitCore currently assumes data.Rows > 0 (n) and later does
mean /= n, causing divide-by-zero; add an early validation at the start of the
FitCore(Matrix<T> data) method to check n (or data.Rows) and throw a clear
exception (e.g., ArgumentException or InvalidOperationException) if n == 0, with
a message like "input data must contain at least one row"; ensure this check
occurs before using n or computing mean so _nInputFeatures and subsequent code
only run on non-empty input.
In `@src/Preprocessing/FeatureSelection/Clustering/HierarchicalFS.cs`:
- Around line 36-46: Validate the constructor parameter `linkage` in
HierarchicalFS instead of letting unknown values fall through: in the
HierarchicalFS constructor check for null/whitespace and normalize the string,
then verify it is one of the supported linkage values that
AgglomerativeClustering accepts (e.g., "average", "complete", "single", "ward" —
match actual supported set in AgglomerativeClustering). If the value is invalid,
throw an ArgumentException describing allowed values and the parameter name;
otherwise assign the normalized value to _linkage. Ensure this validation uses
the same identifiers (_linkage, HierarchicalFS, AgglomerativeClustering) so
callers get immediate feedback on invalid configuration.
🧹 Nitpick comments (6)
src/Preprocessing/FeatureSelection/Bioinformatics/SAM.cs (3)
98-105: Floating-point equality comparison may fail for certain numeric types.Using
==to compare doubles (Lines 101, 103) can be unreliable due to floating-point precision issues, especially whenTisfloatand values undergo conversion. Consider using a tolerance-based comparison.🔧 Proposed fix
for (int i = 0; i < n; i++) { double y = NumOps.ToDouble(target[i]); - if (y == label0) + if (Math.Abs(y - label0) < 1e-12) class0.Add(i); - else if (y == label1) + else if (Math.Abs(y - label1) < 1e-12) class1.Add(i); }
190-221: Q-value assignment lacks monotonicity correction.The
_qValuesarray is assigned directly from_fdr(Line 220), but proper q-values (Storey's method) require a cumulative minimum correction to ensure monotonicity when sorted by p-value/d-statistic. CurrentlyQValuesis just a copy of local FDR estimates.This works for selection purposes but may confuse users expecting standard q-values.
301-304: Silent filtering may return fewer feature names than selected features.The
Whereclause at Line 302 silently skips indices exceedinginputFeatureNames.Length. If a user provides an incompleteinputFeatureNamesarray, the returned array will have fewer entries thanSelectedIndices, potentially causing downstream index mismatches.Consider either throwing when
inputFeatureNames.Length < _nInputFeaturesor documenting this behavior explicitly.src/Preprocessing/FeatureSelection/Bioinformatics/VolcanoPlotSelector.cs (1)
87-94: Floating-point equality comparison on class labels.Same concern as SAM.cs: using
==to compare doubles (Lines 90, 92) may be unreliable. Consider using tolerance-based comparison for consistency.🔧 Proposed fix
for (int i = 0; i < n; i++) { double y = NumOps.ToDouble(target[i]); - if (y == label0) + if (Math.Abs(y - label0) < 1e-12) class0.Add(i); else class1.Add(i); }src/Preprocessing/FeatureSelection/Bayesian/BayesianFeatureSelector.cs (2)
80-81: Remove unused variablespositiveCountandnegativeCount.These variables are computed but never used in the method.
🧹 Proposed fix to remove dead code
- double positiveCount = targetArray.Count(y => y > 0.5); - double negativeCount = n - positiveCount; - for (int j = 0; j < p; j++)
182-193: Silent truncation may hide input/selection mismatches.Lines 189-192 silently filter out selected indices that exceed
inputFeatureNames.Length. If_selectedIndicescontains index 5 butinputFeatureNameshas only 4 elements, that feature name is silently dropped rather than raising an error or using a fallback name.Consider throwing an exception or using a fallback pattern consistent with line 187.
♻️ Proposed fix to handle mismatches explicitly
return _selectedIndices - .Where(i => i < inputFeatureNames.Length) - .Select(i => inputFeatureNames[i]) + .Select(i => i < inputFeatureNames.Length ? inputFeatureNames[i] : $"Feature{i}") .ToArray();
- ARDSelector: add input validation and enforce nFeaturesToSelect <= columns - ARDSelector: validate input column count in TransformCore - ARDSelector: throw instead of returning empty array in GetFeatureNamesOut - SpikeAndSlabSelector: add input-shape validation in TransformCore - VolcanoPlotSelector: handle negative/zero means in log2 fold change - ClusteringBasedFS: add empty input validation - HierarchicalFS: validate linkage values instead of silently defaulting Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Introduces a large feature-selection expansion and migrates the library from legacy “normalizer/feature selector” primitives to a pipeline-based preprocessing approach across model building, evaluation, prediction, and uncertainty estimation.
Changes:
- Added new feature selection transformers (cost-sensitive, constraint-based, compression, complexity, clustering, classification, bioinformatics, Bayesian, bandit, association).
- Replaced
NormalizationInfo/INormalizerusage withPreprocessingPipeline/PreprocessingInfoin core model result/prediction paths and evaluation. - Removed legacy normalizer/feature-selector infrastructure (interfaces, factories, base classes, implementations, and options).
Reviewed changes
Copilot reviewed 76 out of 485 changed files in this pull request and generated 17 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Preprocessing/FeatureSelection/CostSensitive/BudgetConstrainedFS.cs | Adds budget-constrained supervised feature selection. |
| src/Preprocessing/FeatureSelection/Constraint/ConstraintBasedSelector.cs | Adds constraint-aware supervised selector (mandatory/forbidden/groups). |
| src/Preprocessing/FeatureSelection/Compression/NormalizedCompressionSelector.cs | Adds MI-based “compression” relevance selector (named as NCD). |
| src/Preprocessing/FeatureSelection/Compression/CompressionRatioSelector.cs | Adds entropy-based compressibility selector. |
| src/Preprocessing/FeatureSelection/Complexity/LyapunovSelector.cs | Adds Lyapunov exponent-based complexity selector. |
| src/Preprocessing/FeatureSelection/Complexity/LempelZivSelector.cs | Adds Lempel–Ziv complexity selector. |
| src/Preprocessing/FeatureSelection/Complexity/HurstExponentSelector.cs | Adds Hurst exponent selector. |
| src/Preprocessing/FeatureSelection/Complexity/FractalDimensionSelector.cs | Adds Higuchi fractal dimension selector. |
| src/Preprocessing/FeatureSelection/Clustering/SilhouetteBasedSelector.cs | Adds silhouette-score-based selector (uses provided labels). |
| src/Preprocessing/FeatureSelection/Clustering/ClusterSeparabilitySelector.cs | Adds k-means separability selector. |
| src/Preprocessing/FeatureSelection/Classification/NaiveBayesSelector.cs | Adds Naive Bayes KL-based selector. |
| src/Preprocessing/FeatureSelection/Classification/LDAProjectionSelector.cs | Adds LDA variance-ratio selector. |
| src/Preprocessing/FeatureSelection/Classification/GiniImpuritySelector.cs | Adds split-based Gini reduction selector. |
| src/Preprocessing/FeatureSelection/Boundary/MarginSelector.cs | Adds margin-based class separation selector. |
| src/Preprocessing/FeatureSelection/Boundary/DecisionBoundarySelector.cs | Adds threshold boundary clarity selector. |
| src/Preprocessing/FeatureSelection/Bioinformatics/FoldChangeSelector.cs | Adds fold-change selector for 2-class differential analysis. |
| src/Preprocessing/FeatureSelection/Bayesian/SpikeAndSlabSelector.cs | Adds spike-and-slab inclusion probability selector. |
| src/Preprocessing/FeatureSelection/Bayesian/HorseshoeSelector.cs | Adds horseshoe-prior selector. |
| src/Preprocessing/FeatureSelection/Bayesian/BayesianNetworkSelector.cs | Adds MI/redundancy “network” selector. |
| src/Preprocessing/FeatureSelection/Bayesian/BayesianModelAveraging.cs | Adds BMA-based inclusion selector. |
| src/Preprocessing/FeatureSelection/Bayesian/BayesianFeatureSelector.cs | Adds Bayes-factor-like posterior probability selector. |
| src/Preprocessing/FeatureSelection/Bayesian/ARDSelector.cs | Adds ARD selector with threshold fallback. |
| src/Preprocessing/FeatureSelection/Bandit/UCB1Selector.cs | Adds UCB1-inspired exploration/exploitation selector. |
| src/Preprocessing/FeatureSelection/Association/FrequentPatternSelector.cs | Adds frequent-pattern scoring selector. |
| src/Preprocessing/FeatureSelection/Association/AprioriBasedSelector.cs | Adds binning + association-score selector. |
| src/NeuralNetworks/QuantumNeuralNetwork.cs | Switches from normalizer to preprocessing pipeline for state prep. |
| src/Models/Results/AiModelResult.cs | Replaces normalization-based predict flow with preprocessing pipeline. |
| src/Models/Results/AiModelResult.Uncertainty.cs | Updates UQ predict flow to use preprocessing; changes variance denorm. |
| src/Models/Options/AiModelResultOptions.cs | Removes legacy NormalizationInfo option in favor of preprocessing. |
| src/Models/Inputs/ModelEvaluationInput.cs | Updates evaluation input to carry PreprocessingInfo instead of norm info. |
| src/Evaluation/DefaultModelEvaluator.cs | Passes preprocessing info to model-stats computation. |
| src/AiModelBuilder.cs | Removes legacy normalizer/feature selector/preprocessor hooks; uses pipelines. |
| src/Normalizers/ZScoreNormalizer.cs | Removes legacy Z-score normalizer implementation. |
| src/Normalizers/NormalizerBase.cs | Removes legacy normalizer base class. |
| src/Models/Options/DataProcessorOptions.cs | Removes legacy data processor options. |
| src/Models/NormalizationInfo.cs | Removes legacy normalization info container. |
| src/Interfaces/INormalizer.cs | Removes legacy normalizer interface. |
| src/Interfaces/IFeatureSelector.cs | Removes legacy feature selector interface. |
| src/Interfaces/IDataPreprocessor.cs | Removes legacy data preprocessor interface. |
| src/FeatureSelectors/VarianceThresholdFeatureSelector.cs | Removes legacy feature selector implementation. |
| src/FeatureSelectors/RecursiveFeatureElimination.cs | Removes legacy RFE implementation. |
| src/FeatureSelectors/NoFeatureSelector.cs | Removes legacy “no-op” feature selector implementation. |
| src/FeatureSelectors/FeatureSelectorBase.cs | Removes legacy feature selector base class. |
| src/FeatureSelectors/CorrelationFeatureSelector.cs | Removes legacy correlation feature selector. |
| src/Factories/NormalizerFactory.cs | Removes legacy normalizer factory. |
| src/Interfaces/IAiModelBuilder.cs | Removes legacy builder hooks for feature selector/normalizer/data preprocessor. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/Preprocessing/FeatureSelection/Clustering/ClusteringBasedFS.cs`:
- Around line 53-62: FitCore currently validates row counts but misses the
zero-columns edge case; add a check at the start of FitCore that throws an
ArgumentException (or ArgumentOutOfRangeException) when data.Columns == 0 to
fail fast instead of producing empty _selectedIndices. Modify FitCore to
validate data.Columns, include a clear message and parameter name, and ensure
_nInputFeatures is only set after this check so the method never proceeds with
zero columns.
🧹 Nitpick comments (2)
src/Preprocessing/FeatureSelection/Bayesian/SpikeAndSlabSelector.cs (1)
204-215: Consider consistent error handling when not fitted.
GetSupportMask()throwsInvalidOperationExceptionwhen the selector isn't fitted, butGetFeatureNamesOut()silently returns an empty array. Additionally, line 212 silently filters indices that exceedinputFeatureNames.Length, which could mask mismatches between selected indices and provided names.♻️ Proposed fix for consistency
public override string[] GetFeatureNamesOut(string[]? inputFeatureNames = null) { - if (_selectedIndices is null) return []; + if (_selectedIndices is null) + throw new InvalidOperationException("SpikeAndSlabSelector has not been fitted."); if (inputFeatureNames is null) return _selectedIndices.Select(i => $"Feature{i}").ToArray(); + if (inputFeatureNames.Length < _nInputFeatures) + throw new ArgumentException( + $"Expected at least {_nInputFeatures} feature names but got {inputFeatureNames.Length}.", + nameof(inputFeatureNames)); + - return _selectedIndices - .Where(i => i < inputFeatureNames.Length) - .Select(i => inputFeatureNames[i]) - .ToArray(); + return _selectedIndices.Select(i => inputFeatureNames[i]).ToArray(); }src/Preprocessing/FeatureSelection/Clustering/ClusteringBasedFS.cs (1)
33-35: Consider returning defensive copies to protect internal state.These properties return the internal arrays directly, allowing callers to mutate them (e.g.,
selector.SelectedIndices![0] = -1). This could corrupt the fitted state.♻️ Proposed fix using defensive copies
- public int[]? ClusterAssignments => _clusterAssignments; - public double[]? FeatureScores => _featureScores; - public int[]? SelectedIndices => _selectedIndices; + public int[]? ClusterAssignments => _clusterAssignments?.ToArray(); + public double[]? FeatureScores => _featureScores?.ToArray(); + public int[]? SelectedIndices => _selectedIndices?.ToArray();Alternatively, use
IReadOnlyList<int>?as the return type if allocation overhead is a concern.
- Update GlobalUsings.cs to use AiDotNet.Preprocessing.* namespaces - Delete NormalizersBenchmarks.cs (uses deleted Normalizer classes) - Delete Enhanced*Example.cs files (use deleted preprocessing API) - Update Program.cs to remove deleted example references Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 77 out of 491 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.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@src/AiModelBuilder.cs`:
- Around line 1131-1135: The code is attaching
PreprocessingInfo<T,TInput,TOutput> using _preprocessingPipeline even in
inference-only builds where the pipeline may be unfitted; update the
AiModelBuilder code that builds the AiModelResultOptions<T,TInput,TOutput> so
that before creating new PreprocessingInfo(...) you verify the pipeline is
actually fitted (or contains fitted parameters); if the pipeline is not fitted
then either set PreprocessingInfo to null or throw a clear exception (fail-fast)
depending on the intended behavior for inference-only paths. Reference the
symbols _preprocessingPipeline, PreprocessingInfo<T,TInput,TOutput>, and
AiModelResultOptions<T,TInput,TOutput> when making the change.
- Around line 1309-1314: BuildStreamingSupervisedAsync currently returns
AiModelResultOptions with PreprocessingInfo set when _preprocessingPipeline
exists, but streaming training never applies that pipeline to batches, causing
train/infer mismatch; fix by either (A) applying the _preprocessingPipeline
inside BuildStreamingSupervisedAsync’s streaming loop (ensure each batch is
passed through the pipeline before tokenization/optimization) and keep
PreprocessingInfo, or (B) forbid preprocessing for streaming by setting
PreprocessingInfo = null (and/or throwing from BuildStreamingSupervisedAsync
when _preprocessingPipeline != null) so callers don’t get exposed to an
unapplied transform; update references to _preprocessingPipeline,
PreprocessingInfo, and AiModelResultOptions<T,TInput,TOutput> accordingly.
In `@src/Models/Results/AiModelResult.cs`:
- Around line 1379-1381: The code currently calls
PreprocessingInfo.InverseTransformPredictions whenever PreprocessingInfo is
non-null, which can error or distort outputs if only the feature pipeline is
fitted; change the guard to ensure the target pipeline is fitted before
inverse-transforming by checking PreprocessingInfo.IsTargetFitted (or equivalent
property) and only call InverseTransformPredictions when IsTargetFitted is true,
otherwise return normalizedPredictions unchanged; apply this same guard to the
other identical call sites (the other uses of InverseTransformPredictions noted
in the review) so all places use PreprocessingInfo != null &&
PreprocessingInfo.IsTargetFitted before invoking InverseTransformPredictions.
- Support preprocessing in streaming training: fit on first batch, transform each input - Support inference-only builds with pre-fitted pipelines - Guard inverse-transform on IsTargetFitted to avoid errors when target not preprocessed Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
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
Adds a comprehensive feature selection module to AiDotNet with 432 implementations covering all critical, high-priority, and gap algorithms from the feature selection methods catalog.
Closes #324
Categories Implemented
Key Features
TransformerBase<T, Matrix<T>, Matrix<T>>patternFit(),Transform(),FitTransform()methodsFilter Methods Include
Embedded Methods Include
Wrapper Methods Include
Statistical Methods Include
Advanced Methods Include
Build Status
Test Plan
🤖 Generated with Claude Code