Skip to content

Fix seed sensitivity of test_fastica_eigh_low_rank_warning #29612

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

Conversation

ogrisel
Copy link
Member

@ogrisel ogrisel commented Aug 2, 2024

As originally reported in #26802 and more recently in #29609.

I cannot reproduce on my local setup, so let's try via the CI triggered by this draft PR.

EDIT: the ConvergenceWarning happens later (in the fast ica solver itself) while the expected UserWarning should be raised earlier, in the whitening step, irrespective of whether FastICA converges or not. I therefore think that the cause of the problem is that our eps-based detection of rank deficient case is too low to be platform agnostic.

Fixes #26802.

Copy link

github-actions bot commented Aug 2, 2024

✔️ Linting Passed

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

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

@ogrisel
Copy link
Member Author

ogrisel commented Aug 2, 2024

Apparently this fixed the problem on Linux with MKL but not on macos with MKL...

EDIT: not sure in retrospect. See the edited description of the PR.

ogrisel added 2 commits August 2, 2024 10:54
…in test [azure parallel] [all random seeds]

test_fastica_simple
test_fit_transform
test_inverse_transform
test_fastica_simple_different_solvers
test_fastica_eigh_low_rank_warning
@ogrisel ogrisel force-pushed the test_fastica_eigh_low_rank_warning-seed-31 branch from fc42de6 to 8cf7261 Compare August 2, 2024 09:45
@ogrisel ogrisel force-pushed the test_fastica_eigh_low_rank_warning-seed-31 branch from 8cf7261 to ae6c18b Compare August 2, 2024 09:48
@ogrisel
Copy link
Member Author

ogrisel commented Aug 2, 2024

Base on the CI report in 4981cbf, this fixed the sensitivity problem in the test. Note that to fix this test failure, I actually had to change the code in FastICA, not the test itself. So this can actually be considered a bugfix and added a changelog entry accordingly.

@ogrisel ogrisel marked this pull request as ready for review August 2, 2024 09:58
@ogrisel ogrisel added the Bug label Aug 2, 2024
@ogrisel
Copy link
Member Author

ogrisel commented Aug 2, 2024

/cc @lesteve

Copy link
Member

@lesteve lesteve left a comment

Choose a reason for hiding this comment

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

LGTM

@thomasjpfan thomasjpfan merged commit c0e07cf into scikit-learn:main Aug 2, 2024
33 checks passed
MarcBresson pushed a commit to MarcBresson/scikit-learn that referenced this pull request Sep 2, 2024
glemaitre pushed a commit to glemaitre/scikit-learn that referenced this pull request Sep 9, 2024
@ogrisel ogrisel deleted the test_fastica_eigh_low_rank_warning-seed-31 branch December 2, 2024 09:15
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.

⚠️ CI failed on Linux_Runs.pylatest_conda_forge_mkl (last failure: Jul 10, 2024) ⚠️
3 participants