Skip to content

Match sklearn sample weights - #478

Merged
Mec-iS merged 3 commits into
smartcorelib:mainfrom
slievens:match-sklearn-sample-weights
Oct 5, 2026
Merged

Mec-iS merged 3 commits into
smartcorelib:mainfrom
slievens:match-sklearn-sample-weights

Conversation

@slievens

@slievens slievens commented Oct 5, 2026

Copy link
Copy Markdown
Contributor

Fixes #477

Checklist

  • [x ] My branch is up-to-date with main branch.
  • Everything works and tested on latest stable Rust.
  • Coverage and Linting have been applied

Current behaviour

At the moment, smartcore uses the sample weights twice:

  • once for bootstrapping
  • during the fitting of the tree.

New expected behaviour

Sample weights in the same way as in sklearn, i.e. during bootstrapping OR during the fitting of the tree

Change logs

In base_forest_regressor the sample weights are passed as None when bootstrapping is used, otherwise the sample weights are passed in as is.

Nothing in the public API changed.

Added

Additional tests to try to verify that the implementation matches sklearn's behavior
in extra_trees_regressor and random_forest_regressor.

Match the behavior for sklearn on sample weights.
- when bootstrap is true, sample weights are only used during the bootstrap phase
- when bootstrap is false, then sample weights are passed down to individual trees.

Added tests for this:

Compare RandomForestRegressor and ExtraTreesRegressor predictions
(train, probe, OOB) with reference values from sklearn 1.9.1.
Each reference value is the mean of 10 sklearn runs. smartcore and
numpy use different RNGs, so the tests use a tolerance of about
4 x the standard deviation of one sklearn run.

For RandomForestRegressor: cover max_features = all and 2, with and without sample weights.

For ExtraTreesRegressor: cover max_features = all, with and without sample weights.
@slievens
slievens requested a review from Mec-iS as a code owner October 5, 2026 07:03
@Mec-iS

Mec-iS commented Oct 5, 2026

Copy link
Copy Markdown
Collaborator

Thanks for the fix and the thorough parity tests. Review notes (based on the diff and CI results; some points are things to verify rather than confirmed defects):

Correctness

  1. Please confirm the bootstrap sampler actually draws proportionally to the weights. If it samples uniformly, passing None to fit_weak_learner means weights are now ignored entirely. sklearn multiplies the weights by the bootstrap counts. An extreme-weights test (e.g. 1000 on one row) would catch this; the current weights of 1-4 are too gentle.
  2. Please confirm parameters.bootstrap is the same flag that decides whether samples is a real bootstrap draw in this loop, and how fit_weak_learner treats samples when weights are None.
  3. For ExtraTreesRegressor, sklearn defaults to bootstrap=False. Please confirm the smartcore default is also false so this branch does not silently change its behaviour.
  4. This changes results for anyone using fit_with_weights with bootstrap. Please add a CHANGELOG entry and a doc note.
  5. Please check RandomForestClassifier and any other callers of fit_weak_learner for the same double-weighting, so the library stays consistent.

CI

  • check_features (--features ndarray-bindings) and coverage are failing. Could you check whether they are related to this PR (e.g. a timeout from 2000-tree tests under coverage instrumentation) or pre-existing?

Tests

  • Each parity test fits 2000 trees; consider 300-500 trees with wider tolerances, or #[ignore] for the heaviest ones.
  • The OOB tolerance of 0.20 is loose relative to the target range (about +-1.5). Please verify the weighted tests fail on main (double-weighting) and pass with this change.
  • The fixtures and helpers are duplicated between the two test modules; a shared #[cfg(test)] module would help.
  • (0..40).into_iter() is redundant and may trigger clippy useless_conversion.
  • The Python snippet in the comments is not runnable as written (N_SEEDS, N_TREES, rust_vec are undefined). Please commit it as a script, e.g. scripts/gen_sklearn_parity.py.
  • Add a comment or assertion that m=4 means all features in the Extra Trees test.

@Mec-iS

Mec-iS commented Oct 5, 2026

Copy link
Copy Markdown
Collaborator

please check the failure in check_features

@codecov

codecov Bot commented Oct 5, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 75.00000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 63.63%. Comparing base (9eaae9e) to head (f3e7271).
⚠️ Report is 195 commits behind head on main.

Files with missing lines Patch % Lines
src/ensemble/base_forest_regressor.rs 75.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##             main     #478       +/-   ##
===========================================
+ Coverage   43.97%   63.63%   +19.65%     
===========================================
  Files          85       96       +11     
  Lines        7281     8522     +1241     
===========================================
+ Hits         3202     5423     +2221     
+ Misses       4079     3099      -980     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@Mec-iS
Mec-iS merged commit 696e199 into smartcorelib:main Oct 5, 2026
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

sample weights are double counted for random forest regressor

2 participants