-
Notifications
You must be signed in to change notification settings - Fork 78
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
[Fix] l2_exp random fail in half-float32 mixed precision on self-neighboring #596
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.
PR looks good to me - had a couple of minor questions though
@@ -21,6 +21,7 @@ | |||
from cuvs.distance import pairwise_distance | |||
|
|||
|
|||
@pytest.mark.parametrize("times", range(20)) |
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.
Whats this times parameter used for? I don't see it used in the test it self -
Are you just trying to run this test multiple times here to stress test it?
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.
Yeah, it's just for testing multiple times and to guarantee the reproducing on one going because the possibility is close to ~10% empirically.
@@ -79,7 +80,5 @@ def test_distance(n_rows, n_cols, inplace, order, metric, dtype): | |||
actual = output_device.copy_to_host() | |||
|
|||
tol = 1e-3 | |||
if np.issubdtype(dtype, np.float16): | |||
tol = 1e-1 |
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 think I added this reduced tolerance because I was seeing failures - is this no longer needed?
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.
Yeah, I tried it successfully at local machines. I think this change can help us block potential actual failures in the future, so I made it.
Looks like Python tests are now passing. We are waiting on one more C++ test job. I will go ahead and trigger a merge once CI finishes to unblock CI, since several PRs depend on this. |
/merge |
No description provided.