[MRG + 2] Fixed parameter setting in SelectFromModel #7764

amueller · 2016-10-26T21:57:31Z

Fixes #7756.

…electFromModel

jnothman · 2016-10-26T22:39:33Z

but should there be a way to use a model with warm_start=True? Such was certainly possible with the mixin.

amueller · 2016-10-27T15:23:58Z

Why not call fs.estimator_.fit() if you want to warm-start?

jnothman · 2016-10-27T21:08:26Z

Why not call fs.estimator_.fit() if you want to warm-start?

Because that requires understanding (and assuming) that fs.fit() does nothing else?

amueller · 2016-10-27T21:27:10Z

So you'd prefer adding a warm_start parameter to SelectFromModel? I'm ok with that, too.

raghavrv · 2016-10-27T23:16:05Z

doc/whats_new.rst

@@ -102,6 +102,9 @@ Bug fixes
     <https://github.com/scikit-learn/scikit-learn/pull/7676>`_)
     by `Mohammed Affan`_

+   - Fixed setting parameters when calling ``fit`` multiple times on
+     :ref:`feature_selection.SelectFromModel`. :issue:`7756` by `Andreas Müller`_


Should this be ref and not class?

jnothman · 2016-11-01T05:07:15Z

Nah, with prefit, it should be fine without warm_start. Please fix up @raghavrv's issue in what's new and LGTM.

amueller · 2016-11-02T19:48:26Z

done

raghavrv · 2016-11-07T14:03:34Z

LGTM pending rebase

jnothman · 2016-11-07T20:01:26Z

(FWIW, @raghavrv, now that we're mostly relying on squash and merge, I
think we can be asking people to merge with an updated master, which may be
more familiar to many than a rebase and does not involve a force-push. So
by pending rebase you could just mean resolving merge conflicts with
master...)

On 8 November 2016 at 01:03, Raghav RV notifications@github.com wrote:

LGTM pending rebase

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#7764 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAEz64taOmVDdb_hxa6bxuMNmohCJZzdks5q7y-3gaJpZM4Kht9y
.

raghavrv · 2016-11-07T20:05:59Z

So by pending rebase you could just mean resolving merge conflicts with master...

Indeed!

think we can be asking people to merge with an updated master, which may be
more familiar to many than a rebase and does not involve a force-push.

Ah the merge commits can be squashed. Neat. Thanks for letting me know :)

jnothman · 2016-11-07T20:58:01Z

Well, the squash and merge basically takes the diff and ignores commit
history, except for assigning an author, a timestamp, and a proposed
message.

On 8 November 2016 at 07:06, Raghav RV notifications@github.com wrote:

So by pending rebase you could just mean resolving merge conflicts with
master...

Indeed!

think we can be asking people to merge with an updated master, which may be
more familiar to many than a rebase and does not involve a force-push.

Ah the merge commits can be squashed. Neat. Thanks for letting me know :)

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#7764 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAEz64W3vKLyazUuppHg0WLYcr6XACQgks5q74SogaJpZM4Kht9y
.

# Conflicts: # doc/whats_new.rst

amueller · 2016-11-09T18:34:46Z

did a merge commit. Let's see how this goes.

raghavrv · 2016-11-09T19:26:25Z

Merging once appveyor passes, which will be in 8 mins approx ;)

raghavrv · 2016-11-09T19:34:27Z

Thanks @amueller

amueller · 2016-11-09T20:36:08Z

looks like it worked. We should do merges instead of rebases so the review comments don't get messed up. Maybe we should add that to the PR template?

jnothman · 2016-11-09T20:38:52Z

Should do merges instead of rebases to discourage push -f too

On 10 November 2016 at 07:36, Andreas Mueller notifications@github.com
wrote:

looks like it worked. We should do merges instead of rebases so the review
comments don't get messed up. Maybe we should add that to the PR template?

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#7764 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAEz61qaj0qxWRjI78y7tl3x-NhaMX7eks5q8i65gaJpZM4Kht9y
.

* Fixed cloning ``estimator`` again when calling fit a second time in SelectFromModel * fix link in whatsnew

* tag '0.18.1': (144 commits) skip tree-test on 32bit do the warning test as we do it in other places. Replase assert_equal by assert_almost_equal in cosine test version bump 0.18.1 fix merge conflict mess in whatsnew add the python2.6 warning to 0.18.1 fix learning_curve test that I messed up in cherry-picking the "reentrant cv" PR. sync whatsnew with master [MRG] TST Ensure __dict__ is unmodified by predict, transform, etc (scikit-learn#7553) FIX scikit-learn#6420: Cloning decision tree estimators breaks criterion objects (scikit-learn#7680) Add whats new entry for scikit-learn#6282 (scikit-learn#7629) [MGR + 2] fix selectFdr bug (scikit-learn#7490) fixed whatsnew cherry-pick mess (somewhat) [MRG + 2] FIX LogisticRegressionCV to correctly handle string labels (scikit-learn#5874) [MRG + 2] Fixed parameter setting in SelectFromModel (scikit-learn#7764) [MRG+2] DOC adding separate `fit()` methods (and docstrings) for DecisionTreeClassifier and DecisionTreeRegressor (scikit-learn#7824) Fix docstring typo (scikit-learn#7844) n_features --> n_components [MRG + 1] DOC adding :user: role to whats_new (scikit-learn#7818) [MRG+1] label binarizer not used consistently in CalibratedClassifierCV (scikit-learn#7799) DOC : fix docstring of AIC/BIC in GMM ...

* releases: (144 commits) skip tree-test on 32bit do the warning test as we do it in other places. Replase assert_equal by assert_almost_equal in cosine test version bump 0.18.1 fix merge conflict mess in whatsnew add the python2.6 warning to 0.18.1 fix learning_curve test that I messed up in cherry-picking the "reentrant cv" PR. sync whatsnew with master [MRG] TST Ensure __dict__ is unmodified by predict, transform, etc (scikit-learn#7553) FIX scikit-learn#6420: Cloning decision tree estimators breaks criterion objects (scikit-learn#7680) Add whats new entry for scikit-learn#6282 (scikit-learn#7629) [MGR + 2] fix selectFdr bug (scikit-learn#7490) fixed whatsnew cherry-pick mess (somewhat) [MRG + 2] FIX LogisticRegressionCV to correctly handle string labels (scikit-learn#5874) [MRG + 2] Fixed parameter setting in SelectFromModel (scikit-learn#7764) [MRG+2] DOC adding separate `fit()` methods (and docstrings) for DecisionTreeClassifier and DecisionTreeRegressor (scikit-learn#7824) Fix docstring typo (scikit-learn#7844) n_features --> n_components [MRG + 1] DOC adding :user: role to whats_new (scikit-learn#7818) [MRG+1] label binarizer not used consistently in CalibratedClassifierCV (scikit-learn#7799) DOC : fix docstring of AIC/BIC in GMM ... Conflicts: removed sklearn/externals/joblib/__init__.py sklearn/externals/joblib/_parallel_backends.py sklearn/externals/joblib/testing.py

* dfsg: (144 commits) skip tree-test on 32bit do the warning test as we do it in other places. Replase assert_equal by assert_almost_equal in cosine test version bump 0.18.1 fix merge conflict mess in whatsnew add the python2.6 warning to 0.18.1 fix learning_curve test that I messed up in cherry-picking the "reentrant cv" PR. sync whatsnew with master [MRG] TST Ensure __dict__ is unmodified by predict, transform, etc (scikit-learn#7553) FIX scikit-learn#6420: Cloning decision tree estimators breaks criterion objects (scikit-learn#7680) Add whats new entry for scikit-learn#6282 (scikit-learn#7629) [MGR + 2] fix selectFdr bug (scikit-learn#7490) fixed whatsnew cherry-pick mess (somewhat) [MRG + 2] FIX LogisticRegressionCV to correctly handle string labels (scikit-learn#5874) [MRG + 2] Fixed parameter setting in SelectFromModel (scikit-learn#7764) [MRG+2] DOC adding separate `fit()` methods (and docstrings) for DecisionTreeClassifier and DecisionTreeRegressor (scikit-learn#7824) Fix docstring typo (scikit-learn#7844) n_features --> n_components [MRG + 1] DOC adding :user: role to whats_new (scikit-learn#7818) [MRG+1] label binarizer not used consistently in CalibratedClassifierCV (scikit-learn#7799) DOC : fix docstring of AIC/BIC in GMM ...

* Fixed cloning ``estimator`` again when calling fit a second time in SelectFromModel * fix link in whatsnew

Fixed cloning estimator again when calling fit a second time in S…

9bafc65

…electFromModel

amueller added the Blocker label Oct 26, 2016

amueller added this to the 0.18.1 milestone Oct 26, 2016

amueller added Bug and removed Blocker labels Oct 27, 2016

raghavrv reviewed Oct 27, 2016

View reviewed changes

jnothman changed the title ~~Fixed parameter setting in SelectFromModel~~ [MRG+1] Fixed parameter setting in SelectFromModel Nov 1, 2016

fix link in whatsnew

df33230

raghavrv changed the title ~~[MRG+1] Fixed parameter setting in SelectFromModel~~ [MRG + 2] Fixed parameter setting in SelectFromModel Nov 7, 2016

Merge branch 'master' into reseting_estimator_from_model

ee8b415

# Conflicts: # doc/whats_new.rst

raghavrv merged commit 9667ff2 into scikit-learn:master Nov 9, 2016

amueller added a commit to amueller/scikit-learn that referenced this pull request Nov 9, 2016

[MRG + 2] Fixed parameter setting in SelectFromModel (scikit-learn#7764)

b47f36c

* Fixed cloning ``estimator`` again when calling fit a second time in SelectFromModel * fix link in whatsnew

sergeyf pushed a commit to sergeyf/scikit-learn that referenced this pull request Feb 28, 2017

[MRG + 2] Fixed parameter setting in SelectFromModel (scikit-learn#7764)

afcba59

* Fixed cloning ``estimator`` again when calling fit a second time in SelectFromModel * fix link in whatsnew

Sundrique pushed a commit to Sundrique/scikit-learn that referenced this pull request Jun 14, 2017

[MRG + 2] Fixed parameter setting in SelectFromModel (scikit-learn#7764)

a85f143

* Fixed cloning ``estimator`` again when calling fit a second time in SelectFromModel * fix link in whatsnew

NelleV pushed a commit to NelleV/scikit-learn that referenced this pull request Aug 11, 2017

[MRG + 2] Fixed parameter setting in SelectFromModel (scikit-learn#7764)

49c51c3

* Fixed cloning ``estimator`` again when calling fit a second time in SelectFromModel * fix link in whatsnew

paulha pushed a commit to paulha/scikit-learn that referenced this pull request Aug 19, 2017

[MRG + 2] Fixed parameter setting in SelectFromModel (scikit-learn#7764)

d3e2d05

* Fixed cloning ``estimator`` again when calling fit a second time in SelectFromModel * fix link in whatsnew

maskani-moh pushed a commit to maskani-moh/scikit-learn that referenced this pull request Nov 15, 2017

[MRG + 2] Fixed parameter setting in SelectFromModel (scikit-learn#7764)

a46d105

* Fixed cloning ``estimator`` again when calling fit a second time in SelectFromModel * fix link in whatsnew

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MRG + 2] Fixed parameter setting in SelectFromModel #7764

[MRG + 2] Fixed parameter setting in SelectFromModel #7764

amueller commented Oct 26, 2016

jnothman commented Oct 26, 2016

amueller commented Oct 27, 2016

jnothman commented Oct 27, 2016

amueller commented Oct 27, 2016

raghavrv Oct 27, 2016

jnothman commented Nov 1, 2016

amueller commented Nov 2, 2016

raghavrv commented Nov 7, 2016

jnothman commented Nov 7, 2016

raghavrv commented Nov 7, 2016

jnothman commented Nov 7, 2016

amueller commented Nov 9, 2016

raghavrv commented Nov 9, 2016

raghavrv commented Nov 9, 2016

amueller commented Nov 9, 2016

jnothman commented Nov 9, 2016

[MRG + 2] Fixed parameter setting in SelectFromModel #7764

[MRG + 2] Fixed parameter setting in SelectFromModel #7764

Conversation

amueller commented Oct 26, 2016

jnothman commented Oct 26, 2016

amueller commented Oct 27, 2016

jnothman commented Oct 27, 2016

amueller commented Oct 27, 2016

raghavrv Oct 27, 2016

Choose a reason for hiding this comment

jnothman commented Nov 1, 2016

amueller commented Nov 2, 2016

raghavrv commented Nov 7, 2016

jnothman commented Nov 7, 2016

raghavrv commented Nov 7, 2016

jnothman commented Nov 7, 2016

amueller commented Nov 9, 2016

raghavrv commented Nov 9, 2016

raghavrv commented Nov 9, 2016

amueller commented Nov 9, 2016

jnothman commented Nov 9, 2016