Repository: scikit-learn/scikit-learn
Pull request: [MRG+1] Error raised when presort in gradient_boosting.py and tree.py is not a boolean
Fixes #10107
What does this change do?
When "presort" , a parameter in GradientboostingClassifier and GradientboostingRegressor is not a boolean, ValueError is now raised. Which wasn't the case before
Which fixes the above issue.

Problem statement:
Add a test for invalid presort in GradientBoosting{Classifier,Regressor}
This should raise a ValueError and does not:
```py
from sklearn.ensemble import GradientBoostingClassifier
GradientBoostingClassifier(presort='invalid').fit([[1., 2.], [1., 1.]], [0, 1])
```

This should be enough to add the presort argument checking in BaseGradientBoosting._check_params with a test.

Hints from issue discussion:
Can I work on this if anyone else has not already started working on it?
Go for it I recommend reading 
our [contribution guidelines](https://github.com/scikit-learn/scikit-learn/blob/master/CONTRIBUTING.md). https://opensource.guide/how-to-contribute/ is a great resource on contributing to open-source in general. 
Thanks @lesteve , As far as I understood, presort takes bool or  by default auto, but even if we are giving 
`GradientBoostingClassifier(presort='invalid').fit([[1., 2.], [1., 1.]], [0, 1])
 `
It is not throwing a ValueError, so we should add a check for it.
Yep exactly you will need to add a test in sklearn/ensemble/tests/test_gradient_boosting.py. Add your tests in test_classifier_parameter_checks and test_regressor_parameter_checks. Use assert_raise_message to check the error message as well as the exception type.

Commit message: pep8 fix 3

Patch to review:
diff --git a/sklearn/ensemble/gradient_boosting.py b/sklearn/ensemble/gradient_boosting.py
index 2c155f11c6282..1bdd72b95a955 100644
--- a/sklearn/ensemble/gradient_boosting.py
+++ b/sklearn/ensemble/gradient_boosting.py
@@ -1046,12 +1046,14 @@ def fit(self, X, y, sample_weight=None, monitor=None):
         elif presort == 'auto':
             presort = True
 
-        if presort == True:
+        if presort is True:
             if issparse(X):
                 raise ValueError("Presorting is not supported for sparse matrices.")
             else:
                 X_idx_sorted = np.asfortranarray(np.argsort(X, axis=0),
                                                  dtype=np.int32)
+        elif presort is not False:
+            raise ValueError("Presort must be a boolean")
 
         # fit the boosting stages
         n_stages = self._fit_stages(X, y, y_pred, sample_weight, self._rng,
diff --git a/sklearn/ensemble/tests/test_gradient_boosting.py b/sklearn/ensemble/tests/test_gradient_boosting.py
index f4594529e034b..7ef2ff5ebb5b7 100644
--- a/sklearn/ensemble/tests/test_gradient_boosting.py
+++ b/sklearn/ensemble/tests/test_gradient_boosting.py
@@ -82,6 +82,12 @@ def test_classification_toy():
 
 def test_classifier_parameter_checks():
     # Check input parameter validation for GradientBoostingClassifier.
+    assert_raise_message(ValueError, "Presort must be a boolean",
+                         GradientBoostingClassifier(presort='invalid')
+                         .fit, X, y)
+    assert_raise_message(ValueError, "Presort must be a boolean",
+                         GradientBoostingClassifier(presort='valid')
+                         .fit, X, y)
 
     assert_raises(ValueError,
                   GradientBoostingClassifier(n_estimators=0).fit, X, y)
@@ -158,6 +164,14 @@ def test_regressor_parameter_checks():
                          " or an integer. 'invalid' was passed",
                          GradientBoostingRegressor(n_iter_no_change='invalid')
                          .fit, X, y)
+    assert_raise_message(ValueError, "Presort must be a boolean",
+                         GradientBoostingRegressor(loss='huber',
+                                                   presort='invalid')
+                         .fit, X, y)
+    assert_raise_message(ValueError, "Presort must be a boolean",
+                         GradientBoostingRegressor(loss='quantile',
+                                                   presort='valid')
+                         .fit, X, y)
 
 
 def test_loss_function():
@@ -201,7 +215,8 @@ def check_classification_synthetic(presort, loss):
 
 
 def test_classification_synthetic():
-    for presort, loss in product(('auto', True, False), ('deviance', 'exponential')):
+    for presort, loss in product(('auto', True, False),
+                                 ('deviance', 'exponential')):
         yield check_classification_synthetic, presort, loss
 
 


Code review comments:
diff_hunk: @@ -158,6 +164,14 @@ def test_regressor_parameter_checks():
                          " or an integer. 'invalid' was passed",
                          GradientBoostingRegressor(n_iter_no_change='invalid')
                          .fit, X, y)
+    assert_raise_message(ValueError, "Presort must be a boolean",
+                         GradientBoostingRegressor(loss='huber',
+                                                   presort='invalid')
+                         .fit, X, y)
+    assert_raise_message(ValueError, "Presort must be a boolean",
path: sklearn/ensemble/tests/test_gradient_boosting.py
text: @user1:
This one seems also unnecessary? Also, why do you need to specify loss? It seems only a test for presort?