Robust Shared Response Model by TuKo · Pull Request #302 · brainiak/brainiak

TuKo · 2017-11-23T05:35:36Z

The code introduces the Robust SRM, tests and an example with synthetic data.

TuKo · 2017-11-23T05:58:37Z

@CameronTEllis and @hejiaz, please do you mind reviewing this PR?

Thank you and happy thanksgiving!

CameronTEllis

Overall it is looking good, my comments are relatively minor and mainly just focused on fleshing details out

CameronTEllis · 2017-11-24T17:37:54Z

+"""Robust Shared Response Model (RSRM)
+
+The implementation is based on the following publications:
+


It would be good to provide a paragraph explaining what RSRM offers and how it compares to SRM, SSRM etc. You could copy and paste one of the paragraphs from manuscript. Also should mention the influence of RPCA

I added an extended description of the method. However, I don't think that the relation to RPCA is relevant to the code/implementation. The idea is that people can understand the code and relate to the paper if there is something unclear.

Fair enough

CameronTEllis · 2017-11-24T17:39:11Z

+
+    gamma : float, default: 1.0
+        Regularization parameter for the sparseness of the individual
+        components.


Would be good to add direction to this description (Is high more or less sparse?)

CameronTEllis · 2017-11-24T17:40:49Z

+        The shared response.
+
+    s_ : list of array, element i has shape=[voxels_i, samples]
+        The individual components with outlying data for each subject.


You use outlier here and elsewhere but sometimes refer to this as the individual component. It is important to be consistent so check that the same word is used everywehere

CameronTEllis · 2017-11-24T17:43:26Z

+    ----
+
+        The number of voxels may be different between subjects. However, the
+        number of samples for the alignment data must be the same across


'samples' is not a typical way of describing timepoints in fMRI data. Consider changing this to say that 'timepoints must be aligned across participants'

Done (everywhere)

CameronTEllis · 2017-11-24T17:44:23Z

+        X : list of 2D arrays, element i has shape=[voxels_i, samples]
+            Each element in the list contains the fMRI data of one subject.
+
+        y : not used


What is this for then?

CameronTEllis · 2017-11-24T18:09:08Z

+        Parameters
+        ----------
+
+        X : array, shape=[voxels, samples]


Maybe use a different letter, or lower case since this is no longer a list like it is in other functions. same with S below

CameronTEllis · 2017-11-24T18:10:22Z

+        """
+        A = X.dot(R.T)
+        A -= S.dot(R.T)
+        U, _, V = np.linalg.svd(A, full_matrices=False)


This is probably obvious but I am unsure why you are doing the SVD. A comment would help

CameronTEllis · 2017-11-24T19:00:31Z

+    "    \n",
+    "S = [None] * subjects\n",
+    "for s in range(subjects):\n",
+    "    S[s] = (np.random.rand(data[s].shape[0], samples) < p) * ((np.random.rand(data[s].shape[0], samples) * amplitude - amplitude/2) )\n",


Why use uniform noise? I would have thought gaussian noise would be more reasonable?

S is not the noise but the sparse individual component. If we use a Normal distribution, S will mix with the shared signal because sums of Normal distributions are also Normal distributions. In that case you will have a difficult time to identify what comes from where (Shared vs individual). This is also a common assumption for the RPCA.

CameronTEllis · 2017-11-24T19:00:56Z

+    "features = 3\n",
+    "snr = 20 # in dB\n",
+    "amplitude = 8.0 # approximate amplitude used for the values in Si\n",
+    "p = 0.05 # probability of being non-zero in Si"


How does this relate to the gamma value used later? Why are they not the same?

p is the probability of an element to be non-zero in the original Si (drawn from the model).
gamma is selected to approximately have the same number of non-zeros in the estimated Si.
However, there is no reason for them to have the same values. Ones is the probability and the other is a balancing value between regularization terms.

I might still be misunderstanding but shouldn't the non-zero elements of the original Si be thought of as the individual component that RSRM is aiming to extract? Why would there be a different number in the input vs the output?

The math is not translated 1:1. Remember that this is probability, and when you draw a signal there are not exactly 5% of non-zeros. There could be anywhere near the 0.05. Added to that this will be differently with all subjects. So every subject will have approximately that, but not exactly.
Therefore, even if you can obtain a 1:1 translation from p (range 0 to 1) to gamma (range 0 to +inf), still the signal itself will not necessarily behave in that way.

Also, this is a particular case of drawing Si's, and you can change it by other distributions as well. In such case, you will have parameters to generate the signals and still gamma to recover it. Dividing between gamma and p, it is a good idea.

Okay fair enough I understand

CameronTEllis · 2017-11-24T19:02:22Z

+    "    n = noise_level * np.random.randn(voxels, samples)\n",
+    "    data[s] += n\n",
+    "    \n",
+    "S = [None] * subjects\n",


Might be good to make another example where there is no S variability and show that RSRM does as well as SRM

The idea of the example is to show how to use it. I chose the synthetic example as it is easy to replicate and visually show the improvements. If you set gamma to "infinity", you will be solving exactly the (deterministic) SRM.

… added

TuKo · 2017-12-06T01:57:00Z

@CameronTEllis do you have any suggestion or comment for the newest version?

TuKo · 2017-12-11T22:23:07Z

@CameronTEllis do you think there are more changes to be done? If we are done, then don't forget to approve the review. Otherwise let me know.
Thanks!

Javier Turek added 13 commits November 22, 2017 14:40

implemented RSRM

c667a0f

Transform new subject and naming convention change

5f9ebc6

added test for rsrm

44dea7e

corrected some PEP issues

a82ad1b

corrected more PEP issues

decb785

corrected last PEP issues - luckily

9ce3c74

corrected PEP issues for test

2d1df92

corrected test for wrong result

9677ce8

additional funciton tested

a3040c5

corrected test

23adcd2

example with rsrm

88a6c91

corrected notebook

7da4e7a

corrected a few errors in example

2973a67

TuKo added the enhancement label Nov 23, 2017

Added news fragment

97d6c50

CameronTEllis reviewed Nov 24, 2017

View reviewed changes

Javier Turek added 13 commits November 27, 2017 10:30

Gamma doc on sparsity of solution

10a3dc0

Change Outlier to Individual everywhere

0f722c4

Changed samples by timepoints

276c1fd

Removed unused parameters

6c77d45

orthogonalization comment

74b7356

Removed subjs from obj_func()

5974b50

Removed subjs and TRs from unnecessary places, and doc on update of R…

2870d44

… added

Changed variables to show specific subject treatment

4b2f071

Documented transform computation

ae5ca6f

Removed unnecessary Classifier Mixin

e9c3098

Method documentation extended

6ff0682

removed trailing spaces

3a690ef

removed unnecessary code

2563fb1

Javier Turek added 3 commits November 27, 2017 12:27

corrected latex

6d6825c

corrected another latex line

61b9d14

corrected math latex problem in multiple lines

431c130

TuKo force-pushed the rsrm branch from 431c130 to 61b9d14 Compare November 28, 2017 17:33

Javier Turek added 3 commits December 5, 2017 17:51

recorrected latex

e7df744

Merge branch 'master' of github.com:brainiak/brainiak into rsrm

980c6e7

removed trailing spaces

49b96fa

CameronTEllis approved these changes Dec 11, 2017

View reviewed changes

mihaic merged commit f708dfb into brainiak:master Dec 12, 2017

TuKo deleted the rsrm branch December 12, 2017 22:41

		"""Robust Shared Response Model (RSRM)

		The implementation is based on the following publications:

Conversation

TuKo commented Nov 23, 2017

Uh oh!

TuKo commented Nov 23, 2017

Uh oh!

CameronTEllis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TuKo commented Dec 6, 2017

Uh oh!

TuKo commented Dec 11, 2017

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants