MRG Fit pairwise

[amueller]

This is supposed to be (a draft of?) the fit_pairwise proposal.
It creates fit_pairwise and predict_pairwise functions for SVM, KernelPCA, SpectralClustering and AffinityPropagation.
It also makes all these usable using GridSearchCV with a new fit_pairwise for the grid search.

Now uses a property to check whether the estimator was fit using a "pairwise" X.

[mblondel]

In retrospect, I am a bit worried by the fact that this will add 4 new methods (fit_pairwise, predict_pairwise, transform_pairwise and fit_transform_pairwise). Another way to solve the grid search problem would be to agree upon on a flag that must be set by the estimator and can be checked by the grid search. So, for example, if kernel="precomputed", we internally set self.pairwise = True, flag that would be checked by grid search.

In algorithms that use a similarity / affinity matrices in fit, we should add a new metric option. The input X in fit and other methods would then be considered as a similarity / distance matrix if and only if metric is set to "precomputed". Otherwise, X would be conveniently transformed for the user by calling pairwise_distances with the given metric.

[amueller]

Yeah, four new methods are a lot. I didn't really see that until I tried to implement everything and adjust the tests.
Just setting a self.pairwise variable might be a good thing.

I would not call the option you suggest "metric" as I think it does not need to be one. This can probably be decided on an per-algorithm base and made as consistent as possible ;)
As long as internally self.pairwise is set, this should be fine.

[mblondel]

I would not call the option you suggest "metric" as I think it does not

need to be one. This can probably be decided on an per-algorithm base and
made as consistent as possible ;)
As long as internally self.pairwise is set, this should be fine.

Agreed. But, if internally pairwise_distances is used, we can call it
metric (for pairwise_kernels, I would have preferred kernel...)

[agramfort]

self.pairwise is a simple and tempting idea. I would however use a function like is_pairwise() as you probably don't want to have svc.kernel and svc.pairwise defined in the init (required by clone). A property would work too but a function is more explicit.

[mblondel]

If I'm not mistaken, this requirement only holds if the attribute is a constructor parameter, which wouldn't be the case here.

[agramfort]

I certainly agree but to rephrase, svc.pairwise to remain properly defined after a clone should either be an init param or a property. Otherwise we could have svc.is_pairwise()

[mblondel]

Ah ok, I got your point now. A 3rd way is to modify clone so as to check for the presence of pairwise and reset it in the cloned object.

[agramfort]

Ah ok, I got your point now. A 3rd way is to modify clone so as to check for the presence of pairwise and reset it in the cloned object.

why not if it's an exception for pairwise. I am sure @GaelVaroquaux
will have a strong opinion on this :)

[mblondel]

I wouldn't be shocked to have it in clone since pairwise could be thought as a reserved attribute that is part of the API specification. Let's wait for @GaelVaroquaux 's verdict :)

[amueller]

ping @mblondel @agramfort @GaelVaroquaux
I would love some opinions on this PR ;)

[amueller]

Maybe the affinity propagation still needs some love... though I'm not very familiar with that.

[amueller]

One more comment on property vs function:
If _pairwise is a function instead of a property, the test becomes

getattr(est, '_pairwise', lambda: False)()

Which looks a bit ugly to me. Therefor I'm +0 on property vs function.

[amueller]

Any opinions on this at all?

[ogrisel]

I would +1 reusing pairwise_kernels whenever it makes sense, possibly extending it with new affinities (not necessarily PSD as the sigmoid kernel is not PSD) such as "nearest_neighbors" as implemented in the current SpectralClustering fit method.

I don't see why you say that spectral clustering and affinity propagation use different styles of affinities: you can select it according to what make sense depending on the nature of the data and the assumption you make on the cluster structure.

[ogrisel]

As for the _pairwise attribute / property vs method debate I would be +0 on the attribute / property option as it seems more natural that a method without argument.

[amueller]

@ogrisel thanks for your comment. Maybe I'm just not familiar enough with affinity propagation. Have you an example where it is used with something that is not the euclidean distance?
I think I'll add string options to affinity propagation and spectral clustering, extend the pairwise_kernels, rename the attribute in affinity propagation and then hopefully we are good to go.

[ogrisel]

2012/8/22 Andreas Mueller notifications@github.com

@ogrisel https://github.com/ogrisel thanks for your comment. Maybe I'm
just not familiar enough with affinity propagation. Have you an example
where it is used with something that is not the euclidean distance?

I think that cosine similarity (e.g. dot product of normalized TF-IDF
vectors) would make sense for text data for instance.

Olivier
http://twitter.com/ogrisel - http://github.com/ogrisel

[mblondel]

The shape should be [n_samples, n_samples] if affinity == "precomputed" and [n_samples, n_features] otherwise.

[mblondel]

connectivity matrix

[amueller]

@mblondel Thanks for your comments. You gave -1 on something somewhere but i can't see it any more. Was that the thing I just fixed?

[mblondel]

I thought you wanted to rename affinity="precomputed" to affinity="affinity"... Your change is fine :)

[mblondel]

It's nice that SpectralClustering and AffinityPropagation will follow the scikit-learn API, at last :)

As I said on the ML, we could postpone the decision about the code factorization of affinity propagations to later. This way we can address the more cosmetic changes (like using a property or not) and merge soon. This PR has been pending for too long :)

[amueller]

I think this looks ok now. It does not do a lot of code sharing, because of the inclusion loop problem (see ML).
But it does change the API in a way that should make the refactoring very easy.

What this PR accomplishes now is:

• Have a consistent API for clustering, that is the same as in the rest of the scikit
• Enable grid-search for all estimators with precomputed similarities / kernels

This is important because it make these next steps possible:

• Work on the precomputed kernel interface for SVMs (sparse case)
• Unify testing for clustering
• Enable grid search for clustering (needs scoring function)
• Refactor affinity computations

@mblondel I fully agree :)

[ogrisel]

+1 for postponing the affinity == neighbors refactoring and get this merged first.

[ogrisel]

I think I am 👍 for merging if all the tests pass. Can you quickly check the test coverage report to check that the new code does not introduce new untested code blocks?

Also can you check that the affinity / spectral clustering example still run and have their plots look good with your refactoring?

BTW thanks for working on this.

[amueller]

Always a pleasure ;)
I'll add some more tests and then merge.

[amueller]

hurray :)

[GaelVaroquaux]

Good job!!!