Match sklearn random forest feature sampling - #8239
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughRefactors feature sampling to a deterministic Thrust-based device implementation, renames sampled-column buffer to ChangesFeature Sampling and Split Computation Refactor
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
🚥 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)
Warning Review ran into problems🔥 ProblemsStopped waiting for pipeline failures after 30000ms. One of your pipelines takes longer than our 30000ms fetch window to run, so review may not consider pipeline-failure results for inline comments if any failures occurred after the fetch window. Increase the timeout if you want to wait longer or run a Comment |
There was a problem hiding this comment.
Pull request overview
This PR updates cuML’s RandomForest feature sampling to match scikit-learn’s behavior: when a node’s initially sampled max_features subset yields no valid split, training now retries with additional feature slices (up to exhausting all features) instead of prematurely turning the node into a leaf.
Changes:
- Introduces deterministic per-node feature ordering and samples features in bounded “rounds” (slices of
max_features) until a valid split is found or all features are tried. - Simplifies/standardizes seed hashing via new
fnv1a32_hash(...)helpers and reuses it for quantile column seeding. - Adds a regression test covering the “single informative feature + max_features=1” failure mode.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
python/cuml/tests/test_random_forest.py |
Adds a regression test ensuring RF does not stop early when the first sampled feature(s) can’t split. |
cpp/src/decisiontree/batched-levelalgo/random_utils.cuh |
Adds reusable FNV-1a hash combine helpers for deterministic multi-value seeding. |
cpp/src/decisiontree/batched-levelalgo/quantiles.cuh |
Refactors per-column quantile sampling seed derivation to use the new hash helper. |
cpp/src/decisiontree/batched-levelalgo/kernels/builder_kernels.cuh |
Replaces prior feature sampling kernels with a deterministic per-node shuffled-feature sampler and updates kernel interface to consume column_samples. |
cpp/src/decisiontree/batched-levelalgo/kernels/builder_kernels_impl.cuh |
Updates split computation to always index features via column_samples (including the no-subsampling case). |
cpp/src/decisiontree/batched-levelalgo/builder.cuh |
Implements the retry loop over feature slices per node, compacts retry work, and applies the final per-node best split once. |
💡 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: 1
🧹 Nitpick comments (1)
python/cuml/tests/test_random_forest.py (1)
1035-1055: ⚡ Quick winHIGH: Consider adding scikit-learn comparison to verify behavior parity.
The test validates that cuML's retry logic achieves perfect accuracy on this edge case (single informative feature with
max_features=1), but doesn't confirm that this matches scikit-learn's behavior. Since the PR objective is to match sklearn's feature sampling, comparing against sklearn would provide stronger evidence of parity.🔍 Suggested enhancement to add sklearn baseline
def test_rf_feature_sampling_retries_until_valid_split(): + """ + Test that feature sampling retries until a valid split is found. + + When max_features=1 and only one feature is informative, the retry logic + should eventually sample that feature, matching sklearn's behavior. + """ n_samples = 128 n_features = 32 X = np.zeros((n_samples, n_features), dtype=np.float32) y = np.zeros(n_samples, dtype=np.int32) y[n_samples // 2 :] = 1 X[:, 0] = y for random_state in range(8): clf = curfc( n_estimators=1, bootstrap=False, max_depth=None, max_features=1, n_bins=4, n_streams=1, random_state=random_state, ) clf.fit(X, y) - assert accuracy_score(y, clf.predict(X)) == 1.0 + cuml_acc = accuracy_score(y, clf.predict(X)) + assert cuml_acc == 1.0 + + # Verify sklearn achieves the same result + sk_clf = skrfc( + n_estimators=1, + bootstrap=False, + max_depth=None, + max_features=1, + random_state=random_state, + ) + sk_clf.fit(X, y) + sk_acc = accuracy_score(y, sk_clf.predict(X)) + assert sk_acc == 1.0, "sklearn baseline should also achieve perfect accuracy" + assert cuml_acc == sk_acc, "cuML should match sklearn behavior"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@python/cuml/tests/test_random_forest.py` around lines 1035 - 1055, The test test_rf_feature_sampling_retries_until_valid_split currently asserts perfect accuracy from cuML (curfc) but lacks a scikit-learn baseline; add a comparison using sklearn.ensemble.RandomForestClassifier with matching parameters (n_estimators=1, bootstrap=False, max_depth=None, max_features=1, random_state set in the loop) to fit the same X,y and assert that sklearn's accuracy_score(y, skl_clf.predict(X)) equals cuML's accuracy (or both equal 1.0) for each random_state, so the test verifies parity between curfc and sklearn behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cpp/src/decisiontree/batched-levelalgo/kernels/builder_kernels.cuh`:
- Around line 90-100: The branch that returns early when k == n writes the
identity 0..n-1 instead of the per-node permutation; move the per-node RNG and
shuffle construction (rng_seed = fnv1a32_hash(seed, treeid, nodeid) and
cuda::shuffle_iterator<IdxT> shuffled_features(...)) out of the else-path so
they are available for the k==n case, and replace the direct write of
column_index with column_samples[sample_idx] = shuffled_features[column_index]
(using work_items[node_idx].idx, seed, treeid, sample_offset, and column_index
as before) so every node uses the same node-specific permutation.
---
Nitpick comments:
In `@python/cuml/tests/test_random_forest.py`:
- Around line 1035-1055: The test
test_rf_feature_sampling_retries_until_valid_split currently asserts perfect
accuracy from cuML (curfc) but lacks a scikit-learn baseline; add a comparison
using sklearn.ensemble.RandomForestClassifier with matching parameters
(n_estimators=1, bootstrap=False, max_depth=None, max_features=1, random_state
set in the loop) to fit the same X,y and assert that sklearn's accuracy_score(y,
skl_clf.predict(X)) equals cuML's accuracy (or both equal 1.0) for each
random_state, so the test verifies parity between curfc and sklearn behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: de5d1b29-ea36-4d0c-82cb-f721d7935a57
📒 Files selected for processing (6)
cpp/src/decisiontree/batched-levelalgo/builder.cuhcpp/src/decisiontree/batched-levelalgo/kernels/builder_kernels.cuhcpp/src/decisiontree/batched-levelalgo/kernels/builder_kernels_impl.cuhcpp/src/decisiontree/batched-levelalgo/quantiles.cuhcpp/src/decisiontree/batched-levelalgo/random_utils.cuhpython/cuml/tests/test_random_forest.py
dantegd
left a comment
dantegd
left a comment
There was a problem hiding this comment.
Just found one potential mismatch remaining with sklearn.
Additionally, the default min_impurity_decrease=0 regression looks good for the one-informative-feature case. It would be helpful to add a small regression for nonzero min_impurity_decrease too, since that is where retry semantics can diverge from sklearn.
|
@csadorf this decreases the mean gap from about 8.30 percentage points to 0.15 percentage points but there is still something statistically different. I will go a bit deeper in another PR.
|
csadorf
left a comment
csadorf
left a comment
There was a problem hiding this comment.
Thanks for turning this around so quickly after our offline conversation last week.
I ran some targeted RF benchmarks and was not able to observe a performance regression from the retry logic. The expected caveat is that pathological cases where many nodes need to walk through multiple max_features chunks will be slower, but that is expected here: the change intentionally trades extra split-search work for matching sklearn's behavior when the initially sampled features cannot produce a valid partition.
I also like the significant simplification of the sampling algorithm. The PR could potentially use some extended C++ test coverage, but I am happy to approve this already.
|
/merge |
Summary
Fix RandomForest feature sampling so tree growth does not stop just because the first sampled feature subset has no valid split.
Previously, cuML sampled
max_featurescolumns for a node, evaluated only that subset, and made the node a leaf if none of those columns produced a valid split. scikit-learn continues drawing candidate features until it finds a valid split or exhausts the feature set. This PR changes cuML to retry subsequent feature samples for the same node before giving up.Changes
max_features.column_samples, including the no-sampling case.Accuracy Evidence
The new regression test constructs a dataset where only one feature can split the labels. With
max_features=1, the old behavior could stop early and predict a single class. With retrying enabled, RF reaches the informative feature and recovers perfect accuracy on the fixture.Additional checks:
6 passedaccuracy = 1.0, predicted classes[0, 1]accuracy = 0.5Performance
Performance is neutral in a smoke benchmark:
RFClassifier<float>/blobs/0/manual_time2172 ms2150 msValidation
libcuml