-
-
Notifications
You must be signed in to change notification settings - Fork 25.8k
TST Incorporate global_random_seed in test_optics.py #30844
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
TST Incorporate global_random_seed in test_optics.py #30844
Conversation
…reased xi and assertion tolerance to include random seeds
…ed assertion to remove outlier/noise
…ncorporate global_random_seed.
…ssertions to account for cluster changes by random seeds
22d2434
to
00cbb81
Compare
Hi @ogrisel, I was wondering, does this PR require a changelog? The change seems quite small, so I'd like to clarify if it may not need it. Thanks, |
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 for the PR @simarssidhu
sklearn/cluster/tests/test_optics.py
Outdated
@@ -81,10 +82,10 @@ def test_the_extract_xi_labels(ordering, clusters, expected): | |||
assert_array_equal(labels, expected) | |||
|
|||
|
|||
def test_extract_xi(global_dtype): | |||
def test_extract_xi(global_dtype, global_random_seed): |
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.
Maybe we should not add the global_random_seed for this particular test as it's quite dependent on the initial points and labels. What do you think @adrinjalali
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.
Hi @OmarManzoor and @adrinjalali,
Thanks for the suggestion. The test_extract_xi( )
function is quite dependent on the initial points and labels. I've reverted them for now. I've also rebased to main, and added an empty merge commit based on @ArturoSbr's comment. I was wondering if the rest is okay and ready to merge?
Thanks,
Simar
…dified assertions to account for cluster changes by random seeds" This reverts commit f95acbf.
Thanks for adding the |
Hi @ogrisel, @OmarManzoor, and @adrinjalali, I was wondering if there is possible updates or further suggestions for this PR? Thanks, |
test_cluster_hierarchy test_extract_dbscan test_optics_input_not_modified_precomputed_sparse_nodiag
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. Thanks @simarssidhu
Changes
test_optics.py
to incorporateglobal_random_seed
ensuring seed invariance:adjusted_rand_score
is used to compare clustering results and determine if they are roughly equivalent. However, it doesn't work for all seeds, soglobal_random_seed % 10
was introduced. I was wondering if someone can steer me in the right direction for this problem. Thanks!xi
, and assertion tolerance to accommodate randomnessglobal_random_seed
Purpose:
Related Issues: