[slievens]

Fixes #493

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

When the number of attributes considered is equal to the actual number of attributes, the attributes are checked in a fixed order.

New expected behaviour

Attributes are always considered in a random order.

Change logs

Changed

• Always shuffle the attributes.
• One test updated to reflect this new behaviour.

[codecov]

Codecov Report

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

Additional details and impacted files

@@             Coverage Diff             @@
##             main     #494       +/-   ##
===========================================
+ Coverage   43.97%   85.57%   +41.59%     
===========================================
  Files          85       96       +11     
  Lines        7281     9365     +2084     
===========================================
+ Hits         3202     8014     +4812     
+ Misses       4079     1351     -2728     

☔ 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.

[Mec-iS]

Thanks @slievens, the change is small and matches the stated goal of #493. Review notes below, mostly about how randomness differs between sklearn and smartcore.

What looks good

• Removing the mtry < n_attr guard is correct. With mtry == n_attr the old code visited features in a fixed order, so tied splits always picked the same column.
• The new test checks the right things. The duplicated-column tree has the same structure as the single-column tree, and both columns get used across seeds.

Randomness vs sklearn

1. Parity is statistical, not bit-for-bit. sklearn's splitter uses its own Cython RNG (rand_r / xorshift) and a Fisher-Yates draw of features without replacement. Rust's rand uses a different generator and SliceRandom::shuffle. The same random_state / seed can never give the same splits. Parity tests against sklearn should compare distributions (for example the frequency of the chosen tied feature over many seeds, or forest-level scores), not exact trees for one seed.
2. Tie-breaking must match for the shuffle to matter. sklearn keeps the first feature visited among ties, because it only replaces the best split on a strictly greater improvement. After this change, which tied feature wins is uniform only if the comparison in find_best_cutoff is also strict (>, not >=). Please confirm this. A >= would favour the last visited feature, which is still deterministic after the shuffle.
3. Constant features. sklearn skips constant features and does not count them towards max_features. If smartcore counts them within mtry, the set of candidate features differs even when the order is random. This is probably out of scope here, but worth a note in Tree regressor: always shuffle the features, as sklearn does #493 for the parity work.
4. Seeded results change. The shuffle now consumes RNG state at every node, even when mtry == n_attr. Every seeded tree and forest fit with mtry == n_attr changes, and so does anything drawn from the same rng afterwards (for example Splitter::Random thresholds and bootstrap sampling in forests). This should be listed under "Changed" in the CHANGELOG, as a behaviour change for seeded results.
5. Cost. A full shuffle of workspace.variables is O(p) per node, while sklearn draws features lazily and stops after max_features valid ones. This is negligible for typical p, but for wide data a partial shuffle (partial_shuffle(rng, mtry)) would be cheaper. This is optional. Note that re-shuffling an already shuffled buffer stays uniform, so correctness is fine.

Test robustness

• StdRng output is not guaranteed stable across rand versions. The test uses 10 fixed seeds and asserts both columns appear. That is fine today but could break after a rand bump. A larger seed range, or asserting only that more than one column is used, would be less fragile.
• tree_single is compared with tree_double using the same seed, but the RNG stream differs (shuffling 2 vs 1 variables). The equality still holds only because the tied gains make the structure identical. That is a good property to check, but it deserves a comment.

Scope

• Tree regressor: always shuffle the features, as sklearn does #493 also proposes computing the false-side statistics directly instead of parent - true. This PR does not include it, and Fixes #493 will auto-close the issue. Either split that item into a follow-up issue or drop the closing keyword.
• The diff only touches base_tree_regressor.rs. Please check that the classifier, or any other code using the same mtry < n_attr pattern, is consistent (grep -rn "mtry < " src/).

Otherwise LGTM once the strict-comparison point (2) and the CHANGELOG note (4) are addressed.

[slievens]

Some answers:

• Parity is statistical, not bit-for-bit Correct. We cannot produce bit-identical results.
• Tie-breaking must match for the shuffle to matter. I do not think this is true, with >= we will choose the last tied feature but this will also be random.
• Constant features. We do not have the same treatment as sklearn w.r.t. constant features. This was the case well before this PR.
• Seeded results change. Correct and added.

Test robutness

• The constructed trees in the test have 9 nodes. We produce 10 trees, that is 90 nodes in total. The probability of all of them choosing the same feature by chance (and the test failing) is 2**(-90). I do not think we have to worry about that ;-)

Scope:

• DecisionTreeClassifier has the same line, but I will not "fix" this at this time. The plan is to move the *Classifier models onto the same BaseTree and then they will get the fix.