Repository: scikit-learn/scikit-learn
Pull request: [MRG] Raise exception on providing complex data to estimators
<!--
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#Contributing-Pull-Requests
-->
#### Reference Issue
Fixes #9528 


#### What does this implement/fix? Explain your changes.
An exception should be raised on giving complex data as input to the estimators. The changed code
does the same, via the function check_array. A new test function has been added to test_validation.py
corresponding to the code changes.

#### 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:
Estimators run with complex data and give wrong results
As discussed on the mailing list, estimators run on complex data without error but might produce wrong results.
We should either raise explicit error messages or ensure correctness in each case.

I think as a first step we should error everywhere, and then enable on an estimator basis.

Do we want to add yet another flag to ``check_array`` and ``check_X_y``? That seems the easiest way, though there's already a lot of logic in there....

Hints from issue discussion:
I'm not entirely convinced supporting complex data anywhere is worth the maintenance burden, thinking about it.... but I don't feel very strongly right now? It seems @GaelVaroquaux agrees with this sentiment from the mailing list (though I think he feels more strongly).

I'm quite happy to not deal with this anywhere given the rare use. Dealing with one more finicky detail doesn't sound like great fun.
I believe that we should decide to support complex data nowhere.

The reason is that the support for complex data will always be incomplete and risks being buggy. Indeed, complex data is very infrequent in machine learning (unlike with signal processing). Hence, it will receive little usage. In addition, many machine learning algorithms cannot easily be adapted to complex data. As @amueller puts it, it feels like a case of feature creep (or scope creep).

To manage user expectation and to ensure quality of the codebase, let us error on complex data.
Can I have a look at this one?
I believe that checking dtype_orig.kind == "c" in the function check_array, should be sufficient, is that right?
I'm all good for not supporting complex. @pravarmahajan that sounds
reasonable.

On 13 August 2017 at 04:37, pravarmahajan <notifications@github.com> wrote:

> Can I have a look at this one?
> I believe that checking dtype_orig.kind == "C" in the function
> check_array, should be sufficient, is that right?
>
> —
> You are receiving this because you are subscribed to this thread.
> Reply to this email directly, view it on GitHub
> <https://github.com/scikit-learn/scikit-learn/issues/9528#issuecomment-321998427>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/AAEz6x12ze51IeOkhlq453bv3t2t9Xclks5sXfD3gaJpZM4O03_j>
> .
>

Thank you @jnothman 

I have created a pull request.

The function check_array has grown big and is complicated, would it be worth splitting it into smaller parts?
I think check_array remains quite focused in function. Is there a way to
break it up which actually improves readability?

On 15 Aug 2017 4:24 am, "pravarmahajan" <notifications@github.com> wrote:

> Thank you @jnothman <https://github.com/jnothman>
>
> I have created a pull request.
>
> The function check_array has grown big and is complicated, would it be
> worth splitting it into smaller parts?
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <https://github.com/scikit-learn/scikit-learn/issues/9528#issuecomment-322268831>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/AAEz66l20qM_2cpKzpk4Qna3_G1HaZmpks5sYJDFgaJpZM4O03_j>
> .
>

I looked at it again and I think it's actually not that bad - but then again, I might have written fair chunks of it ;)

Commit message: adding documentation for complex data handling

Patch to review:
diff --git a/sklearn/utils/estimator_checks.py b/sklearn/utils/estimator_checks.py
index c3b066e5e31be..fa8e24c588555 100644
--- a/sklearn/utils/estimator_checks.py
+++ b/sklearn/utils/estimator_checks.py
@@ -76,6 +76,7 @@ def _yield_non_meta_checks(name, estimator):
     yield check_sample_weights_pandas_series
     yield check_sample_weights_list
     yield check_estimators_fit_returns_self
+    yield check_complex_data
 
     # Check that all estimator yield informative messages when
     # trained on empty datasets
@@ -458,6 +459,16 @@ def check_dtype_object(name, estimator_orig):
     assert_raises_regex(TypeError, msg, estimator.fit, X, y)
 
 
+def check_complex_data(name, estimator_orig):
+    # check that estimators raise an exception on providing complex data
+    X = np.random.sample(10) + 1j * np.random.sample(10)
+    X = X.reshape(-1, 1)
+    y = np.random.sample(10) + 1j * np.random.sample(10)
+    estimator = clone(estimator_orig)
+    assert_raises_regex(ValueError, "Complex data not supported",
+                        estimator.fit, X, y)
+
+
 @ignore_warnings
 def check_dict_unchanged(name, estimator_orig):
     # this estimator raises
diff --git a/sklearn/utils/tests/test_validation.py b/sklearn/utils/tests/test_validation.py
index 1fe27f199ac63..59ed3a76403a9 100644
--- a/sklearn/utils/tests/test_validation.py
+++ b/sklearn/utils/tests/test_validation.py
@@ -433,6 +433,44 @@ def test_check_array_min_samples_and_features_messages():
     assert_array_equal(y, y_checked)
 
 
+def test_check_array_complex_data_error():
+    X = np.array([[1 + 2j, 3 + 4j, 5 + 7j], [2 + 3j, 4 + 5j, 6 + 7j]])
+    assert_raises_regexp(
+        ValueError, "Complex data not supported", check_array, X)
+
+    X = [[1 + 2j, 3 + 4j, 5 + 7j], [2 + 3j, 4 + 5j, 6 + 7j]]
+    assert_raises_regexp(
+        ValueError, "Complex data not supported", check_array, X)
+
+    X = ((1 + 2j, 3 + 4j, 5 + 7j), (2 + 3j, 4 + 5j, 6 + 7j))
+    assert_raises_regexp(
+        ValueError, "Complex data not supported", check_array, X)
+
+    X = [np.array([1 + 2j, 3 + 4j, 5 + 7j]),
+         np.array([2 + 3j, 4 + 5j, 6 + 7j])]
+    assert_raises_regexp(
+        ValueError, "Complex data not supported", check_array, X)
+
+    X = (np.array([1 + 2j, 3 + 4j, 5 + 7j]),
+         np.array([2 + 3j, 4 + 5j, 6 + 7j]))
+    assert_raises_regexp(
+        ValueError, "Complex data not supported", check_array, X)
+
+    X = (np.array([1 + 2j, 3 + 4j, 5 + 7j]),
+         np.array([2 + 3j, 4 + 5j, 6 + 7j]))
+    assert_raises_regexp(
+        ValueError, "Complex data not supported", check_array, X)
+
+    X = MockDataFrame(
+        np.array([[1 + 2j, 3 + 4j, 5 + 7j], [2 + 3j, 4 + 5j, 6 + 7j]]))
+    assert_raises_regexp(
+        ValueError, "Complex data not supported", check_array, X)
+
+    X = sp.coo_matrix([[0, 1 + 2j], [0, 0]])
+    assert_raises_regexp(
+        ValueError, "Complex data not supported", check_array, X)
+
+
 def test_has_fit_parameter():
     assert_false(has_fit_parameter(KNeighborsClassifier, "sample_weight"))
     assert_true(has_fit_parameter(RandomForestRegressor, "sample_weight"))
diff --git a/sklearn/utils/validation.py b/sklearn/utils/validation.py
index 460f20673feaf..4af88a7ed9bb6 100644
--- a/sklearn/utils/validation.py
+++ b/sklearn/utils/validation.py
@@ -13,6 +13,7 @@
 
 import numpy as np
 import scipy.sparse as sp
+from numpy.core.numeric import ComplexWarning
 
 from ..externals import six
 from ..utils.fixes import signature
@@ -276,6 +277,13 @@ def _ensure_sparse_format(spmatrix, accept_sparse, dtype, copy,
     return spmatrix
 
 
+def _ensure_no_complex_data(array):
+    if hasattr(array, 'dtype') and array.dtype is not None \
+            and hasattr(array.dtype, 'kind') and array.dtype.kind == "c":
+        raise ValueError("Complex data not supported\n"
+                         "{}\n".format(array))
+
+
 def check_array(array, accept_sparse=False, dtype="numeric", order=None,
                 copy=False, force_all_finite=True, ensure_2d=True,
                 allow_nd=False, ensure_min_samples=1, ensure_min_features=1,
@@ -396,10 +404,28 @@ def check_array(array, accept_sparse=False, dtype="numeric", order=None,
     context = " by %s" % estimator_name if estimator is not None else ""
 
     if sp.issparse(array):
+        _ensure_no_complex_data(array)
         array = _ensure_sparse_format(array, accept_sparse, dtype, copy,
                                       force_all_finite)
     else:
-        array = np.array(array, dtype=dtype, order=order, copy=copy)
+        # If np.array(..) gives ComplexWarning, then we convert the warning
+        # to an error. This is needed because specifying a non complex
+        # dtype to the function converts complex to real dtype,
+        # thereby passing the test made in the lines following the scope
+        # of warnings context manager.
+        with warnings.catch_warnings():
+            try:
+                warnings.simplefilter('error', ComplexWarning)
+                array = np.array(array, dtype=dtype, order=order, copy=copy)
+            except ComplexWarning:
+                raise ValueError("Complex data not supported\n"
+                                 "{}\n".format(array))
+
+        # It is possible that the np.array(..) gave no warning. This happens
+        # when no dtype conversion happend, for example dtype = None. The
+        # result is that np.array(..) produces an array of complex dtype
+        # and we need to catch and raise exception for such cases.
+        _ensure_no_complex_data(array)
 
         if ensure_2d:
             if array.ndim == 1:


Code review comments:
diff_hunk: @@ -433,6 +433,44 @@ def test_check_array_min_samples_and_features_messages():
     assert_array_equal(y, y_checked)
 
 
+def test_check_array_complex_data_error():
+    X = np.array([[1 + 2j, 3 + 4j, 5 + 7j], [2 + 3j, 4 + 5j, 6 + 7j]])
path: sklearn/utils/tests/test_validation.py
text: @user1:
maybe instead of repeating the content every time create a list of lists in the beginning and then create everything else from that, so it's more obvious what is tested? Don't have a strong opinion though.

@author:
I had thought of that, but it becomes very messy for some of the cases. For example converting list of list to tuple of tuple, or to tuple of np-arrays. Maybe I can add one line comment for what case is being tested.