Repository: scikit-learn/scikit-learn
Pull request: Fix predict method for multiclass multioutput ensemble models
<!--
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/master/CONTRIBUTING.md#pull-request-checklist
-->

#### Reference Issues/PRs
Fixes #12831.
<!--
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
-->


#### What does this implement/fix? Explain your changes.
This PR fixes a bug where the `predict` method would fail for multiclass multioutput ensemble models, if any of the dependent variables were strings. The underlying issue was preallocating the `predict` output using `np.zeros`, which would then error when string predictions were inserted. I replaced the function call with a more dtype-agnostic call to `np.empty`.

#### Any other comments?


<!--
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:
`predict` fails for multioutput ensemble models with non-numeric DVs
#### Description
<!-- Example: Joblib Error thrown when calling fit on LatentDirichletAllocation with evaluate_every > 0-->
Multioutput forest models assume that the dependent variables are numeric. Passing string DVs returns the following error:

`ValueError: could not convert string to float:`

I'm going to take a stab at submitting a fix today, but I wanted to file an issue to document the problem in case I'm not able to finish a fix.

#### Steps/Code to Reproduce
I wrote a test based on `ensemble/tests/test_forest:test_multioutput` which currently fails:

```
def check_multioutput_string(name):
    # Check estimators on multi-output problems with string outputs.

    X_train = [[-2, -1], [-1, -1], [-1, -2], [1, 1], [1, 2], [2, 1], [-2, 1],
               [-1, 1], [-1, 2], [2, -1], [1, -1], [1, -2]]
    y_train = [["red", "blue"], ["red", "blue"], ["red", "blue"], ["green", "green"],
               ["green", "green"], ["green", "green"], ["red", "purple"],
               ["red", "purple"], ["red", "purple"], ["green", "yellow"],
               ["green", "yellow"], ["green", "yellow"]]
    X_test = [[-1, -1], [1, 1], [-1, 1], [1, -1]]
    y_test = [["red", "blue"], ["green", "green"], ["red", "purple"], ["green", "yellow"]]

    est = FOREST_ESTIMATORS[name](random_state=0, bootstrap=False)
    y_pred = est.fit(X_train, y_train).predict(X_test)
    assert_array_almost_equal(y_pred, y_test)

    if name in FOREST_CLASSIFIERS:
        with np.errstate(divide="ignore"):
            proba = est.predict_proba(X_test)
            assert_equal(len(proba), 2)
            assert_equal(proba[0].shape, (4, 2))
            assert_equal(proba[1].shape, (4, 4))

            log_proba = est.predict_log_proba(X_test)
            assert_equal(len(log_proba), 2)
            assert_equal(log_proba[0].shape, (4, 2))
            assert_equal(log_proba[1].shape, (4, 4))


@pytest.mark.filterwarnings('ignore:The default value of n_estimators')
@pytest.mark.parametrize('name', FOREST_CLASSIFIERS_REGRESSORS)
def test_multioutput_string(name):
    check_multioutput_string(name)
```

#### Expected Results
No error is thrown, can run `predict` for all ensemble multioutput models
<!-- Example: No error is thrown. Please paste or describe the expected results.-->

#### Actual Results
<!-- Please paste or specifically describe the actual output or traceback. -->
`ValueError: could not convert string to float: <DV class>`

#### Versions
I replicated this error using the current master branch of sklearn (0.21.dev0).
<!--
Please run the following snippet and paste the output below.
For scikit-learn >= 0.20:
import sklearn; sklearn.show_versions()
For scikit-learn < 0.20:
import platform; print(platform.platform())
import sys; print("Python", sys.version)
import numpy; print("NumPy", numpy.__version__)
import scipy; print("SciPy", scipy.__version__)
import sklearn; print("Scikit-Learn", sklearn.__version__)
-->


<!-- Thanks for contributing! -->

Hints from issue discussion:
Is numeric-only an intentional limitation in this case? There are lines that explicitly cast to double (https://github.com/scikit-learn/scikit-learn/blob/e73acef80de4159722b11e3cd6c20920382b9728/sklearn/ensemble/forest.py#L279). It's not an issue for single-output models, though.
Sorry what do you mean by "DV"?
You're using the regressors:
FOREST_CLASSIFIERS_REGRESSORS

They are not supposed to work with classes, you want the classifiers.

Can you please provide a minimum self-contained example?
Ah, sorry. "DV" is "dependent variable". 

Here's an example:
```
X_train = [[-2, -1], [-1, -1], [-1, -2], [1, 1], [1, 2], [2, 1], [-2, 1],
               [-1, 1], [-1, 2], [2, -1], [1, -1], [1, -2]]
y_train = [["red", "blue"], ["red", "blue"], ["red", "blue"], ["green", "green"],
               ["green", "green"], ["green", "green"], ["red", "purple"],
               ["red", "purple"], ["red", "purple"], ["green", "yellow"],
               ["green", "yellow"], ["green", "yellow"]]
X_test = [[-1, -1], [1, 1], [-1, 1], [1, -1]]
est = RandomForestClassifier()
est.fit(X_train, y_train)
y_pred = est.predict(X_test)
```

Returns:
```
---------------------------------------------------------------------------
ValueError                                Traceback (most recent call last)
<ipython-input-5-a3b5313a012b> in <module>
----> 1 y_pred = est.predict(X_test)

~/repos/forks/scikit-learn/sklearn/ensemble/forest.py in predict(self, X)
    553                 predictions[:, k] = self.classes_[k].take(np.argmax(proba[k],
    554                                                                     axis=1),
--> 555                                                           axis=0)
    556
    557             return predictions

ValueError: could not convert string to float: 'green'
```
Thanks, this indeed looks like bug.

The multi-output multi-class support is fairly untested tbh and I'm not a big fan of it (we don't implement ``score`` for that case!).
So even if we fix this it's likely you'll run into more issues. I have been arguing for removing this feature for a while (which is probably not what you wanted to hear ;)
For what it's worth, I think this specific issue may be resolved with a one-line fix. For my use case, having `predict` and `predict_proba` work is all I need.
Feel free to submit a PR if you like

Commit message: BUG fix predict method for multiclass multioutput ensemble models

Patch to review:
diff --git a/sklearn/ensemble/forest.py b/sklearn/ensemble/forest.py
index 33d166d00969e..579d3f3d7b94f 100644
--- a/sklearn/ensemble/forest.py
+++ b/sklearn/ensemble/forest.py
@@ -547,7 +547,8 @@ def predict(self, X):
 
         else:
             n_samples = proba[0].shape[0]
-            predictions = np.zeros((n_samples, self.n_outputs_))
+            predictions = np.empty((n_samples, self.n_outputs_),
+                                   dtype='object')
 
             for k in range(self.n_outputs_):
                 predictions[:, k] = self.classes_[k].take(np.argmax(proba[k],
diff --git a/sklearn/ensemble/tests/test_forest.py b/sklearn/ensemble/tests/test_forest.py
index 478c9b2dd1cab..275dfb523fe6d 100644
--- a/sklearn/ensemble/tests/test_forest.py
+++ b/sklearn/ensemble/tests/test_forest.py
@@ -548,6 +548,37 @@ def test_multioutput(name):
     check_multioutput(name)
 
 
+@pytest.mark.filterwarnings('ignore:The default value of n_estimators')
+@pytest.mark.parametrize('name', FOREST_CLASSIFIERS)
+def test_multioutput_string(name):
+    # Check estimators on multi-output problems with string outputs.
+
+    X_train = [[-2, -1], [-1, -1], [-1, -2], [1, 1], [1, 2], [2, 1], [-2, 1],
+               [-1, 1], [-1, 2], [2, -1], [1, -1], [1, -2]]
+    y_train = [["red", "blue"], ["red", "blue"], ["red", "blue"],
+               ["green", "green"], ["green", "green"], ["green", "green"],
+               ["red", "purple"], ["red", "purple"], ["red", "purple"],
+               ["green", "yellow"], ["green", "yellow"], ["green", "yellow"]]
+    X_test = [[-1, -1], [1, 1], [-1, 1], [1, -1]]
+    y_test = [["red", "blue"], ["green", "green"],
+              ["red", "purple"], ["green", "yellow"]]
+
+    est = FOREST_ESTIMATORS[name](random_state=0, bootstrap=False)
+    y_pred = est.fit(X_train, y_train).predict(X_test)
+    assert_array_equal(y_pred, y_test)
+
+    with np.errstate(divide="ignore"):
+        proba = est.predict_proba(X_test)
+        assert_equal(len(proba), 2)
+        assert_equal(proba[0].shape, (4, 2))
+        assert_equal(proba[1].shape, (4, 4))
+
+        log_proba = est.predict_log_proba(X_test)
+        assert_equal(len(log_proba), 2)
+        assert_equal(log_proba[0].shape, (4, 2))
+        assert_equal(log_proba[1].shape, (4, 4))
+
+
 def check_classes_shape(name):
     # Test that n_classes_ and classes_ have proper shape.
     ForestClassifier = FOREST_CLASSIFIERS[name]


Code review comments:
diff_hunk: @@ -548,6 +548,37 @@ def test_multioutput(name):
     check_multioutput(name)
 
 
+@pytest.mark.filterwarnings('ignore:The default value of n_estimators')
+@pytest.mark.parametrize('name', FOREST_CLASSIFIERS)
+def test_multioutput_string(name):
+    # Check estimators on multi-output problems with string outputs.
+
+    X_train = [[-2, -1], [-1, -1], [-1, -2], [1, 1], [1, 2], [2, 1], [-2, 1],
+               [-1, 1], [-1, 2], [2, -1], [1, -1], [1, -2]]
+    y_train = [["red", "blue"], ["red", "blue"], ["red", "blue"],
+               ["green", "green"], ["green", "green"], ["green", "green"],
+               ["red", "purple"], ["red", "purple"], ["red", "purple"],
+               ["green", "yellow"], ["green", "yellow"], ["green", "yellow"]]
+    X_test = [[-1, -1], [1, 1], [-1, 1], [1, -1]]
+    y_test = [["red", "blue"], ["green", "green"],
+              ["red", "purple"], ["green", "yellow"]]
+
+    est = FOREST_ESTIMATORS[name](random_state=0, bootstrap=False)
+    y_pred = est.fit(X_train, y_train).predict(X_test)
+    assert_array_equal(y_pred, y_test)
+
+    with np.errstate(divide="ignore"):
+        proba = est.predict_proba(X_test)
+        assert_equal(len(proba), 2)
path: sklearn/ensemble/tests/test_forest.py
text: @user1:
With the adoption of pytest, we are phasing out use of test helpers `assert_equal`, `assert_true`, etc. Please use bare `assert` statements, e.g. `assert x == y`, `assert not x`, etc.

diff_hunk: @@ -547,7 +547,8 @@ def predict(self, X):
 
         else:
             n_samples = proba[0].shape[0]
-            predictions = np.zeros((n_samples, self.n_outputs_))
+            predictions = np.empty((n_samples, self.n_outputs_),
+                                   dtype='object')
path: sklearn/ensemble/forest.py
text: @user1:
wouldn't it be better to have `dtype=self.classes_.dtype` or something?