Skip to content

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

Merged

Conversation

simarssidhu
Copy link
Contributor

Changes

  • updated four functions in test_optics.py to incorporate global_random_seed ensuring seed invariance:
    • test_extract_xi( )
      • this one was the most challenging one to update, and I'm still uncertain about the solution. Since the random seed affect clustering, expected labels are no longer hardcoded. Instead, adjusted_rand_score is used to compare clustering results and determine if they are roughly equivalent. However, it doesn't work for all seeds, so global_random_seed % 10 was introduced. I was wondering if someone can steer me in the right direction for this problem. Thanks!
    • test_cluster_hierarchy_( )
      • increased xi, and assertion tolerance to accommodate randomness
    • test_extract_dbscan( )
      • modified assertion to handle outliers/noises
    • test_optics_input_not_modified_precomputed_sparse_nodiag( )
      • no functional changes, only incorporated global_random_seed

Purpose:

  • improved test robustness by handling randomness more effectively

Related Issues:

Copy link

github-actions bot commented Feb 17, 2025

✔️ Linting Passed

All linting checks passed. Your pull request is in excellent shape! ☀️

Generated for commit: 11297ab. Link to the linter CI: here

@simarssidhu simarssidhu force-pushed the svm_optics_global_random_seed branch from 22d2434 to 00cbb81 Compare February 19, 2025 03:31
@simarssidhu
Copy link
Contributor Author

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,
Simar

Copy link
Contributor

@OmarManzoor OmarManzoor left a 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

@@ -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):
Copy link
Contributor

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

Copy link
Contributor Author

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

@simarssidhu
Copy link
Contributor Author

Thanks for adding the No Changelog Needed tag @StefanieSenger!

@simarssidhu
Copy link
Contributor Author

Hi @ogrisel, @OmarManzoor, and @adrinjalali,

I was wondering if there is possible updates or further suggestions for this PR?

Thanks,
Simar

test_cluster_hierarchy
test_extract_dbscan
test_optics_input_not_modified_precomputed_sparse_nodiag
Copy link
Member

@jeremiedbb jeremiedbb left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks @simarssidhu

@jeremiedbb jeremiedbb merged commit 5d4ba49 into scikit-learn:main Apr 11, 2025
34 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants