Fix off-by-one in tree growth loop for DecisionTreeClassifier and BaseTreeRegressor

[slievens]

Changed the tree growth loop in base_tree_regressor.rs and decision_tree_classifier.rs. Added two tests.
Removed the unused depth method in BaseTreeRegressor.

Fixes #462

Checklist

• [ x] My branch is up-to-date with main branch.
• [ x] Everything works and tested on latest stable Rust.
• [ x] Coverage and Linting have been applied

Current behaviour

Last level of tree not fully constructed.

New expected behaviour

Last level of tree fully constructed.

Change logs

Added

Two tests.

Changed

• Tree growth loop logic.
• Removed unused method.

[Mec-iS]

Reviewed the diff and traced through the logic against the new tests — nice fix, and good catch on top of the issue report. A couple of notes:

Correctness looks right, and slightly better than the issue's suggestion. The issue proposed node.level <= max_depth, but this PR uses node.level < max_depth. Tracing it through with max_depth: Some(3): splits happen at levels 1 and 2, giving a tree with 1+2+4 = 7 nodes, which matches full_depth's assert_eq!(tree.nodes().len(), 7). Using <= would have over-split by one extra level, so the strict < here is correct and validated by the test — good that it wasn't copied verbatim from the issue.

Dead code removal is safe. BaseTreeRegressor::depth() was private and unused after the loop rewrite (and its doc comment was stale/copy-pasted: "Get estimate of intercept, return value"), so dropping it is reasonable cleanup.

One thing worth double-checking: DecisionTreeClassifier::depth() is pub fn depth() — public API — and wasn't touched here, which is correct since we can't remove a public method. But since the loop no longer relies on the shared depth counter to decide stopping, could you confirm self.depth is still updated correctly inside split() so depth() keeps returning an accurate value for callers? A quick assertion on tree.depth() in full_depth for the classifier would make that explicit and guard against regressions.

Question on the min_samples_split: 1, // Fix later in the classifier test. That reads like a known-but-unfixed edge case (a split threshold of 1 is unusual - normally you need at least 2 samples to split). Could you open a separate issue for it, or explain here why it was needed for this test, so it doesn't get merged as a silent TODO?

Otherwise this looks solid: targeted, minimal diff, and both regressor/classifier paths get coverage. Thanks for tackling this!

[Mec-iS]

I found the actual root cause behind that // Fix later comment — it's a real off-by-one bug in DecisionTreeClassifier, not just a quirky test choice.

The bug

Both trees gate whether a node is eligible to split using min_samples_split, but the two implementations disagree:

File	Split-eligibility check	Meaning
base_tree_regressor.rs	if n < min_samples_split { return false; }	Splits when n >= min_samples_split (correct, sklearn-consistent)
decision_tree_classifier.rs	if n <= min_samples_split { return false; }	Splits only when n > min_samples_split (off by one)

With the classifier's <=, setting min_samples_split: 2 (the value used in the regressor's equivalent test) would refuse to split any node with exactly 2 samples — which is exactly the case needed to fully expand the test tree down to singleton leaves. The author's workaround, min_samples_split: 1, happens to produce the same effective threshold (n <= 1 ⇔ split only when n >= 2) as the correct formula would with min_samples_split: 2. That's why it "worked" but needed a // Fix later flag — it's papering over the real bug rather than fixing it.

Proposed fix

1. Fix the off-by-one in decision_tree_classifier.rs:

// before
if n <= self.parameters().min_samples_split {
    return false;
}

// after — align with base_tree_regressor.rs's convention
if n < self.parameters().min_samples_split {
    return false;
}

2. Update the full_depth test to use the real, documented value instead of the workaround:

let parameters = DecisionTreeClassifierParameters {
    max_depth: Some(3),
    min_samples_leaf: 1,
    min_samples_split: 2, // matches base_tree_regressor's equivalent test
    seed: None,
    criterion: SplitCriterion::Gini,
};

I traced this dataset through both formulas: for x = [1..6], splitting at the same thresholds (2.5, then 4.5) still yields node sizes of 6 → {2, 4} → {1, 1, 2, 2}, so min_samples_split: 2 under the fixed < check produces the identical 7-node tree and 5/6 accuracy the test already asserts — no other test values need to change.

3. Optional hardening: add a small dedicated test with min_samples_split: 3 and a node of exactly 3 samples to explicitly pin the boundary behavior (n >= min_samples_split should split), so this off-by-one can't silently regress again.

Want me to post this as a follow-up review comment on PR #464, or open a separate GitHub issue for the min_samples_split off-by-one (since it's a distinct bug from the one #464 already fixes)?

Citations:
[1] CLAUDE.md

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 64.01%. Comparing base (9eaae9e) to head (010c702).
⚠️ Report is 187 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff             @@
##             main     #464       +/-   ##
===========================================
+ Coverage   43.97%   64.01%   +20.03%     
===========================================
  Files          85       96       +11     
  Lines        7281     8359     +1078     
===========================================
+ Hits         3202     5351     +2149     
+ Misses       4079     3008     -1071     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[slievens]

Also added a fix for issue 463.

[Mec-iS]

👍🏼 thanks