brainiak icon indicating copy to clipboard operation
brainiak copied to clipboard

question about the need of a perturbation term

Open lcnature opened this issue 1 year ago • 2 comments

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) 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!

lcnature avatar Jun 19 '23 10:06 lcnature

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 https://github.com/brainiak/brainiak/issues/528, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAVISETCW2GQIVXFKMOVDFDXMASERANCNFSM6AAAAAAZLXGV6M . You are receiving this because you were mentioned.Message ID: @.***>

cameronphchen avatar Jun 21 '23 04:06 cameronphchen

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?

lcnature avatar Jun 27 '23 19:06 lcnature