Properly handle bounds in isotonic regression with np.clip (scikit-learn#6949)

Author: bnaul

sklearn/isotonic.py

@@ -129,24 +129,19 @@ def isotonic_regression(y, sample_weight=None, y_min=None, y_max=None,
         y = y[::-1]
         sample_weight = sample_weight[::-1]
 
-    if y_min is not None or y_max is not None:
-        y = np.copy(y)
-        sample_weight = np.copy(sample_weight)
-        # upper bound on the cost function
-        C = np.dot(sample_weight, y * y) * 10
-        if y_min is not None:
-            y[0] = y_min
-            sample_weight[0] = C
-        if y_max is not None:
-            y[-1] = y_max
-            sample_weight[-1] = C
-
     solution = np.empty(len(y))
     y_ = _isotonic_regression(y, sample_weight, solution)
-    if increasing:
-        return y_
-    else:
-        return y_[::-1]
+    if not increasing:
+        y_ = y_[::-1]
+
+    if y_min is not None or y_max is not None:
+        # Older versions of np.clip don't accept None as a bound, so use np.inf
+        if y_min is None:
+            y_min = -np.inf
+        if y_max is None:
+            y_max = np.inf
+        np.clip(y_, y_min, y_max, y_)
+    return y_
 
 
 class IsotonicRegression(BaseEstimator, TransformerMixin, RegressorMixin):

sklearn/tests/test_isotonic.py

@@ -338,6 +338,29 @@ def test_isotonic_duplicate_min_entry():
     assert_true(all_predictions_finite)
 
 
+def test_isotonic_ymin_ymax():
+    # Test from @NelleV's issue:
+    # https://github.com/scikit-learn/scikit-learn/issues/6921
+    x = np.array([1.263, 1.318, -0.572, 0.307, -0.707, -0.176, -1.599, 1.059,
+                  1.396, 1.906, 0.210, 0.028, -0.081, 0.444, 0.018, -0.377,
+                  -0.896, -0.377, -1.327, 0.180])
+    y = isotonic_regression(x, y_min=0., y_max=0.1)
+
+    assert(np.all(y >= 0))
+    assert(np.all(y <= 0.1))
+
+    # Also test decreasing case since the logic there is different
+    y = isotonic_regression(x, y_min=0., y_max=0.1, increasing=False)
+
+    assert(np.all(y >= 0))
+    assert(np.all(y <= 0.1))
+
+    # Finally, test with only one bound
+    y = isotonic_regression(x, y_min=0., increasing=False)
+
+    assert(np.all(y >= 0))
+
+
 def test_isotonic_zero_weight_loop():
     # Test from @ogrisel's issue:
     # https://github.com/scikit-learn/scikit-learn/issues/4297