Repository navigation
Deterministic SRM - #102
Conversation
|
@cameronphchen Could you please review this PR? |
| """Shared Response Model (SRM) | ||
|
|
||
| This implementation is based on the following publications: | ||
| The implementations are based on the following publications: |
There was a problem hiding this comment.
Could you please change the second reference to our Arxiv paper?
https://arxiv.org/abs/1608.04647
cameronphchen
left a comment
There was a problem hiding this comment.
Thanks Javier!! Overall it looks good to me. I have added a comment on a new test case and some other minor comments.
| self.verbose = verbose | ||
| return | ||
|
|
||
| def fit(self, X, y=None): |
There was a problem hiding this comment.
if y is not used, why not remove it?
There was a problem hiding this comment.
This is the interface for fit in BaseEstimator in scikit-learn.
| number of samples must be the same across subjects. | ||
|
|
||
| The Deterministic Shared Response Model is approximated using the | ||
| Block Coordinate Descent (BCD) algorithm proposed in [Chen2015]_. |
There was a problem hiding this comment.
Can we remove the _ after [Chen2015]?
There was a problem hiding this comment.
That creates a link in the HTML version:
http://pythonhosted.org/brainiak/brainiak.funcalign.html
| 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 |
There was a problem hiding this comment.
same here, if y is not used, why not remove it?
| for m in range(subjects): | ||
| objective += \ | ||
| (np.linalg.norm(data[m] - w[m].dot(s), 'fro')**2)\ | ||
| * (0.5 / data[m].shape[1]) |
There was a problem hiding this comment.
do we need this? since data[m] for all subjects should have the same number of TRs. and this is checked at the beginning of entering DetSRM.
|
|
||
| This is a single node version. | ||
|
|
||
| The run-time complexity is :math:`O(I (V T K))` and the memory |
There was a problem hiding this comment.
Is this the complexity for matrix multiplication only? We would also need to incorporate the complexity of SVD.
| except ValueError: | ||
| print("Catched Exception number 2: different number of samples per subject") | ||
|
|
||
|
|
There was a problem hiding this comment.
Can we add a test with the synthetic data to make sure the output of DetSRM is consistent. To be specific, a test to check that the output Ws are all orthonormal and W_i*S are close to input X within a threshold delta. This doesn't have to be in this PR but it's necessary to have one in case some future editing break the code.
There was a problem hiding this comment.
I've added the orthogonality check for both SRM and DetSRM.
Added also the other test, but it is difficult to say what is the right delta.
|
@cameronphchen, please check the modifications and let us know if everything is fine. |
cameronphchen
left a comment
There was a problem hiding this comment.
LGTM. Thanks for the revision!
Added the Deterministic Shared Response Model to Functional Alignment methods: