@amueller @agramfort Do you agree with the general direction in which this PR is going?

Basically a meta-transformer is coupled with an estimator to provide the transform methods, instead of inheriting from _LearntSelectorMixin.

hum. What's the vision? too fuzzy for me now

Can you please summarize the discussion of #3011 in the description of this PR?

Sure, just give me some time.

#3011 was meant to pilot the idea of having selection based on coefficients/importances performed by a metaestimator rather than a mixin to predictors. Part of the point is not to clutter the individual estimators' APIs with parameters only used in rare cases. Instead, a metaestimator should:

• make code much more explicit as to the meaning of the transformation (i.e. constructing a metaestimator provides intrinsic documentation, while using a predictor as a transformer without comment is a bit awkward);
• provide a focal point for documenting what has become a fairly sophisticated threshold argument, and;
• potentially make it easier to play with thresholds over a pre-fitted model (although the interface for this is new territory without my magic freeze_model wrapper, without non-standardly adding a parameter to transform.

By way of Zen tradeoffs, it prefers "explicit is better than implicit" over "flat is better than nested". Is the vision clearer, @agramfort?

Also, it's possible @maheshakya had no intention to continue working on this, but @MechCoder it may be wise to check before simply taking it over, even if it's stale.

The main immediate benefits of this are: there's no need to move the threshold parameter from transform to the class to make it gridsearchable; there's no need to add the mixin to all classes that have feature_importances_ or coef_ for the sake of consistency.

@ogrisel, I think most of the discussion on #3011 was implementation detail. A surprising amount needs to be touched to make this complete as a deprecation, including examples, tests, etc.

Thank you @MechCoder for bringing this up again. You can continue this :)
I was not able to complete it since we did not have enough opinion whether to change every example and test according to this and so. So, as @jnothman mentioned the amount of work that needs to be done on those is quite large. I think we should wait to hear what others have to say and for a final resolution about changes.

@jnothman I think I get the design decision but my question was more the end user point of view. Somehing along the line of "Users with be able to do XXX with estimators A, B and C by only adding this piece of code in ...".

@jnothman @maheshakya Sorry for taking this PR over without saying. I incorrectly assumed that the lack of activity for around 6 months that the Pull Request is stale.

@agramfort A transform can meet two different things now.
1] Implementing the transform method in a transformer means transforming the sample points to a centroid space using the euclidean distances.
2] Inheriting from _LearntSelectorMixin, which means extracting the features of X which are below a certain threshold.

Though both are indeed transforming, inheriting from _LearntSelectorMixin classifies all models as transformers, which transform into different dimensional space. By using this metatransformer, such ambiguity is resolved.

So it goes like "Users will be able to extract the n most important features by wrapping around this Metatransformer with all estimators, without having to subclass from _LearntSelectorMixin"

(It's more that stale doesn't strictly imply ripe for adoption. There may
be reasons why something is stale.)

On 17 February 2015 at 04:00, Alexandre Gramfort [email protected]
wrote:

ok got it. I like the idea. I guess we need to document this new
SelectFromModel class and explain its usage. It should also appear in
one of the examples.

—
Reply to this email directly or view it on GitHub
#4242 (comment)
.

I agree with the direction, thought I didn't have too close a look. Is the idea to remove transform from the models that already have the mixin?

Sorry, for the huge delay. I just had a look at the pull request today and found out all the hard work has already been done by @maheshakya . I've updated the pull request description to list out todo's about what all is left to be done.

I will not be able to work on this in the near future. I've updated the label as easy as this should be a very good issue for a new developer.

@MechCoder maybe this would be a good project for you to finish? It would be very helpful.

OK, if you insist.

Thinking about it, is it necessary to test each and every model that has a coef_ and feature_importances_ attribute after fitting. I don't think so because the implementation in SelectFromModel is independent of the implementation of the underlying base estimators.

@amueller could you verify that the tests in test_from_model are enough?

doctests raised some Function transform is deprecated warnings.

sklearn.utils.tests.test_estimator_checks.test_check_estimator also has one. the rest seems good. [you could raise instead to see if it happens in the tests]

@amueller thanks for your comments. The last commit should address all of them.

Once those are fixed up, it's good for merge (LGTM).

@jnothman I'll merge after Travis passes?

Yes, with two whats_new entries: one under API changes to tell people their transforms will be disappearing, and one under new features. thank you.

Thanks a lot! @maheshakya @glouppe @amueller @jnothman

🍻 was lange währt währt endlich gut. Thanks @MechCoder :)