Repository: scikit-learn/scikit-learn
Pull request: FIX validate handle_unkown strategies in OrdinalEncoder
closes #19228 

Check the strategy passed to `handle_unknown` in `OrdinalEncoder`.

Problem statement:
OrdinalEncoder does not check the input of handle_unknown
During a lecture today, the following was working:

```python
from sklearn.preprocessing import OrdinalEncoder

enc = OrdinalEncoder(handle_unknown="ignore")
X = [['Male', 1], ['Female', 3], ['Female', 2]]
enc.fit_transform(X)
```

```
array([[1., 0.],
       [0., 2.],
       [0., 1.]])
```

However, `handle_unknown` should be one of the following strategy: `{‘error’, ‘use_encoded_value’}`.
Thus, we should probably raise an error because the strategy is not definend.

Hints from issue discussion:

Commit message: pr number

Patch to review:
diff --git a/doc/whats_new/v0.24.rst b/doc/whats_new/v0.24.rst
index ffa6fe96ec3c5..f549b31f51aa7 100644
--- a/doc/whats_new/v0.24.rst
+++ b/doc/whats_new/v0.24.rst
@@ -2,6 +2,24 @@
 
 .. currentmodule:: sklearn
 
+.. _changes_0_24_2:
+
+Version 0.24.2
+==============
+
+**TBD 2021**
+
+Changelog
+---------
+
+:mod:`sklearn.preprocessing`
+............................
+
+- |Fix| Validate the constructor parameter `handle_unknown` in
+  :class:`preprocessing.OrdinalEncoder` to only allow for `'error'` and
+  `'use_encoded_value'` strategies.
+  :pr:`19234` by `Guillaume Lemaitre <glemaitre>`.
+
 .. _changes_0_24_1:
 
 Version 0.24.1
diff --git a/sklearn/preprocessing/_encoders.py b/sklearn/preprocessing/_encoders.py
index 6d59cb5a0d858..b78431a652c1c 100644
--- a/sklearn/preprocessing/_encoders.py
+++ b/sklearn/preprocessing/_encoders.py
@@ -740,6 +740,14 @@ def fit(self, X, y=None):
         -------
         self
         """
+        handle_unknown_strategies = ("error", "use_encoded_value")
+        if self.handle_unknown not in handle_unknown_strategies:
+            raise ValueError(
+                f"Unknown strategy for handle_unknown attribute. The strategy "
+                f"should be one of: {', '.join(handle_unknown_strategies)}. "
+                f"Got {self.handle_unknown} instead."
+            )
+
         if self.handle_unknown == 'use_encoded_value':
             if is_scalar_nan(self.unknown_value):
                 if np.dtype(self.dtype).kind != 'f':
diff --git a/sklearn/preprocessing/tests/test_encoders.py b/sklearn/preprocessing/tests/test_encoders.py
index 213aa85047574..95c39739ae944 100644
--- a/sklearn/preprocessing/tests/test_encoders.py
+++ b/sklearn/preprocessing/tests/test_encoders.py
@@ -624,33 +624,48 @@ def test_ordinal_encoder_handle_unknowns_numeric(dtype):
     assert_array_equal(X_trans_inv, inv_exp)
 
 
-def test_ordinal_encoder_handle_unknowns_raise():
+@pytest.mark.parametrize(
+    "params, err_type, err_msg",
+    [
+        (
+            {"handle_unknown": "use_encoded_value"},
+            TypeError,
+            "unknown_value should be an integer or np.nan when handle_unknown "
+            "is 'use_encoded_value', got None.",
+        ),
+        (
+            {"unknown_value": -2},
+            TypeError,
+            "unknown_value should only be set when handle_unknown is "
+            "'use_encoded_value', got -2.",
+        ),
+        (
+            {"handle_unknown": "use_encoded_value", "unknown_value": "bla"},
+            TypeError,
+            "unknown_value should be an integer or np.nan when handle_unknown "
+            "is 'use_encoded_value', got bla.",
+        ),
+        (
+            {"handle_unknown": "use_encoded_value", "unknown_value": 1},
+            ValueError,
+            "The used value for unknown_value (1) is one of the values "
+            "already used for encoding the seen categories.",
+        ),
+        (
+            {"handle_unknown": "ignore"},
+            ValueError,
+            "Unknown strategy for handle_unknown attribute. The strategy "
+            "should be one of: error, use_encoded_value. Got ignore instead.",
+        ),
+    ],
+)
+def test_ordinal_encoder_handle_unknowns_raise(params, err_type, err_msg):
+    # Check error message when validating input parameters
     X = np.array([['a', 'x'], ['b', 'y']], dtype=object)
 
-    enc = OrdinalEncoder(handle_unknown='use_encoded_value')
-    msg = ("unknown_value should be an integer or np.nan when handle_unknown "
-           "is 'use_encoded_value', got None.")
-    with pytest.raises(TypeError, match=msg):
-        enc.fit(X)
-
-    enc = OrdinalEncoder(unknown_value=-2)
-    msg = ("unknown_value should only be set when handle_unknown is "
-           "'use_encoded_value', got -2.")
-    with pytest.raises(TypeError, match=msg):
-        enc.fit(X)
-
-    enc = OrdinalEncoder(handle_unknown='use_encoded_value',
-                         unknown_value='bla')
-    msg = ("unknown_value should be an integer or np.nan when handle_unknown "
-           "is 'use_encoded_value', got bla.")
-    with pytest.raises(TypeError, match=msg):
-        enc.fit(X)
-
-    enc = OrdinalEncoder(handle_unknown='use_encoded_value', unknown_value=1)
-    msg = ("The used value for unknown_value (1) is one of the values already "
-           "used for encoding the seen categories.")
-    with pytest.raises(ValueError, match=msg):
-        enc.fit(X)
+    encoder = OrdinalEncoder(**params)
+    with pytest.raises(err_type, match=err_msg):
+        encoder.fit(X)
 
 
 def test_ordinal_encoder_handle_unknowns_nan():


Code review comments:
diff_hunk: @@ -624,33 +624,48 @@ def test_ordinal_encoder_handle_unknowns_numeric(dtype):
     assert_array_equal(X_trans_inv, inv_exp)
 
 
-def test_ordinal_encoder_handle_unknowns_raise():
+@pytest.mark.parametrize(
+    "params, err_type, err_msg",
+    [
+        (
+            {"handle_unknown": "use_encoded_value"},
+            TypeError,
+            "unknown_value should be an integer or np.nan when handle_unknown "
+            "is 'use_encoded_value', got None.",
+        ),
+        (
+            {"unknown_value": -2},
+            TypeError,
+            "unknown_value should only be set when handle_unknown is "
+            "'use_encoded_value', got -2.",
+        ),
+        (
+            {"handle_unknown": "use_encoded_value", "unknown_value": "bla"},
+            TypeError,
+            "unknown_value should be an integer or np.nan when handle_unknown "
+            "is 'use_encoded_value', got bla.",
+        ),
+        (
+            {"handle_unknown": "use_encoded_value", "unknown_value": 1},
+            ValueError,
+            "The used value for unknown_value (1) is one of the values "
+            "already used for encoding the seen categories.",
+        ),
+        (
+            {"handle_unknown": "ignore"},
+            ValueError,
+            "Unknown strategy for handle_unknown attribute. The strategy "
path: sklearn/preprocessing/tests/test_encoders.py
text: @user1:
For consistency, I think we should use error message from `OneHotEncoder` for `handle_unknown`.

https://github.com/scikit-learn/scikit-learn/blob/4db51e6cbc1639f2e08059bc625bdd0c16f03a78/sklearn/preprocessing/_encoders.py#L325-L327