-
-
Notifications
You must be signed in to change notification settings - Fork 25.9k
[MRG] use pytest and not unitest in test_sgd.py #13291
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.
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a nicer way not to repeat the klass parametrization so often?
@@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still find these strange but better than before ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
basically I need 4 factories. I could not come up with a more elegant solution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you forgot SGD regressors
@pytest.mark.parametrize('klass', [SGDClassifier, SparseSGDClassifier,
SGDRegressor, SparseSGDRegressor])
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is where you included the regressors in the tests compared to master, which is fine I guess.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two nitpicks in addition to Albert's comments, for the rest looks good (and thanks Albert to checking the number of test!)
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 ?