[WIP] MAINT: Return self for fit in Spectral Biclustering and CoClustering #6141

maniteja123 · 2016-01-08T13:07:17Z

Fix fit method for SpectralBiclustering and SpectralCoclustering.
Make them pass the common tests
Minimal changes to the code to fix some test failures (they might not be completely the right solution, but the bicluster tests are still passing)

Please do have a look at the changes and let me know if changes need to be done. I have also tried to address the comments. Hope they address the issues.

This change is

maniteja123 · 2016-01-08T13:08:18Z

Please let me know if it is a good idea to chain the returning of the self object ?

Thanks

agramfort · 2016-01-08T21:17:40Z

this was not caught by common tests?

MechCoder · 2016-01-08T23:00:28Z

@agramfort there is a DONT_TEST list ;)

agramfort · 2016-01-09T14:08:41Z

then it should be updated in this PR :)

maniteja123 · 2016-01-09T20:15:48Z

I am sorry if I am missing something but I couldn't see the BaseSpectral class or any of its subclasses in the DONT_TEST list in testing.py, and it would also help if anyone could point to the common test which checks for this.
Thanks.

TomDLT · 2016-01-13T14:22:53Z

Subclasses of BiclusterMixin are not tested in the common tests, ~~since they use fit(self, X) instead of fit(self, X, y)~~. (see the estimator check)

maniteja123 · 2016-01-15T20:20:07Z

Thanks for the information. This means none of the non_meta estimator checks are being done for subclasses of BiClusterMixin. I understand that one of the reasons is the working of `fit`` method as mentioned by @TomDLT above.

What is the best thing to do now ? Would it be better to just add a test to address this issue or actually look at all the tests and figure out the ones which are doable rather than skipping all of them altogether ?

Will move forward based on the opinion of experts. Thanks.

amueller · 2016-01-15T20:30:08Z

@TomDLT that is not the reason. The reason is that they take a distance matrix, not a data matrix, methinks. They should totally take a y. I'm not sure how to improve upon this.
Maybe we can "just" remove them from the DONT_TEST? I wasn't clear on the requirements on X in biclustering when I expanded the common tests, I think.

amueller · 2016-01-15T20:30:28Z

Fix looks good, but we should try and expand the tests.

maniteja123 · 2016-01-22T12:43:06Z

I suppose it would be better to expand the tests, but right now none of the non_meta_estimator checks are being done for the BiClusterMixin classes. What would be the best way to move forward ?
Thanks

amueller · 2016-01-25T23:03:37Z

What happens if you remove the classes from DONT_TEST?

maniteja123 · 2016-01-25T23:20:08Z

Thanks for the reply! I haven't tried that. Will do that and get back.

maniteja123 · 2016-01-26T05:10:05Z

Ah sorry was in my sleep then :) It seems that this causes the non meta estimator checks to not be performed for classes of BiClusterMixin.

maniteja123 · 2016-01-26T13:56:19Z

Hi, sorry for the delay. If common tests are allowed for these classes, the only error as of now is with regard to fit

TypeError: fit() takes exactly 2 arguments (3 given)

amueller · 2016-02-09T20:43:16Z

Then there should be a y=None in the fit method.

maniteja123 · 2016-02-10T13:36:09Z

Sorry for the trivial doubt but should I modify the signature of the fit method in the BiCluster classes?

maniteja123 · 2016-02-22T18:34:56Z

Sorry for disturbing again, but could you clarify what you mean by having y=None ? Thanks.

TomDLT · 2016-02-23T09:44:49Z

def fit(self, X, y=None):
    ...

maniteja123 · 2016-02-23T11:55:12Z

@TomDLT thanks for the input. What would be better to mention in the documentation regarding the parameter y ? Thanks !

TomDLT · 2016-02-23T12:10:52Z

You don't need to mention it.
Do the class go through the non-meta estimator tests when you add y=None?

maniteja123 · 2016-02-23T12:16:11Z

Thanks for the clarification. Yeah, the class goes through the non-meta estimator checks after this change. Sorry didn't push the modified code. Wanted to clarify first and then push.

maniteja123 · 2016-02-23T12:42:40Z

@TomDLT, it seems that the tests are failing in travis but not on my local system. Any help regarding the reason for this behavior ? Do I need to rebase ?

amueller · 2016-07-15T22:12:14Z

sorry for the delay. Can you rebase and see where the error comes from?

maniteja123 · 2016-07-16T03:35:41Z

Sure I will rebase and try to debug the error but in case anyone wants to reproduce, AFAIR they will occur when you remove the condition for the non meta estimator checks to skip bicluster modules.

maniteja123 · 2016-07-24T09:55:00Z

Hi, I have rebased on master and tried to fix some of the failing tests. In particular when the input is 1D, the svd decomposition in SpectralBiClustering discards the only feature/sample in the input. I suppose the checks weren't written to handle this case. Also in SpectralCoclustering the svd decompostion and further kmeans clustering is failing because of nan and inf in the decomposed vectors. It would be really helpful if someone can look into this and let me know if we should skip these tests or make some modifications for biclustering algorithms. Thanks !

amueller · 2016-07-26T17:59:00Z

We should make modifications. If n_samples < n_row_clusters or n_features < n_column_clusters we should raise an error. That can be done in check_array with min_samples and min_features maybe (or otherwise to give a more explicit error message).

amueller · 2016-09-13T16:18:30Z

@maniteja123 are you still working on this?

maniteja123 · 2016-09-13T21:17:49Z

Hi @amueller, sorry for not completing it. Would it be okay to work on it tomorrow and implement your suggestions ?

maniteja123 · 2016-09-15T12:42:28Z

Hi, I have tried to get this PR to merge state but have done some changes to the code of bicluster package for the tests to pass. In particular

It was failing mostly in cases of either 1 feature or 1 sample. I couldn't come up with a better solution rather than specifically checking for dimensions.
In sparse case, the SpectralBiclustering class was failing due to nan and inf in the data and there is a finite check. I skipped it right now but I suppose sparse should be handled

Please do have a look at it and let me know if anything else could be done here. Thanks.

amueller · 2016-09-16T16:51:18Z

sklearn/cluster/bicluster.py

-    row_diag = np.asarray(1.0 / np.sqrt(X.sum(axis=1))).squeeze()
-    col_diag = np.asarray(1.0 / np.sqrt(X.sum(axis=0))).squeeze()
+    row_diag = np.asarray(1.0 / np.sqrt(X.sum(axis=1)))
+    if row_diag.shape[0] != 1:


Sorry, can you explain this change. It's been a while since I looked at this.

You could run squeeze and then reshape. Or, probably better, make sure min_samples=2 in check_array

Maybe actually ensure n_samples >= n_clusters and n_clusters > 1?

Thanks for the suggestion. I suppose this will take care of this issue.

Hey. are you still working on this? I'd like to get this into 0.12.1, which I'm planning to do over the weekend.

Hi @amueller sorry for the delay. I will really try to get this into 0.18.1.

Actually looking into this, the failing common tests set n_clusters = 1. So if we ensure this, it might still need refactoring to the common tests. In particular, they are check_fit2d_1sample, check_fit1d_1feature and check_fit1d_1sample in estimator_checks. What would be better to do here ?

Yeah the tests seem to be odd. In particular the comment is wrong. We should allow estimators to raise the error that is raised by check_X_y with min_samples.

amueller · 2016-09-16T16:51:58Z

sklearn/cluster/bicluster.py

@@ -156,6 +169,8 @@ def _svd(self, array, n_components, n_discard):

        assert_all_finite(u)
        assert_all_finite(vt)
+        if u.shape[1] == 1 and vt.shape[0] == 1:


Again, explain please? Can you also add a comment maybe? So their's only one row in X?

checking number of samples in the beginning should fix this, too, right?

AFAIU this is happening when n_components is 1 and even that is getting discarded. So I suppose we can check n_discard < n_components. Please do let me know what you think. Thanks.

Does the algorithm make sense for n_components = 1? I should work, right? Who's the expert on these algorithms?

It looks like the number of support vectors is increased here: https://github.com/scikit-learn/scikit-learn/blob/65a4de5/sklearn/cluster/bicluster.py#L442 so it should work, right?

Who's the expert on these algorithms?

Well, they were implemented by @kemaleren, and it looks like @larsmans attempted some cleanup, but I've had the impression for a while that they are somewhat orphaned, given how long issues like #2484 have been around.

amueller · 2016-09-16T16:52:47Z

sklearn/utils/estimator_checks.py

@@ -293,7 +293,9 @@ def set_testing_parameters(estimator):
    if "decision_function_shape" in params:
        # SVC
        estimator.set_params(decision_function_shape='ovo')
-
+    if "n_best" in params:


why is this needed?

ah, makes sense. Shouldn't it be set to that by default?

Sorry could you clarify what should be set by default ?

n_best should be set to n_components by default, I think. Seems like a usability issue in the class.

Thanks for clarifying. It seems right. Should I add this logic to the class then ?

I think so.

Sure will do it

amueller · 2016-09-16T16:53:05Z

sklearn/utils/estimator_checks.py

@@ -340,6 +342,8 @@ def check_estimator_sparse_data(name, Estimator):
    X_csr = sparse.csr_matrix(X)
    y = (4 * rng.rand(40)).astype(np.int)
    for sparse_format in ['csr', 'csc', 'dok', 'lil', 'coo', 'dia', 'bsr']:
+        if name == 'SpectralCoclustering':


Why? It should raise an appropriate error.

Even here, similar to above scenario this call is failing. This is the stacktrace

sklearn/cluster/bicluster.py:298: RuntimeWarning: invalid value encountered in multiply z = np.vstack((row_diag[:, np.newaxis] * u, Estimator SpectralCoclustering doesn't seem to fail gracefully on sparse data: it should raise a TypeError if sparse input is explicitly not supported. Traceback (most recent call last): File "<stdin>", line 1, in <module> File "sklearn/utils/estimator_checks.py", line 353, in check_estimator_sparse_data estimator.fit(X, y) File "sklearn/cluster/bicluster.py", line 136, in fit return self._fit(X) File "sklearn/cluster/bicluster.py", line 301, in _fit _, labels = self._k_means(z, self.n_clusters) File "sklearn/cluster/bicluster.py", line 189, in _k_means model.fit(data) File "sklearn/cluster/k_means_.py", line 880, in fit X = self._check_fit_data(X) File "sklearn/cluster/k_means_.py", line 855, in _check_fit_data X = check_array(X, accept_sparse='csr', dtype=[np.float64, np.float32]) File "sklearn/utils/validation.py", line 407, in check_array _assert_all_finite(array) File "sklearn/utils/validation.py", line 58, in _assert_all_finite " or a value too large for %r." % X.dtype) ValueError: Input contains NaN, infinity or a value too large for dtype('float64').

Well is that a consequence of the data content or the data being sparse?

It seems it is due to the data content rather than sparsity.

amueller · 2016-09-16T16:53:16Z

sklearn/utils/estimator_checks.py

@@ -687,7 +691,11 @@ def check_fit_score_takes_y(name, Estimator):
 @ignore_warnings
 def check_estimators_dtypes(name, Estimator):
    rnd = np.random.RandomState(0)
-    X_train_32 = 3 * rnd.uniform(size=(20, 5)).astype(np.float32)
+    if name in ["BoxCoxTransformer", "SpectralCoclustering",


The array being randomly generated contains 0s which is causing inf and nan which results in ValueError due to this. The only approach was to generate positive numbers. Is there something I am doing wrong here ?

BoxCoxTransformer is not in master yet, right?
Why do zeros create inf an nan?

Sorry yeah BoxCoxTransformer is not in master yet, but the tests were failing in my local machine when I was switching between the branches. will remove that. But in BiCluster module, the svd vectors u and v are failing finite tests when there are zeroes in X_train_32.

I'm not very familiar with the algorithm but that seems strange to me. Have you understood why this is happening?

Even I am not familiar at all with the algorithm, but when the current code is executed, normalized_data here has NaN and fails finite check during this call.

so that's when an entire row is zero?

There's still the BoxCoxTransformer here. I guess we could leave it like this for now with a FIXME and open an issue. It's more important to me that the basic API is correct than having all the edge-cases.

Yeah it is happening when an entire row is zero. But surprisingly happens only for X_train_32. Will remove boxcox transformer

amueller · 2016-09-16T16:53:26Z

sklearn/utils/estimator_checks.py

@@ -1318,7 +1326,7 @@ def check_class_weight_balanced_linear_classifier(name, Classifier):


 def check_estimators_overwrite_params(name, Estimator):
-    X, y = make_blobs(random_state=0, n_samples=9)
+    X, y = make_blobs(random_state=0, n_samples=9, n_features=3)


For bicluster classes, there is singular vector vt of shape [n_features, n_components] on which k_means is applied for which the default value for n_clusters is 3 which should be greater than equal to the number of samples of vt, which is n_features(which has default of 2 in make_blobs), hence changed it to 3. And the other estimators were still passing, so I hoped it would be okay.

amueller · 2016-09-16T16:53:56Z

Thanks for the changes. It would be great if you could explain them a bit more, I haven't looked at this in a while.

maniteja123

Hi @amueller , thanks for the review. I tried to give an explanation to the best of my understanding. Please do have a look at your convenience and let me know if something can be made better. Thanks.

maniteja123 · 2016-09-16T19:21:24Z

sklearn/utils/estimator_checks.py

@@ -687,7 +691,11 @@ def check_fit_score_takes_y(name, Estimator):
 @ignore_warnings
 def check_estimators_dtypes(name, Estimator):
    rnd = np.random.RandomState(0)
-    X_train_32 = 3 * rnd.uniform(size=(20, 5)).astype(np.float32)
+    if name in ["BoxCoxTransformer", "SpectralCoclustering",


The array being randomly generated contains 0s which is causing inf and nan which results in ValueError due to this. The only approach was to generate positive numbers. Is there something I am doing wrong here ?

maniteja123 · 2016-09-16T19:38:16Z

sklearn/utils/estimator_checks.py

@@ -1318,7 +1326,7 @@ def check_class_weight_balanced_linear_classifier(name, Classifier):


 def check_estimators_overwrite_params(name, Estimator):
-    X, y = make_blobs(random_state=0, n_samples=9)
+    X, y = make_blobs(random_state=0, n_samples=9, n_features=3)


For bicluster classes, there is singular vector vt of shape [n_features, n_components] on which k_means is applied for which the default value for n_clusters is 3 which should be greater than equal to the number of samples of vt, which is n_features(which has default of 2 in make_blobs), hence changed it to 3. And the other estimators were still passing, so I hoped it would be okay.

Closes scikit-learngh-6126

amueller · 2016-10-27T17:45:49Z

On 10/27/2016 01:36 PM, Maniteja Nandana wrote:

Sorry could you clarify what should be set by default ?

Sorry I can't find where this comment was.

maniteja123 · 2016-10-27T17:53:09Z

I was referring to your comment here about the n_best parameter for bicluster classes.

raghavrv · 2016-11-01T19:55:18Z

@maniteja123 Could you refactor the return self part out of this PR into a separate PR so we can merge it in time for 0.18.1... Fixing the testing setup can be done for 0.19... if @amueller is okay with that...

maniteja123 · 2016-11-02T02:52:13Z

Sure @raghavrv , I can do that if that is okay. Sorry I don't think I can fix all that failures by 0.18.1 but will definitely do that. If anyone can do it by then, please feel free to take over.

amueller · 2016-11-02T19:56:15Z

I mean I can just push the two "return self" lines to master... but without any tests... well... I guess it's better than nothing?

jnothman · 2016-11-02T20:07:17Z

do it

On 3 November 2016 at 06:56, Andreas Mueller notifications@github.com
wrote:

I mean I can just push the two "return self" lines to master... but
without any tests... well... I guess it's better than nothing?

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#6141 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAEz62JyCJRXi5LR0tfVzqXmgzfuGs5Wks5q6OrggaJpZM4HBJfG
.

maniteja123 · 2016-11-03T09:18:20Z

Hi, I just created a PR at gh-7814 for returning self but not touching the tests. Please let me know if it is fine for the purpose of 0.18.1. Thanks.

jnothman · 2017-02-23T11:55:09Z

Please rebase / merge with master.

amueller mentioned this pull request Jul 15, 2016

SpectralBiclustering.fit doesn't return self #6126

Closed

maniteja123 force-pushed the issue6126 branch from 444b803 to f2988bd Compare July 24, 2016 09:35

maniteja123 force-pushed the issue6126 branch from dc57cf6 to f71f6d6 Compare September 15, 2016 05:47

maniteja123 changed the title ~~MAINT: Return self for fit in Spectral Biclustering and CoClustering~~ [MRG] MAINT: Return self for fit in Spectral Biclustering and CoClustering Sep 15, 2016

amueller reviewed Sep 16, 2016

View reviewed changes

maniteja123 commented Sep 16, 2016

View reviewed changes

MAINT: Return self for fit in Spectral Biclustering and CoClustering

4dd790a

Closes scikit-learngh-6126

maniteja123 force-pushed the issue6126 branch from bf459f4 to 978c48b Compare September 29, 2016 04:00

modify common tests

fe4a8aa

maniteja123 force-pushed the issue6126 branch from 978c48b to fe4a8aa Compare September 29, 2016 04:15

jnothman mentioned this pull request Oct 13, 2016

[MRG + 1] Add test for __dict__ #7553

Merged

amueller added the Waiting for Reviewer label Oct 13, 2016

amueller modified the milestones: 0.19, 0.18.1 Oct 13, 2016

maniteja123 mentioned this pull request Nov 3, 2016

[MRG + 1] MAINT: Return self for fit in Spectral Biclustering and CoClustering #7814

Merged

raghavrv modified the milestones: 0.19, 0.18.1 Nov 6, 2016

raghavrv changed the title ~~[MRG] MAINT: Return self for fit in Spectral Biclustering and CoClustering~~ [WIP] MAINT: Return self for fit in Spectral Biclustering and CoClustering Nov 6, 2016

raghavrv removed the Waiting for Reviewer label Nov 6, 2016

amueller mentioned this pull request Jun 8, 2017

[MRG+1] fix biclustering API #9053

Merged

agramfort closed this in #9053 Jun 8, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] MAINT: Return self for fit in Spectral Biclustering and CoClustering #6141

[WIP] MAINT: Return self for fit in Spectral Biclustering and CoClustering #6141

[WIP] MAINT: Return self for fit in Spectral Biclustering and CoClustering #6141

[WIP] MAINT: Return self for fit in Spectral Biclustering and CoClustering #6141

Conversation

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment