[MRG] use pytest and not unitest in test_sgd.py #13291
Conversation
| @@ -57,19 +64,39 @@ def predict_proba(self, X): | |||
| return super().predict_proba(X) | |||
|
|
|||
|
|
|||
| class SparseSGDRegressor(SGDRegressor): | |||
|
|
|||
| class _SparseSGDRegressor(linear_model.SGDRegressor): | |||
There was a problem hiding this comment.
I still find these strange but better than before ...
There was a problem hiding this comment.
basically I need 4 factories. I could not come up with a more elegant solution.
There was a problem hiding this comment.
On master pytest collects 212 tests, on your PR pytest collects 204 tests. With my comments (fixing missed tests) you will have 214 tests, i.e. 2 more than master. That's because you added the regressors for test_sgd_bad_alpha. Otherwise LGTM! Thanks @agramfort for this refactoring, I'm more familiar with this syntax :).
|
|
||
| class DenseSGDRegressorTestCase(unittest.TestCase, CommonTest): | ||
| """Test suite for the dense representation variant of SGD""" | ||
| @pytest.mark.parametrize('klass', [SGDClassifier, SparseSGDClassifier]) |
There was a problem hiding this comment.
you forgot SGD regressors
@pytest.mark.parametrize('klass', [SGDClassifier, SparseSGDClassifier,
SGDRegressor, SparseSGDRegressor])
There was a problem hiding this comment.
Maybe move those 3 test cases that test both regressor as classifier a little bit up to above the "Classification Test Case" section title? (the diff is already impossible anyway, so don't need to care about that)
| p = clf.predict_proba([[3, 2]]) | ||
| if klass != SparseSGDClassifier: | ||
| assert_equal(np.argmax(d, axis=1), np.argmax(p, axis=1)) | ||
| else: |
There was a problem hiding this comment.
remove underscore -> test_partial_fit_equal_fit
| """Run exactly the same tests using the sparse representation variant""" | ||
| @pytest.mark.parametrize('klass', [SGDClassifier, SparseSGDClassifier, | ||
| SGDRegressor, SparseSGDRegressor]) | ||
| def test_sgd_bad_alpha(klass): |
There was a problem hiding this comment.
this is where you included the regressors in the tests compared to master, which is fine I guess.
| @pytest.mark.parametrize('lr', | ||
| ["constant", "optimal", "invscaling", "adaptive"]) | ||
| def test_warm_start(klass, lr): | ||
| _test_warm_start(klass, X, Y, lr) |
There was a problem hiding this comment.
You can now just include the code above from this _test_warm_start here in the test function? (now it is only called once, before 4 times)
There was a problem hiding this comment.
_test_warm_start is called twice with another X2, Y2 below
|
|
||
| class DenseSGDRegressorTestCase(unittest.TestCase, CommonTest): | ||
| """Test suite for the dense representation variant of SGD""" | ||
| @pytest.mark.parametrize('klass', [SGDClassifier, SparseSGDClassifier]) |
There was a problem hiding this comment.
Maybe move those 3 test cases that test both regressor as classifier a little bit up to above the "Classification Test Case" section title? (the diff is already impossible anyway, so don't need to care about that)
The "pytest" way would be to make this a parametrized fixture. This would look like: and then we don't need to repeat the |
|
do I need to go the pytest.fixture way or we consider it's ok as it is? |
|
Only the mac build on travis is still in the queue, so merging. |
this is getting rid of unitest and OO style of test we don't use anywhere else.
it's a pain to review but it's just a big unindent and pass klass instead of self with pytest parametrize
maybe for @jorisvandenbossche ?