question about the need of a perturbation term #528

Activity

1. cameronphchen commented on Jun 21, 2023

   

   cameronphchen

   on Jun 21, 2023 via email

   Contributor

   More actions

   Thanks for reaching out, Mingbo! The diagnocal matrix is indeed added for the numerical stability of the following SVD. I would expect the code to work most of the time without the diagnonal matrix. But if I recall correctly, I have also ran into situations where SVD broke.

   …

   On Mon, Jun 19, 2023 at 3:24 AM Mingbo Cai ***@***.***> wrote: Hi @cameronphchen <https://github.com/cameronphchen> sorry for bothering you. We were reading this line of code of adding a small diagonal term to the product (Line 595-597) https://github.com/brainiak/brainiak/blob/ee093597c6c11597b0a59e95b48d2118e40394a5/brainiak/funcalign/srm.py#L596 It is a bit confusing to us what is the purpose of this term. I am guessing that it serves some purpose for numerical stability. But it seems that a_subject is not a square matrix, but rather with the same of n_voxels * n_features. Because the voxels are not necessarily ordered in a meaningful way, the diagonal matrix of 0.001 will only impact some of the voxles but not all. So I wonder whether this addition is actually necessary. Of course, it is a small term which might not impact the result in any way. But maybe the code can work without it? Thanks! — Reply to this email directly, view it on GitHub <#528>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAVISETCW2GQIVXFKMOVDFDXMASERANCNFSM6AAAAAAZLXGV6M> . You are receiving this because you were mentioned.Message ID: ***@***.***>
2. lcnature commented on Jun 27, 2023

   

   lcnature

   on Jun 27, 2023

   ContributorAuthor

   More actions

   Thanks Cameron! I guess it is a bit unsafe to remove it given that you encountered numerical issues before. If I get a chance to try it intensively to ensure that removing this term does not cause problem, or identify the situation when SVD breaks, then I can try to make a PR removing it. Alternatively, maybe we can make it a try and except logic: by default we don't add this term, but if SVD fails, then we add the perturbation just for that iteration?