[MRG+2] Norm inconsistency between RFE and SelectFromModel (was _LearntSelectorMixin) #2121 by antoinewdg · Pull Request #6181 · scikit-learn/scikit-learn

antoinewdg · 2016-01-18T17:12:37Z

As @jnothman mentionned in the mailing list it would be nice to also add this option for RFE. The unit tests for RFE cover the case of a sparse coeff matrix, which I have trouble to handle, so I would need a little help if this were to be done.

…orMixin) scikit-learn#2121

jnothman · 2016-01-18T23:19:30Z

Thanks! Unfortunately, norm along an axis is not supported in the earliest version of numpy we support. You could add an axis-supporting variant to sklearn.utils.fixes. Or reuse sklearn.preprocessing.normalize which supports fewer ord options.

Also, please add tests.

MechCoder · 2016-01-20T21:13:45Z

sklearn/feature_selection/from_model.py

As @jnothman says, you can use sklearn.preprocessing.normalize to handle ord not being supported in older versions of NumPy

MechCoder · 2016-01-20T21:24:01Z

For the test, I would just say to

Fit SelectFromModel on multi-target data, transform the data
Fit the same base estimator on the same data, manually compute the mask by comparing norm(coefficients) < threshold, mask the data
Check 1] and 2] are same. Similar to (https://github.com/scikit-learn/scikit-learn/blob/master/sklearn/feature_selection/tests/test_from_model.py#L73)

jnothman · 2016-01-20T21:50:42Z

(I think this is more about multiclass than multi-target.) You could
alternatively create a dummy estimator where fit() stores a fixed coef_.

On 21 January 2016 at 08:24, Manoj Kumar notifications@github.com wrote:

For the test, I would just say to

Fit SelectFromModel on multi-target data, transform the data

Fit the same base estimator on the same data, manually compute the mask
by comparing norm(coefficients) < threshold, mask the data
Check 1] and 2] are same. Similar to (
https://github.com/scikit-learn/scikit-learn/blob/master/sklearn/feature_selection/tests/test_from_model.py#L73
)

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

MechCoder · 2016-01-20T21:57:02Z

Were you referring to the common use-case or to the test that I linked to?

jnothman · 2016-01-20T22:10:11Z

Probably taking your comments out of context.

On 21 January 2016 at 08:57, Manoj Kumar notifications@github.com wrote:

Were you referring to the common use-case or to the test that I linked to?

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

jnothman · 2016-01-20T22:11:11Z

Is multi-target supported here?

MechCoder · 2016-01-20T23:16:47Z

It seems so, although untested.

jnothman · 2016-01-21T02:42:18Z

Remind me what the shape of multitarget multiclass coef_ is?

MechCoder · 2016-01-21T03:21:02Z

Oh I meant multitarget/multioutput in a regression setting, not "multitarget multiclass"

jnothman · 2016-01-21T03:29:43Z

Oh, okay.

On 21 January 2016 at 14:21, Manoj Kumar notifications@github.com wrote:

Oh I meant multitarget in a regression setting, not "multitarget
multiclass"

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

antoinewdg · 2016-01-25T17:20:54Z

Thank you for all these pieces of advice, if all goes well I will update the pull request within this week.

antoinewdg · 2016-01-26T15:53:10Z

I chose to implement a fix for the norm function in sklearn.utils.fixes inspired by sklearn.preprocessing.normalize The code in normalize is consequently not really DRY, is it OK to refractor it to use the fix ?

antoinewdg · 2016-01-26T22:20:02Z

It seems I got confused with the numpy version to check when implementing the fix (keepdims only exists in numpy 1.10), I'll fix this as soon as possible.

MechCoder · 2016-02-02T05:43:39Z

sklearn/utils/fixes.py

The keepdims argument seems to be YAGNI to me atleast in this context.

Now that you mention it, it seems silly to have bothered with it !

MechCoder · 2016-02-02T05:56:40Z

LGTM pending comments

…into fix-6181

MechCoder · 2016-02-05T06:00:32Z

[MRG] -> [MRG+1]

MechCoder · 2016-04-13T16:17:58Z

@jnothman Merge?

jnothman · 2016-04-14T02:53:38Z

sklearn/feature_selection/from_model.py


+    norm_order : non-zero int, inf, -inf, default 1
+        Order of the norm used to filter the vectors of coefficients below
+        ``threshold`` in the case where the ``coeff_`` attribute of the


*coef_ (one f)

jnothman · 2016-04-14T03:00:45Z

Please handle that not (0, 1) case
Fix that typo
Add an entry in doc/whats_new.rst
LGTM; Merge!

amueller · 2016-10-11T01:13:41Z

any updates on this?

# Conflicts: # sklearn/feature_selection/tests/test_from_model.py # sklearn/utils/fixes.py # sklearn/utils/tests/test_fixes.py

antoinewdg · 2016-10-23T07:31:50Z

Here are the last fixes, sorry about this ridiculous delay.

amueller · 2016-10-24T15:50:32Z

@antoinewdg don't sweat it, recently found an issue from 2013 that I forgot to reply to ;)

amueller · 2016-10-24T15:51:47Z

sklearn/utils/fixes.py

 else:
    from numpy.ma import MaskedArray    # noqa
+
+if 'axis' not in signature(np.linalg.norm).parameters:


Can you maybe add the numpy version this was added in a comment?

amueller · 2016-10-24T15:52:56Z

Other than that, LGTM, too

antoinewdg · 2016-10-24T16:25:09Z

Sure! I'm not really sure what version that is though. The docs start mentioning it from 1.8 so I'll go with that, can't find any patch note about that.

antoinewdg · 2016-10-24T16:49:59Z

While looking for patch notes I stumbled upon this stackoverflow thread.
~~This makes me kinda sad, but maybe using einsum directly instead of implementing a whole fix is simpler ?~~

EDIT: no this is stupid, we would have to make a big 'switch' over the order then.

amueller · 2016-10-24T18:01:47Z

Yeah you should go with when the docs mention it. I'm confused on why you would do einsum instead of the transpose but whatever. The patch looks good :)

@jnothman

…ntSelectorMixin) scikit-learn#2121 (scikit-learn#6181) * Norm inconsistency between RFE and SelectFromModel (was _LearntSelectorMixin) scikit-learn#2121 * safe_pwr utility * Norm fix * Removed safe_pwr * 1D arrays support for norm fix * Test case for 2d coef in SelectFromModel * Fix numpy version requirement for norm fix * Implement fixes suggested by @jnothman * Add numpy version requiring the fix.

@jnothman

…ntSelectorMixin) scikit-learn#2121 (scikit-learn#6181) * Norm inconsistency between RFE and SelectFromModel (was _LearntSelectorMixin) scikit-learn#2121 * safe_pwr utility * Norm fix * Removed safe_pwr * 1D arrays support for norm fix * Test case for 2d coef in SelectFromModel * Fix numpy version requirement for norm fix * Implement fixes suggested by @jnothman * Add numpy version requiring the fix.

@jnothman

…ntSelectorMixin) scikit-learn#2121 (scikit-learn#6181) * Norm inconsistency between RFE and SelectFromModel (was _LearntSelectorMixin) scikit-learn#2121 * safe_pwr utility * Norm fix * Removed safe_pwr * 1D arrays support for norm fix * Test case for 2d coef in SelectFromModel * Fix numpy version requirement for norm fix * Implement fixes suggested by @jnothman * Add numpy version requiring the fix.

@jnothman

…ntSelectorMixin) scikit-learn#2121 (scikit-learn#6181) * Norm inconsistency between RFE and SelectFromModel (was _LearntSelectorMixin) scikit-learn#2121 * safe_pwr utility * Norm fix * Removed safe_pwr * 1D arrays support for norm fix * Test case for 2d coef in SelectFromModel * Fix numpy version requirement for norm fix * Implement fixes suggested by @jnothman * Add numpy version requiring the fix.

Norm inconsistency between RFE and SelectFromModel (was _LearntSelect…

4a9f8a4

…orMixin) scikit-learn#2121

antoinewdg added 3 commits January 19, 2016 13:55

safe_pwr utility

8b09177

Norm fix

ecaa20d

Removed safe_pwr

06428c3

MechCoder changed the title ~~Norm inconsistency between RFE and SelectFromModel (was _LearntSelectorMixin) #2121~~ [MRG] Norm inconsistency between RFE and SelectFromModel (was _LearntSelectorMixin) #2121 Jan 20, 2016

MechCoder reviewed Jan 20, 2016
View reviewed changes

1D arrays support for norm fix

b2aba79

antoinewdg added 2 commits January 26, 2016 16:20

Test case for 2d coef in SelectFromModel

71999eb

Merge remote-tracking branch 'central/master'

cc48b1a

Fix numpy version requirement for norm fix

7fc8297

MechCoder reviewed Feb 2, 2016
View reviewed changes

Merge branch 'master' of https://github.com/scikit-learn/scikit-learn …

8b59e16

…into fix-6181

MechCoder changed the title ~~[MRG] Norm inconsistency between RFE and SelectFromModel (was _LearntSelectorMixin) #2121~~ [MRG+1] Norm inconsistency between RFE and SelectFromModel (was _LearntSelectorMixin) #2121 Feb 5, 2016

jnothman reviewed Apr 14, 2016
View reviewed changes

antoinewdg added 2 commits October 22, 2016 19:32

Merge remote-tracking branch 'upstream/master'

c6e71bf

# Conflicts: # sklearn/feature_selection/tests/test_from_model.py # sklearn/utils/fixes.py # sklearn/utils/tests/test_fixes.py

Implement fixes suggested by @jnothman

6052f33

amueller reviewed Oct 24, 2016

View reviewed changes

amueller changed the title ~~[MRG+1] Norm inconsistency between RFE and SelectFromModel (was _LearntSelectorMixin) #2121~~ [MRG+2] Norm inconsistency between RFE and SelectFromModel (was _LearntSelectorMixin) #2121 Oct 24, 2016

Add numpy version requiring the fix.

b4e0127

antoinewdg force-pushed the master branch from e5593a6 to b4e0127 Compare October 24, 2016 16:22

amueller merged commit 74a9756 into scikit-learn:master Oct 24, 2016

haiatn mentioned this pull request Aug 26, 2023

Norm inconsistency between RFE and SelectFromModel #2121

Closed

Uh oh!

Conversation

antoinewdg commented Jan 18, 2016

Uh oh!

jnothman commented Jan 18, 2016

Uh oh!

MechCoder Jan 20, 2016

Choose a reason for hiding this comment

Uh oh!

MechCoder commented Jan 20, 2016

Uh oh!

jnothman commented Jan 20, 2016

Uh oh!

MechCoder commented Jan 20, 2016

Uh oh!

jnothman commented Jan 20, 2016

Uh oh!

jnothman commented Jan 20, 2016

Uh oh!

MechCoder commented Jan 20, 2016

Uh oh!

jnothman commented Jan 21, 2016

Uh oh!

MechCoder commented Jan 21, 2016

Uh oh!

jnothman commented Jan 21, 2016

Uh oh!

antoinewdg commented Jan 25, 2016

Uh oh!

antoinewdg commented Jan 26, 2016

Uh oh!

antoinewdg commented Jan 26, 2016

Uh oh!

MechCoder Feb 2, 2016

Choose a reason for hiding this comment

Uh oh!

antoinewdg Feb 4, 2016

Choose a reason for hiding this comment

Uh oh!

MechCoder commented Feb 2, 2016

Uh oh!

MechCoder commented Feb 5, 2016

Uh oh!

MechCoder commented Apr 13, 2016

Uh oh!

jnothman Apr 14, 2016

Choose a reason for hiding this comment

Uh oh!

jnothman commented Apr 14, 2016

Uh oh!

amueller commented Oct 11, 2016

Uh oh!

antoinewdg commented Oct 23, 2016

Uh oh!

amueller commented Oct 24, 2016

Uh oh!

amueller Oct 24, 2016

Choose a reason for hiding this comment

Uh oh!

amueller commented Oct 24, 2016

Uh oh!

antoinewdg commented Oct 24, 2016

Uh oh!

antoinewdg commented Oct 24, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

amueller commented Oct 24, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

antoinewdg commented Oct 24, 2016 •

edited

Loading