[MRG] Ensure determinism of SVD output in dict_learning #18433
Conversation
There was a problem hiding this comment.
Thank you for the PR @brcharron !
The concern I have with this, is that it will change the behavior of dict_learning, which makes it backward incompatible.
We can either add a flip_sign to dict_learning or consider this a bug fix by making it more consistent with dict_learning_online. WDYT @ogrisel ?
|
@thomasjpfan I'd consider non determinism a bug. Moreover there are ongoing fixes that will change the behavior (see #19198 for instance) so one more change won't make a difference. |
brcharron
force-pushed
the
deterministic_dict_learning
branch
from
759593e
to
bfc22a6
Compare
|
@jeremiedbb Thank you for the review, I added a comment and a What's New entry. |
There was a problem hiding this comment.
I also consider this a bug fix. I am not sure how to test this because I believe the non-determinism could only be observed by switching from one platform to another or from one version of numpy / scipy / openblas to the next.
+1 for merging as it is. The existing tests pass which is a clue that this should not break to much our users' code.
|
Shall we consider to backport this bug in 0.24.2 instead of 1.0? |
|
I don't think it's that urgent. It's been there for a long time and it's not a nasty bug. |
Reference Issues/PRs
Fixes #12826
See also #12799
What does this implement/fix? Explain your changes.
Adds a call to
sklearn.utils.extmath.svd_flipafter thescipy.linalg.svdcall indict_learning(used bysklearn.decomposition.DictionaryLearning) to ensure that the output is deterministic across platforms.This could allow to put back the example in #12799 (reverting 4a7075a and fixing the signs) but there is already another docstring example now so not sure if needed.
Any other comments?