Repository: scikit-learn/scikit-learn
Pull request: FIX raise an error if nan is not the last element in user provided categories
<!--
Thanks for contributing a pull request! Please ensure you have taken a look at
the contribution guidelines: https://github.com/scikit-learn/scikit-learn/blob/main/CONTRIBUTING.md
-->

#### Reference Issues/PRs
<!--
Example: Fixes #1234. See also #3456.
Please use keywords (e.g., Fixes) to create link to the issues or pull requests
you resolved, so that they will automatically be closed when your pull request
is merged. See https://github.com/blog/1506-closing-issues-via-pull-requests
-->
Fixes #27088

#### What does this implement/fix? Explain your changes.
For encoders, we should check if user provided categories contain `nan`. If there is and nan is not the last element, we should raise an error.

#### Any other comments?
There is another issue to handle duplicated values in user provided categories. I'll follow up another PR.

<!--
Please be aware that we are a loose team of volunteers so patience is
necessary; assistance handling other issues is very welcome. We value
all user contributions, no matter how minor they are. If we are slow to
review, either the pull request needs some benchmarking, tinkering,
convincing, etc. or more likely the reviewers are simply busy. In either
case, we ask for your understanding during the review process.
For more information, see our FAQ on this topic:
http://scikit-learn.org/dev/faq.html#why-is-my-pull-request-not-getting-any-attention.

Thanks for contributing!
-->


Problem statement:
Wrong infrequent categories and error in OrdinalEncoder
### Describe the bug

When I manually set the numpy object to `categories` in OrdinalEncoder, I got wrong `infrequent_categories_`. 
If I run `fit_transform`, then I got an error. See the code below.

### Steps/Code to Reproduce

```python
import numpy as np
from sklearn.preprocessing import OneHotEncoder, OrdinalEncoder

categories = [np.array([np.nan, 'b', 'c', 'a'], dtype=object)]
X = np.array([[np.nan]*2+['b']*2+['a']],dtype=object).T

ohe = OneHotEncoder(categories=categories, min_frequency=2)
ode = OrdinalEncoder(categories=categories, min_frequency=2)

ohe.fit(X)
ode.fit(X)

print('onehot', ohe.infrequent_categories_)
print('ordinal', ode.infrequent_categories_)

print(ohe.fit_transform(X))
print(ode.fit_transform(X))
```

### Expected Results

```
onehot [array(['c', 'a'], dtype=object)]
ordinal [array(['c', 'a'], dtype=object)]
  (0, 0)	1.0
  (1, 0)	1.0
  (2, 1)	1.0
  (3, 1)	1.0
  (4, 2)	1.0
[[nan]
 [nan]
 [0.]
 [0.]
 [ 1.]]
```

### Actual Results

```
onehot [array(['c', 'a'], dtype=object)]
ordinal [array(['b', 'c'], dtype=object)]
  (0, 0)	1.0
  (1, 0)	1.0
  (2, 1)	1.0
  (3, 1)	1.0
  (4, 2)	1.0
Traceback (most recent call last):
  File "tt.py", line 17, in <module>
    print(ode.fit_transform(X))
  File "/Users/xxf/miniconda3/lib/python3.8/site-packages/sklearn/utils/_set_output.py", line 140, in wrapped
    data_to_wrap = f(self, X, *args, **kwargs)
  File "/Users/xxf/miniconda3/lib/python3.8/site-packages/sklearn/base.py", line 915, in fit_transform
    return self.fit(X, **fit_params).transform(X)
  File "/Users/xxf/miniconda3/lib/python3.8/site-packages/sklearn/utils/_set_output.py", line 140, in wrapped
    data_to_wrap = f(self, X, *args, **kwargs)
  File "/Users/xxf/miniconda3/lib/python3.8/site-packages/sklearn/preprocessing/_encoders.py", line 1573, in transform
    X_int, X_mask = self._transform(
  File "/Users/xxf/miniconda3/lib/python3.8/site-packages/sklearn/preprocessing/_encoders.py", line 236, in _transform
    self._map_infrequent_categories(X_int, X_mask, ignore_category_indices)
  File "/Users/xxf/miniconda3/lib/python3.8/site-packages/sklearn/preprocessing/_encoders.py", line 437, in _map_infrequent_categories
    X_int[rows_to_update, i] = np.take(mapping, X_int[rows_to_update, i])
  File "<__array_function__ internals>", line 200, in take
  File "/Users/xxf/miniconda3/lib/python3.8/site-packages/numpy/core/fromnumeric.py", line 190, in take
    return _wrapfunc(a, 'take', indices, axis=axis, out=out, mode=mode)
  File "/Users/xxf/miniconda3/lib/python3.8/site-packages/numpy/core/fromnumeric.py", line 57, in _wrapfunc
    return bound(*args, **kwds)
IndexError: index 3 is out of bounds for axis 0 with size 3
```

### Versions

```shell
System:
    python: 3.8.16 (default, Jan 17 2023, 16:39:35)  [Clang 14.0.6 ]
executable: /Users/xxf/miniconda3/bin/python
   machine: macOS-13.5-arm64-arm-64bit

Python dependencies:
      sklearn: 1.3.0
          pip: 22.3.1
   setuptools: 65.6.3
        numpy: 1.24.2
        scipy: 1.10.1
       Cython: None
       pandas: 1.5.3
   matplotlib: 3.7.2
       joblib: 1.2.0
threadpoolctl: 3.1.0

Built with OpenMP: True

threadpoolctl info:
       user_api: openmp
   internal_api: openmp
         prefix: libomp
       filepath: /Users/xxf/miniconda3/lib/python3.8/site-packages/sklearn/.dylibs/libomp.dylib
        version: None
    num_threads: 8

       user_api: blas
   internal_api: openblas
         prefix: libopenblas
       filepath: /Users/xxf/miniconda3/lib/python3.8/site-packages/numpy/.dylibs/libopenblas64_.0.dylib
        version: 0.3.21
threading_layer: pthreads
   architecture: armv8
    num_threads: 8

       user_api: blas
   internal_api: openblas
         prefix: libopenblas
       filepath: /Users/xxf/miniconda3/lib/python3.8/site-packages/scipy/.dylibs/libopenblas.0.dylib
        version: 0.3.18
threading_layer: pthreads
   architecture: armv8
    num_threads: 8
```

Hints from issue discussion:
For numerical categories, nan should be the last element if there are nans. But for other types of categories, e.g., object, do we need to add this constraint?
Would you like to take a look at this? @thomasjpfan 
Yea, the current infrequent API assumes that nans are the last element even for object dtypes. For now, I think we should check user provided categories and raise an error if the `nan` is not at the end.
Okay, I will work on that. What if user provided categories contain duplicated values, maybe duplicated nans, does that need to raise an error?
> What if user provided categories contain duplicated values, maybe duplicated nans, does that need to raise an error?

I'll say handling duplicate values is a separate issue and should be in a different PR. It's better to handle nan order first to keep the PRs scope smaller. Smaller PRs have are easier to review and usually get merged faster.
Ok, I'll follow up another PR to fix the issue of duplicate values.
PR #27309 fixes the nan issue. And PR #27328 fixes the duplicate values issue.

Commit message: add an entry in changelog v1.4

Patch to review:
diff --git a/doc/whats_new/v1.4.rst b/doc/whats_new/v1.4.rst
index 2a631d43ac5c2..1390ee594ceff 100644
--- a/doc/whats_new/v1.4.rst
+++ b/doc/whats_new/v1.4.rst
@@ -450,6 +450,10 @@ Changelog
 - |Enhancement| :class:`preprocessing.TargetEncoder` now supports `target_type`
   'multiclass'. :pr:`26674` by :user:`Lucy Liu <lucyleeow>`.
 
+- |Fix| :class:`preprocessing.OneHotEncoder` and :class:`preprocessing.OrdinalEncoder`
+  will raise an error if `nan` is not the last element in user provided categories.
+  :pr:`27309` by :user:`Xuefeng Xu <xuefeng-xu>`.
+
 :mod:`sklearn.tree`
 ...................
 
diff --git a/sklearn/preprocessing/_encoders.py b/sklearn/preprocessing/_encoders.py
index e0b0ccfd4c239..6e42a0247e1a7 100644
--- a/sklearn/preprocessing/_encoders.py
+++ b/sklearn/preprocessing/_encoders.py
@@ -123,6 +123,15 @@ def _fit(
                     )
                     raise ValueError(msg)
 
+                # `nan` can only be the last stated category
+                for category in cats[:-1]:
+                    if is_scalar_nan(category):
+                        raise ValueError(
+                            "Nan should be the last element in user"
+                            f" provided categories, see categories {cats}"
+                            f" in column #{i}"
+                        )
+
                 if Xi.dtype.kind not in "OUS":
                     sorted_cats = np.sort(cats)
                     error_msg = (
@@ -130,9 +139,7 @@ def _fit(
                     )
                     # if there are nans, nan should be the last element
                     stop_idx = -1 if np.isnan(sorted_cats[-1]) else None
-                    if np.any(sorted_cats[:stop_idx] != cats[:stop_idx]) or (
-                        np.isnan(sorted_cats[-1]) and not np.isnan(sorted_cats[-1])
-                    ):
+                    if np.any(sorted_cats[:stop_idx] != cats[:stop_idx]):
                         raise ValueError(error_msg)
 
                 if handle_unknown == "error":
diff --git a/sklearn/preprocessing/tests/test_encoders.py b/sklearn/preprocessing/tests/test_encoders.py
index 368640bba7fd2..e33a3695ed037 100644
--- a/sklearn/preprocessing/tests/test_encoders.py
+++ b/sklearn/preprocessing/tests/test_encoders.py
@@ -494,12 +494,6 @@ def test_one_hot_encoder_categories(X, cat_exp, cat_dtype):
             [["a", None, "z"]],
             object,
         ),
-        (
-            np.array([["a", np.nan]], dtype=object).T,
-            np.array([["a", None]], dtype=object).T,
-            [["a", np.nan, "z"]],
-            object,
-        ),
     ],
     ids=[
         "object",
@@ -508,7 +502,6 @@ def test_one_hot_encoder_categories(X, cat_exp, cat_dtype):
         "object-string-none",
         "object-string-nan",
         "object-None-and-nan",
-        "object-nan-and-None",
     ],
 )
 def test_one_hot_encoder_specified_categories(X, X2, cats, cat_dtype, handle_unknown):
@@ -548,11 +541,19 @@ def test_one_hot_encoder_unsorted_categories():
     with pytest.raises(ValueError, match=msg):
         enc.fit_transform(X)
 
-    # np.nan must be the last category in categories[0] to be considered sorted
-    X = np.array([[1, 2, np.nan]]).T
-    enc = OneHotEncoder(categories=[[1, np.nan, 2]])
-    with pytest.raises(ValueError, match=msg):
-        enc.fit_transform(X)
+
+@pytest.mark.parametrize("Encoder", [OneHotEncoder, OrdinalEncoder])
+def test_encoder_nan_ending_specified_categories(Encoder):
+    """Test encoder for specified categories that nan is at the end.
+
+    Non-regression test for:
+    https://github.com/scikit-learn/scikit-learn/issues/27088
+    """
+    cats = [np.array([0, np.nan, 1])]
+    enc = Encoder(categories=cats)
+    X = np.array([[0, 1]], dtype=object).T
+    with pytest.raises(ValueError, match="Nan should be the last element"):
+        enc.fit(X)
 
 
 def test_one_hot_encoder_specified_categories_mixed_columns():
@@ -1681,7 +1682,7 @@ def test_ordinal_encoder_missing_value_support_pandas_categorical(
             (
                 np.array([["a", np.nan]], dtype=object).T,
                 np.array([["a", "b"]], dtype=object).T,
-                [np.array(["a", np.nan, "d"], dtype=object)],
+                [np.array(["a", "d", np.nan], dtype=object)],
                 np.object_,
             )
         ),
@@ -1689,7 +1690,7 @@ def test_ordinal_encoder_missing_value_support_pandas_categorical(
             (
                 np.array([["a", np.nan]], dtype=object).T,
                 np.array([["a", "b"]], dtype=object).T,
-                [np.array(["a", np.nan, "d"], dtype=object)],
+                [np.array(["a", "d", np.nan], dtype=object)],
                 np.object_,
             )
         ),


Code review comments:
diff_hunk: @@ -123,13 +123,13 @@ def _fit(
                     )
                     raise ValueError(msg)
 
-                last_idx = len(cats) - 1
-                for idx, category in enumerate(cats):
-                    if is_scalar_nan(category) and idx != last_idx:
+                # `nan` can only be the last stated category
path: sklearn/preprocessing/_encoders.py
text: @user1:
```suggestion
                # `nan` must be the last stated category
```

diff_hunk: @@ -450,6 +450,10 @@ Changelog
 - |Enhancement| :class:`preprocessing.TargetEncoder` now supports `target_type`
   'multiclass'. :pr:`26674` by :user:`Lucy Liu <lucyleeow>`.
 
+- |Fix| :class:`preprocessing.OneHotEncoder` and :class:`preprocessing.OrdinalEncoder`
+  will raise an error if `nan` is not the last element in user provided categories.
path: doc/whats_new/v1.4.rst
text: @user1:
```suggestion
  raises an error when `nan` is a category and is not the last in the user's
  provided categories.
```