[MRG] Pass sample_weight when predicting on stacked folds by wderose · Pull Request #16539 · scikit-learn/scikit-learn

wderose · 2020-02-25T02:11:23Z

Reference Issues/PRs

What does this implement/fix? Explain your changes.

This PR passes sample_weight via fit_params when we call cross_val_predict in _BaseStacking, as per the comment here.

scikit-learn/sklearn/ensemble/_stacking.py

Lines 169 to 175 in 136ef79

    
           predictions = Parallel(n_jobs=self.n_jobs)( 
        
               delayed(cross_val_predict)(clone(est), X, y, cv=deepcopy(cv), 
        
                                          method=meth, n_jobs=self.n_jobs, 
        
                                          verbose=self.verbose) 
        
               for est, meth in zip(all_estimators, self.stack_method_) 
        
               if est != 'drop' 
        
           )

Any other comments?

cc: @caioaao @glemaitre

jnothman

Please add a |Fix| entry to the change log at doc/whats_new/v0.23.rst. Like the other entries there, please reference this pull request with :pr: and credit yourself (and other contributors if applicable) with :user:

Please add a versionchanged note in the docstring of sample_weight

sklearn/ensemble/tests/test_stacking.py

jnothman · 2020-02-25T09:30:02Z

But tests are failing. Should have checked that first

wderose · 2020-02-25T18:53:39Z

But tests are failing. Should have checked that first

@jnothman : Thank you for the prompt review.

I've added the requested doc changes and fixed the failing specs.

Please let me know if you'd like more details in the versionchange/release notes or if you'd like to see more tests.

glemaitre

This looks good. Only a couple of comments

doc/whats_new/v0.23.rst

sklearn/ensemble/_stacking.py

…kingRegressor instead of _BaseStacking

…into is/16537

wderose · 2020-02-29T00:33:08Z

@glemaitre Thank you for the comments.

I updated the release notes to reference StackingClassifier and StackingRegressor directly.

I also created a follow-on issue (#16595) regarding the naming of _parallel_fit_estimator and added a comment noting the strange naming.

Please take another look at these changes and let me know if there is anything else you'd like me to include.

…into is/16537

wderose · 2020-02-29T19:20:57Z

sklearn/utils/_mocking.py

            assert self.check_X(X)
        if self.check_y is not None:
            assert self.check_y(y)
+        self._check_n_features(X, reset=True)


I needed to add this call so that n_features_in_ is set correctly in CheckingClassifier.

The property is read during the fit method of BaseStacking: d205638#diff-c8f397939f2393da5b2eef4285761da1R143. We use CheckingClassifier in the unit tests for the stacking estimators: https://github.com/scikit-learn/scikit-learn/pull/16539/files#diff-3979301645c7cbae79edc4085d997029R443

…into is/16537

thomasjpfan

Thank you for the PR @wderose

LGTM!

…#16539)

wderose force-pushed the is/16537 branch from cf8cfd1 to e62c07e Compare February 25, 2020 02:11

pass sample_weight when predicting on stacked folds

840cd39

wderose force-pushed the is/16537 branch from e62c07e to 840cd39 Compare February 25, 2020 02:12

wderose requested review from glemaitre, jnothman, qinhanmin2014 and thomasjpfan February 25, 2020 02:43

use more descriptive spec def

7199799

jnothman approved these changes Feb 25, 2020

View reviewed changes

sklearn/ensemble/tests/test_stacking.py Outdated Show resolved Hide resolved

Bill DeRose added 6 commits February 25, 2020 09:50

fix specs by conditionally passing sample_weight

676a720

Add entry in release notes describing improvement

9d70031

add versionchanged note about sample_weight

324a594

fixup witespace in docstring

c032fd3

indent bullet list in release notes

8996d3e

Merge remote-tracking branch 'upstream/master' into is/16537

652a374

glemaitre reviewed Feb 28, 2020

View reviewed changes

doc/whats_new/v0.23.rst Outdated Show resolved Hide resolved

sklearn/ensemble/_stacking.py Outdated Show resolved Hide resolved

sklearn/ensemble/_stacking.py Outdated Show resolved Hide resolved

Bill DeRose added 4 commits February 28, 2020 15:02

fix: update release notes to reference to StackingClassifier and Stac…

9f14750

…kingRegressor instead of _BaseStacking

fix: minor formatting suggestion from review comments

4a17b2c

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

e4d39a5

…into is/16537

fix: add comment describing odd naming of _parallel_fit_estimator

7bae037

wderose mentioned this pull request Feb 28, 2020

Maintenance: _parallel_fit_estimator is used to fit a single estimator but has parallel in the name #16595

Closed

fix: update link to PR with direct link to issue

c240791

wderose requested a review from glemaitre February 29, 2020 00:33

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

84520e3

…into is/16537

wderose mentioned this pull request Feb 29, 2020

[MRG] Rename _parallel_fit_estimator to _fit_single_estimator #16599

Merged

Bill DeRose added 2 commits February 29, 2020 10:43

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

163142e

…into is/16537

set n_features_in_ when CheckingClassifier is fit

25cca55

wderose commented Feb 29, 2020

View reviewed changes

Bill DeRose added 4 commits February 29, 2020 11:23

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

471c80c

…into is/16537

fix formatting

78d75b8

use len(X) as n_features_in_

8bcbbe9

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

569169d

…into is/16537

jnothman approved these changes Mar 2, 2020

View reviewed changes

github-actions bot added module:ensemble module:utils labels Mar 2, 2020

Bill DeRose added 2 commits March 2, 2020 16:53

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

4baaa35

…into is/16537

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

85cd8b9

…into is/16537

thomasjpfan approved these changes Mar 6, 2020

View reviewed changes

thomasjpfan merged commit f1acf83 into scikit-learn:master Mar 6, 2020

wderose deleted the is/16537 branch March 7, 2020 01:27

ashutosh1919 pushed a commit to ashutosh1919/scikit-learn that referenced this pull request Mar 13, 2020

FIX Pass sample_weight when predicting on stacked folds (scikit-learn…

2131504

…#16539)

gio8tisu pushed a commit to gio8tisu/scikit-learn that referenced this pull request May 15, 2020

FIX Pass sample_weight when predicting on stacked folds (scikit-learn…

7161a83

…#16539)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[MRG] Pass sample_weight when predicting on stacked folds#16539

[MRG] Pass sample_weight when predicting on stacked folds#16539
thomasjpfan merged 22 commits intoscikit-learn:masterfrom
wderose:is/16537

wderose commented Feb 25, 2020

Uh oh!

jnothman left a comment

Uh oh!

Uh oh!

jnothman commented Feb 25, 2020

Uh oh!

wderose commented Feb 25, 2020

Uh oh!

glemaitre left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

wderose commented Feb 29, 2020

Uh oh!

wderose Feb 29, 2020 •

edited

Loading

Uh oh!

thomasjpfan left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

	predictions = Parallel(n_jobs=self.n_jobs)(
	delayed(cross_val_predict)(clone(est), X, y, cv=deepcopy(cv),
	method=meth, n_jobs=self.n_jobs,
	verbose=self.verbose)
	for est, meth in zip(all_estimators, self.stack_method_)
	if est != 'drop'
	)

Uh oh!

Conversation

wderose commented Feb 25, 2020

Reference Issues/PRs

What does this implement/fix? Explain your changes.

Any other comments?

Uh oh!

jnothman left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jnothman commented Feb 25, 2020

Uh oh!

wderose commented Feb 25, 2020

Uh oh!

glemaitre left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

wderose commented Feb 29, 2020

Uh oh!

wderose Feb 29, 2020 • 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!

thomasjpfan left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

wderose Feb 29, 2020 •

edited

Loading