Prepare RF bins and objectives for weights support - #8247
Conversation
RAMitchell
added
non-breaking
|
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 as they are similar to previous changes (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR introduces typed bin structures (ClassificationBin, WeightedClassificationBin, RegressionBin, WeightedRegressionBin) with atomic count/weight/label-sum updates, refactors objective functions to conditionally select bin types via a weighted template parameter and compute gains from bin accessors, updates kernel type aliases to use the new bins, and extends tests to validate weighted objectives and edge cases. ChangesWeighted bins and objective refactoring
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested labels
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 |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cpp/src/decisiontree/batched-levelalgo/objectives.cuh (1)
52-117:⚠️ Potential issue | 🟠 Major | ⚡ Quick winZero-weight children can still reach the weighted normalization path.
Gain()still gates splits onCount(), but the weighted criteria andSetLeafVector()normalize byWeight(). A branch or leaf containing only zero-weight samples can therefore satisfy the count check withleft_weight,right_weight, or total leaf weight equal to 0, which turns the inverse/mean computations here into NaN/Inf and can poison split selection or leaf predictions. Please add an explicit<= eps_guard, or another zero-weight fallback, before any weight-based division in the weighted path. As per coding guidelines, "Add epsilon checks for division by zero or near-zero values" and "Handle numerical edge cases (near-zero eigenvalues, degenerate matrices, extreme values)".Also applies to: 142-149, 154-163, 181-268, 296-312
🤖 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 `@cpp/src/decisiontree/batched-levelalgo/objectives.cuh` around lines 52 - 117, GiniGain and EntropyGain compute invLeft/invRight/invLen using Weight() values which can be zero or near-zero; before any division in GiniGain and EntropyGain (and similar blocks referenced), check total_weight, left_weight and right_weight against eps_ (e.g., if total_weight <= eps_ or left_weight <= eps_ or right_weight <= eps_) and early-return a safe fallback (such as DataT(0) or the unweighted criterion) or handle the branch without performing inv* divisions; apply the same epsilon-guarding pattern around any subsequent weight-based divides in these functions so no raft::log or multiplications receive Inf/NaN from 1/0.Source: Coding guidelines
🤖 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 1197-1227: GenHist() currently assumes every bin has size
bin_width and iterates to bin_end, which reads past data and writes an incorrect
count for the tail bin; clamp the tail by computing actual_bin_end =
std::min(bin_end, params.n_rows) (or equivalent) and use actual_count =
actual_bin_end - bin_begin for loops and for any count stored in
RegressionBin/WeightedRegressionBin::count (and when summing weights or
label_sum), replace uses of bin_width for the last bin with actual_count, and
ensure classification loops (the data[i] equality/weight accumulation) also
iterate to actual_bin_end so pdf_hist and downstream
Count()/min_samples_leaf/gain checks reflect the real sample count.
---
Outside diff comments:
In `@cpp/src/decisiontree/batched-levelalgo/objectives.cuh`:
- Around line 52-117: GiniGain and EntropyGain compute invLeft/invRight/invLen
using Weight() values which can be zero or near-zero; before any division in
GiniGain and EntropyGain (and similar blocks referenced), check total_weight,
left_weight and right_weight against eps_ (e.g., if total_weight <= eps_ or
left_weight <= eps_ or right_weight <= eps_) and early-return a safe fallback
(such as DataT(0) or the unweighted criterion) or handle the branch without
performing inv* divisions; apply the same epsilon-guarding pattern around any
subsequent weight-based divides in these functions so no raft::log or
multiplications receive Inf/NaN from 1/0.
🪄 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: eeb03614-a149-4168-9af2-b7bea26fa4c5
📒 Files selected for processing (7)
cpp/src/decisiontree/batched-levelalgo/bins.cuhcpp/src/decisiontree/batched-levelalgo/kernels/classification-double.cucpp/src/decisiontree/batched-levelalgo/kernels/classification-float.cucpp/src/decisiontree/batched-levelalgo/kernels/regression-double.cucpp/src/decisiontree/batched-levelalgo/kernels/regression-float.cucpp/src/decisiontree/batched-levelalgo/objectives.cuhcpp/tests/sg/rf_test.cu
chyunsu3
left a comment
chyunsu3
left a comment
There was a problem hiding this comment.
Why do we have separate bin types for regression and classification? The implementations for WeightedRegressionBin and WeightedClassificationBin appear to be almost identical.
| CountBin(CountBin const&) = default; | ||
| HDI CountBin(double x_) : x(x_) {} | ||
| HDI CountBin() : x(0.0) {} | ||
| using BinCountT = unsigned long long int; |
There was a problem hiding this comment.
64 bit atomics need unsigned long long int they don't work for uint64_t.
|
Unweighted classification needs only count. Regression needs label sum + count. So classification can be half the size. |
|
/merge |
Summary
Refs #8093, #1279.
Builds on #8132 and #8233.
Related follow-ups: #8186, #8146.
This PR prepares the RF objective/bin layer for weighted training without threading
sample_weightthrough the public estimator APIs yet.ClassificationBinandRegressionBinweightedbool template parameterNotes
This is groundwork only. It does not yet route
sample_weightthrough Python/Cython, sampling, or RF training kernels.Testing
git diff --checkSG_RF_TEST.localnvforest --gtest_filter="*ObjectiveTest*"A full normal
ninja -C cpp/build-ninja-gcc12 SG_RF_TEST -j2run was started after reconfiguring the local build cache to use the local nvforest artifact, but was paused before completion.