Skip to content

Commit a955334

Browse files
authored
fix: fuse StandardScaler::transform to single-allocation pass (#450)
* fix: fuse StandardScaler::transform to single-allocation pass Replace per-column take_column/sub_scalar/div_scalar/build_matrix_from_columns pattern with a single M::fill + element-wise (x - mean) / std 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 per issue #449). Remove now-dead build_matrix_from_columns helper and its test. Add comprehensive test covering all parameter combinations (with_mean, with_std, zero-variance columns, column-count mismatch) verified against numpy. * fix(xgboost): harden empty-data guard per #448 review - Extend guard to also reject zero-feature matrices (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 - Add comment on scaffold matrix in test_fit_on_empty_data_returns_error * fix(tree): guard all tree/ensemble fit methods against empty data Add n_samples == 0 || n_features == 0 guards to: - DecisionTreeClassifier::fit - BaseTreeRegressor::fit - RandomForestClassifier::fit - BaseForestRegressor::fit All return FailedError::ParametersError with message: 'Training data must contain at least one sample and one feature.' Previously these could panic (divide-by-zero, empty-range) or produce undefined models when called with zero-row or zero-column matrices. Regression tests added for each guarded path. * fix(preprocessing): row-major loop order in StandardScaler::transform Swap fused transform loop from column-outer/row-inner to row-outer/ col-inner with pre-computed (mean, std) Vec. DenseMatrix uses row-major layout, so the previous ordering caused strided reads and writes on large matrices. Also add zero-features regression tests for DecisionTreeClassifier and BaseForestRegressor to match BaseTreeRegressor coverage. Audit: ExtraTreesRegressor and RandomForestRegressor both delegate to BaseForestRegressor::fit which already has the guard — no changes needed. Addresses review feedback from Mec-iS on PR #450.
1 parent ad63f09 commit a955334

7 files changed

Lines changed: 345 additions & 60 deletions

File tree

Cargo.toml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@
22
name = "smartcore"
33
description = "Machine Learning in Rust."
44
homepage = "https://smartcorelib.github.io/"
5-
version = "0.6.12"
5+
version = "0.6.13"
66
authors = ["smartcore Developers"]
77
edition = "2024"
88
rust-version = "1.85"

src/ensemble/base_forest_regressor.rs

Lines changed: 61 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -89,6 +89,12 @@ impl<TX: Number + FloatNumber + PartialOrd, TY: Number, X: Array2<TX>, Y: Array1
8989
if n_rows != y.shape() {
9090
return Err(Failed::fit("Number of rows in X should = len(y)"));
9191
}
92+
if n_rows == 0 || num_attributes == 0 {
93+
return Err(Failed::because(
94+
FailedError::ParametersError,
95+
"Training data must contain at least one sample and one feature.",
96+
));
97+
}
9298

9399
let mtry = parameters
94100
.m
@@ -223,6 +229,7 @@ impl<TX: Number + FloatNumber + PartialOrd, TY: Number, X: Array2<TX>, Y: Array1
223229
#[cfg(test)]
224230
mod tests {
225231
use super::*;
232+
use crate::linalg::basic::arrays::Array;
226233
use crate::linalg::basic::matrix::DenseMatrix;
227234

228235
#[test]
@@ -244,4 +251,58 @@ mod tests {
244251
assert_eq!(regressor.trees.unwrap().len(), 5);
245252
assert!(regressor.samples.is_some());
246253
}
254+
255+
#[test]
256+
fn test_fit_on_empty_data_returns_error() {
257+
// 2 rows x 2 features — values are arbitrary; only the empty-row case is under test
258+
let full = DenseMatrix::from_2d_vec(&vec![vec![1.0, 2.0], vec![3.0, 4.0]]).unwrap();
259+
let empty = full.take(&[] as &[usize], 0);
260+
assert_eq!(empty.shape(), (0, 2));
261+
262+
let y: Vec<f64> = vec![];
263+
let result = BaseForestRegressor::fit(
264+
&empty,
265+
&y,
266+
BaseForestRegressorParameters {
267+
max_depth: None,
268+
min_samples_leaf: 1,
269+
min_samples_split: 2,
270+
n_trees: 5,
271+
m: None,
272+
keep_samples: false,
273+
seed: 0,
274+
bootstrap: true,
275+
splitter: crate::tree::base_tree_regressor::Splitter::Best,
276+
},
277+
);
278+
assert!(result.is_err());
279+
assert_eq!(result.err().unwrap().error(), FailedError::ParametersError);
280+
}
281+
282+
#[test]
283+
fn test_fit_on_zero_features_returns_error() {
284+
// 2 rows x 2 features — values are arbitrary; only the zero-feature case is under test
285+
let full = DenseMatrix::from_2d_vec(&vec![vec![1.0, 2.0], vec![3.0, 4.0]]).unwrap();
286+
let no_features = full.take(&[] as &[usize], 1);
287+
assert_eq!(no_features.shape(), (2, 0));
288+
289+
let y: Vec<f64> = vec![1.0, 2.0];
290+
let result = BaseForestRegressor::fit(
291+
&no_features,
292+
&y,
293+
BaseForestRegressorParameters {
294+
max_depth: None,
295+
min_samples_leaf: 1,
296+
min_samples_split: 2,
297+
n_trees: 5,
298+
m: None,
299+
keep_samples: false,
300+
seed: 0,
301+
bootstrap: true,
302+
splitter: crate::tree::base_tree_regressor::Splitter::Best,
303+
},
304+
);
305+
assert!(result.is_err());
306+
assert_eq!(result.err().unwrap().error(), FailedError::ParametersError);
307+
}
247308
}

src/ensemble/random_forest_classifier.rs

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -461,6 +461,12 @@ impl<TX: FloatNumber + PartialOrd, TY: Number + Ord, X: Array2<TX>, Y: Array1<TY
461461
if x_nrows != y_ncols {
462462
return Err(Failed::fit("Number of rows in X should = len(y)"));
463463
}
464+
if x_nrows == 0 || num_attributes == 0 {
465+
return Err(Failed::because(
466+
FailedError::ParametersError,
467+
"Training data must contain at least one sample and one feature.",
468+
));
469+
}
464470

465471
let mut yi: Vec<usize> = vec![0; y_ncols];
466472
let classes = y.unique();
@@ -619,6 +625,7 @@ impl<TX: FloatNumber + PartialOrd, TY: Number + Ord, X: Array2<TX>, Y: Array1<TY
619625
#[cfg(test)]
620626
mod tests {
621627
use super::*;
628+
use crate::linalg::basic::arrays::Array;
622629
use crate::linalg::basic::matrix::DenseMatrix;
623630
use crate::metrics::*;
624631

@@ -779,6 +786,32 @@ mod tests {
779786
assert!(fail.is_err());
780787
}
781788

789+
#[test]
790+
fn test_fit_on_empty_data_returns_error() {
791+
// 2 rows x 2 features — values are arbitrary; only the empty-row case is under test
792+
let full = DenseMatrix::from_2d_vec(&vec![vec![1.0_f64, 1.0], vec![0.0, 1.0]]).unwrap();
793+
let empty = full.take(&[] as &[usize], 0);
794+
assert_eq!(empty.shape(), (0, 2));
795+
796+
let y: Vec<u32> = vec![];
797+
let result = RandomForestClassifier::fit(
798+
&empty,
799+
&y,
800+
RandomForestClassifierParameters {
801+
criterion: SplitCriterion::Gini,
802+
max_depth: None,
803+
min_samples_leaf: 1,
804+
min_samples_split: 2,
805+
n_trees: 10,
806+
m: None,
807+
keep_samples: false,
808+
seed: 0,
809+
},
810+
);
811+
assert!(result.is_err());
812+
assert_eq!(result.err().unwrap().error(), FailedError::ParametersError);
813+
}
814+
782815
#[cfg_attr(
783816
all(target_arch = "wasm32", not(target_os = "wasi")),
784817
wasm_bindgen_test::wasm_bindgen_test

src/preprocessing/numerical.rs

Lines changed: 141 additions & 54 deletions
Original file line numberDiff line numberDiff line change
@@ -138,76 +138,41 @@ impl<T: Number + RealNumber, M: Array2<T>> UnsupervisedEstimator<M, StandardScal
138138
/// standard deviation to one.
139139
impl<T: Number + RealNumber, M: Array2<T>> Transformer<M> for StandardScaler<T> {
140140
fn transform(&self, x: &M) -> Result<M, Failed> {
141-
let (_, n_cols) = x.shape();
142-
if n_cols != self.means.len() {
141+
let (nrows, ncols) = x.shape();
142+
if ncols != self.means.len() {
143143
return Err(Failed::because(
144144
FailedError::TransformFailed,
145145
&format!(
146146
"Expected {} columns, but got {} columns instead.",
147147
self.means.len(),
148-
n_cols,
148+
ncols,
149149
),
150150
));
151151
}
152152

153-
Ok(build_matrix_from_columns(
154-
self.means
155-
.iter()
156-
.zip(self.stds.iter())
157-
.enumerate()
158-
.map(|(column_index, (column_mean, column_std))| {
159-
x.take_column(column_index)
160-
.sub_scalar(T::from(self.adjust_column_mean(*column_mean)).unwrap())
161-
.div_scalar(T::from(self.adjust_column_std(*column_std)).unwrap())
162-
})
163-
.collect(),
164-
)
165-
.unwrap())
166-
}
167-
}
168-
169-
/// From a collection of matrices, that contain columns, construct
170-
/// a matrix by stacking the columns horizontally.
171-
fn build_matrix_from_columns<T, M>(columns: Vec<M>) -> Option<M>
172-
where
173-
T: Number + RealNumber,
174-
M: Array2<T>,
175-
{
176-
columns.first().cloned().map(|output_matrix| {
177-
columns
178-
.iter()
179-
.skip(1)
180-
.fold(output_matrix, |current_matrix, new_colum| {
181-
current_matrix.h_stack(new_colum)
153+
let mut output = M::fill(nrows, ncols, T::zero());
154+
let col_params: Vec<(T, T)> = (0..ncols)
155+
.map(|j| {
156+
let mean = T::from(self.adjust_column_mean(self.means[j])).unwrap();
157+
let std = T::from(self.adjust_column_std(self.stds[j])).unwrap();
158+
(mean, std)
182159
})
183-
})
160+
.collect();
161+
for i in 0..nrows {
162+
for (j, &(mean, std)) in col_params.iter().enumerate() {
163+
let val = *x.get((i, j));
164+
output.set((i, j), (val - mean) / std);
165+
}
166+
}
167+
Ok(output)
168+
}
184169
}
185170

186171
#[cfg(test)]
187172
mod tests {
188173

189174
mod helper_functionality {
190-
use super::super::{build_matrix_from_columns, ensure_std_valid};
191-
use crate::linalg::basic::matrix::DenseMatrix;
192-
193-
#[test]
194-
fn combine_three_columns() {
195-
assert_eq!(
196-
build_matrix_from_columns(vec![
197-
DenseMatrix::from_2d_vec(&vec![vec![1.0], vec![1.0], vec![1.0],]).unwrap(),
198-
DenseMatrix::from_2d_vec(&vec![vec![2.0], vec![2.0], vec![2.0],]).unwrap(),
199-
DenseMatrix::from_2d_vec(&vec![vec![3.0], vec![3.0], vec![3.0],]).unwrap()
200-
]),
201-
Some(
202-
DenseMatrix::from_2d_vec(&vec![
203-
vec![1.0, 2.0, 3.0],
204-
vec![1.0, 2.0, 3.0],
205-
vec![1.0, 2.0, 3.0]
206-
])
207-
.unwrap()
208-
)
209-
)
210-
}
175+
use super::super::ensure_std_valid;
211176

212177
#[test]
213178
fn negative_value_should_be_replace_with_minimal_positive_value() {
@@ -426,6 +391,128 @@ mod tests {
426391
)
427392
}
428393

394+
/// Verify transform correctness across all parameter combinations.
395+
#[test]
396+
fn transform_all_parameter_combinations() {
397+
let data = DenseMatrix::from_2d_vec(&vec![
398+
vec![1.0, 10.0, 100.0],
399+
vec![2.0, 20.0, 200.0],
400+
vec![3.0, 30.0, 300.0],
401+
vec![4.0, 40.0, 400.0],
402+
])
403+
.unwrap();
404+
405+
// Default: with_mean=true, with_std=true
406+
// std = population std: sqrt(mean of squared deviations)
407+
// For [1,2,3,4]: mean=2.5, pop_std = sqrt(5/4) ≈ 1.1180339887
408+
let scaler = StandardScaler::fit(&data, StandardScalerParameters::default()).unwrap();
409+
let result = scaler.transform(&data).unwrap();
410+
let expected = DenseMatrix::from_2d_vec(&vec![
411+
vec![
412+
-1.3416407864998738,
413+
-1.3416407864998738,
414+
-1.3416407864998738,
415+
],
416+
vec![
417+
-0.4472135954999579,
418+
-0.4472135954999579,
419+
-0.4472135954999579,
420+
],
421+
vec![0.4472135954999579, 0.4472135954999579, 0.4472135954999579],
422+
vec![1.3416407864998738, 1.3416407864998738, 1.3416407864998738],
423+
])
424+
.unwrap();
425+
assert!(
426+
result.approximate_eq(&expected, 1e-10),
427+
"Default transform failed:\n{result}\nexpected:\n{expected}"
428+
);
429+
430+
// with_mean=true, with_std=false
431+
let scaler = StandardScaler::fit(
432+
&data,
433+
StandardScalerParameters {
434+
with_mean: true,
435+
with_std: false,
436+
},
437+
)
438+
.unwrap();
439+
let result = scaler.transform(&data).unwrap();
440+
let expected = DenseMatrix::from_2d_vec(&vec![
441+
vec![-1.5, -15.0, -150.0],
442+
vec![-0.5, -5.0, -50.0],
443+
vec![0.5, 5.0, 50.0],
444+
vec![1.5, 15.0, 150.0],
445+
])
446+
.unwrap();
447+
assert!(
448+
result.approximate_eq(&expected, 1e-10),
449+
"with_mean=true, with_std=false transform failed:\n{result}\nexpected:\n{expected}"
450+
);
451+
452+
// with_mean=false, with_std=true: (x - 0) / std = x / std
453+
let scaler = StandardScaler::fit(
454+
&data,
455+
StandardScalerParameters {
456+
with_mean: false,
457+
with_std: true,
458+
},
459+
)
460+
.unwrap();
461+
let result = scaler.transform(&data).unwrap();
462+
let expected = DenseMatrix::from_2d_vec(&vec![
463+
vec![0.8944271909999159, 0.8944271909999159, 0.8944271909999159],
464+
vec![1.7888543819998317, 1.7888543819998317, 1.7888543819998317],
465+
vec![2.6832815729997477, 2.6832815729997477, 2.6832815729997477],
466+
vec![3.5777087639996634, 3.5777087639996634, 3.5777087639996634],
467+
])
468+
.unwrap();
469+
assert!(
470+
result.approximate_eq(&expected, 1e-10),
471+
"with_mean=false, with_std=true transform failed:\n{result}\nexpected:\n{expected}"
472+
);
473+
474+
// with_mean=false, with_std=false (passthrough)
475+
let scaler = StandardScaler::fit(
476+
&data,
477+
StandardScalerParameters {
478+
with_mean: false,
479+
with_std: false,
480+
},
481+
)
482+
.unwrap();
483+
let result = scaler.transform(&data).unwrap();
484+
assert!(
485+
result.approximate_eq(&data, 1e-10),
486+
"with_mean=false, with_std=false should return data unchanged:\n{result}\nexpected:\n{data}"
487+
);
488+
489+
// Zero-variance column mixed with normal columns
490+
let mixed = DenseMatrix::from_2d_vec(&vec![
491+
vec![1.0, 5.0],
492+
vec![2.0, 5.0],
493+
vec![3.0, 5.0],
494+
vec![4.0, 5.0],
495+
])
496+
.unwrap();
497+
let scaler = StandardScaler::fit(&mixed, StandardScalerParameters::default()).unwrap();
498+
let result = scaler.transform(&mixed).unwrap();
499+
let expected = DenseMatrix::from_2d_vec(&vec![
500+
vec![-1.3416407864998738, 0.0],
501+
vec![-0.4472135954999579, 0.0],
502+
vec![0.4472135954999579, 0.0],
503+
vec![1.3416407864998738, 0.0],
504+
])
505+
.unwrap();
506+
assert!(
507+
result.approximate_eq(&expected, 1e-10),
508+
"Zero-variance mixed column transform failed:\n{result}\nexpected:\n{expected}"
509+
);
510+
511+
// Column count mismatch returns error: scaler expects 2 cols, data has 1
512+
let narrow = DenseMatrix::from_2d_vec(&vec![vec![1.0]]).unwrap();
513+
assert!(scaler.transform(&narrow).is_err());
514+
}
515+
429516
/// Same as `fit_for_random_values` test, but using a `StandardScaler` that has been
430517
/// serialized and deserialized.
431518
#[cfg_attr(

0 commit comments

Comments
 (0)