[slievens]

Fixes #515

Checklist

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

Current behaviour

The forest estimators (RandomForestClassifier, RandomForestRegressor, ExtraTreesClassifier, ExtraTreesRegressor) use m: Option<usize> for the number of features to test at each split.

• None gives floor(sqrt(p)).
• You cannot set a fraction of the features or "all features".
• The forest does not validate the value: Some(0) or a value larger than the number of features is accepted.

New expected behaviour

m is now a MaxFeatures enum, similar to max_features in scikit-learn:

Variant	Number of features	Condition
Sqrt (default)	max(1, floor(sqrt(p)))
Fraction(f)	max(1, floor(f * p))	0 < f <= 1
Count(n)	n	1 <= n <= p
All	p

• with_m accepts impl Into<MaxFeatures>, so with_m(3) still works and gives Count(3).
• fit returns a ParametersError when the value is not valid for the data (for example Count(0), Count(p + 1), Fraction(1.5) or Fraction(NaN)).
• The default (Sqrt) gives the same number of features as before, so default results do not change.
• New module ensemble::max_features with docs, a doctest and references.
• Tests:
  • Unit tests for each variant, the boundary values and the error messages.
  • Tests that fit returns an error for each invalid value with both splitters and accepts the boundary values.

Change logs

Added

• ensemble: MaxFeatures enum (Sqrt, Fraction, Count, All) sets the number of features to test at each split in the forest estimators.

Changed

• Breaking change: ensemble: m on the four forest parameter structs is now MaxFeatures (was Option<usize>). The search parameters use Vec<MaxFeatures>. with_m(3) still works. fit returns an error when m is not valid for the data.
• Breaking change: parameters serialized with an earlier version that contain m no longer deserialize. This release already breaks loading of serialized trees and forests, so we accept this.

[codecov]

Codecov Report

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

Additional details and impacted files

@@             Coverage Diff             @@
##             main     #531       +/-   ##
===========================================
+ Coverage   43.97%   86.99%   +43.01%     
===========================================
  Files          85      100       +15     
  Lines        7281     9913     +2632     
===========================================
+ Hits         3202     8624     +5422     
+ Misses       4079     1289     -2790     

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

Reviewed at ab7fc76148ab7e85712c165b10f80a69770e8fd7 (static review; I have not run the Rust tests).

The implementation looks well scoped. Resolving MaxFeatures once in BaseForest::fit, after the empty-data checks and before RNG initialization/forest allocations, is the right place. The valid path introduces no new heap allocation or per-node work, and the fraction comparison correctly rejects NaN and infinities. Keeping Sqrt as the default avoids an unrelated change to the regressor defaults. I did not find a confirmed algorithmic correctness bug in this diff.

A few improvements before merging:

1. Test equivalent variants through fitting, not just resolution. The new valid-boundary test only checks that three trees were created, and only exercises Splitter::Best. Please run it for both splitters and add fixed-seed equivalence checks for All == Count(p) == Fraction(1.0) and Sqrt == Count(p.isqrt()). Compare the individual trees and predictions (and retained bootstrap samples where enabled). These checks would guard the claim that translating the parameter does not change RNG consumption or fitted behavior when the resolved count is identical. Avoid hard-coding a particular RNG sequence.

2. Add small public-estimator and search-grid checks. The new invalid-value integration tests exercise the shared base forest with MSE, which is useful, but do not directly cover the four public estimators. A small smoke test per estimator should check that a new enum variant is forwarded and an invalid count returns ParametersError. Extend both random-forest search iterator tests with Fraction and All; their current cases only cover the old None/Some equivalents. This can stay small—no need to duplicate the entire resolver test matrix.

3. Make migration and serde coverage explicit. Please include the Vec<Option<usize>> -> Vec<MaxFeatures> search-parameter break in CHANGELOG, plus the mappings None -> Sqrt, Some(n) -> Count(n), and Some(p) -> All when all features were intended. The change to serde_without_bootstrap_field_uses_default removes m entirely, so it now also exercises the missing-m fallback. Prefer an explicit current-format m in that fixture to isolate the missing-bootstrap contract, with a separate missing-m test asserting Sqrt. Parameter-struct round trips with the new variants would complement the enum-only serde test. The intentional rejection of legacy explicit null/integer m values should be documented/tested rather than implying old parameter JSON remains compatible.

4. Update the discoverable API docs. The four with_m methods still describe only a number of sampled predictors. Link them to MaxFeatures and mention both integer and enum arguments and fit-time validation. Qualify the module's bagging statement: all-feature random forests still bootstrap rows and can randomize tied-split selection; all-feature extra trees retain randomized thresholds. All removes feature subsampling, not all randomness.

5. CI and changelog hygiene. At review time, the current head's lint check was failing; MSRV passed, while other checks were still running/queued. Please resolve lint and wait for the current-head checks, including serde and WASM. The existing Codecov comment refers to an older head, not ab7fc76. Also add the missing - before the new breaking-change entry in CHANGELOG.

Overall, the enum and centralized validation are a good improvement. The remaining feedback is primarily regression coverage, migration clarity, and merge hygiene, rather than a request for a broader refactor.