perf(ensemble): pre-allocate tree and sample vector capacities in forest models

[Aditya-9-6]

Summary

This PR resolves pending // TODO: use with_capacity here comments in smartcore::ensemble by pre-allocating memory capacities for tree vectors and sample tracking buffers based on parameters.n_trees.

Key Changes:

• src/ensemble/base_forest_regressor.rs:

  • Replaced Vec::new() with Vec::with_capacity(n_trees) for trees container during forest regressor fitting.
  • Pre-allocated maybe_all_samples with Vec::with_capacity(n_trees) when parameters.keep_samples is active.

• src/ensemble/random_forest_classifier.rs:

  • Pre-allocated trees with Vec::with_capacity(n_trees) for RandomForestClassifier.
  • Pre-allocated maybe_all_samples with Vec::with_capacity(n_trees).

Benefits:

• Eliminates repeated heap re-allocations and buffer copying during tree iteration in random forest training.
• Cleanly addresses existing codebase TODOs without changing public API behavior.

Verification:

All unit tests, integration tests, and benchmarks pass clean (cargo test --all-features).

[Mec-iS]

thanks! did you run some profiling and benching to see if the new code is faster/slower? it would be nice to know.
I will run the review asap.

[codecov]

Codecov Report

❌ Patch coverage is 66.66667% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 44.36%. Comparing base (70d8a0f) to head (e189691).
⚠️ Report is 27 commits behind head on development.

Files with missing lines	Patch %	Lines
src/ensemble/base_forest_regressor.rs	66.66%	1 Missing ⚠️
src/ensemble/random_forest_classifier.rs	66.66%	1 Missing ⚠️

Additional details and impacted files

@@               Coverage Diff               @@
##           development     #386      +/-   ##
===============================================
- Coverage        45.59%   44.36%   -1.24%     
===============================================
  Files               93       95       +2     
  Lines             8034     8070      +36     
===============================================
- Hits              3663     3580      -83     
- Misses            4371     4490     +119     

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

Code Review

Thanks for the PR @Aditya-9-6! This is a clean, focused, and correct performance improvement. Here's a summary of the review:

✅ Strengths

• Correctness: Replacing Vec::new() with Vec::with_capacity(n_trees) is safe and sound — push() calls downstream are unaffected.
• TODO resolution: All three pre-existing // TODO: use with_capacity here comments are cleanly removed.
• No API changes: Public interfaces are entirely untouched — this is a pure internal optimization.
• Minimal scope: The diff is surgical with no unrelated changes or noise.
• DRY improvement: Extracting let n_trees = parameters.n_trees as usize avoids repeating the cast and improves readability.

⚠️ Points to Address

1. Missing file — random_forest_regressor.rs: This PR covers base_forest_regressor.rs and random_forest_classifier.rs, but there is likely a random_forest_regressor.rs (or similar) that also iterates over n_trees and may have the same Vec::new() pattern or remaining TODOs. Could you check and apply the same fix there if needed?

2. n_trees type cast: parameters.n_trees as usize will silently truncate if n_trees is a signed type with a negative value. It's worth confirming that upstream validation already guards against this edge case.

3. No benchmark data: The PR is labelled perf but the description only states benchmarks pass — it doesn't include before/after numbers. Adding a quick criterion benchmark comparison (especially for large n_trees values) would strengthen the justification.

4. Option::None style (pre-existing, not introduced here): Idiomatic Rust typically uses bare None rather than Option::None. Low priority, but could be cleaned up opportunistically.

Verdict

This is a correct, low-risk, and welcome improvement. The main ask before merging is to verify whether other ensemble files need the same treatment. Benchmark numbers would be a nice-to-have. Happy to approve once the scope question is resolved!

[Aditya-9-6]

Thanks for the thorough review @Mec-iS!

1. Scope & Other Ensemble Models: Verified all files in src/ensemble. RandomForestRegressor and ExtraTreesRegressor both delegate directly to BaseForestRegressor::fit, and ExtraTreesClassifier delegates to RandomForestClassifier::fit. All ensemble tree allocations in Smartcore are now covered by Vec::with_capacity(n_trees).
2. Clippy Cast: Fixed parameters.n_trees in base_forest_regressor.rs (removed redundant as usize cast).
3. Codecov Coverage: Added test_base_forest_regressor_keep_samples unit test covering the keep_samples: true allocation path.

All CI linting checks and unit tests are passing clean!

[Mec-iS]

Merged, will go in v0.5.4