question about the need of a perturbation term

[lcnature]

Hi @cameronphchen sorry for bothering you.
We were reading this line of code of adding a small diagonal term to the product (Line 595-597)

brainiak/brainiak/funcalign/srm.py

Line 596 in ee09359

np.fill_diagonal(perturbation, 0.001)

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!

[cameronphchen]

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: ***@***.***>

[lcnature]

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?