Skip to content

[WIP] Common test for sample weight #5461

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 2 commits into from
Closed

[WIP] Common test for sample weight #5461

wants to merge 2 commits into from

Conversation

eickenberg
Copy link
Contributor

Trying to address #5444

I wrote a common test that goes through all classifiers and regressors and checks whether sample weights correspond to data augmentation. If the coef_ attribute is available, then it is also compared.

The estimators that fail this test are the following

[('AdaBoostRegressor', 'pred'),
 ('BaggingClassifier', 'pred'),
 ('BaggingRegressor', 'pred'),
 ('DecisionTreeRegressor', 'pred'),
 ('ExtraTreeClassifier', 'pred'),
 ('ExtraTreeRegressor', 'pred'),
 ('ExtraTreesClassifier', 'pred'),
 ('ExtraTreesRegressor', 'pred'),
 ('GradientBoostingRegressor', 'pred'),
 ('LogisticRegressionCV', 'coef_'),
 ('Perceptron', 'coef_'),
 ('Perceptron', 'pred'),
 ('RandomForestRegressor', 'pred'),
 ('RidgeCV', 'coef_'),
 ('RidgeCV', 'pred'),
 ('RidgeClassifierCV', 'coef_'),
 ('SGDClassifier', 'coef_'),
 ('SGDClassifier', 'pred'),
 ('SGDRegressor', 'coef_'),
 ('SGDRegressor', 'pred')]

MechCoder
MechCoder reviewed Oct 19, 2015
View reviewed changes
sklearn/tests/test_common.py
train, test = train_test_split(range(n_samples))

for name, Estimator in estimators:
if 'sample_weight' not in signature(Estimator.fit).parameters.keys():
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can use in utils.validation.has_fit_parameter

@MechCoder
Copy link
Member

Oops just realized this is a WIP. Sorry for commenting @eickenberg

@MechCoder MechCoder changed the title Common test for sample weight [WIP] Common test for sample weight Oct 19, 2015
@amueller
Copy link
Member

for SGD it could be a convergence issue.
for LogisticRegressionCV, that has issues in master, see #5008.
I'm surprised by RidgeCV. It's odd that Ridge works but RidgeCV doesn't. Maybe sample_weights are not passed to the loss? Actually, I don't think we have a proper mechanism of passing sample_weights to scores yet :-/ RidgeCV doesn't use a score, though. So this smells fishy.

@amueller
Copy link
Member

I have no explanation for the tree-based models. Maybe @arjoly or @glouppe or @jmschrei have?

@amueller
Copy link
Member

Actually, sample weight support in scorers is fine.

@glouppe
Copy link
Contributor

glouppe commented Oct 21, 2015

For random forest with bootstrap=True, it is very unlikely that will get identical results, even with the same random_state, because of the subsampling.

For methods using randomization, you should make sure to use the same random_state, which is not the case as far as I can tell.

Finally, because of numerical issues, it may happen that trees differ slightly, though the top parts should be consistent.

glouppe
glouppe reviewed Oct 21, 2015
View reviewed changes
sklearn/tests/test_common.py
continue

try:
estimator_sw = Estimator().fit(X[train], y[train],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

random_state should be enforced here, if it is a parameter of Estimator.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there is the set_random_state helper for that.

@amueller
Copy link
Member

whoops, I was confused about the bootstrapping. Obviously it's different.

@amueller
Copy link
Member

Interestingly RandomForestClassifier passes but ExtraTreesClassifier fails

@amueller
Copy link
Member

Oh, I had assumed the random state was fixed...

@eickenberg
Copy link
Contributor Author

I'm surprised by RidgeCV

This is because sample_weight is broken for _RidgeGCV, see #4490

Discussing with @arjoly and @agramfort yesterday, the random nature of the subsampling causes the augmented data to be split in ways that are impossible with sample_weight set. If sample_weight is set to e.g. 3 for one sample, then in a random subsampling it will either be there with weight 3 or not be there, whereas the data augmented version can have e.g. representations of 2.

@eickenberg
Copy link
Contributor Author

For random forest with bootstrap=True, it is very unlikely that will get identical results, even with the same random_state, because of the subsampling.

Oh, sorry, @glouppe, you had already mentioned that.

@eickenberg
Copy link
Contributor Author

So the question becomes:

  1. Is there a test that should invariably pass for everybody? Again, discussing with @arjoly and @agramfort, one thing that should always work is that a sample_weight of 0 should lead to neglect of the sample and thus be equivalent to omission of the sample. (I guess up to random_state again with ensembles ... :/)
  2. If this cannot be applied globally, which subsets can we work with? E.g. the test that I wrote should definitely work with the linear_models. So I can remove it from test_common and make it more specific.

Any opinions on this?

@amueller
Copy link
Member

Well you definitely have to fix the random state. Otherwise even the same parameters won't produce the same results ;)
The sample weight of 0 test sounds good. We can add tests that are only for linear models to the common test. I think there are some for class_weight. I would try to test as much as possible, though. Maybe just skip the RandomForestClassifier?

@eickenberg
Copy link
Contributor Author

OK. @ainafp is taking over this PR.

@ainafp ainafp mentioned this pull request Oct 21, 2015
Closed
@eickenberg
Copy link
Contributor Author

superseded by #5515

@eickenberg eickenberg closed this Oct 21, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@eickenberg @MechCoder @amueller @glouppe