FIX Draw indices using sample_weight in Forest #31529
Conversation
|
The forest estimators now pass the statistical repeated/weighted equivalence test, for example |
|
Relative (float) |
|
The |
|
The |
| if sample_weight is None: | ||
| sample_indices = random_instance.randint(0, n_samples, n_samples_bootstrap) |
There was a problem hiding this comment.
There two options for the random draw of indices when sample_weight=None
- Convert to all ones
if sample_weight is None:
sample_weight = np.ones(n_samples)
normalized_sample_weight = sample_weight / np.sum(sample_weight)
sample_indices = random_instance.choice(
n_samples, n_samples_bootstrap, replace=True, p=normalized_sample_weight
)- Use the old code path when
sample_weight=None
if sample_weight is None:
sample_indices = random_instance.randint(0, n_samples, n_samples_bootstrap)
else:
normalized_sample_weight = sample_weight / np.sum(sample_weight)
sample_indices = random_instance.choice(
n_samples,
n_samples_bootstrap,
replace=True,
p=normalized_sample_weight,
)The two options use different rng functions: choice with uniform p for 1 and randint for 2. They are statistically the same but they don't give the same deterministic output for a given random state.
The benefit of 2. is that the code is backward compatible when sample_weight=None. A fit without sample_weight reproduce the same fit as main for a given random_state.
The benefit of 1. is that sample_weight=None and sample_weight=np.ones(n_samples) give the same fit for a given random_state.
Here we chose 2.
There was a problem hiding this comment.
I haven't the time to finish my review today, but this looks great: I tried running the notebook of github.com/snath-xoc/sample-weight-audit-nondet/ against this branch and I confirm the statistical tests pass for RandomForestClassifier/Regressor and ExtraTreesClassifier/Regressor.
| - if None, this indicates the total number of samples. | ||
| The maximum number of samples to draw. | ||
|
|
||
| - If None, then draw `n_samples` samples. |
There was a problem hiding this comment.
| - If None, then draw `n_samples` samples. | |
| - If None, then draw `n_samples` (or `sample_weight.sum()`) samples. |
There was a problem hiding this comment.
Currently the max_samples = None option (in this helper function and the forest estimators) will always draw n_samples, contrary to the max_samples = 1.0 option which returns n_samples or sample_weight.sum().
Would you prefer max_samples = None (which is the default) to be equivalent to max_samples = 1.0 ?
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
|
NB: this PR uses the weighting strategy to represent the drawn But for this to be correct, that is giving the same fit as the indexing strategy (directly passing |
Part of #16298. Similar to #31414 (Bagging estimators) but for Forest estimators.
What does this implement/fix? Explain your changes.
When subsampling is activated (
bootstrap=True),sample_weightare now used as probabilities to draw the indices. Forest estimators then pass the statistical repeated/weighted equivalence test.Comments
This PR does not fix Forest estimators when
bootstrap=False(no subsampling).sample_weightare still passed to the decision trees. Forest estimators then fail the statistical repeated/weighted equivalence test because the individual treesalso fail this test (probably because of tied splits in decision trees #23728).
TODO
sample_weight=Nonecasemax_samplesas done in FIX Draw indices using sample_weight in Bagging #31414class_weight = "balanced"as done in Fix linear svc handling sample weights under class_weight="balanced" #30057class_weight = "balanced_subsample"