[MRG] FIX raise error for max_samples if no bootstrap & optimize forest tests

[PSSF23]

Reference Issues/PRs

Close #21294
Close #21299

What does this implement/fix? Explain your changes.

• Raise ValueError with message saying max_samples is only available if bootstrap=True.
• Fix forest tests by adding bootstrap=True parameter.

Any other comments?

[simonandras]

Nice enhancement! Actually the behaviour here before your PR was ambiguous:
in your example after RandomForestClassifier(n_estimators=100, bootstrap=False, max_samples=0.5) the bootstrap was made, because in line 378 in _forest.py the n_samples_bootstrap parameter is set to 0.5 * n_samples by the _get_n_samples_bootstrap function whether bootstrap was False or not.

After writing the tests LGTM.

[PSSF23]

#21299 When checking the test errors, I found out that some of the tests for max_samples do not specify the bootstrap parameter. Yet the classifiers/regressors tested might have bootstrap=False as default, so the test results would be invalid.

The max_samples and bootstrap parameters in IsolationForest and bagging estimators seem to follow different logistics, so I didn't change those tests.

[adrinjalali]

pinging @glemaitre and @NicolasHug on this one.

[NicolasHug]

Thanks @PSSF23 , I made some minor comments but overall this LGTM

[PSSF23]

@NicolasHug I changed the error message, but setting bootstrap=True explicitly is still required in the checks to achieve the same results. Otherwise, the fitting would not randomize the sample indices:

scikit-learn/sklearn/ensemble/_forest.py

Lines 164 to 186 in 28567a5

	if forest.bootstrap:
	n_samples = X.shape[0]
	if sample_weight is None:
	curr_sample_weight = np.ones((n_samples,), dtype=np.float64)
	else:
	curr_sample_weight = sample_weight.copy()
	indices = _generate_sample_indices(
	tree.random_state, n_samples, n_samples_bootstrap
	)
	sample_counts = np.bincount(indices, minlength=n_samples)
	curr_sample_weight *= sample_counts
	if class_weight == "subsample":
	with catch_warnings():
	simplefilter("ignore", DeprecationWarning)
	curr_sample_weight *= compute_sample_weight("auto", y, indices=indices)
	elif class_weight == "balanced_subsample":
	curr_sample_weight *= compute_sample_weight("balanced", y, indices=indices)
	tree.fit(X, y, sample_weight=curr_sample_weight, check_input=False)
	else:
	tree.fit(X, y, sample_weight=sample_weight, check_input=False)

[PSSF23]

@simonandras @adrinjalali @NicolasHug @glemaitre Anything else needed for this PR?

[adrinjalali]

I kinda feel like we need a deprecation cycle here, but not entirely sure.

[glemaitre]

I agree with @adrinjalali. We should raise a FutureWarning for 2 versions before to error. The warning can be educational by explaining that we are discarding max_samples indeed and that to avoid that ambiguity, an error will be raised in the future.

[NicolasHug]

I think we tend to consider these to be bugfixes (hence no deprecation), but I don't mind going through the deprecation cycle to be extra nice.

[glemaitre]

But here, the parameter was only ignore that was more or less in line with the docstring from what I understand.

[PSSF23]

@glemaitre The current code will calculate n_samples_bootstrap based on max_samples no matter bootstrap is True or False. In that process it might raise unnecessary errors if the max_samples format is wrong. As it's eventually ignored, why check it?

scikit-learn/sklearn/ensemble/_forest.py

Lines 384 to 386 in 3626a34

	n_samples_bootstrap = _get_n_samples_bootstrap(
	n_samples=X.shape[0], max_samples=self.max_samples
	)

Also in a naive user perspective, I would assume any parameter entered reflects how the program actually runs. So for example, I would think that ExtraTreesClassifier(max_samples=0.5) runs with half sample sizes.

In addition, for consistency, as oob_score raises errors when bootstrap is False, max_samples should do the same thing.

scikit-learn/sklearn/ensemble/_forest.py

Lines 407 to 408 in 3626a34

	if not self.bootstrap and self.oob_score:
	raise ValueError("Out of bag estimation only available if bootstrap=True")

[glemaitre]

@glemaitre The current code will calculate n_samples_bootstrap based on max_samples no matter bootstrap is True or False. In that process it might raise unnecessary errors if the max_samples format is wrong.
Also in a naive user perspective, I would assume any parameter entered reflects how the program actually runs. So for example, I would think that ExtraTreesClassifier(max_samples=0.5) runs with half sample sizes.

OK so let's consider it as a bug fix and directly raise an error. If would have beforehand a safeguard if boostrap: and silently ignore it, then it would have been different.

In addition, for consistency, as oob_score raises errors when bootstrap is False, max_samples should do the same thing.

This is a different case here because there is no way to have an OOB sample without bootstraping while you could consider getting a smaller sample size with max_sample=0.5.

[PSSF23]

@glemaitre I modified the code as you suggested. Let me know if there are other improvements needed!

[glemaitre]

Otherwise it looks good.

[PSSF23]

@glemaitre Changes implemented~

[glemaitre]

LGTM. Before to merge, I would like to have a last review of @adrinjalali

[adrinjalali]

LGTM, happy with it being a fix