Refactor RF objectives by objective family - #8233
Conversation
|
@switch527 thanks for motivating this. I think I have the solution now and will implement weights faithfully to sklearn. |
fe24d06 to
c048c5d
Compare
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughConsolidates per-criterion objective implementations into ClassificationObjectiveFunction and RegressionObjectiveFunction, wires the runtime split_criterion through builder/fit/kernels, updates tests to parameterize by criterion, and replaces per-criterion kernel sources with consolidated classification/regression kernel files. ChangesDecision Tree Objective Consolidation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
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)
Comment |
c048c5d to
66efd8c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/tests/sg/rf_test.cu`:
- Around line 1392-1403: The test currently calls compile-time helpers like
GainPerSplit<ObjectiveConfig::splitCriteria> and MSEGroundTruthGain directly, so
add at least one assertion that exercises the runtime dispatch path: construct
an objective/family instance that stores the criterion (the same enum value used
in the test), call its runtime Gain(data, split_bin_index) (or Gain() method on
the objective object that switches on objective.criterion), and assert the
result equals the compile-time GainPerSplit result (e.g., compare
objective.Gain(...) to GainPerSplit<ObjectiveConfig::splitCriteria>(...)). Do
this for at least one criterion (reuse MSE or another from the existing list) so
the test validates that the stored criterion drives the same gain formula as the
compile-time helpers. Ensure you reference the same data and split_bin_index
values used in the existing assertions.
🪄 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: f56f43f4-e32a-4c49-a407-6cedf732b525
📒 Files selected for processing (17)
cpp/CMakeLists.txtcpp/src/decisiontree/batched-levelalgo/builder.cuhcpp/src/decisiontree/batched-levelalgo/kernels/classification-double.cucpp/src/decisiontree/batched-levelalgo/kernels/classification-float.cucpp/src/decisiontree/batched-levelalgo/kernels/gamma-double.cucpp/src/decisiontree/batched-levelalgo/kernels/gamma-float.cucpp/src/decisiontree/batched-levelalgo/kernels/gini-double.cucpp/src/decisiontree/batched-levelalgo/kernels/gini-float.cucpp/src/decisiontree/batched-levelalgo/kernels/inverse_gaussian-double.cucpp/src/decisiontree/batched-levelalgo/kernels/inverse_gaussian-float.cucpp/src/decisiontree/batched-levelalgo/kernels/poisson-double.cucpp/src/decisiontree/batched-levelalgo/kernels/poisson-float.cucpp/src/decisiontree/batched-levelalgo/kernels/regression-double.cucpp/src/decisiontree/batched-levelalgo/kernels/regression-float.cucpp/src/decisiontree/batched-levelalgo/objectives.cuhcpp/src/decisiontree/decisiontree.cuhcpp/tests/sg/rf_test.cu
💤 Files with no reviewable changes (8)
- cpp/src/decisiontree/batched-levelalgo/kernels/gini-float.cu
- cpp/src/decisiontree/batched-levelalgo/kernels/poisson-float.cu
- cpp/src/decisiontree/batched-levelalgo/kernels/inverse_gaussian-double.cu
- cpp/src/decisiontree/batched-levelalgo/kernels/poisson-double.cu
- cpp/src/decisiontree/batched-levelalgo/kernels/inverse_gaussian-float.cu
- cpp/src/decisiontree/batched-levelalgo/kernels/gamma-double.cu
- cpp/src/decisiontree/batched-levelalgo/kernels/gamma-float.cu
- cpp/src/decisiontree/batched-levelalgo/kernels/gini-double.cu
🚧 Files skipped from review as they are similar to previous changes (8)
- cpp/CMakeLists.txt
- cpp/src/decisiontree/batched-levelalgo/kernels/classification-float.cu
- cpp/src/decisiontree/batched-levelalgo/kernels/regression-float.cu
- cpp/src/decisiontree/batched-levelalgo/kernels/regression-double.cu
- cpp/src/decisiontree/decisiontree.cuh
- cpp/src/decisiontree/batched-levelalgo/builder.cuh
- cpp/src/decisiontree/batched-levelalgo/kernels/classification-double.cu
- cpp/src/decisiontree/batched-levelalgo/objectives.cuh
66efd8c to
57d5c78
Compare
|
@RAMitchell, I appreciate you picking it up. This is going to make forest estimators much more powerful for my use cases when everything is buttoned up. |
dantegd
left a comment
There was a problem hiding this comment.
The compile-time improvement is clear and great to see, didn't find any correctness issues. One thing that would be helpful to document is a quick train-time sanity check for common Gini/MSE cases. A small before/after would make any compile-time/runtime tradeoff explicit.
| params.max_n_bins, | ||
| NumLeftOfBin(cdf_hist, params.max_n_bins - 1), | ||
| NumLeftOfBin(cdf_hist, split_bin_index)); | ||
| auto hypothesis_gain = objective.GainPerSplit( |
There was a problem hiding this comment.
What do you think about driving this through objective.Gain(...), or adding a focused test for Gain, now that the shared split-bin loop lives there? Calling GainPerSplit still validates the per-criterion formula dispatch, but it bypasses the min-samples guard and the shared outer iteration that this PR is mainly refactoring.
There was a problem hiding this comment.
I wanted to do this but objective.Gain is a device function so it means rewriting all of the tests to go through a kernel. The min samples parameter is definitely tested elsewhere.
|
Benchmarking shows only minor differences.
|
|
/merge |
## Summary Refs [#8093](#8093), [#1279](#1279). Builds on [#8132](#8132) and [#8233](#8233). Related follow-ups: [#8186](#8186), [#8146](#8146). This PR prepares the RF objective/bin layer for weighted training without threading `sample_weight` through the public estimator APIs yet. - Renames RF histogram bins to `ClassificationBin` and `RegressionBin` - Adds weighted bin variants that preserve integer sample counts while separately accumulating sample weight - Switches objective families to a `weighted` bool template parameter - Expands objective tests across weighted and unweighted regression/classification criteria - Adds weighted ground-truth checks for MSE, Poisson, Gamma, Inverse Gaussian, Entropy, and Gini ## Notes This is groundwork only. It does not yet route `sample_weight` through Python/Cython, sampling, or RF training kernels. ## Testing - `git diff --check` - `SG_RF_TEST.localnvforest --gtest_filter="*ObjectiveTest*"` - 96 tests passed A full normal `ninja -C cpp/build-ninja-gcc12 SG_RF_TEST -j2` run was started after reconfiguring the local build cache to use the local nvforest artifact, but was paused before completion. Authors: - Rory Mitchell (https://github.com/RAMitchell) Approvers: - Philip Hyunsu Cho (https://github.com/hcho3) URL: #8247
Summary
Refactors the batched-level random forest objective dispatch from one objective class/kernel instantiation per split criterion to one objective class/kernel instantiation per objective family.
This reduces the explicit RF objective kernel instantiations from 12 criterion-specific files to 4 family-specific files:
The individual gain formulas remain separate named functions, but the outer split-bin iteration is shared within each objective family and dispatches on the criterion at runtime.
Motivation
This reduces compile-time work and prepares the objective layer for future sample-weight support without introducing weight semantics in this PR.
From the local Ninja build log, summed compile time for the RF objective instantiation objects dropped from approximately:
669.99sacross 12 criterion-specific objects110.36sacross 4 family-specific objectsThat is roughly an 84% reduction in per-object objective-instantiation compile work. This is not a full clean-build wall-clock measurement, but it captures the targeted compile-time improvement from reducing these explicit instantiations.
Details
ClassificationObjectiveFunctionfor Gini and entropy.RegressionObjectiveFunctionfor MSE, Poisson, Gamma, and Inverse Gaussian.split_criterioninto the objective object from the tree builder.This PR does not change sample-weight behavior or add weighted histogram bins.
Verification
git diff --check upstream/main..HEADcmake --build cpp/build-ninja-gcc12 --target cuml_objs -j 8