TST use global_random_seed in sklearn/cluster/tests/test_hierarchical.py #25281
Conversation
test_linkage_misc test_ward_agglomeration test_ward_tree_children_order test_small_distance_threshold test_cluster_distances_with_distance_threshold
glemaitre
left a comment
glemaitre
left a comment
There was a problem hiding this comment.
I would add the fixture to the following tests.
test_agglomerative_clusteringtest_sparse_scikit_vs_scipytest_vector_scikit_single_vs_scipy_singlethis on is already parameterized with some seed. We can remove them.test_vector_scikit_single_vs_scipy_singletest_agglomerative_clustering_with_distance_threshold
Somehow these tests check some equivalence and I think this is worth checking with different inputs to find if there are some corner cases.
|
|
||
|
|
||
| def test_linkage_misc(): | ||
| def test_linkage_misc(global_random_seed): |
There was a problem hiding this comment.
I would not parametrize this test. There are in general smoke testing more than statistical check. This test would benefit from a refactoring but we can let it as-is for the moment.
|
I think this is fine to let the test with the memmap out. For the other test since we are running by default a single seed this is fine to have an increase time because it would only be triggered if we enable the test for it. |
test_sparse_scikit_vs_scipy test_vector_scikit_single_vs_scipy_single test_ward_linkage_tree_return_distance test_agglomerative_clustering_with_distance_threshold
|
Thanks for your review @glemaitre. I updated the PR. When I added the global_random_seed fixture to the test |
|
We can add the fixture to this test. The increased time only happens when we request it for this test specifically which is never the case. Here, we only make sure that the test is passing locally whatever the seed. So this is fine. |
test_agglomerative_clustering
|
Ok, thanks. I updated the test. |
glemaitre
left a comment
glemaitre
left a comment
There was a problem hiding this comment.
LGTM. Thanks @marenwestermann.
Noting as awaiting a second review.
glemaitre
added
the
Waiting for Second Reviewer
thomasjpfan
left a comment
thomasjpfan
left a comment
There was a problem hiding this comment.
Thank you for the PR! LGTM
3 participants
Reference Issues/PRs
towards #22827
What does this implement/fix? Explain your changes.
I used the global_random_seed fixture in the
sklearn/cluster/tests/test_hierarchical.pymodule where I thought it's appropriate in accordance with the description of the referenced issue.A few comments about tests where I at first thought the global_random_seed fixture would be useful but didn't apply it:
test_agglomerative_clustering_memory_mappedandtest_mst_linkage_core_memory_mapped.In the documentation of the numpy
memmapfunction on which these tests are based it is said, "Memory-mapped files are used for accessing small segments of large files on disk, without reading the entire file into memory." (for reference see here). Therefore I thought the global_random_seed fixture could be useful in these two tests because changing the seed might result in a different small segment being chosen (but I'm not sure if this is correct). When I applied this fixture to these two tests, all tests passed but the former test ran in 1.83 s on my machine (Apple M1 Pro) and the latter ran in 14 s. Because of the rather long run time, I didn't apply the fixture in these tests.I also tried applying the fixture to the function
test_ward_linkage_tree_return_distanceand all tests ran successfully. However, the run time on my machine was 1.4 s and I therefore decided against applying this fixture here.Any other comments?
#PyLadiesParis
I started working on this issue during the PyLadies Paris scikit-learn sprint on 8 Dec 2022.