10000 [MRG] use pytest and not unitest in test_sgd.py by agramfort · Pull Request #13291 · scikit-learn/scikit-learn · GitHub
[go: up one dir, main page]

Skip to content

[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

Merged
merged 5 commits into from
Feb 27, 2019

Conversation

agramfort
Copy link
Member

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 ?

Copy link
Member
@amueller amueller left a 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):
Copy link
Member

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 ...

Copy link
Member Author

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.

Copy link
Contributor
@albertcthomas albertcthomas left a 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])
Copy link
Contributor

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])

Copy link
Member

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:
Copy link
Contributor

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):
Copy link
Contributor

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.

Copy link
Member
@jorisvandenbossche jorisvandenbossche left a 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)
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 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)

Copy link
Member Author

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])
Copy link
Member

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)

@jorisvandenbossche
Copy link
Member
jorisvandenbossche commented Feb 27, 2019

Is there a nicer way not to repeat the klass parametrization so often?

The "pytest" way would be to make this a parametrized fixture.

This would look like:

@pytest.fixture(params=[SGDClassifier, SparseSGDClassifier,
                        SGDRegressor, SparseSGDRegressor])
def klass(request):
    return request.param

def test_something(klass):
    ...

and then we don't need to repeat the pytest.mark.parametrize each time (and then two other fixtures for only the regressors and only the classifiers).
In general, I don't really like the magic of pytest fixtures, but this might be a case where it is actually appropriate and still quite clear.

@agramfort agramfort changed the title use pytest and not unitest in test_sgd.py [MRG] use pytest and not unitest in test_sgd.py Feb 27, 2019
@agramfort
Copy link
Member Author

do I need to go the pytest.fixture way or we consider it's ok as it is?

@jorisvandenbossche
Copy link
Member

Only the mac build on travis is still in the queue, so merging.

@jorisvandenbossche jorisvandenbossche merged commit 798e1df into scikit-learn:master Feb 27, 2019
xhluca pushed a commit to xhluca/scikit-learn that referenced this pull request Apr 28, 2019
xhluca pushed a commit to xhluca/scikit-learn that referenced this pull request Apr 28, 2019
xhluca pushed a commit to xhluca/scikit-learn that referenced this pull request Apr 28, 2019
koenvandevelde pushed a commit to koenvandevelde/scikit-learn that referenced this pull request Jul 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants
0