Skip to content

Fix RandomForestClassifier sklearn child metadata - #8291

Merged
rapids-bot[bot] merged 4 commits into
NVIDIA:mainfrom
csadorf:fix/rf-classifier-export-metadata
Jun 25, 2026
Merged

Fix RandomForestClassifier sklearn child metadata#8291
rapids-bot[bot] merged 4 commits into
NVIDIA:mainfrom
csadorf:fix/rf-classifier-export-metadata

Conversation

@csadorf

@csadorf csadorf commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

Fix RandomForestClassifier.as_sklearn() so exported child estimators use sklearn-compatible encoded classes_ and n_classes_ metadata, while the exported forest keeps the original labels.

Adds coverage with non-contiguous classifier labels and compares the exported child estimator metadata against a fitted sklearn random forest.

Contributes to #8180.
Related to #8238.

@csadorf
csadorf requested a review from a team as a code owner June 24, 2026 20:43
@csadorf csadorf added bug Something isn't working non-breaking Non-breaking change labels Jun 24, 2026
@csadorf
csadorf requested a review from divyegala June 24, 2026 20:43
@github-actions github-actions Bot added the Cython / Python Cython or Python issue label Jun 24, 2026
@coderabbitai

coderabbitai Bot commented Jun 24, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 982059e5-2719-4e51-a022-b12cc40acbe3

📥 Commits

Reviewing files that changed from the base of the PR and between 6ba691f and b1323b7.

📒 Files selected for processing (2)
  • python/cuml/cuml/ensemble/randomforestclassifier.py
  • python/cuml/tests/test_sklearn_import_export.py

📝 Walkthrough

Summary by CodeRabbit

  • Bug Fixes
    • Improved scikit-learn export for random forest classifiers so the converted model now keeps class metadata consistent at both the model and estimator level.
    • Fixed per-tree class information to better match expected scikit-learn behavior after conversion.
  • Tests
    • Added and expanded checks to verify exported random forest classifiers preserve class labels and class counts correctly.

Walkthrough

The PR updates cuML RandomForestClassifier CPU export so estimator-level class metadata is rewritten consistently, and extends sklearn import/export tests to verify top-level and per-estimator class attributes during conversion.

Changes

RandomForestClassifier class metadata

Layer / File(s) Summary
CPU export class metadata
python/cuml/cuml/ensemble/randomforestclassifier.py
RandomForestClassifier._attrs_to_cpu now post-processes exported attrs to update each estimator’s classes_ and n_classes_, and returns matching top-level classes_/n_classes_.
sklearn export tests
python/cuml/tests/test_sklearn_import_export.py
The existing round-trip test now checks each exported estimator’s class metadata, and a new as_sklearn() test compares top-level and per-estimator class attributes against scikit-learn.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main change: fixing sklearn child metadata for RandomForestClassifier.
Description check ✅ Passed The description matches the code changes and tests around sklearn-compatible child metadata and exported labels.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

@jcrist jcrist left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice fix. One question, but otherwise LGTM.

Comment thread python/cuml/cuml/ensemble/randomforestclassifier.py
@csadorf

csadorf commented Jun 25, 2026

Copy link
Copy Markdown
Contributor Author

/merge

@rapids-bot
rapids-bot Bot merged commit 7247403 into NVIDIA:main Jun 25, 2026
250 of 256 checks passed
@csadorf
csadorf deleted the fix/rf-classifier-export-metadata branch June 25, 2026 22:25
rapids-bot Bot pushed a commit that referenced this pull request Jun 26, 2026
This PR removes xfail markers that caused failures in nightly test: https://github.com/rapidsai/cuml/actions/runs/28229406180/job/83629207110

These tests pass now because of the following PRs merged yesterday:
- #8290: Fixed `plot_pca_vs_fa_model_selection` and `plot_compare_gpr_krr` because it adds CPU fallbacks
- #8291: Fixed `plot_forest_iris`

Authors:
  - Jinsol Park (https://github.com/jinsolp)

Approvers:
  - Divye Gala (https://github.com/divyegala)
  - Jim Crist-Harif (https://github.com/jcrist)

URL: #8297
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working Cython / Python Cython or Python issue non-breaking Non-breaking change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants