-
Notifications
You must be signed in to change notification settings - Fork 514
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
Silhouette Score make_monotonic
for non-monotonic label set
#3619
Silhouette Score make_monotonic
for non-monotonic label set
#3619
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.
Code looks good, just one request
@@ -105,6 +106,16 @@ def _silhouette_coeff( | |||
labels.to_output(output_type='cupy', output_dtype='int') | |||
).shape[0] | |||
|
|||
if not check_labels(labels, cp.arange(n_labels, dtype=np.int32)): |
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 adding a test case that exercises this would be a good idea unless there is one already? Didn't see it on a quick look
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.
Any suggestions on how to test this without hard-coding some examples?
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.
Hard-coding is great! Allows you to exactly control the case. Much nicer than just using some big dataset that's hard to debug.
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.
Thanks! I'll just add the examples in the issue
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.
lgtm
@gpucibot merge |
closes #3584