[MRG+2] ENH: used SelectorMixin in BaseRandomizedLinearModel #8263

dalmia · 2017-02-01T19:42:17Z

Reference Issue

What does this implement/fix? Explain your changes.

This uses SelectorMixin in BaseRandomizedLinearModel renaming the get_support function to _get_support_mask in order to utilize the transform() and inverse_transform() methods provided by SelectorMixin.

jnothman · 2017-02-01T21:45:43Z

get_support() is public and cannot just be removed from one release to the next. Please keep it.

jnothman · 2017-02-03T01:35:10Z

sklearn/linear_model/randomized_l1.py

@@ -128,6 +128,9 @@ def _get_support_mask(self, indices=False):
        mask = self.scores_ > self.selection_threshold
        return mask if not indices else np.where(mask)[0]

+    def get_support(self, indices=False):


docstring, please

jnothman · 2017-02-07T06:36:24Z

LGTM. I note that this also fixes a bug where Randomized* breaks with sparse input.

dalmia · 2017-02-07T07:13:30Z

So does this need an extra review or is it okay for merge?

jnothman · 2017-02-07T10:54:49Z

I would have merged if i thought that was the right thing to do. we make changes by consensus, and so far no other core dev has suggested this is even a worthwhile thing to do.

…

On 7 Feb 2017 6:13 pm, "Aman Dalmia" ***@***.***> wrote: So does this need an extra review or is it okay for merge? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#8263 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAEz6_l-59JxknZk1OX8DjfPLR-mEelPks5raBmbgaJpZM4L0S-_> .

dalmia · 2017-02-07T11:49:39Z

Thanks for clearing it up. Just intended to know if there is some other improvement under consideration.

tguillemot · 2017-02-08T09:08:18Z

LGTM
Thanks @dalmia

TomDLT

LGTM

Can you add a bug fix entry in doc/whats_new.rst?

dalmia · 2017-02-10T13:47:20Z

@TomDLT 👍

lesteve · 2017-02-10T13:52:19Z

sklearn/linear_model/randomized_l1.py

-        Xt = np.zeros((X.shape[0], support.size))
-        Xt[:, support] = X
-        return Xt
+    def get_support(self, indices=False):


I am confused why do you need to override SelectorMixin.get_support? Should _get_support_mask not be:

def _get_support_mask(self): check_is_fitted(self, 'scores_') return self.scores_ > self.selection_threshold

and then SelectorMixin.get_support would take over and do what it is supposed to do?

I agree, it is better

with a docstring

""" Get the boolean mask indicating which features are selected Returns ------- support : boolean array of shape [# input features] An element is True iff its corresponding feature is selected for retention. """

It was specified to me earlier by @jnothman that since get_support was already public, we shouldn't remove it from one release to the next.

I guess he overlooked it:
As the class now inherits from SelectorMixin, the method get_support is inherited so it is not removed.

I just realized that as you removed TransformerMixin, the class does not inherit anymore from the method fit_transform, which is public and should not be removed.
Why don't you keep TransformerMixin ?

Actually I removed it since it seemed like SelectorMixin inherits TransformerMixin?

class SelectorMixin(six.with_metaclass(ABCMeta, TransformerMixin)):

oh yes of course, my mistake

Please address the comment on get_support

Yes, I'll make the change.

codecov · 2017-02-10T15:32:51Z

Codecov Report

❗ No coverage uploaded for pull request base (master@dfcf632). Click here to learn what that means.

@@            Coverage Diff            @@
##             master    #8263   +/-   ##
=========================================
  Coverage          ?   94.74%           
=========================================
  Files             ?      342           
  Lines             ?    60722           
  Branches          ?        0           
=========================================
  Hits              ?    57531           
  Misses            ?     3191           
  Partials          ?        0

Impacted Files	Coverage Δ
sklearn/linear_model/randomized_l1.py	`94.7% <100%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dfcf632...873a8e7. Read the comment docs.

…learn#8263) * ENH: used SelectorMixin in BaseRandomizedLinearModel * FIX: added get_support to return _get_support_mask * FIX: added docstring for get_support() * DOC: added bug fix entry to whats_new * FIX: removed redundant get_support()

ENH: used SelectorMixin in BaseRandomizedLinearModel

dd7e959

FIX: added get_support to return _get_support_mask

dc9ae89

jnothman reviewed Feb 3, 2017

View reviewed changes

FIX: added docstring for get_support()

538480a

jnothman changed the title ~~[MRG] ENH: used SelectorMixin in BaseRandomizedLinearModel~~ [MRG+1] ENH: used SelectorMixin in BaseRandomizedLinearModel Feb 7, 2017

jnothman added the Bug label Feb 7, 2017

tguillemot approved these changes Feb 8, 2017

View reviewed changes

TomDLT approved these changes Feb 10, 2017

View reviewed changes

TomDLT changed the title ~~[MRG+1] ENH: used SelectorMixin in BaseRandomizedLinearModel~~ [MRG+2] ENH: used SelectorMixin in BaseRandomizedLinearModel Feb 10, 2017

lesteve reviewed Feb 10, 2017

View reviewed changes

DOC: added bug fix entry to whats_new

617472e

FIX: removed redundant get_support()

873a8e7

TomDLT merged commit 38d59c7 into scikit-learn:master Feb 13, 2017

dalmia deleted the 8259 branch February 13, 2017 14:20

Przemo10 mentioned this pull request Mar 17, 2017

update fork (#1) #8606

Closed

Uh oh!

[MRG+2] ENH: used SelectorMixin in BaseRandomizedLinearModel #8263

[MRG+2] ENH: used SelectorMixin in BaseRandomizedLinearModel #8263

Uh oh!

Conversation

dalmia commented Feb 1, 2017

Reference Issue

What does this implement/fix? Explain your changes.

Uh oh!

jnothman commented Feb 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jnothman Feb 3, 2017

Choose a reason for hiding this comment

Uh oh!

jnothman commented Feb 7, 2017

Uh oh!

dalmia commented Feb 7, 2017

Uh oh!

jnothman commented Feb 7, 2017 via email

Uh oh!

dalmia commented Feb 7, 2017

Uh oh!

tguillemot commented Feb 8, 2017

Uh oh!

TomDLT left a comment

Choose a reason for hiding this comment

Uh oh!

dalmia commented Feb 10, 2017

Uh oh!

lesteve Feb 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TomDLT Feb 10, 2017

Choose a reason for hiding this comment

Uh oh!

TomDLT Feb 10, 2017

Choose a reason for hiding this comment

Uh oh!

dalmia Feb 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TomDLT Feb 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TomDLT Feb 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dalmia Feb 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TomDLT Feb 10, 2017

Choose a reason for hiding this comment

Uh oh!

TomDLT Feb 10, 2017

Choose a reason for hiding this comment

Uh oh!

dalmia Feb 10, 2017

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Feb 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

jnothman commented Feb 1, 2017 •

edited

Loading

lesteve Feb 10, 2017 •

edited

Loading

dalmia Feb 10, 2017 •

edited

Loading

TomDLT Feb 10, 2017 •

edited

Loading

TomDLT Feb 10, 2017 •

edited

Loading

dalmia Feb 10, 2017 •

edited

Loading

codecov bot commented Feb 10, 2017 •

edited

Loading