FIX change the meaning of include_boundaries in check_scalar (scikit-learn#20921)

glemaitre · jeremiedbb · ogrisel · web-flow · commit 8bf3aa58df6b · 2021-09-07T19:10:56.000+02:00
Co-authored-by: Jérémie du Boisberranger &lt;34657725+jeremiedbb@users.noreply.github.com&gt;
Co-authored-by: Olivier Grisel &lt;olivier.grisel@ensta.org&gt;
diff --git a/sklearn/cluster/_affinity_propagation.py b/sklearn/cluster/_affinity_propagation.py
@@ -272,7 +272,7 @@ class AffinityPropagation(ClusterMixin, BaseEstimator):
     Parameters
     ----------
     damping : float, default=0.5
-        Damping factor (between 0.5 and 1) is the extent to
+        Damping factor in the range `[0.5, 1.0)` is the extent to
         which the current value is maintained relative to
         incoming values (weighted 1 - damping). This in order
         to avoid numerical oscillations when updating these
@@ -469,7 +469,7 @@ def fit(self, X, y=None):
             target_type=numbers.Real,
             min_val=0.5,
             max_val=1,
-            closed="right",
+            include_boundaries="left",
         )
         check_scalar(self.max_iter, "max_iter", target_type=numbers.Integral, min_val=1)
         check_scalar(
diff --git a/sklearn/utils/tests/test_validation.py b/sklearn/utils/tests/test_validation.py
@@ -1032,14 +1032,14 @@ def test_check_scalar_valid(x):
             target_type=numbers.Real,
             min_val=2,
             max_val=5,
-            closed="neither",
+            include_boundaries="both",
         )
     assert len(record) == 0
     assert scalar == x
 
 
 @pytest.mark.parametrize(
-    "x, target_name, target_type, min_val, max_val, closed, err_msg",
+    "x, target_name, target_type, min_val, max_val, include_boundaries, err_msg",
     [
         (
             1,
@@ -1059,7 +1059,7 @@ def test_check_scalar_valid(x):
             2,
             4,
             "neither",
-            ValueError("test_name2 == 1, must be >= 2."),
+            ValueError("test_name2 == 1, must be > 2."),
         ),
         (
             5,
@@ -1068,15 +1068,15 @@ def test_check_scalar_valid(x):
             2,
             4,
             "neither",
-            ValueError("test_name3 == 5, must be <= 4."),
+            ValueError("test_name3 == 5, must be < 4."),
         ),
         (
             2,
             "test_name4",
             int,
             2,
             4,
-            "left",
+            "right",
             ValueError("test_name4 == 2, must be > 2."),
         ),
         (
@@ -1085,13 +1085,25 @@ def test_check_scalar_valid(x):
             int,
             2,
             4,
-            "right",
+            "left",
             ValueError("test_name5 == 4, must be < 4."),
         ),
+        (
+            4,
+            "test_name6",
+            int,
+            2,
+            4,
+            "bad parameter value",
+            ValueError(
+                "Unknown value for `include_boundaries`: 'bad parameter value'. "
+                "Possible values are: ('left', 'right', 'both', 'neither')."
+            ),
+        ),
     ],
 )
 def test_check_scalar_invalid(
-    x, target_name, target_type, min_val, max_val, closed, err_msg
+    x, target_name, target_type, min_val, max_val, include_boundaries, err_msg
 ):
     """Test that check_scalar returns the right error if a wrong input is
     given"""
@@ -1102,7 +1114,7 @@ def test_check_scalar_invalid(
             target_type=target_type,
             min_val=min_val,
             max_val=max_val,
-            closed=closed,
+            include_boundaries=include_boundaries,
         )
     assert str(raised_error.value) == str(err_msg)
     assert type(raised_error.value) == type(err_msg)
diff --git a/sklearn/utils/validation.py b/sklearn/utils/validation.py
@@ -1242,7 +1242,7 @@ def check_scalar(
     *,
     min_val=None,
     max_val=None,
-    closed="neither",
+    include_boundaries="both",
 ):
     """Validate scalar parameters type and value.
 
@@ -1265,9 +1265,15 @@ def check_scalar(
         The maximum valid value the parameter can take. If None (default) it
         is implied that the parameter does not have an upper bound.
 
-    closed : {"left", "right", "both", "neither"}, default="neither"
-        Whether the interval is closed on the left-side, right-side, both or
-        neither.
+    include_boundaries : {"left", "right", "both", "neither"}, default="both"
+        Whether the interval defined by `min_val` and `max_val` should include
+        the boundaries. Possible choices are:
+
+        - `"left"`: only `min_val` is included in the valid interval;
+        - `"right"`: only `max_val` is included in the valid interval;
+        - `"both"`: `min_val` and `max_val` are included in the valid interval;
+        - `"neither"`: neither `min_val` nor `max_val` are included in the
+          valid interval.
 
     Returns
     -------
@@ -1286,22 +1292,29 @@ def check_scalar(
     if not isinstance(x, target_type):
         raise TypeError(f"{name} must be an instance of {target_type}, not {type(x)}.")
 
-    expected_closed = {"left", "right", "both", "neither"}
-    if closed not in expected_closed:
-        raise ValueError(f"Unknown value for `closed`: {closed}")
+    expected_include_boundaries = ("left", "right", "both", "neither")
+    if include_boundaries not in expected_include_boundaries:
+        raise ValueError(
+            f"Unknown value for `include_boundaries`: {repr(include_boundaries)}. "
+            f"Possible values are: {expected_include_boundaries}."
+        )
 
-    comparison_operator = operator.le if closed in ("left", "both") else operator.lt
+    comparison_operator = (
+        operator.lt if include_boundaries in ("left", "both") else operator.le
+    )
     if min_val is not None and comparison_operator(x, min_val):
         raise ValueError(
             f"{name} == {x}, must be"
-            f" {'>' if closed in ('left', 'both') else '>='} {min_val}."
+            f" {'>=' if include_boundaries in ('left', 'both') else '>'} {min_val}."
         )
 
-    comparison_operator = operator.ge if closed in ("right", "both") else operator.gt
+    comparison_operator = (
+        operator.gt if include_boundaries in ("right", "both") else operator.ge
+    )
     if max_val is not None and comparison_operator(x, max_val):
         raise ValueError(
             f"{name} == {x}, must be"
-            f" {'<' if closed in ('right', 'both') else '<='} {max_val}."
+            f" {'<=' if include_boundaries in ('right', 'both') else '<'} {max_val}."
         )
 
     return x
