fix: prevent data leakage — fit preprocessing after train/test split - #934
Conversation
In BuildSupervisedInternalAsync(), the preprocessing pipeline (e.g., StandardScaler) was fitted on the ENTIRE dataset before the train/test split. This leaked test/validation statistics (mean, std dev) into the training pipeline, causing artificially inflated metrics. The fix restructures the method: 1. Split data into train/val/test FIRST using DataSplitter.Split() 2. FitTransform preprocessing pipeline on training data ONLY 3. Transform (not FitTransform) validation and test data The federated learning path is unchanged — it correctly uses all data as training data, so FitTransform on everything remains correct. Fixes #929 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughSplits data before preprocessing to prevent leakage: fit preprocessing only on the training partition and transform val/test; adds partitioned-federated aggregation and per-client/total-sample validations; demotes global PreprocessingRegistry to a compatibility shim; adds integration tests verifying training-only fitting and end-to-end behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant User as "User"
participant Builder as "AiModelBuilder"
participant Splitter as "DataSplitter"
participant Preproc as "PreprocessingPipeline"
participant Trainer as "Trainer"
participant Clients as "Federated Clients"
User->>Builder: BuildSupervisedInternalAsync(data, config)
Builder->>Splitter: Split(data) -> XTrain,XVal,XTest,yTrain,yVal,yTest
alt non-federated & pipeline configured
Builder->>Preproc: FitTransform(XTrain)
Preproc-->>Builder: preprocessedXTrain
Builder->>Preproc: Transform(XVal), Transform(XTest)
Preproc-->>Builder: preprocessedXVal, preprocessedXTest
else partitioned federated data
Clients->>Builder: Provide per-client partitions
Builder->>Builder: Aggregate client partitions -> aggregatedTrain, record per-client ranges
Builder->>Preproc: FitTransform(aggregatedTrain)
Preproc-->>Builder: preprocessedAggregatedTrain
Builder->>Builder: Validate per-client ranges & total sample counts
end
Builder->>Trainer: Train(modelSpec, preprocessedXTrain, yTrain)
Trainer-->>Builder: trainedModel
Builder-->>User: AiModelResult(trainedModel, PreprocessingInfo)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull request overview
This PR fixes supervised-training data leakage by ensuring the preprocessing pipeline is fitted after the train/validation/test split (fit on train only; transform val/test), and adds integration tests intended to prevent regressions.
Changes:
- Refactors
AiModelBuilder.BuildSupervisedInternalAsync()to split first, thenFitTransformonXTrainandTransformonXVal/XTest. - Keeps the partitioned federated-learning path fitting on all data (since all data is training data there).
- Adds
DataLeakagePreventionIntegrationTestscovering preprocessing fit/transform behavior and inference-time transform via stored pipeline.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
src/AiModelBuilder.cs |
Reorders split vs preprocessing to prevent leakage; adjusts preprocessing flow for supervised vs partitioned federated paths. |
tests/AiDotNet.Tests/IntegrationTests/Regression/DataLeakagePreventionIntegrationTests.cs |
Adds integration tests for leakage prevention and pipeline persistence/transform. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/AiModelBuilder.cs (1)
2052-2086:⚠️ Potential issue | 🟠 MajorClient-range partitioning is unsafe without a row-order guarantee.
This branch only verifies X/Y counts after preprocessing, then reuses the original
StartRow/SampleCountranges whenCreateFederatedClientPartitionsFromClientRangesslices the transformed dataset. A transformer that reorders rows without changing the sample count will silently assign examples to the wrong client. Either carry stable row IDs through preprocessing and validate the output order, or reject partitioned-federated preprocessing stages unless they explicitly guarantee per-client row-order preservation. Based on learnings: Federated preprocessing invariant: When using IFederatedClientDataLoader in PredictionModelBuilder.BuildSupervisedInternalAsync (src/PredictionModelBuilder.cs), any preprocessing must preserve per-client row ordering and total sample counts.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/AiModelBuilder.cs` around lines 2052 - 2086, The code currently applies _preprocessingPipeline across the aggregated dataset and only checks row counts, which allows row-reordering transformers to misassign examples to clients; fix by either (A) propagating stable row IDs through preprocessing and validating they remain in the original per-client order before calling CreateFederatedClientPartitionsFromClientRanges (ensure ConversionsHelper.ConvertToMatrix/ConvertToVector preserve/return row IDs and compare them to the original StartRow/SampleCount ranges created for CreateFederatedClientPartitionsFromClientRanges), or (B) forbid global preprocessing when using IFederatedClientDataLoader in PredictionModelBuilder.BuildSupervisedInternalAsync unless the pipeline explicitly advertises it is order-preserving (add a flag/interface on the pipeline and throw an InvalidOperationException from AiModelBuilder.BuildSupervisedInternalAsync if _preprocessingPipeline != null and not order-preserving). Include references to _preprocessingPipeline, PreprocessingInfo<T,TInput,TOutput>, CreateFederatedClientPartitionsFromClientRanges, ConversionsHelper.ConvertToMatrix/ConvertToVector and IFederatedClientDataLoader when implementing the check.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/AiModelBuilder.cs`:
- Around line 2031-2039: BuildSupervisedInternalAsync still fits preprocessing
on the full dataset in two places (_dataPreparationPipeline.FitResample(...)
before splitting and the AutoML branch where
_preprocessingPipeline.FitTransform(autoMLPreparedX) runs before
DataSplitter.Split), which leaks validation/test statistics; change both flows
to split raw data first via DataSplitter.Split into XTrain/yTrain and XVal/yVal
(and XTest if used), run Fit/Resample only on XTrain/yTrain (e.g., call
_dataPreparationPipeline.FitResample on the training partition and
_preprocessingPipeline.Fit on XTrain), then call Transform on validation/test
partitions with the training-fitted pipeline; update the AutoML branch to
prepare and fit transforms only on its training partition (use
autoMLPreparedXTrain) before model selection/evaluation so all paths
consistently avoid leaking test/validation data.
- Around line 2042-2048: The CS8600/CS8604 suppression is left active for the
rest of AiModelBuilder.cs; narrow its scope by adding a matching `#pragma` warning
restore CS8600, CS8604 immediately after the four local declarations (TInput
XVal, TOutput yVal, TInput XTest, TOutput yTest) in the method around those
lines (or alternatively replace the suppression by making the validation/test
slot variables explicitly nullable or initialized via explicit non-null checks),
so that only that small block around those declarations disables the warnings
rather than the entire file.
In
`@tests/AiDotNet.Tests/IntegrationTests/Regression/DataLeakagePreventionIntegrationTests.cs`:
- Around line 25-236: Add a test exercising the federated path to ensure
preprocessing preserves per-client ordering and total sample counts: create a
minimal IFederatedClientDataLoader fixture (multiple clients with known row
counts and deterministic per-client row order), call
PredictionModelBuilder.BuildSupervisedInternalAsync (via
AiModelBuilder.BuildAsync) with a StandardScaler preprocessing pipeline, and
assert that the resulting PreprocessingInfo.Pipeline is fitted and that
transforming each client's data preserves row ordering and that the sum of
transformed rows equals the original total sample count; also include a negative
check comparing a scaler fitted on ALL concatenated data to ensure
per-client-fitted behavior differs if the implementation is wrong.
- Around line 27-106: The two tests
BuildAsync_WithPreprocessing_FitsOnlyOnTrainingData and
BuildAsync_WithPreprocessing_ModelTrainsSuccessfully are too weak (only
non-null/IsFitted checks) and won't detect preprocessing-fit-on-all-data
leakage; update the first test to compute training-only feature means (from rows
0..trainEnd) and overall feature means, then retrieve the fitted StandardScaler
parameters from result.PreprocessingInfo (mean/scale) and assert they match the
training-only means (and differ from overall means) to prove the scaler was
fitted on train only; for the second test either collapse into a single smoke
test with a clearer name or replace the trivial IsFitted assertion with a
concrete numeric check (e.g., transform a held-out test sample with
result.PreprocessingInfo + model and assert predictions follow expected linear
relationship) so both tests assert specific expected values rather than just
non-null.
- Around line 108-138: Update the test
BuildAsync_WithPreprocessing_PipelineInResultCanTransformNewData to assert
concrete properties of the transformed output rather than only non-null: after
calling preprocessingInfo.Pipeline.Transform(newData) verify the transformed
Matrix<double> has the same row/column dimensions as newData and assert at least
one expected scaled value for a known input (e.g., compute the expected
standardized value for newData[0,0] using the training data/statistics from
CreateLinearDataset or by applying the same StandardScaler logic) so the test
checks both shape preservation and a specific numeric result.
- Around line 164-233: The test currently only verifies the pipeline differs
from a scaler fit on ALL data; instead, update
BuildAsync_WithPreprocessing_ScalerStatisticsReflectTrainingDataOnly to
deterministically obtain the exact training subset and assert the pipeline was
fitted only on that training data: configure the data splitting used by
AiModelBuilder (or DataLoaders.FromMatrixVector) to a deterministic
split/no-shuffle or expose the training indices, then create a
StandardScaler<double> and FitTransform it on that exact training subset (using
the same Matrix<double> rows), and finally assert that
result.PreprocessingInfo.Pipeline.Transform(testPoint) equals the scaler fitted
on that exact training subset (compare per-feature within Tolerance) and not
merely unequal to the all-data fit; reference the test method
BuildAsync_WithPreprocessing_ScalerStatisticsReflectTrainingDataOnly,
AiModelBuilder.ConfigurePreprocessing, StandardScaler<double>,
DataLoaders.FromMatrixVector, and result.PreprocessingInfo.Pipeline to locate
where to change the test.
---
Outside diff comments:
In `@src/AiModelBuilder.cs`:
- Around line 2052-2086: The code currently applies _preprocessingPipeline
across the aggregated dataset and only checks row counts, which allows
row-reordering transformers to misassign examples to clients; fix by either (A)
propagating stable row IDs through preprocessing and validating they remain in
the original per-client order before calling
CreateFederatedClientPartitionsFromClientRanges (ensure
ConversionsHelper.ConvertToMatrix/ConvertToVector preserve/return row IDs and
compare them to the original StartRow/SampleCount ranges created for
CreateFederatedClientPartitionsFromClientRanges), or (B) forbid global
preprocessing when using IFederatedClientDataLoader in
PredictionModelBuilder.BuildSupervisedInternalAsync unless the pipeline
explicitly advertises it is order-preserving (add a flag/interface on the
pipeline and throw an InvalidOperationException from
AiModelBuilder.BuildSupervisedInternalAsync if _preprocessingPipeline != null
and not order-preserving). Include references to _preprocessingPipeline,
PreprocessingInfo<T,TInput,TOutput>,
CreateFederatedClientPartitionsFromClientRanges,
ConversionsHelper.ConvertToMatrix/ConvertToVector and IFederatedClientDataLoader
when implementing the check.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8e851d3d-db47-4da6-a6a5-8b870354e85d
📒 Files selected for processing (2)
src/AiModelBuilder.cstests/AiDotNet.Tests/IntegrationTests/Regression/DataLeakagePreventionIntegrationTests.cs
…istry sync - Scope #pragma warning disable CS8600/CS8604 with matching restore - Restore PreprocessingRegistry.Current in all ConfigurePreprocessing overloads - Update PreprocessingRegistry docs to reflect thread-safety and usage pattern - Rewrite tests to actually detect leakage: verify scaler produces different output than all-data scaler, verify transform shape and non-identity - Add end-to-end predict test with preprocessing - Add test verifying no-preprocessing path still works with predict Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (6)
src/AiModelBuilder.cs (2)
2039-2047:⚠️ Potential issue | 🔴 CriticalLeakage is still possible in the remaining full-dataset fit paths.
The split-before-fit logic below fixes the plain supervised branch, but this method still learns from validation/test rows earlier via
_dataPreparationPipeline.FitResample(...)/FitResampleTensor(...)on the full dataset, and the AutoML branch still calls_preprocessingPipeline.FitTransform(autoMLPreparedX)before its split. That means the stated leakage fix is still incomplete.Concrete fix direction
-// fit row-changing preparation before splitting -var (prepX, prepY) = _dataPreparationPipeline.FitResample(...); -... -(XTrain, yTrain, XVal, yVal, XTest, yTest) = DataSplitter.Split(... preparedX, preparedY, ...); +// split raw data first +(XTrain, yTrain, XVal, yVal, XTest, yTest) = DataSplitter.Split(... x, y, ...); + +// fit row-changing preparation on training only +if (_dataPreparationPipeline != null && _dataPreparationPipeline.Count > 0) +{ + (XTrain, yTrain) = FitResampleTrainingOnly(XTrain, yTrain); +} + +// then fit preprocessing on XTrain and transform XVal/XTestApply the same ordering in the AutoML branch as well.
As per coding guidelines: "Every PR must contain production-ready code." and "Incomplete features: Half-implemented patterns where some code paths work but others silently do nothing."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/AiModelBuilder.cs` around lines 2039 - 2047, The current change prevents leakage for the plain supervised path but still fits preprocessing on the whole dataset in other paths; update the AutoML and full-dataset fit paths to follow the same split-then-fit ordering: move calls to _dataPreparationPipeline.FitResample and FitResampleTensor and any call to _preprocessingPipeline.FitTransform(autoMLPreparedX) so they are executed only on training partitions after you perform the train/validation/test split (or skip split only for true federated-client-all-training mode); ensure the AutoML branch uses the same split-before-fit flow as the supervised branch and rewire any variables it expects (e.g., autoMLPreparedX -> autoMLTrainX/autoMLValX) so no preprocessing Fit/FitTransform is run on validation/test rows.
2050-2057:⚠️ Potential issue | 🟠 MajorScope the nullability suppression to the placeholder declarations only.
TInputandTOutputare unconstrained here, sodefaultcan still benull, and the#pragma warning disable CS8600, CS8604stays active until Line 2813. That hides unrelated nullability diagnostics across almost the entire method.Suggested change
-// These generic types are always value types (Matrix<T>, Vector<T>) at runtime, -// so default produces valid zero-initialized values, not null. The suppression covers -// both the declarations and all downstream usage sites in this method. -#pragma warning disable CS8600, CS8604 // Generic type defaults - T is always a value type at runtime -TInput XVal = default; -TOutput yVal = default; -TInput XTest = default; -TOutput yTest = default; +// Validation/test slots are optional in the federated branch. +TInput XVal = default!; +TOutput yVal = default!; +TInput XTest = default!; +TOutput yTest = default!; ... -#pragma warning restore CS8600, CS8604If downstream sites still warn after this, make those slots explicitly nullable or branch-specific instead of suppressing the whole method.
This checks the suppression span and whether the repository actually treats these generics as reference-capable. Expected result: the disable begins near Line 2053, the restore is only at Line 2813, and the repo contains evidence that the “always value types” comment is not safe.
#!/bin/bash set -euo pipefail file="src/AiModelBuilder.cs" printf '%s\n' '-- suppression block --' sed -n '2048,2058p' "$file" printf '\n%s\n' '-- restore location --' sed -n '2808,2814p' "$file" printf '\n%s\n' '-- Matrix/Vector declarations in repo --' rg -nP --type=cs '^\s*(public|internal)\s+(readonly\s+)?(partial\s+)?(struct|class)\s+(Matrix|Vector)<' printf '\n%s\n' '-- reference-type AiModelBuilder usages/examples --' rg -nP --type=cs 'AiModelBuilder<[^>]+,\s*string\s*,|AiModelBuilder<[^>]+,[^>]+,\s*string\s*>' src tests || trueAlso applies to: 2813-2813
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/AiModelBuilder.cs` around lines 2050 - 2057, The nullability suppression (`#pragma` warning disable CS8600, CS8604) currently spans the entire method; narrow it so it only applies to the placeholder declarations for TInput and TOutput: keep the pragma immediately above the four default assignments (TInput XVal, TOutput yVal, TInput XTest, TOutput yTest) and add a matching `#pragma` warning restore directly after those declarations; remove the broad suppression that extends to Line 2813 and instead address any downstream warnings by making specific variables nullable or adding targeted, minimal suppressions at the usage sites (reference the TInput/TOutput placeholder declarations to locate where to move the restore).tests/AiDotNet.Tests/IntegrationTests/Regression/DataLeakagePreventionIntegrationTests.cs (4)
150-188:⚠️ Potential issue | 🔴 Critical
PredictEndToEndWorksnever verifies prediction correctness.Right now this only proves the pipeline returns finite numbers. For an end-to-end regression test, use a dataset with known coefficients—or return them from
CreateLinearDataset—and assert concrete predictions for fixed inputs within tolerance.As per coding guidelines, tests must “Assert specific expected values, not just non-null.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/AiDotNet.Tests/IntegrationTests/Regression/DataLeakagePreventionIntegrationTests.cs` around lines 150 - 188, Update the test BuildAsync_WithPreprocessing_PredictEndToEndWorks to assert concrete prediction values: modify CreateLinearDataset (or overload it) to generate and return the true coefficient vector and intercept for the synthetic linear model, then after building the pipeline (AiModelBuilder.BuildAsync) use those known coefficients to compute expected outputs for the fixed newData and compare against result.Predict(newData) with a numeric tolerance (e.g., using Assert.InRange or Math.Abs difference). Ensure you reference CreateLinearDataset to obtain the true parameters and use result.Predict for the actual predictions, replacing the current only-finite checks with assertions that each predicted value is within tolerance of the analytically computed expected value.
25-272:⚠️ Potential issue | 🟠 MajorThe federated preprocessing branch is still untested.
All five cases go through
DataLoaders.FromMatrixVector, so the path the PR explicitly promises to preserve never gets exercised here. Please add one federated regression case that proves preprocessing keeps per-client row ordering intact and preserves the total sample count after transform.Based on learnings, “When using IFederatedClientDataLoader in PredictionModelBuilder.BuildSupervisedInternalAsync (src/PredictionModelBuilder.cs), any preprocessing must preserve per-client row ordering and total sample counts.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/AiDotNet.Tests/IntegrationTests/Regression/DataLeakagePreventionIntegrationTests.cs` around lines 25 - 272, Add a new integration test (e.g., BuildAsync_WithPreprocessing_FederatedPreservesOrderingAndCount) that exercises the federated preprocessing branch by using an IFederatedClientDataLoader implementation instead of DataLoaders.FromMatrixVector; arrange multiple clients each with known row sequences and counts, configure the AiModelBuilder with a StandardScaler via ConfigurePreprocessing, call BuildAsync so PredictionModelBuilder.BuildSupervisedInternalAsync runs the federated path, then assert that result.PreprocessingInfo.Pipeline.IsFitted is true and that transforming each client's data preserves per-client row ordering and that the sum of transformed row counts equals the total input samples (use unique markers or sequential values per row to verify ordering and equality of counts).
27-92:⚠️ Potential issue | 🔴 CriticalThis still doesn’t prove the scaler was fit on the exact training split.
Comparing only against an all-data scaler leaves a false-positive gap: a buggy implementation that fits on
train + validationwould still differ fromallDataScalerand pass. Rebuild the exact deterministic training partition and assert equality with aStandardScaler<double>fit on those rows, then keep the inequality check against the all-data fit as the secondary guard.As per coding guidelines, good tests should “Assert specific expected values, not just non-null.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/AiDotNet.Tests/IntegrationTests/Regression/DataLeakagePreventionIntegrationTests.cs` around lines 27 - 92, The test must prove the scaler was fitted on the exact training rows — reconstruct the deterministic training partition used by AiModelBuilder and assert the pipeline equals a scaler fit on those rows. Recreate the train indices using the same splitter/settings used by the builder (e.g., call DataSplitter.Split or the same split routine/seed used by AiModelBuilder), fit a new StandardScaler<double> on x[trainIndices,*], and Assert that trainFittedPipeline.Transform(testPoint) equals that scaler's Transform(testPoint); keep the existing inequality check against allDataScaler as a secondary guard. Locate symbols: BuildAsync_WithPreprocessing_ScalerStatisticsReflectTrainingDataOnly, AiModelBuilder.ConfigurePreprocessing, result.PreprocessingInfo?.Pipeline, StandardScaler<double>, and DataSplitter.Split (or the project’s split function) to implement this change.
95-148:⚠️ Potential issue | 🔴 Critical
TransformProducesCorrectShapeis still a smoke test.Shape preservation plus “some value changed” will pass for a lot of broken transforms. This should assert one or more exact standardized values for a fixed input, ideally by fitting a reference scaler on the same deterministic training rows and comparing the transformed cells directly.
As per coding guidelines, tests must “Assert specific expected values, not just non-null.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/AiDotNet.Tests/IntegrationTests/Regression/DataLeakagePreventionIntegrationTests.cs` around lines 95 - 148, The test BuildAsync_WithPreprocessing_TransformProducesCorrectShape is currently a weak smoke test; replace the vague "some value changed" check with deterministic assertions by reproducing the StandardScaler fit on the same training rows (the CreateLinearDataset output used by the builder) to compute expected standardized values and then compare specific transformed cells from preprocessingInfo.Pipeline.Transform(newData) against those expected values (use Tolerance for floating comparison). Locate the fit used by the builder (the ConfigurePreprocessing pipeline / StandardScaler<double>), fit an independent StandardScaler on the same training matrix x, transform the same newData with that reference scaler, and assert equality for one or more specific cells (e.g., transformed[0,0] and transformed[1,2]) to ensure exact standardized behavior rather than just shape or any-change checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Preprocessing/PreprocessingRegistry.cs`:
- Around line 16-19: The static mutable registry (PreprocessingRegistry.Current
/ field _preprocessingPipeline) allows cross-build contamination; remove/stop
using the global accessor in the active path by eliminating reads from
PreprocessingRegistry.Current inside Transform() and FitTransform(), instead
thread-scope the fitted pipeline on the builder and propagate the fitted
pipeline via PreprocessingInfo (or a builder-owned property) to all consumers;
update callers of Transform(), FitTransform(), and any code that previously read
PreprocessingRegistry.Current to accept the fitted pipeline/PreprocessingInfo as
an explicit parameter so no concurrent build ever reads a shared static.
---
Duplicate comments:
In `@src/AiModelBuilder.cs`:
- Around line 2039-2047: The current change prevents leakage for the plain
supervised path but still fits preprocessing on the whole dataset in other
paths; update the AutoML and full-dataset fit paths to follow the same
split-then-fit ordering: move calls to _dataPreparationPipeline.FitResample and
FitResampleTensor and any call to
_preprocessingPipeline.FitTransform(autoMLPreparedX) so they are executed only
on training partitions after you perform the train/validation/test split (or
skip split only for true federated-client-all-training mode); ensure the AutoML
branch uses the same split-before-fit flow as the supervised branch and rewire
any variables it expects (e.g., autoMLPreparedX -> autoMLTrainX/autoMLValX) so
no preprocessing Fit/FitTransform is run on validation/test rows.
- Around line 2050-2057: The nullability suppression (`#pragma` warning disable
CS8600, CS8604) currently spans the entire method; narrow it so it only applies
to the placeholder declarations for TInput and TOutput: keep the pragma
immediately above the four default assignments (TInput XVal, TOutput yVal,
TInput XTest, TOutput yTest) and add a matching `#pragma` warning restore directly
after those declarations; remove the broad suppression that extends to Line 2813
and instead address any downstream warnings by making specific variables
nullable or adding targeted, minimal suppressions at the usage sites (reference
the TInput/TOutput placeholder declarations to locate where to move the
restore).
In
`@tests/AiDotNet.Tests/IntegrationTests/Regression/DataLeakagePreventionIntegrationTests.cs`:
- Around line 150-188: Update the test
BuildAsync_WithPreprocessing_PredictEndToEndWorks to assert concrete prediction
values: modify CreateLinearDataset (or overload it) to generate and return the
true coefficient vector and intercept for the synthetic linear model, then after
building the pipeline (AiModelBuilder.BuildAsync) use those known coefficients
to compute expected outputs for the fixed newData and compare against
result.Predict(newData) with a numeric tolerance (e.g., using Assert.InRange or
Math.Abs difference). Ensure you reference CreateLinearDataset to obtain the
true parameters and use result.Predict for the actual predictions, replacing the
current only-finite checks with assertions that each predicted value is within
tolerance of the analytically computed expected value.
- Around line 25-272: Add a new integration test (e.g.,
BuildAsync_WithPreprocessing_FederatedPreservesOrderingAndCount) that exercises
the federated preprocessing branch by using an IFederatedClientDataLoader
implementation instead of DataLoaders.FromMatrixVector; arrange multiple clients
each with known row sequences and counts, configure the AiModelBuilder with a
StandardScaler via ConfigurePreprocessing, call BuildAsync so
PredictionModelBuilder.BuildSupervisedInternalAsync runs the federated path,
then assert that result.PreprocessingInfo.Pipeline.IsFitted is true and that
transforming each client's data preserves per-client row ordering and that the
sum of transformed row counts equals the total input samples (use unique markers
or sequential values per row to verify ordering and equality of counts).
- Around line 27-92: The test must prove the scaler was fitted on the exact
training rows — reconstruct the deterministic training partition used by
AiModelBuilder and assert the pipeline equals a scaler fit on those rows.
Recreate the train indices using the same splitter/settings used by the builder
(e.g., call DataSplitter.Split or the same split routine/seed used by
AiModelBuilder), fit a new StandardScaler<double> on x[trainIndices,*], and
Assert that trainFittedPipeline.Transform(testPoint) equals that scaler's
Transform(testPoint); keep the existing inequality check against allDataScaler
as a secondary guard. Locate symbols:
BuildAsync_WithPreprocessing_ScalerStatisticsReflectTrainingDataOnly,
AiModelBuilder.ConfigurePreprocessing, result.PreprocessingInfo?.Pipeline,
StandardScaler<double>, and DataSplitter.Split (or the project’s split function)
to implement this change.
- Around line 95-148: The test
BuildAsync_WithPreprocessing_TransformProducesCorrectShape is currently a weak
smoke test; replace the vague "some value changed" check with deterministic
assertions by reproducing the StandardScaler fit on the same training rows (the
CreateLinearDataset output used by the builder) to compute expected standardized
values and then compare specific transformed cells from
preprocessingInfo.Pipeline.Transform(newData) against those expected values (use
Tolerance for floating comparison). Locate the fit used by the builder (the
ConfigurePreprocessing pipeline / StandardScaler<double>), fit an independent
StandardScaler on the same training matrix x, transform the same newData with
that reference scaler, and assert equality for one or more specific cells (e.g.,
transformed[0,0] and transformed[1,2]) to ensure exact standardized behavior
rather than just shape or any-change checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c13c6717-e3e5-4491-aaa3-ddd4a7422f4a
📒 Files selected for processing (3)
src/AiModelBuilder.cssrc/Preprocessing/PreprocessingRegistry.cstests/AiDotNet.Tests/IntegrationTests/Regression/DataLeakagePreventionIntegrationTests.cs
FitResample (SMOTE, outlier removal, augmentation) was applied to the full dataset before splitting, allowing synthetic samples derived from test/validation data to leak into training. Now FitResample runs after the split on training data only in both standard and AutoML paths. Also fixed AutoML preprocessing to FitTransform on training only and Transform on validation. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/AiModelBuilder.cs (1)
2088-2102:⚠️ Potential issue | 🟠 MajorSample-count checks alone don't protect federated client boundaries.
CreateFederatedClientPartitionsFromClientRanges()later slices by the originalStartRow/SampleCount. A preprocessing step that reorders rows but preserves count will pass these checks and silently assign samples to the wrong clients. This path needs an order-preservation contract, or the preprocessing/data-preparation has to run per client before aggregation.Based on learnings, Federated preprocessing invariant: When using
IFederatedClientDataLoaderinPredictionModelBuilder.BuildSupervisedInternalAsync(src/PredictionModelBuilder.cs), any preprocessing must preserve per-client row ordering and total sample counts. The code now enforces X/Y alignment and total count checks afterPreprocessDatawhen federated client ranges are used, failing fast with a clear exception if violated.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/AiModelBuilder.cs` around lines 2088 - 2102, The current checks in BuildSupervisedInternalAsync only validate global X/Y alignment and total sample count after PreprocessData but do not guarantee per-client row ordering, which can let reordered-but-count-preserving preprocessing misassign samples when CreateFederatedClientPartitionsFromClientRanges slices by original StartRow/SampleCount; fix by enforcing an order-preservation contract: either run preprocessing per-client before aggregating (i.e., apply PreprocessData for each IFederatedClientDataLoader partition), or add a post-preprocess validation that for each federated client range (using the original StartRow/SampleCount from IFederatedClientDataLoader) the corresponding rows in preprocessedMatrix remain contiguous and in the same relative order (and throw a clear InvalidOperationException referencing PreprocessData, CreateFederatedClientPartitionsFromClientRanges, and PredictionModelBuilder.BuildSupervisedInternalAsync if any client’s rows are missing/reordered); ensure error messages name preprocessedMatrix/preprocessedVector and expectedFederatedSampleCount so callers know to switch to per-client preprocessing or disable reordering filters.
♻️ Duplicate comments (1)
src/AiModelBuilder.cs (1)
2043-2047:⚠️ Potential issue | 🟠 MajorDon't leave validation/test slots unset in the partitioned federated flow.
When
usePartitionedFederatedDatais true, these locals stay atdefault, but the HPO block later still buildsOptimizationInputDatafrom them before the federated branch takes over. That makes the warning suppression hide a real runtime path, not just noise. Either short-circuit HPO for partitioned federated builds or keep the federated path from sharing validation/test locals at all.Possible guard
+ if (usePartitionedFederatedData && + _hyperparameterOptimizer is not null && + _hyperparameterSearchSpace is not null) + { + throw new InvalidOperationException( + "Hyperparameter optimization is not supported with partitioned federated data because validation/test splits are not materialized in that flow."); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/AiModelBuilder.cs` around lines 2043 - 2047, The locals XVal, yVal, XTest, yTest are left as default when usePartitionedFederatedData is true, but later the HPO block still constructs OptimizationInputData from them causing a real runtime path; update the flow so that when usePartitionedFederatedData is true you either (a) short-circuit/skip the HPO/OptimizationInputData construction entirely, or (b) ensure the federated branch provides valid validation/test values before building OptimizationInputData. Concretely, modify the logic around usePartitionedFederatedData and the HPO block (the code that constructs OptimizationInputData and runs HPO) so it is not executed with uninitialized XVal/yVal/XTest/yTest, or route federated validation/test data into those variables before use.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/AiModelBuilder.cs`:
- Around line 2111-2115: The supervised split is hardcoded to shuffle:true which
reintroduces temporal leakage for time-series tasks; update the
DataSplitter.Split call in AiModelBuilder (the block that creates
XTrain,yTrain,XVal,yVal,XTest,yTest) to reuse the same shuffle policy used
earlier for AutoML time-series branches (i.e. set shuffle = false when the task
is TimeSeriesForecasting or TimeSeriesAnomalyDetection, or reuse the previously
computed shuffle flag/variable), so the final training/evaluation uses a
non-shuffled chronological split for time-series builds instead of always true.
---
Outside diff comments:
In `@src/AiModelBuilder.cs`:
- Around line 2088-2102: The current checks in BuildSupervisedInternalAsync only
validate global X/Y alignment and total sample count after PreprocessData but do
not guarantee per-client row ordering, which can let
reordered-but-count-preserving preprocessing misassign samples when
CreateFederatedClientPartitionsFromClientRanges slices by original
StartRow/SampleCount; fix by enforcing an order-preservation contract: either
run preprocessing per-client before aggregating (i.e., apply PreprocessData for
each IFederatedClientDataLoader partition), or add a post-preprocess validation
that for each federated client range (using the original StartRow/SampleCount
from IFederatedClientDataLoader) the corresponding rows in preprocessedMatrix
remain contiguous and in the same relative order (and throw a clear
InvalidOperationException referencing PreprocessData,
CreateFederatedClientPartitionsFromClientRanges, and
PredictionModelBuilder.BuildSupervisedInternalAsync if any client’s rows are
missing/reordered); ensure error messages name
preprocessedMatrix/preprocessedVector and expectedFederatedSampleCount so
callers know to switch to per-client preprocessing or disable reordering
filters.
---
Duplicate comments:
In `@src/AiModelBuilder.cs`:
- Around line 2043-2047: The locals XVal, yVal, XTest, yTest are left as default
when usePartitionedFederatedData is true, but later the HPO block still
constructs OptimizationInputData from them causing a real runtime path; update
the flow so that when usePartitionedFederatedData is true you either (a)
short-circuit/skip the HPO/OptimizationInputData construction entirely, or (b)
ensure the federated branch provides valid validation/test values before
building OptimizationInputData. Concretely, modify the logic around
usePartitionedFederatedData and the HPO block (the code that constructs
OptimizationInputData and runs HPO) so it is not executed with uninitialized
XVal/yVal/XTest/yTest, or route federated validation/test data into those
variables before use.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: f5950163-0315-48d3-9fb2-3fbbca8482af
📒 Files selected for processing (1)
src/AiModelBuilder.cs
Updated PreprocessingRegistry docs to clearly warn about concurrent build safety issues. Applied same time-series non-shuffle policy to the final supervised training split that AutoML already uses. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
♻️ Duplicate comments (3)
src/Preprocessing/PreprocessingRegistry.cs (1)
10-16:⚠️ Potential issue | 🔴 CriticalBlocking: documenting the singleton hazard is not a production fix.
The warning is accurate, but
Current,Transform(), andFitTransform()still keep the process-global pipeline on the active path, and theCurrentremarks below still call it thread-safe. Parallel builds can still observe another builder’s fitted transformer, so this remains cross-build contamination/data leakage, not just a docs issue.Concrete direction
public static class PreprocessingRegistry<T, TInput> { + [Obsolete("Use the builder-owned preprocessing pipeline / AiModelResult.PreprocessingInfo instead of process-global state.")] public static IDataTransformer<T, TInput, TInput>? Current { get { lock (_lock) @@ } } + [Obsolete("Pass the fitted pipeline explicitly; do not use process-global preprocessing state.")] public static TInput Transform(TInput input) { var pipeline = Current; if (pipeline != null && pipeline.IsFitted) { return pipeline.Transform(input); } return input; } + [Obsolete("Pass the fitted pipeline explicitly; do not use process-global preprocessing state.")] public static TInput FitTransform(TInput input)Then migrate active callers to take the fitted pipeline explicitly from
AiModelBuilder/PreprocessingInfoso this registry is compatibility-only until it can be removed.As per coding guidelines, "Users should ONLY interact with
AiModelBuilder.csandAiModelResult.cs" and blocking issues include "Incomplete features" and "Every PR must contain production-ready code."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Preprocessing/PreprocessingRegistry.cs` around lines 10 - 16, The PreprocessingRegistry singleton still exposes and mutates a process-global pipeline via Current, Transform(), and FitTransform(), causing cross-build contamination; update callers so the registry is strictly compatibility-only and no longer holds or returns the active pipeline: remove or stop using the global _preprocessingPipeline in PreprocessingRegistry by changing Current, Transform, and FitTransform to no-op or to throw a clear compatibility-only exception, and update all call sites to take the fitted pipeline explicitly from AiModelBuilder (use its instance-level _preprocessingPipeline) or from PreprocessingInfo passed in the result; ensure any code that previously relied on PreprocessingRegistry.Current or its Transform/FitTransform now receives the pipeline via AiModelBuilder/PreprocessingInfo to prevent any shared state across parallel builds.src/AiModelBuilder.cs (2)
1816-1820:⚠️ Potential issue | 🟠 MajorThis still shuffles manual time-series builds.
shuffleBeforeSplitis derived only from_autoMLOptions?.TaskFamilyOverride. Direct time-series model builds, and customConfigureAutoML(IAutoMLModel<...>)flows where_autoMLOptionsstays null, still fall back toshuffle: true, so chronological evaluation is still wrong in those paths. Compute this once from the actual task/model being trained and reuse that helper in both branches.Also applies to: 2114-2117
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/AiModelBuilder.cs` around lines 1816 - 1820, The shuffle logic currently only checks _autoMLOptions?.TaskFamilyOverride so manual/time-series builds (or ConfigureAutoML flows where _autoMLOptions is null) still get shuffle:true; add a small helper (e.g., IsTimeSeriesTask or ComputeShuffleBeforeSplit) that determines whether the current training target is a time-series task by inspecting the actual task/model being trained (not just _autoMLOptions), then use that helper to compute shuffleBeforeSplit and pass it into DataSplitter.Split<T,TInput,TOutput> in both places (the block around DataSplitter.Split and the other occurrence at the later branch) so chronological splits are consistently unshuffled for time-series models.
2040-2047:⚠️ Potential issue | 🟠 MajorThe nullability suppression is still too broad.
Line 2040's rationale is incorrect:
TInputandTOutputare unconstrained type parameters, sodefaultcan still benull. KeepingCS8600/CS8604disabled until Line 2841 hides nullable-flow problems across the rest ofBuildSupervisedInternalAsync, not just the optional validation/test placeholders. Replace this with explicit optional slots, or at minimum narrow the suppression to the smallest possible block.Verify the scope and missing type constraints with:
#!/bin/bash set -euo pipefail file="src/AiModelBuilder.cs" printf '%s\n' '-- generic declaration (no class/struct constraints) --' sed -n '139,145p' "$file" printf '\n%s\n' '-- pragma disable site --' sed -n '2040,2048p' "$file" printf '\n%s\n' '-- pragma restore site --' sed -n '2840,2842p' "$file"Also applies to: 2841-2841
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/AiModelBuilder.cs`:
- Around line 1816-1820: The shuffle logic currently only checks
_autoMLOptions?.TaskFamilyOverride so manual/time-series builds (or
ConfigureAutoML flows where _autoMLOptions is null) still get shuffle:true; add
a small helper (e.g., IsTimeSeriesTask or ComputeShuffleBeforeSplit) that
determines whether the current training target is a time-series task by
inspecting the actual task/model being trained (not just _autoMLOptions), then
use that helper to compute shuffleBeforeSplit and pass it into
DataSplitter.Split<T,TInput,TOutput> in both places (the block around
DataSplitter.Split and the other occurrence at the later branch) so
chronological splits are consistently unshuffled for time-series models.
In `@src/Preprocessing/PreprocessingRegistry.cs`:
- Around line 10-16: The PreprocessingRegistry singleton still exposes and
mutates a process-global pipeline via Current, Transform(), and FitTransform(),
causing cross-build contamination; update callers so the registry is strictly
compatibility-only and no longer holds or returns the active pipeline: remove or
stop using the global _preprocessingPipeline in PreprocessingRegistry by
changing Current, Transform, and FitTransform to no-op or to throw a clear
compatibility-only exception, and update all call sites to take the fitted
pipeline explicitly from AiModelBuilder (use its instance-level
_preprocessingPipeline) or from PreprocessingInfo passed in the result; ensure
any code that previously relied on PreprocessingRegistry.Current or its
Transform/FitTransform now receives the pipeline via
AiModelBuilder/PreprocessingInfo to prevent any shared state across parallel
builds.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9448b4cf-fb40-480b-9739-3a60449ac1e8
📒 Files selected for processing (2)
src/AiModelBuilder.cssrc/Preprocessing/PreprocessingRegistry.cs
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
When _dataPreparationPipeline runs (resampling/outlier removal) it modifies XTrain/yTrain. The else branch (no preprocessing) was incorrectly using preparedX/preparedY (pre-preparation) instead of XTrain/yTrain, losing training-only preparation results for downstream logic. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/AiModelBuilder.cs (2)
2040-2047:⚠️ Potential issue | 🟠 MajorCS8600/CS8604 are still suppressed for almost the entire method.
TInput/TOutputare unconstrained here, and later paths in this class handle reference-shaped datasets (TInput[],IEnumerable<TInput>), sodefaultis not guaranteed non-null. Keeping the pragma active until Line 2841 hides unrelated nullability warnings across the rest ofBuildSupervisedInternalAsync; make these slots explicitly nullable and restore immediately after the declarations.Also applies to: 2841-2841
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/AiModelBuilder.cs` around lines 2040 - 2047, The pragma suppressions around BuildSupervisedInternalAsync are too broad; change the four locals XVal, yVal, XTest, and yTest to be explicitly nullable (e.g., TInput? XVal, TOutput? yVal, etc.) so their types reflect possible null for unconstrained generics, and then restore/remove the `#pragma` warning disable CS8600, CS8604 immediately after those declarations so the suppression no longer spans the rest of the method; keep all other logic unchanged but ensure subsequent code handles the nullable locals or uses null-forgiving only where safe.
1816-1820:⚠️ Potential issue | 🟠 MajorNon-AutoML time-series builds still shuffle.
Both split sites derive
shuffleBeforeSplitonly from_autoMLOptions?.TaskFamilyOverride. A caller that configures a forecasting or time-series anomaly model directly viaConfigureModel()never sets that flag, so chronological data can still be randomized and leak future points into training.Also applies to: 2114-2117
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/AiModelBuilder.cs` around lines 1816 - 1820, The shuffle flag is only keyed off _autoMLOptions?.TaskFamilyOverride so direct ConfigureModel() time-series builds still shuffle and leak future data; update the shuffleBeforeSplit condition to also check the explicit model configuration set by ConfigureModel (e.g., the field/property that records the configured task family such as _modelOptions or _configuredTaskFamily) and return false when that configured task family is TimeSeriesForecasting or TimeSeriesAnomalyDetection; apply the same change at both locations that compute shuffleBeforeSplit (the site feeding DataSplitter.Split<T, TInput, TOutput> and the other occurrence noted) so chronological splits are preserved for any time-series build whether set via AutoML or ConfigureModel().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/AiModelBuilder.cs`:
- Around line 2051-2072: The federated branch in AiModelBuilder (the block using
_dataPreparationPipeline.FitResample / FitResampleTensor and
_preprocessingPipeline.FitTransform that writes preparedX/preparedY ->
preprocessedX) can silently break per-client row ordering; update this code to
either (a) apply FitResample/FitTransform per-client using the original
clientRanges from IFederatedClientDataLoader and rebuild clientRanges from the
per-client outputs, or (b) detect and reject pipelines that are not guaranteed
order-preserving by adding a validation step after FitResample/FitTransform
which verifies per-client sample counts and exact row ordering (e.g. a stable
index/sequence check) and throws when violated; reference
_dataPreparationPipeline, FitResample, FitResampleTensor,
_preprocessingPipeline, FitTransform, preparedX/preparedY, preprocessedX,
clientRanges, and PredictionModelBuilder.BuildSupervisedInternalAsync when
implementing the per-client processing or the validation rejection path.
- Around line 390-393: The code assigns the shared static
PreprocessingRegistry<T, TInput>.Current from AiModelBuilder (the
_preprocessingPipeline in ConfigurePreprocessing), which allows races and
pipeline leakage across models; to fix, stop writing the global: remove or guard
the assignment to PreprocessingRegistry<T, TInput>.Current and instead propagate
_preprocessingPipeline via instance-level injection into consumers (e.g., change
DocumentNeuralNetworkBase to accept an IPreprocessingPipeline instance or pass
_preprocessingPipeline into its inference methods), or if modifying consumers is
not possible, use a tightly-scoped set/restore pattern with a lock around any
inference that must read the static (save old = Current; Current =
_preprocessingPipeline; run inference; Current = old) to ensure no cross-model
swaps. Ensure all references to ConfigurePreprocessing and
DocumentNeuralNetworkBase are updated to use the injected/parameterized pipeline
rather than relying on the static Current.
---
Duplicate comments:
In `@src/AiModelBuilder.cs`:
- Around line 2040-2047: The pragma suppressions around
BuildSupervisedInternalAsync are too broad; change the four locals XVal, yVal,
XTest, and yTest to be explicitly nullable (e.g., TInput? XVal, TOutput? yVal,
etc.) so their types reflect possible null for unconstrained generics, and then
restore/remove the `#pragma` warning disable CS8600, CS8604 immediately after
those declarations so the suppression no longer spans the rest of the method;
keep all other logic unchanged but ensure subsequent code handles the nullable
locals or uses null-forgiving only where safe.
- Around line 1816-1820: The shuffle flag is only keyed off
_autoMLOptions?.TaskFamilyOverride so direct ConfigureModel() time-series builds
still shuffle and leak future data; update the shuffleBeforeSplit condition to
also check the explicit model configuration set by ConfigureModel (e.g., the
field/property that records the configured task family such as _modelOptions or
_configuredTaskFamily) and return false when that configured task family is
TimeSeriesForecasting or TimeSeriesAnomalyDetection; apply the same change at
both locations that compute shuffleBeforeSplit (the site feeding
DataSplitter.Split<T, TInput, TOutput> and the other occurrence noted) so
chronological splits are preserved for any time-series build whether set via
AutoML or ConfigureModel().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 39eeefa7-8e64-4db6-92f4-59909a504895
📒 Files selected for processing (1)
src/AiModelBuilder.cs
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
67726be to
02e459c
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…alues Replace clustered bimodal data with linearly-spaced unique values (0, 10, 20, ..., 990) to guarantee any 70/30 shuffle split produces significantly different scaler statistics from the full dataset, eliminating flaky test failures. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ing field! patterns Replaces default! with proper initialization (NumOps.Zero, throw, or nullable types), null! with nullable parameters or removed dead code, and field! with extracted local variables with null-coalescing throw expressions. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Remove static PreprocessingRegistry writes from ConfigurePreprocessing methods, aligning with master's instance-based pipeline approach. Take master's deprecation docs for PreprocessingRegistry. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Deployment failed with the following error: Learn More: https://vercel.com/franklins-projects-02a0b5a0?upgradeToPro=build-rate-limit |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 19 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Fix exception text to reference ConfigureModel() instead of WithModel() - Remove nullable T? from TOTEM._lastCommitmentLoss field, initialize with NumOps.Zero in both constructors to avoid ambiguous nullability - Fix incorrect comment about Matrix<T>/Vector<T> being value types Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace EqualityComparer<T>.Default.Equals with _numOps.Equals to fix CS8604 null reference error on net471 and follow codebase patterns. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Deployment failed with the following error: Learn More: https://vercel.com/franklins-projects-02a0b5a0?upgradeToPro=build-rate-limit |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 19 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Narrow CS8600/CS8604 pragma scopes to specific declaration and call sites instead of spanning 800+ lines - Clarify StatisticsHelper comment about default(T) vs NumOps.Zero - Snapshot original values before Transform in test to prevent in-place mutation from invalidating the comparison Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Summary
StandardScaler) was fitted on the ENTIRE dataset before the train/test split, causing data leakage — test/validation statistics (mean, std dev) leaked into the training pipelineBuildSupervisedInternalAsync()to split data FIRST, thenFitTransformonly on training data, andTransform(notFitTransform) validation/test dataWhat Changed
src/AiModelBuilder.cs—BuildSupervisedInternalAsync():DataSplitter.Split()call BEFORE preprocessing pipeline fitting_preprocessingPipeline.FitTransform(XTrain)— learns statistics from training set only_preprocessingPipeline.Transform(XVal)/Transform(XTest)— applies training-fitted statisticsusePartitionedFederatedData): unchanged, stillFitTransformon all data (correct since all is training)tests/.../DataLeakagePreventionIntegrationTests.cs— 5 new integration tests:AiModelResultcan transform new dataTest plan
DataLeakagePreventionIntegrationTestspassLinearRegressionIntegrationTestspass (no regressions)dotnet build src/AiDotNet.csproj -f net10.0succeedsdotnet build tests/AiDotNet.Tests/AiDotNetTests.csproj -f net10.0succeeds🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
New Features
Documentation
Tests