Skip to content
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

[REVIEW] Speeding up MNMG KNN Cl&Re testing #3052

Merged

Conversation

viclafargue
Copy link
Contributor

Answers #3028

@viclafargue viclafargue requested a review from a team as a code owner October 23, 2020 15:27
@GPUtester
Copy link
Contributor

Please update the changelog in order to start CI tests.

View the gpuCI docs here.

Copy link
Contributor

@JohnZed JohnZed left a comment

Choose a reason for hiding this comment

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

Thank you for being the first to start squashing slow CI tests! I have a general suggestion about parametrization and am curious about your thoughts... also would be nice to post the new runtime in these PRs.

@pytest.mark.parametrize("n_neighbors", [1, 3, 8])
@pytest.mark.parametrize("n_parts", [2, 4, 12])
@pytest.mark.parametrize("batch_size", [128, 1024])
@pytest.mark.parametrize("n_neighbors", [8])
Copy link
Contributor

Choose a reason for hiding this comment

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

Testing the degrenerate n_neighbors=1 case to me seems important. I always like to test the extremes. What about doing mark_parametrize with a list of tuples, like:

[(1, 3, 256),
 (8, 8, 256),
 (9, 3, 128)]

Then you still only run 3 times but still examine the extremes

@pytest.mark.parametrize("n_neighbors", [1, 3, 8])
@pytest.mark.parametrize("n_parts", [2, 4, 12])
@pytest.mark.parametrize("batch_size", [128, 1024])
@pytest.mark.parametrize("n_neighbors", [8])
Copy link
Contributor

Choose a reason for hiding this comment

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

same note as above

@codecov-io
Copy link

codecov-io commented Oct 23, 2020

Codecov Report

Merging #3052 into branch-0.17 will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##           branch-0.17    #3052   +/-   ##
============================================
  Coverage        59.20%   59.20%           
============================================
  Files              142      142           
  Lines             8966     8966           
============================================
  Hits              5308     5308           
  Misses            3658     3658           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7dbd33d...2a82867. Read the comment docs.

@viclafargue viclafargue requested a review from a team as a code owner October 26, 2020 11:21
@viclafargue viclafargue changed the title [ENH] Speeding up MNMG KNN Cl&Re testing [WIP] Speeding up MNMG KNN Cl&Re testing Oct 29, 2020
@viclafargue viclafargue added the 4 - Waiting on Reviewer Waiting for reviewer to review or respond label Nov 2, 2020
@viclafargue viclafargue changed the title [WIP] Speeding up MNMG KNN Cl&Re testing [REVIEW] Speeding up MNMG KNN Cl&Re testing Nov 2, 2020
Copy link
Contributor

@JohnZed JohnZed left a comment

Choose a reason for hiding this comment

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

Great! New test sets look very reasonable and still hit the corners.

@pytest.mark.parametrize("batch_size", [128, 1024])
def test_predict_proba(dataset, datatype, n_neighbors,
n_parts, batch_size, client):
@pytest.mark.parametrize("parameters", [(1, 3, 256),
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks good!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4 - Waiting on Reviewer Waiting for reviewer to review or respond
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants