Thanks for tackling this. A number of points:

• please put this in the same file as _LearntSelectorMixin
• although I suggested inheriting from _LearntSelectorMixin, it isn't sufficient for correct operation. _LearntSelectorMixin.transform inspects the estimator directly, e.g. to inspect the penalty parameter. You will need to turn _LearntSelectorMixin.transform into one or more functions.
• You shouldn't set any underscore-suffixed attributes in your estimator's __init__. This should be done in fit. It's fine to postpone validation to fit.
• You shouldn't need to have estimator_params. clone will keep any settings, and BaseEstimator will handle the delegation of attribute setting so that this can work in grid search.
• SelectFromModel should accept two parameters: estimator and threshold.
• I am not sure about duplicating the coefficients here to store them as an attribute. It would be more relevant to store the aggregate feature importances: because _LearntSelectorMixin currently has to sum over the coefficients for multiclass linear models, it might be useful to see the summed features.) But it's not essential to store these locally.
• It would also be nice to have an attribute threshold_ since _LearntSelectorMixin's calculation of the actual threshold can be non-trivial.
• SelectFromModel should inherit from sklearn.feature_selection.base.SelectorMixin and implement _get_support_mask.
• This PR should include the deprecation of _LearntSelectorMixin. Use the @deprecated decorator on transform.

I'm changing this PR to a [WIP]. I hope that's alright with you.

Also, the failing tests happen because this can't be constructed as it is. I think it should be included here: https://github.com/scikit-learn/scikit-learn/blob/master/sklearn/utils/testing.py#L441.

And although it does nothing at the moment, please inherit from sklearn.base.MetaEstimatorMixin.

I have separated out the section where threshold, importances and penalty is retrieved in _LearntSelectorMixin into two functions. One of them uses @depricated decorator.

Other issues are fixed.

Since threshold in now a parameter to SelectFromModel, transform function should use that value. So in that case the value taken from the transform function will be useless.
Is this a correct way?

When transform function is called from another estimator, it will act in the usual way.

Coverage remained the same when pulling 9506120 on maheshakya:select_from_model into eb10c4c on scikit-learn:master.

So in that case the value taken from the transform function will be useless.

The correct way is to redefine transform in the metaestimator not to take a threshold parameter.

I don't think it's a good idea to store support_mask_. It is inconsistent with other feature selectors, duplicates the get_support method provided by SelectorMixin, and means you can't change the threshold and just call get_support to reevaluate it.

Btw, it may be cleaner to implement this with the metaestimator not inheriting from the mixin. Please feel free to do it that way if you think it improves the code.

I couldn't find a way to implement _get_support_mask function without storing the mask in the transform function without duplicating the code. So I made it a private member.

Coverage remained the same when pulling be4f070 on maheshakya:select_from_model into fbe974b on scikit-learn:master.

This should probably support partial_fit. It should possibly also support warm_start, which would involve not using clone.

@jnothman, I apologize for the inconvenience, Can you explain what exactly needs to be done in partial_fit. I checked out several estimators that defines it. But those seem to be performing different tasks.

partial_fit allows a model to be trained without keeping all the training data in memory (or providing it) at once. Here, partial_fit should create estimator_ only on the first call, and call estimator_.partial_fit for each call.

The only reason I say it's necessary is because it's a way the current mixin can be used.

I have added partial_fit and warm_start.

BTW is there something wrong with Travis CI? It doesn't seem to be building my last two commits.

BTW is there something wrong with Travis CI? It doesn't seem to be building my last two commits.

It's highly non user-friendly, but this means that it can't automatically merge your work with master, which it does before testing. Could you please rebase on the current master and force-push the rebased branch?

Coverage remained the same when pulling 9a41f2f on maheshakya:select_from_model into fec2867 on scikit-learn:master.

Coverage remained the same when pulling 1469c48 on maheshakya:select_from_model into fec2867 on scikit-learn:master.

Coverage remained the same when pulling ccbab10 on maheshakya:select_from_model into fec2867 on scikit-learn:master.

Coverage remained the same when pulling 77b30e4 on maheshakya:select_from_model into fec2867 on scikit-learn:master.

I'm not sure the tests are quite satisfactory yet (and I might not be around for the next few days to take another look), but I think we need to seek opinions as to whether a meta-estimator improves on the mixin. @glouppe, WDYT, and who else is likely to be opinionated on this?

What are the tests that need to be improved. I can work on those.

I'm not able to look in great detail at the tests right now, but in order
for this patch to be complete, internal uses of the old (mixin) behaviour
need to be changed, including in documentation and examples, including
https://github.com/scikit-learn/scikit-learn/blob/master/doc/modules/feature_selection.rst#l1-based-feature-selection,
https://github.com/scikit-learn/scikit-learn/blob/master/doc/modules/feature_selection.rst#tree-based-feature-selectionand
probably other places.

On 2 April 2014 02:35, maheshakya [email protected] wrote:

What are the tests that need to be improved. I can work on those.

Reply to this email directly or view it on GitHubhttps://github.com//pull/3011#issuecomment-39220058
.

Thanks. I will give it a shot. (for improved test cases, examples and documentation)

Coverage remained the same when pulling 7f12af3 on maheshakya:select_from_model into fec2867 on scikit-learn:master.

Coverage remained the same when pulling 11d1e81 on maheshakya:select_from_model into fec2867 on scikit-learn:master.

I suppose we need to change every example that uses feature selection based on LearntSelectorMixin, right?

Yes, unfortunately.

It seems that there are unrelated changes. I'm not sure how adding a meta-transformer should affect travis.yml

Closed in favor of #4242 . I rescued this PR from git hell!