[MRG+1] slight cleanup of common tests. #4058
Conversation
d5db44f to
e56211b
Compare
e56211b to
3f4443a
Compare
|
@jnothman could you please have a quick look at this one? Should be easy.... |
| # FIXME! | ||
| # in particular GaussianProcess! | ||
| yield check_estimators_overwrite_params, name, Estimator | ||
| if hasattr(Estimator, 'sparsify'): |
There was a problem hiding this comment.
I gather this is equivalent to the current test, but it seems strange to not just do this in test_classifier... Why here?
Also, if removing check_sparsify_binary_classifier, check_sparsify_multiclass_classifier can be renamed to simplify.
There was a problem hiding this comment.
I guess I did it here to be equivalent to the previous test. I guess adding it to the regressor and classifier tests would be possible. But maybe it will be added to other estimators in the future? Does it hurt here?
There was a problem hiding this comment.
No, but it is awkward with the function name check_sparsify_multiclass_classifier
a13da6a to
2813df3
Compare
2813df3 to
1666274
Compare
|
This LGTM |
|
ping @ogrisel if you have any time, so I can untangle the rest of my PRs ;) |
| est = pickle.loads(pickle.dumps(est)) | ||
| assert_true(sparse.issparse(est.coef_)) | ||
| pred = est.predict(X) | ||
| assert_array_equal(pred, pred_orig) |
There was a problem hiding this comment.
This looks wrong: this block of code should not have been deleted: est.sparsify() is no longer called in this branch.
There was a problem hiding this comment.
Actually my mistake, I was fooled by the folding of the github diff for the next test.
|
|
|
Merging then. |
[MRG+1] slight cleanup of common tests.
|
Thanks for the review :) |
Cleanup of common tests (somewhat in anticipation of #4052).
The goal would is to have one function for all, including meta-estimators, one for all default-constructible estimators, and then more specialized ones, in the spirit of #3810, and keeping in mind untested estimators from #4056.
This is only some of the way there.