Repository navigation
Conversation
Added a test (with random data) to see whether the implemenation matches that of sklearn. Discovered an existing bug in the implementation that made that all trees of the forest used the same "random" features. An additional test for this was also added: it tests that without bootstrapping and with a single feature, the trees are still different. This test failed with the "old" implementation.
- Use plain vec instead of vecdeque for stack. - Loop over the split feature to determine true/false elements. This saves some lookups.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #481 +/- ##
===========================================
+ Coverage 43.97% 63.40% +19.42%
===========================================
Files 85 96 +11
Lines 7281 8629 +1348
===========================================
+ Hits 3202 5471 +2269
+ Misses 4079 3158 -921 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Thanks @slievens, this is a well-motivated change. Sorting once per forest, using range-based nodes with stable partitioning, and switching to a DFS stack are all sound. I read the full diff (2 files, +420/-174). Findings below, roughly by priority. Correctness / robustness
Performance / design
Tests / coverage
Process / docs
Overall this looks promising. The main asks are the equivalence/coverage tests, benchmark evidence, the lost |
|
Please use |
- Add check_row_count to BaseTreeRegressor::fit_weak_learner. It returns a Failed error when x has more rows than u32::MAX, because node elements store row indices and counts as u32. - Stop the random-split scan at the first element above the split value. The elements are sorted by the feature, so no later element can be in the true branch. - Add tests: check_row_count limits, and fit with an empty x (0x0, 0x3, 3x0) returns an error for the decision tree, random forest and extra trees regressors. - Use std::mem::size_of in test_node_element_sizes and make comments clearer.
|
I tried to address some of the concerns in commit slievens@d071328 |
|
I do not know how to show the results of the benchmark. Here is the output of running the benchmark on my machine, comparing against the version in the repo. The new version is always faster. RandomForestRegressor::fit/n_samples: 100, n_features: 10, n_trees: 10 |
|
thanks @slievens I have approved but:
You can go on and finish this or I will have a pass at those. |
|
Split search compared feature values in TX, but partitioning and predict compare them as f64 against an f64 threshold. For large integers and adjacent or very large f64 values, the applied split was then different from the split that was scored. - find_best_split: detect ties on f64 values. Compute the midpoint as prev + (x - prev) / 2. Use prev as the threshold if the midpoint rounds up to x or overflows. - find_random_split: convert the bounds to f64 before the range check, so random_range does not get an empty range and panic. - CountedElement/UnitElement: add debug_asserts that sample weights are not used, because their mass() ignores weights. - Add tests for i64 values above 2^53, adjacent f64 values and f64 midpoint overflow.
2 participants
Fixes #
Checklist
Current behaviour
BaseTreeRegressor(used byRandomForestRegressorandExtraTreesRegressor) could be faster duringfit.n_treestimes.samples: Vec<usize>with one entry per row ofx. To split a node, the code makes a new vector of lengthn_rows. A node deep in the tree with few samples still costs O(n_rows).order[j]for all rows ofxand skips rows with a zero count.LinkedListas the queue of nodes.New expected behaviour
The tree gives statistically equivalent results, but fitting is faster. The memory use for each tree is O(n_features × n_rows_in_bag).
start_idx..end_idxin these arrays. A split partitions the range of each column with a stable, branchless partition. Thus each column stays sorted in each child, and the work at each node is proportional to the number of samples in the node.UnitElement(4 bytes): no weights, each row at most one time (for example, no bootstrap).CountedElement(8 bytes): no weights, rows can repeat (bootstrap).WeightedElement(16 bytes): with sample weights.max_depthormin_samples_split). It does not search for a split in a child that cannot split.splitreads only the sorted column of the split feature, and stops at the first false sample.Behaviour that is visible to users:
main. This includesmtry < n_features,Splitter::Random, and bootstrap. The nodes are now processed depth-first (stack) instead of breadth-first (queue), so the code uses the RNG in a different order. Also, children that cannot split do not use the RNG now. The statistical behaviour does not change.Splitter::Bestis now(a.to_f64() + b.to_f64()) / 2instead of(a + b).to_f64() / 2. This prevents overflow for integerTXand loss of precision forf32.Change logs
No changes to the public API.
Changed
BaseForestRegressor::fitsorts the feature columns one time for all trees.BaseTreeRegressorkeeps one sorted array for each feature, and a node is a range in he vector of lengthn_rowsfor each node.Vecas a stack. This replaces theLinkedListqueue.Splitternow derivesCopy.Added
test_node_element_sizes, which makes sure that the element types stay small.