Repository: scikit-learn/scikit-learn
Pull request: Fix _estimate_mi discrete_features str and value check
#### Reference Issues/PRs

Fixes #13481

#### What does this implement/fix? Explain your changes.

In `_estimate_mi` the parameter `discrete_features` can be a string value 'auto' as well as an array of indices or a boolean mask.  Code initially checks if `discrete_features == 'auto'` which will error in future versions of numpy.  Fix first checks str instance and then for valid value.


Problem statement:
Comparing string to array in _estimate_mi
In ``_estimate_mi`` there is ``discrete_features == 'auto'`` but discrete features can be an array of indices or a boolean mask.
This will error in future versions of numpy.
Also this means we never test this function with discrete features != 'auto', it seems?

Hints from issue discussion:
I'll take this
@hermidalc go for it :)
i'm not sure ,but i think user will change the default value if it seem to be array or boolean mask....bcz auto is  default value it is not fixed.
I haven't understood, @punkstar25

Commit message: discrete_features str and value check

Patch to review:
diff --git a/sklearn/feature_selection/mutual_info_.py b/sklearn/feature_selection/mutual_info_.py
index d5d1b7fb59db9..3a32261e5b61d 100644
--- a/sklearn/feature_selection/mutual_info_.py
+++ b/sklearn/feature_selection/mutual_info_.py
@@ -247,8 +247,11 @@ def _estimate_mi(X, y, discrete_features='auto', discrete_target=False,
     X, y = check_X_y(X, y, accept_sparse='csc', y_numeric=not discrete_target)
     n_samples, n_features = X.shape
 
-    if discrete_features == 'auto':
-        discrete_features = issparse(X)
+    if isinstance(discrete_features, str):
+        if discrete_features == 'auto':
+            discrete_features = issparse(X)
+        else:
+            raise ValueError("Invalid string value for discrete_features.")
 
     if isinstance(discrete_features, bool):
         discrete_mask = np.empty(n_features, dtype=bool)


Code review comments:
diff_hunk: @@ -247,8 +247,11 @@ def _estimate_mi(X, y, discrete_features='auto', discrete_target=False,
     X, y = check_X_y(X, y, accept_sparse='csc', y_numeric=not discrete_target)
     n_samples, n_features = X.shape
 
-    if discrete_features == 'auto':
-        discrete_features = issparse(X)
+    if isinstance(discrete_features, str):
path: sklearn/feature_selection/mutual_info_.py
text: @user1:
Maybe have this whole block:

```
    if isinstance(discrete_features, str):
        if discrete_features == 'auto':
            discrete_features = issparse(X)
        else:
            raise ValueError("Invalid string value for discrete_features.")

    if isinstance(discrete_features, bool):
        discrete_mask = np.empty(n_features, dtype=bool)
        discrete_mask.fill(discrete_features)
    else:
        discrete_features = np.asarray(discrete_features)
        if discrete_features.dtype != 'bool':
            discrete_mask = np.zeros(n_features, dtype=bool)
            discrete_mask[discrete_features] = True
        else:
            discrete_mask = discrete_features
```

in a series of if/elses and also use `utils.validaton.check_array` instead of
`np.asarray`?

Then in the last `else` you can raise an exception and complain about the given
`discrete_features` not supported.

@author:
@user1 if I'm understanding you correctly, in order to change this to a series of if/elifs with a final else where exception is raised then will need to explicitly test if `discrete_features` is "array like" instead of letting this case fall to final else.  Currently numpy.asarray conversion is implicitly doing that.  For second elif is there a better way to do that?

```python
if isinstance(discrete_features, str):
    ...
elif isinstance(discrete_features, bool):
    ...
elif check_array(discrete_features):
    discrete_features = check_array(discrete_features)
    ...
else:
   raise ValueError("Invalid value for discrete_features.")

```

@user1:
Yes, I think it's a good idea to explicitly check if it's array-like. Just note that `check_array` returns the array, not a boolean. Maybe something like:
``` python
if isinstance(discrete_features, str):
    ...
elif isinstance(discrete_features, bool):
    ...
else:
    try:
        discrete_features = check_array(discrete_features)
        ...
    except:
       raise ValueError("Invalid value for discrete_features.")

@author:
> ```python
> if isinstance(discrete_features, str):
>     ...
> elif isinstance(discrete_features, bool):
>     ...
> else:
>     try:
>         discrete_features = check_array(discrete_features)
>         ...
>     except:
>        raise ValueError("Invalid value for discrete_features.")
> ```

Don't think we need the try/except around `check_array` since function already raises appropriate errors from within.  Also since str and bool cases use overlapping code I structured the if statement a bit differently. In latest commit you will see finished impl.