8000 TST Accelerating ensemble/tests/test_voting.py::test_gridsearch by mohaseeb · Pull Request #21422 · scikit-learn/scikit-learn · GitHub
[go: up one dir, main page]

Skip to content

TST Accelerating ensemble/tests/test_voting.py::test_gridsearch #21422

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

Conversation

mohaseeb
Copy link
Contributor

Reference Issues/PRs

Towards #21407

What does this implement/fix? Explain your changes.

These changes are accelerating test case ensemble/tests/test_voting.py::test_gridsearch

Any other comments?

Programming partner: @erodrago
#DataUmbrella Sprint

@@ -263,7 +263,7 @@ def test_multilabel():
def test_gridsearch():
"""Check GridSearch support."""
clf1 = LogisticRegression(random_state=1)
clf2 = RandomForestClassifier(random_state=1)
clf2 = RandomForestClassifier(random_state=1, n_estimators=3)
Copy link
Member
@ogrisel ogrisel Oct 23, 2021

Choose a reason for hiding this comment

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

If it's still slow, we could set max_depth=3 to make it run even faster.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If it's still slow, we could set max_depth=3 to make it run even faster.

@ogrisel the test case is not part of the 20 slowest anymore.

We tried max_depth=3 locally, but it didn't result in a visible speedup; not sure why.

Copy link
Member

Choose a reason for hiding this comment

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

did you measure the time? I think this should be enough and we should merge :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes we did measure the time (for each test impl we ran the test 3 times); the average running time is reported below:

  • original impl: 5.3 seconds
  • ogrisel recommended changes: 0.57 seconds
  • after adding max_depth=3: 0.59 seconds

@mohaseeb mohaseeb changed the title [WIP] Accelerating ensemble/tests/test_voting.py::test_gridsearch [MRG] Accelerating ensemble/tests/test_voting.py::test_gridsearch Oct 23, 2021
@thomasjpfan thomasjpfan changed the title [MRG] Accelerating ensemble/tests/test_voting.py::test_gridsearch TST Accelerating ensemble/tests/test_voting.py::test_gridsearch Oct 23, 2021
Copy link
Member
@ogrisel ogrisel left a comment

Choose a reason for hiding this comment

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

Thanks @mohaseeb!

@ogrisel ogrisel merged commit ec1248a into scikit-learn:main Oct 24, 2021
@ogrisel
Copy link
Member
ogrisel commented Oct 24, 2021

and @erodrago!

ogrisel pushed a commit to ogrisel/scikit-learn that referenced this pull request Oct 28, 2021
samronsin pushed a commit to samronsin/scikit-learn that referenced this pull request Nov 30, 2021
glemaitre pushed a commit to glemaitre/scikit-learn that referenced this pull request Dec 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants
0