Output of metric() method

[bellet]

Description

Currently the metric() method of MahalanobisMixin returns the PSD matrix self.transformer_.T.dot(self.transformer_).

I am not sure when this is really useful in practice?Shouldn't we rather return the metric function itself as a callable, or better a DistanceMetric that can be plugged easily into some sklearn estimators? This seems more useful.

Defined this way, metric() could be an abstract method of BaseMetricLearner (since every metric learner should be based on some kind of metric).

[perimosocordiae]

I'd prefer to keep some way of accessing the "metric" matrix as currently implemented for maximum flexibility, but I do agree we could expose a more friendly API across all learner classes.

[wdevazelhes]

I agree it would be very convenient to have an object that can directly be used as a metric.
Regarding on whether returning a callable or a DistanceMetric, I am not sure whether DistanceMetric is supposed to be used by a user. I mean yes they provide a way to build a custom DistanceMetric using DistanceMetric.get_metric("pyfunc", func=myfunction), and this allows to call the pairwise routine. But trying to use it with KNeighborsClassifier, it does not work with a DistanceMetric but it works with a callable (and by the way the stack trace if you give a bad metric like print('test'), shows that KNeighborsClassifier calls in fact DistanceMetric):

from sklearn.neighbors import KNeighborsClassifier, DistanceMetric
from sklearn.datasets import load_iris
from sklearn.model_selection import cross_val_score
def euc(arr1, arr2): 
    return np.sqrt(np.sum((arr1 - arr2)**2)) 
d_euc = DistanceMetric.get_metric('pyfunc', func=euc) 
X, y = load_iris(return_X_y=True)
clf = KNeighborsClassifier(metric=d_euc)  # works if I replace with euc
cross_val_score(clf, X, y)
# returns an error

[wdevazelhes]

Also, there's a thing: scikit-learn, when using a custom distance function say in KNN, needs a function that takes as input two 1D arrays (just two vectors) and returns a scalar. Then this function is called in a loop (in Cython) inside the algorithm on all pairs needed. Knowing that we have a distance parameterized by a matrix, we could probably do faster vectorizing the operations. But I don't know how to integrate it with scikit-learn then...
I'll look into scikit-learn issues to see whether this is a subject that is tackled already

[bellet]

I agree it would be very convenient to have an object that can directly be used as a metric.
Regarding on whether returning a callable or a DistanceMetric, I am not sure whether DistanceMetric is supposed to be used by a user. I mean yes they provide a way to build a custom DistanceMetric using DistanceMetric.get_metric("pyfunc", func=myfunction), and this allows to call the pairwise routine. But trying to use it with KNeighborsClassifier, it does not work with a DistanceMetric but it works with a callable (and by the way the stack trace if you give a bad metric like print('test'), shows that KNeighborsClassifier calls in fact DistanceMetric):

from sklearn.neighbors import KNeighborsClassifier, DistanceMetric
from sklearn.datasets import load_iris
from sklearn.model_selection import cross_val_score
def euc(arr1, arr2): 
    return np.sqrt(np.sum((arr1 - arr2)**2)) 
d_euc = DistanceMetric.get_metric('pyfunc', func=euc) 
X, y = load_iris(return_X_y=True)
clf = KNeighborsClassifier(metric=d_euc)  # works if I replace with euc
cross_val_score(clf, X, y)
# returns an error

Strange, because even KNeighborsClassifier(metric=DistanceMetric.get_metric('euclidean')) returns an error...

Note that DistanceMetric already has Mahalanobis distance where one can simply pass the matrix parameter so in principle it would be easy to construct. But returning a simple scalar function is fine as well.

[bellet]

Also, there's a thing: scikit-learn, when using a custom distance function say in KNN, needs a function that takes as input two 1D arrays (just two vectors) and returns a scalar. Then this function is called in a loop (in Cython) inside the algorithm on all pairs needed. Knowing that we have a distance parameterized by a matrix, we could probably do faster vectorizing the operations. But I don't know how to integrate it with scikit-learn then...
I'll look into scikit-learn issues to see whether this is a subject that is tackled already

We should think of this ability to get the metric function as a convenience - it is okay if it is not necessarily the most efficient way of doing things in all situations. (In many cases, I guess the fastest way is probably to first transform the data and then use Euclidean distance.)

[wdevazelhes]

Strange, because even KNeighborsClassifier(metric=DistanceMetric.get_metric('euclidean')) returns an error...

Note that DistanceMetric already has Mahalanobis distance where one can simply pass the matrix parameter so in principle it would be easy to construct. But returning a simple scalar function is fine as well.

Ah yes I didn't see the MahalanobisDistance, it's cool then they already have it implemented in cython !

Looking at this issue: scikit-learn/scikit-learn#11807, I understood a new way to use custom DistanceMetric:

clf = KNeighborsClassifier(metric='pyfunc', metric_params={'func': euc})  

(but it is probably not so useful here since clf = KNeighborsClassifier(metric=euc) is a shortcut for that)
or for MahalanobisMixin:

clf = KNeighborsClassifier(metric='mahalanobis', metric_params={'VI': mymatrix}) 

But then here we only use the raw matrix...

So for now, to have a coherent returned object from all the metric learners even those that wouldn't be MahalanobisMixin, I guess metric should return a callable ? But it is annoying not to be able to put a DistanceMetric because it would have allowed us to just return DistanceMetric.get_metric('mahalanobis', VI=mymatrix) and have a cython function out of the box. Maybe at one point we'll have our own callables returned by some function metric(), in cython, but I guess for now we'll just return a regular python callable ?

[wdevazelhes]

Also, there's a thing: scikit-learn, when using a custom distance function say in KNN, needs a function that takes as input two 1D arrays (just two vectors) and returns a scalar. Then this function is called in a loop (in Cython) inside the algorithm on all pairs needed. Knowing that we have a distance parameterized by a matrix, we could probably do faster vectorizing the operations. But I don't know how to integrate it with scikit-learn then...
I'll look into scikit-learn issues to see whether this is a subject that is tackled already

We should think of this ability to get the metric function as a convenience - it is okay if it is not necessarily the most efficient way of doing things in all situations. (In many cases, I guess the fastest way is probably to first transform the data and then use Euclidean distance.)

Yes I guess you're right. And if users really want performance they can always precompute distances in a vectorized fashion, and use the option 'precomputed' of scikit-learn estimators

[bellet]

Maybe at one point we'll have our own callables returned by some function metric(), in cython, but I guess for now we'll just return a regular python callable ?

Yes, I think this is good enough for now

[wdevazelhes]

I was about to start implementing this when I realized: should we call metric() and that would return a function ? Or should metric be a method that itself computes the distance between pairs (like self.metric(vector1, vector2) ? We could still pass MyEstimator.metric as a "metric" argument to a KNN.
In this case the metric method would be very similar to the score_pairs method, so maybe we could make a unified method. But then since this method would have a lot of possible inputs, (2 1D arrays for the KNN use case, or in the batch case: 2 1D arrays for using the preprocessor or 2 2D arrays for use without the preprocessor), we should add flags to specify the usage and it would be come complicated.
And this means the metric would be updated if we refit MyEstimator for instance. Also, I don't know what would happen if users want to copy/pickle the KNN.
Therefore maybe they would prefer a function that is independent of the original estimator.

So I guess I'll do a metric function that returns a function with the metric matrix copied inside so that it is independent of the estimator that created it, and I'll emphasize in the docs what is the difference with the score_pairs function

What do you think ?

[bellet]

I agree, I think you raised good arguments in favor of returning an independent function. I would maybe call this method get_metric().

If @perimosocordiae wants to keep the current functionality, you could rename the current metric() method to get_mahalanobis_matrix() or something like this.