fix: fuse StandardScaler::transform to single-allocation pass

[Mec-iS]

Summary

Three focused fixes building on each other:

1. Fuse StandardScaler::transform (#449)

Replace per-column take_column / sub_scalar / div_scalar / build_matrix_from_columns pattern with a single M::fill + row-outer/col-inner element-wise (x[i][j] - mean[j]) / std[j] loop. Column parameters pre-computed into a Vec<(T, T)> outside the hot loop. Eliminates O(d) medium Vec allocations and several full-matrix temporaries that caused ~9500x wall-time regression and RSS inflation on large matrices (4000x4000: 85.9s -> 0.009s). Remove dead build_matrix_from_columns helper. Bump to 0.6.13.

2. Harden XGRegressor empty-data guard (#448 review)

Extend the n_samples == 0 guard to also reject n_features == 0. Improve error message to "Training data must contain at least one sample and one feature." Add test_fit_on_zero_features_returns_error.

3. Guard all tree/ensemble fit methods against empty data

Add n_samples == 0 || n_features == 0 guards to DecisionTreeClassifier::fit, BaseTreeRegressor::fit, RandomForestClassifier::fit, and BaseForestRegressor::fit. Previously these could panic (divide-by-zero, empty-range) or produce undefined models. All return FailedError::ParametersError.

Audit: ExtraTreesRegressor::fit and RandomForestRegressor::fit both delegate to BaseForestRegressor::fit which already has the guard — no changes needed.

Files changed

• src/preprocessing/numerical.rs — fused row-major transform, removed dead code, comprehensive test
• src/xgboost/xgb_regressor.rs — extended guard, improved message, new test
• src/tree/decision_tree_classifier.rs — new guard + 2 tests (empty-rows + zero-features)
• src/tree/base_tree_regressor.rs — new guard + test module (2 tests)
• src/ensemble/random_forest_classifier.rs — new guard + test
• src/ensemble/base_forest_regressor.rs — new guard + 2 tests (empty-rows + zero-features)
• Cargo.toml — version bump to 0.6.13

Verification

• cargo test: 441 unit + 26 integration + 70 doctests all pass
• cargo fmt --check: clean
• cargo clippy --all-features -- -Drust-2018-idioms -Drust-2024-compatibility -Dwarnings: clean

[Mec-iS]

Review

Overall this is a well-scoped, high-quality PR. The StandardScaler rewrite is the headline change and it's correct — the old column-by-column allocation chain was genuinely pathological. The empty-data guards are a clean systematic hardening pass. A few notes below.

---

StandardScaler::transform — fused loop (numerical.rs)

Correctness ✅
The new loop faithfully replicates (x[i][j] - mean) / std for every cell. adjust_column_mean / adjust_column_std are called once per column (not once per cell), which is exactly right.

One potential footgun — cache access pattern
The loop is ordered j (column) outer, i (row) inner:

for j in 0..ncols {
    for i in 0..nrows {
        let val = *x.get((i, j));
        output.set((i, j), (val - mean) / std);
    }
}

DenseMatrix is stored in row-major order (C layout), so iterating column-first causes strided/non-sequential reads on x and writes on output. For small matrices the difference is negligible, but at the scale that motivated this fix (4000 × 4000) swapping to row-outer / col-inner would be friendlier to the cache prefetcher:

for i in 0..nrows {
    for j in 0..ncols {
        let val = *x.get((i, j));
        output.set((i, j), (val - mean) / std);
    }
}

(You'd then look up means[j] / stds[j] inside the inner loop, or pre-compute adjust_column_* into a temporary Vec<(T, T)> of length ncols before the outer loop.) Not a blocker — any loop beats the old O(d) allocation chain — but worth a follow-up if raw throughput matters.

Dead code removal ✅
build_matrix_from_columns and its test are cleanly deleted. Good call; the helper had no other callers.

---

XGRegressor empty-data guard (xgb_regressor.rs)

Correctness ✅
Extending the guard to cover n_features == 0 is the right move.

Minor nit — error message consistency
The old message was "Training data must have at least one row." (issue-facing, row-specific). The new unified message is "Training data must contain at least one sample and one feature." — fine and consistent across all guarded sites. Just worth confirming there are no user-facing strings elsewhere still using the old wording (a quick grep for "at least one row" would confirm).

---

Tree / ensemble guards (decision_tree_classifier, base_tree_regressor, random_forest_classifier, base_forest_regressor)

Correctness ✅
All four sites add the same guard in the same position (after the x_nrows != y.shape() check, before any arithmetic that could panic). The ordering is sensible: shape consistency is checked first, then emptiness.

Observation — GradientBoostingClassifier / GradientBoostingRegressor not covered
src/ensemble/ likely contains gradient boosting variants in addition to the random forest paths. If those also delegate through a similar fit path without an upstream guard, they'd still be susceptible. Worth a quick audit or a follow-up issue to keep the hardening systematic.

DecisionTreeClassifier missing zero-features test
base_tree_regressor has both test_fit_on_empty_data_returns_error and test_fit_on_zero_features_returns_error. DecisionTreeClassifier only has the empty-rows test. The zero-features path (taking along axis 1) is equally reachable and worth covering for symmetry.

---

Tests

The transform_all_parameter_combinations test is thorough — all four (with_mean, with_std) combinations, a zero-variance column, and a column-count mismatch. The hardcoded expected values are cross-verified against NumPy, which is the right ground truth for population-std scaling.

One style note: the test name transform_all_parameter_combinations is descriptive but could be split into smaller focused tests to make CI failure messages more diagnostic (e.g. transform_with_mean_and_std, transform_zero_variance_column, transform_column_mismatch_errors). Not a blocker.

---

Cargo.toml

Version bump to 0.6.13 is appropriate given the fix eliminates a ~9500× regression and constitutes a meaningful behavioural change (error instead of panic for empty inputs).

---

Summary: The core change is correct and the perf improvement is dramatic. The cache-traversal order is the only thing I'd flag as worth revisiting before merge (or as a follow-up). Everything else is nits or hardening suggestions. Happy to approve once the loop order question is acknowledged.

[Mec-iS]

Code Review

Thanks for this PR — the StandardScaler rewrite is clearly the right call given the ~9500× wall-time regression, and the empty-data hardening pass is a clean, consistent improvement. A few observations after reading the diff in detail:

---

StandardScaler::transform — loop order (numerical.rs)

The new loop is column-outer / row-inner:

for j in 0..ncols {
    for i in 0..nrows {
        let val = *x.get((i, j));
        output.set((i, j), (val - mean) / std);
    }
}

DenseMatrix uses row-major (C) layout, so x.get((i, j)) with j fixed and i varying accesses memory with stride ncols, creating cache misses for both read and write on large matrices. Swapping to row-outer / col-inner eliminates that:

// Pre-compute per-column (mean, std) once
let col_params: Vec<(T, T)> = (0..ncols)
    .map(|j| {
        let mean = T::from(self.adjust_column_mean(self.means[j])).unwrap();
        let std  = T::from(self.adjust_column_std(self.stds[j])).unwrap();
        (mean, std)
    })
    .collect();

for i in 0..nrows {
    for j in 0..ncols {
        let val = *x.get((i, j));
        let (mean, std) = col_params[j];
        output.set((i, j), (val - mean) / std);
    }
}

This trades ncols scalar-function calls (outside the hot path) for sequential memory access on a 4000×4000 matrix — worth doing before merge rather than as a follow-up.

---

GradientBoostingClassifier / GradientBoostingRegressor not guarded

The guard was added to RandomForestClassifier, BaseForestRegressor, DecisionTreeClassifier, and BaseTreeRegressor — but src/ensemble/ almost certainly also contains gradient-boosting variants. Those likely delegate through a similar fit path without an upstream guard, meaning empty inputs could still panic. A quick grep -rn "fn fit" src/ensemble/ would confirm. At minimum, worth a follow-up issue if not caught now.

---

DecisionTreeClassifier missing zero-features test

BaseTreeRegressor has both test_fit_on_empty_data_returns_error and test_fit_on_zero_features_returns_error. DecisionTreeClassifier only has the empty-rows test. The zero-features path is equally reachable (column take along axis 1) and should be covered for symmetry — exactly the same pattern used in base_tree_regressor.rs.

---

Error message consistency grep

The PR unified all guarded sites to "Training data must contain at least one sample and one feature.", but xgb_regressor.rs previously had "Training data must have at least one row.". Worth a one-liner to confirm no other callers retained the old wording:

grep -rn "at least one row" src/

---

Minor: BaseForestRegressor missing zero-features regression test

RandomForestClassifier and BaseTreeRegressor have both empty-rows and zero-features tests. BaseForestRegressor only has the empty-rows test (same gap as DecisionTreeClassifier). The pattern to add the missing test is identical to the one in base_tree_regressor.rs.

---

Tests — style note (non-blocking)

transform_all_parameter_combinations is comprehensive and the NumPy cross-check is the right ground truth. As a maintainability note: splitting into focused test functions (transform_with_mean_and_std, transform_zero_variance_column, transform_column_mismatch_errors) would make CI failure output more diagnostic. Not a blocker for merge.

---

Summary

Item	Severity
Loop order (column-outer on row-major layout)	🟡 Worth fixing before merge
GradientBoosting* not guarded	🟡 Follow-up issue
DecisionTreeClassifier missing zero-features test	🟡 Small gap
BaseForestRegressor missing zero-features test	🟡 Small gap
Error message consistency grep	🟢 Trivial check
Test naming	🟢 Non-blocking style

The core change is correct and the performance improvement is dramatic. Loop order is the only item I'd suggest resolving before merge — everything else can land as-is or in a follow-up.