[MRG] FIX keep at least one feature when max_features is small fraction

[ghost]

Reference Issues/PRs

Fixes #12386.

What does this implement/fix? Explain your changes.

Often the max_features parameter of a Bagging estimator is set as a float, to represent a fraction of the number of features to use. To convert to an integer, this equation is currently used:
max_features = int(self.max_features * self.n_features_)

However, this often leads to a ValueError if the result is rounded down to zero. This may occur if the number of features is often unknown (for example, due to hyperparameter tuning in an earlier stage).

This PR ensures a minimum of one feature is kept in this situation:
max_features = max(1, int(self.max_features * self.n_features_) )

Any other comments?

Would be grateful to check that unit test is implemented in the right place in an appropriate manner. I've tried to be consistent with other tests.

I've tried to find the cleanest implementation that still raises a ValueError if max_features is negative, zero, too large, or not an int nor float.

[jnothman]

Otherwise LGTM

[jnothman]

Can't you use <= instead of not?

[jnothman]

Put this before the above test and you can simplify the code. Don't worry about validating that it is numeric. Comparing to a number is good enough for that unexpected case.

[ghost]

Thanks for the review @jnothman ! I'm struggling to find a simpler implementation that handles unexpected cases. Could you expand on your comment above?

Comparing self.max_features to a number without first casting to an integer causes an unexpected Type error if it's a string: TypeError: unorderable types: int() < str().

However, casting max_features to an integer means that 0.1 would be rounded down to 0, and hence a Value error is raised (which is the behaviour the PR is trying to avoid).

Some relevant existing unit tests:

scikit-learn/sklearn/ensemble/tests/test_bagging.py

Lines 424 to 434 in fa4de83

	# Test max_features
	assert_raises(ValueError,
	BaggingClassifier(base, max_features=-1).fit, X, y)
	assert_raises(ValueError,
	BaggingClassifier(base, max_features=0.0).fit, X, y)
	assert_raises(ValueError,
	BaggingClassifier(base, max_features=2.0).fit, X, y)
	assert_raises(ValueError,
	BaggingClassifier(base, max_features=5).fit, X, y)
	assert_raises(ValueError,
	BaggingClassifier(base, max_features="foobar").fit, X, y)

[jnothman]

Well... TypeError might really be the more appropriate error anyway, but let's not quibble with the tests. Why not:

if not numeric:
   raise ValueError
if real:
   max_features = max_features * features
if not 0 < max_features <= n_features:
   raise ValueError
max_features = int(max_features)

but perhaps that logic is no less complicated than the present?

[ghost]

Good point: I could just modify that test to expect a TypeError if a string is input rather than a ValueError.

Here's a suggested modification of the logic that ensures 0.1 is not rounded down to zero:

if isinstance(self.max_features, (numbers.Integral, np.integer)):
    max_features = self.max_features
else:  # float
   max_features = self.max_features * self.n_features_

if not (0 < max_features <= self.n_features_):
    raise ValueError

max_features = max(1, int(max_features))

[ghost]

Ah, it looks like 'foobar' > 0 raises TypeError in python 3, but is True in python 2.7. Who'd have guessed. I'll add a line to explicitly raise a ValueError if the input is not numeric.

[rth]

LGTM, thanks. Please add a what's new to doc/whats_new/v0.20.rst under the 0.20.1 section mentioning all estimators that are affected by this fix (excluding BaseBagging class itself).

[ghost]

Thanks for the review @rth . I've added a comment to the doc as requested.

[rth]

Thanks! (Fixed the formatting in what's new a bit).