Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deterministic SRM #102

Merged
merged 17 commits into from Sep 21, 2016
Merged

Deterministic SRM #102

merged 17 commits into from Sep 21, 2016

Conversation

@TuKo
Copy link
Contributor

@TuKo TuKo commented Sep 14, 2016

Added the Deterministic Shared Response Model to Functional Alignment methods:

  • Added tests
  • Modified examples (python & notebook) to run with both Probabilistic and Deterministic SRM.
@mihaic
Copy link
Contributor

@mihaic mihaic commented Sep 14, 2016

@cameronphchen Could you please review this PR?

This implementation is based on the following publications:
The implementations are based on the following publications:

This comment has been minimized.

@mihaic

mihaic Sep 14, 2016
Contributor

Could you please change the second reference to our Arxiv paper?
https://arxiv.org/abs/1608.04647

TuKo added 4 commits Sep 14, 2016
@mihaic
mihaic approved these changes Sep 14, 2016
@cameronphchen
Copy link
Contributor

@cameronphchen cameronphchen commented Sep 14, 2016

@mihaic @TuKo Sure, I'll review this.

Copy link
Contributor

@cameronphchen cameronphchen left a 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):

This comment has been minimized.

@cameronphchen

cameronphchen Sep 15, 2016
Contributor

if y is not used, why not remove it?

This comment has been minimized.

@TuKo

TuKo Sep 15, 2016
Author Contributor

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]_.

This comment has been minimized.

@cameronphchen

cameronphchen Sep 15, 2016
Contributor

Can we remove the _ after [Chen2015]?

This comment has been minimized.

@mihaic

mihaic Sep 15, 2016
Contributor

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

This comment has been minimized.

@cameronphchen

cameronphchen Sep 15, 2016
Contributor

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

This comment has been minimized.

@cameronphchen

cameronphchen Sep 15, 2016
Contributor

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 comment has been minimized.

@TuKo

TuKo Sep 20, 2016
Author Contributor

Will change it.

This is a single node version.
The run-time complexity is :math:`O(I (V T K))` and the memory

This comment has been minimized.

@cameronphchen

cameronphchen Sep 15, 2016
Contributor

Is this the complexity for matrix multiplication only? We would also need to incorporate the complexity of SVD.

This comment has been minimized.

@TuKo

TuKo Sep 20, 2016
Author Contributor

Correcting this one...

except ValueError:
print("Catched Exception number 2: different number of samples per subject")


This comment has been minimized.

@cameronphchen

cameronphchen Sep 15, 2016
Contributor

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.

This comment has been minimized.

@TuKo

TuKo Sep 20, 2016
Author Contributor

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.

@TuKo
Copy link
Contributor Author

@TuKo TuKo commented Sep 20, 2016

@cameronphchen, please check the modifications and let us know if everything is fine.

Copy link
Contributor

@cameronphchen cameronphchen left a comment

LGTM. Thanks for the revision!

@mihaic mihaic merged commit e010328 into brainiak:master Sep 21, 2016
3 checks passed
3 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
linux Build finished.
Details
macos Build finished.
Details
@TuKo TuKo deleted the TuKo:deterministicsrm branch Sep 28, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants