Repository: scikit-learn/scikit-learn
Pull request: [MRG+1] Fix KernelCenterer requires square input but doesn't error on non-square
<!--
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
Fix #14240 
<!--
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.
KernelCenterer.fit will now raise a ValueError when called on a non-square kernel matrix.

Added `pairwise_estimator_convert_X` to `check_fit2d_1sample` and `check_transformer_data_not_an_array` as described by https://github.com/scikit-learn/scikit-learn/issues/14240#issuecomment-511134976.

Added logical check X.T != X to assert_raises statement in `_check_transformer`.  Previously, `_check_transformer` assumed that X.T had a different number of features than X.  However, if `pairwise_estimator_convert_X` is called where transformer is pairwise, then X.T == X therefore no error ValueError will be raised so the check is invalid.  

- Adding `pairwise_estimator_convert_X` to `check_transformer_general` caused two tests to fail through  `_check_transformer` assert_raises statement that "The transformer {} does not raise an error when the number of features in transform is different from the number of features in fit."



#### Any other comments?
Fitting KernelCenterer on an nxn matrix and transforming a kxk matrix where n!=k is not currently tested by any unit test.  Doing this manually raises the error`ValueError: operands could not be broadcast together with shapes (k,k) (n,) (k,k)`.  `_check_transformer` could be modified to transpose X before `pairwise_estimator_convert_X` is called to make the test work on pairwise estimators.  KernelCenterer would need to be modified to give a more useful error message.

<!--
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:
KernelCenterer requires square input but doesn't error on non-square
I'm pretty sure the KernelCenter should check that the input is square.
Actually it has ``pairwise=True`` so it's a bit weird that it works on non-square training data and I think we should throw an error instead.

Also common tests should probably check that ``pairwise=True`` estimators error on non-square training data.

Hints from issue discussion:
Hi, is anyone working on this?
I added some code to throw a ValueError when KernelCenterer.fit() is called on a non-square kernel matrix.  However, this causes some unit tests to fail (output attached). [KernelCenterer_FailedTests.txt](https://github.com/scikit-learn/scikit-learn/files/3383244/KernelCenterer_FailedTests.txt)

It seems the errors all occur when KernelCenterer.fit(K, y) is called where y is not none, but the KernelCenterer does not seem to use y in any way.  The output of fitting/transforming a kernel matrix K is the same whether y equals none or any other value.  What is the best way to handle this? 


@gdex1 it should be ``y=None`` in the signature of the function if ``y`` is not used.
@amueller I looked at the failed tests again and I think the problem is actually that checks such as `check_transformer_general` provide non-square data and then count the test as failed when an error is raised.  I am not sure what the best way to address this is.

I believe that adding a tag `"require_square_input"` to `KernelCenterer` and then modifying `_yield_all_checks` to only provide checks with square input for this tag would solve the failed tests.  Is there a better/cleaner solution?
the ``_pairwise`` attribute basically means "require_square_input`` but predates the estimator tags. I don't think we need to add a tag for that right now.

You need to use ``pairwise_estimator_convert_X`` on ``X`` to make it square for these tests. It's an issue with the tests if it wasn't used before.
Can you please open a PR? That will make this easier to review, and we'll also be able to see the error messages.

Commit message: Add test validity check to _check_transformer

Patch to review:
diff --git a/sklearn/preprocessing/data.py b/sklearn/preprocessing/data.py
index 823eedc8b7dd9..ccc3e3ca2edac 100644
--- a/sklearn/preprocessing/data.py
+++ b/sklearn/preprocessing/data.py
@@ -1925,7 +1925,14 @@ def fit(self, K, y=None):
         -------
         self : returns an instance of self.
         """
+        
         K = check_array(K, dtype=FLOAT_DTYPES)
+
+        if K.shape[0] != K.shape[1]:
+            raise ValueError("Kernel matrix must be a square matrix."
+                             " Input is a {}x{} matrix."
+                             .format(K.shape[0], K.shape[1]))
+                             
         n_samples = K.shape[0]
         self.K_fit_rows_ = np.sum(K, axis=0) / n_samples
         self.K_fit_all_ = self.K_fit_rows_.sum() / n_samples
diff --git a/sklearn/utils/estimator_checks.py b/sklearn/utils/estimator_checks.py
index 249cb022f8e87..0cd6d571de364 100644
--- a/sklearn/utils/estimator_checks.py
+++ b/sklearn/utils/estimator_checks.py
@@ -875,6 +875,8 @@ def check_fit2d_1sample(name, estimator_orig):
     # the number of samples or the number of classes.
     rnd = np.random.RandomState(0)
     X = 3 * rnd.uniform(size=(1, 10))
+    X = pairwise_estimator_convert_X(X, estimator_orig)
+    
     y = X[:, 0].astype(np.int)
     estimator = clone(estimator_orig)
     y = enforce_estimator_tags_y(estimator, y)
@@ -962,6 +964,7 @@ def check_transformer_general(name, transformer, readonly_memmap=False):
                       random_state=0, n_features=2, cluster_std=0.1)
     X = StandardScaler().fit_transform(X)
     X -= X.min()
+    X = pairwise_estimator_convert_X(X, transformer)
 
     if readonly_memmap:
         X, y = create_memmap_backed_data([X, y])
@@ -977,6 +980,7 @@ def check_transformer_data_not_an_array(name, transformer):
     # We need to make sure that we have non negative data, for things
     # like NMF
     X -= X.min() - .1
+    X = pairwise_estimator_convert_X(X, transformer) 
     this_X = NotAnArray(X)
     this_y = NotAnArray(np.asarray(y))
     _check_transformer(name, transformer, this_X, this_y)
@@ -1059,7 +1063,8 @@ def _check_transformer(name, transformer_orig, X, y):
             assert _num_samples(X_pred3) == n_samples
 
         # raises error on malformed input for transform
-        if hasattr(X, 'T') and not _safe_tags(transformer, "stateless"):
+        if hasattr(X, 'T') and not _safe_tags(transformer, "stateless") \
+            and np.array((X != X.T)).all():
             # If it's not an array, it does not have a 'T' property
             with assert_raises(ValueError, msg="The transformer {} does "
                                "not raise an error when the number of "


Code review comments:
diff_hunk: @@ -1059,7 +1063,8 @@ def _check_transformer(name, transformer_orig, X, y):
             assert _num_samples(X_pred3) == n_samples
 
         # raises error on malformed input for transform
-        if hasattr(X, 'T') and not _safe_tags(transformer, "stateless"):
+        if hasattr(X, 'T') and not _safe_tags(transformer, "stateless") \
path: sklearn/utils/estimator_checks.py
text: @user1:
The fix I've used in other places in this file (I think) is to slice X instead of using ``X.T``. We just want something with different shape, so if you pass ``X[:, ::-1]`` instead of ``X.T`` the test works for all cases.
Otherwise you're now skipping this test for pairwise estimators.

@author:
Thanks for the suggestions.  One issue I am thinking is that for pairwise estimators, a ValueError could be triggered on `X[:, :-1]` because the input would be non-square instead of different number of dimensions.  So if X was 10x10, the estimator would throw an error on 10x9 input but possibly not 9x9 input.  Is this a problem? 

Would it make sense to do `pairwise_estimator_convert_X(X[:-1, :]).T`?  This would take a 10x10 array X to 10x9 for most estimators and 9x9 for pairwise estimators.

I realized this assumes that all pairwise operators that are fit on an NxN array must also be transformed on an NxN array.  I guess I should ask if this is true in general and for KernelCenterer specifically. 

@user1:
Pairwise estimators should allow rectangular data in ``transform`` or ``predict``. I think they all do, the number of features should be the number of rows==columns in the training data.

@user1:
It doesn't look to me as if ``KernelCenterer`` requires square data in ``transform``.