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

Adding tol to dask test_kmeans #3426

Merged
merged 2 commits into from
Feb 1, 2021

Conversation

lowener
Copy link
Contributor

@lowener lowener commented Jan 29, 2021

Closes #2969.

This PR add tolerance to dask test_kmeans in order to be flexible with small accumulation mistakes in a distributed context.

@lowener lowener requested a review from a team as a code owner January 29, 2021 00:57
@github-actions github-actions bot added the Cython / Python Cython or Python issue label Jan 29, 2021
@dantegd dantegd added bug Something isn't working Dask / cuml.dask Issue/PR related to Python level dask or cuml.dask features. tests Unit testing for project labels Jan 29, 2021
@dantegd dantegd added the non-breaking Non-breaking change label Jan 29, 2021
Copy link
Member

@dantegd dantegd left a comment

Choose a reason for hiding this comment

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

looks good (pending copyright year update), just had one question before we merge

@@ -193,4 +193,4 @@ def test_score(nrows, ncols, nclusters, n_parts,
local_model = cumlModel.get_combined_model()
expected_score = local_model.score(X_train.compute())

assert actual_score == expected_score
assert abs(actual_score - expected_score) < 1e-3
Copy link
Member

Choose a reason for hiding this comment

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

Why was 1e-3 in particular chosen?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because it's small enough compared to the numbers in the assertion (15k and 5k).

The error reported in the issue were in the order of 2e-4.

@JohnZed
Copy link
Contributor

JohnZed commented Jan 30, 2021

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 1f693ab into rapidsai:branch-0.18 Feb 1, 2021
@lowener lowener deleted the 018_fix_dask_kmeans branch February 5, 2021 17:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working Cython / Python Cython or Python issue Dask / cuml.dask Issue/PR related to Python level dask or cuml.dask features. non-breaking Non-breaking change tests Unit testing for project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] dask test_kmeans stress test failures
3 participants