Skip to content

Investigate test_precomputed_nearest_neighbors_filtering[60] failure on CI #31262

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
merged 5 commits into from
Apr 30, 2025

Conversation

ogrisel
Copy link
Member

@ogrisel ogrisel commented Apr 28, 2025

Trying to reproduce #31256 on the CI.

test_precomputed_nearest_neighbors_filtering
@ogrisel ogrisel added Build / CI module:test-suite everything related to our tests and removed Build / CI labels Apr 28, 2025
Copy link

github-actions bot commented Apr 28, 2025

✔️ Linting Passed

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

Generated for commit: d444653. Link to the linter CI: here

@ogrisel
Copy link
Member Author

ogrisel commented Apr 28, 2025

Ok, so this test is definitely platform sensitive.

@ogrisel
Copy link
Member Author

ogrisel commented Apr 28, 2025

I can also reproduce the failure on arm64 macOS with

SKLEARN_TESTS_GLOBAL_RANDOM_SEED="60" pytest -k test_precomputed_nearest_neighbors_filtering sklearn/cluster/tests/test_spectral.py

if I use openblas instead of accelerate in my dev env:

mamba install "libblas=*=*openblas"

ogrisel added 2 commits April 28, 2025 11:02
…zure parallel] [all random seeds]

test_precomputed_nearest_neighbors_filtering
…graph [azure parallel] [all random seeds]

test_precomputed_nearest_neighbors_filtering
@ogrisel ogrisel marked this pull request as ready for review April 28, 2025 10:02
@ogrisel
Copy link
Member Author

ogrisel commented Apr 28, 2025

I am not 100% sure, but I think the previous way the test was written could lead to tied neighbors. Adding a 3rd data dimension and using the continuous valued distance graph instead of the discretized connectivity graph reduces this likelihood and makes the tests robust, even with the original small dataset size.

@ogrisel ogrisel added the Quick Review For PRs that are quick to review label Apr 28, 2025
@adrinjalali adrinjalali merged commit e29d727 into scikit-learn:main Apr 30, 2025
36 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Build / CI module:test-suite everything related to our tests No Changelog Needed Quick Review For PRs that are quick to review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants