Fix importance-based feature subsampling never sharing pools across estimators

[LeoGrin]

Didn't check performance on actual datasets with this change, but can look into it if we're not confident in the fix!

Summary

With FEATURE_SUBSAMPLING_METHOD="gini_feature_importance" (or "auto" on >100k-sample datasets needing subsampling), each estimator keeps the top-K most important features and fills the rest of its budget from the remaining features. Since #913 that fill is documented — and unit-tested — as balanced round-robin from a pool shared by estimators using the same importance ordering, guaranteeing every feature lands in at least one estimator when the combined budget allows.

That sharing never happened in production. _collect_importance_orderings expanded its result to one entry per estimator ([ordering] * n_estimators, or cyclic expansion of the subsample orderings), so in _subsample_features_importance_based the position-keyed pools (ordering_idx = i % n_orderings with n_orderings == n_estimators) were always private and initially empty: every draw degenerated to an independent uniform sample — the pre-#913 behavior. The unit tests passed because they feed short orderings lists (1–2 entries for many estimators), a shape the production caller never produces.

Measured on the minimal case (12 features, top-4, budget 8, 2 estimators — combined budget exactly covers the tail): the production shape misses at least one feature in 20/20 seeds; the fixed shape covers all features in 20/20. At production scale the auto-scaler raises n_estimators to the exact minimum where round-robin would barely cover everything — the point where independent sampling misses the most (~30–50% of non-top features), directly contradicting the scaler's "every feature is included in at least one ensemble member" warning.

Fix

_collect_importance_orderings now returns only the unique orderings (one for small datasets, up to n_subsamples for large). The consumer's existing i % n_orderings cycling assigns orderings to estimators identically to the old expansion, so the estimator→ordering mapping is unchanged — the only behavioral difference is that estimators sharing an ordering now genuinely share its pool, which is what #913's docstrings, comments, and tests already describe.

Not addressed here (separate issue): scale_n_estimators_for_feature_coverage computes the required estimator count from the raw budget, ignoring that the importance method re-includes top-K in every member; it can under-provision for gini even with this fix.

Behavior impact

Results change (deterministically, given a seed) only for fits where the gini path is active: FEATURE_SUBSAMPLING_METHOD="gini_feature_importance", or "auto" with >100k training samples and more features than max_features_per_estimator. All other configurations are unaffected.

Tests

• Production-shaped regression test: orderings from _collect_importance_orderings fed to _subsample_features_importance_based must cover all features across 20 seeds when the combined budget suffices (fails 20/20 on main).
• _collect_importance_orderings small-data shape test.
• Existing length assertions updated to the compact contract; the large-dataset test now pins min(n_estimators, n_samples // max_samples + 1) distinct orderings.

🤖 Generated with Claude Code

[bejaeger]

Thanks for fixing. IIUC this fixes one issue but not the other one where n_samples is large (the case for "auto" running the gini_importance method). See comment

Should we kill two birds with one stone and also this path here?

[bejaeger]

So if we're dealing with more samples than max_samples (which has 100k as default), we'd still get separate orderings per estimator it seems and the issue that's tackled here remains IIUC?