Fit constraints

[svecon]

I needed to use the Metric Learners inside scikit Pipeline, which only calls .fit(X,y).

Therefore I renamed original .fit() to .fit_constraints(), added all of its parameters to constructor and created new .fit(), which uses the parameters from constructor to build constraints and then call .fit_constraints().

I understand this is a breaking change. If there is a better approach I want to hear it :)

This only affects methods which need special constraints: ITML, SDML, LSML, RCA

[perimosocordiae]

Thanks for the PR! This is a tricky situation, partly due to the breaking change, but also because it brings up a good question about how people are expected to use this API.

From my perspective, the main use-cases for metric learning are unsupervised/semisupervised learning. In those settings, you want to use all of the constraint information available, because it's likely pretty hard to get.

The existing prepare_constraints functions are mostly there for testing purposes. After all, if you have full label information for all your data, why would you select some random subset of it to convert to constraints?

Even so, I agree that there should be a better / more compatible API for use in the greater scikit-learn ecosystem. The fact that most of the metric learners use different representations of their constraint information is a big pain, and the existing way of generating random constraints isn't useful for real world learning.

In light of all this, I think it makes sense to:

• scrap the prepare_constraints functions as they currently exist, and move their sampling functionality to the test suite.
• create a ConstraintTransformer class for each kind of constraint, which should be sklearn Pipeline-compatible and do the work of converting partial label vectors to the appropriate format.
• look for places where the fit() methods are expecting parameters that should be moved to the constructor, and fix them.

Let me know what you think.

[svecon]

Problem with ConstraintTransformer classes is, that .transform() in scikit only transforms X, not y. And it would also not be a solution for me, because my Pipeline currently looks like this (notice the kNN after the SDML - I need original labels after the metric learner):

    Pipeline([
        ('imputer', defaultImputer),
        ('standardizer', defaultStandardizer),
        ('sdml', SDML(...)),
        ('knn', KNeighborsClassifier(...)),
    ]),

The only solution which comes to my mind for this scenario is to create ConstraintWrapper classes for each type of constraint and they would expect the metric-transformer in their constructor.
This would separate the concerns (original metric learner classes would expect only the constraints, not labels) and it would allow me to use my current pipelines.

What do you think?

[svecon]

I thinking about this some more and even started implementing a skeleton. However I came to a conclusion that it adds up too much unnecessary complexity. Having two different methods (.fit(), .fit_with_constraints()) is the simplest solution and I think it works well in most scenarios.

.fit_with_constraints() works exactly as it used to and .fit() follows scikit-learn conventions and essentially is only a helper function, which calls original .fit_with_constraints()

I think it makes more sense than having separate classes for different constraints.

[svecon]

I have reverted the classes to their original functionality (where .fit() takes constraints) and created new classes XXX_Supervised which are sub-classes of the original classes, and use supervised .fit(X,y) where the constraints are created ad-hoc.
I also moved generating constraints to separate class Constraints.
Does this make sense?

[perimosocordiae]

I really like the new direction you've taken, thank you! I think this fits with the overall design of the package much better, and supports your use-case without too much friction.

I have a number of small nitpicks, which I'll address inline, but I'm +1 for this idea.

[perimosocordiae]

Here and below, I'd put these into one import statement:

from .itml import ITML, ITML_Supervised

[svecon]

I have fixed the problems you pointed out and marked the code with #TODO where you suggested bigger changes.

Anything else to do here before merge?

[perimosocordiae]

Looks good now, merging. Thanks!