Repository navigation
Add Bayesian RSA to the package (reprsimil) - #98
Conversation
|
Can one of the admins verify this patch? |
1 similar comment
|
Can one of the admins verify this patch? |
|
Add to whitelist. |
|
Thank you for the PR, @lcnature. Could you please recommend someone who is familiar with your work and can assess the neuroscience validity of the code? Also, is |
|
Sorry @mihaic . I made some changes to the docstring after it passed the last time. |
|
That's perfectly OK. You can make as many commits you want, there is no issue with having test failures. :) The line number in the doc building errors are very misleading (see issue #42). You need an empty line before "Parameters". |
|
Thanks @mshvartsman who has kindly agreed to review. I will let you know after I upload more documentation of the maths. |
|
@mihaic How can I know why the check with macos failed? |
|
@yidawang In fact I do not have read permission -- access denied even after logging in. |
|
@lcnature I don't even know there are permission settings out there. In the "console output" I can see the code tries to import brainiak.fcma.cython_blas which is not supposed to be in the current codebase. |
|
Retest this please. |
|
The error was caused by leftovers from previous PR builds, which were preventing the documentation from building. So not related to the new code. I added a clean step to the builder (I am sure I had added it before and it magically disappeared...) to prevent this from happening again. Unfortunately, the Jenkins results are only visible to people with Jenkins accounts. I will talk to the admin to see if we can make the results public. |
| .. [Cai2016] "Unbiased Bayesian estimation of | ||
| neural representational similarity structure", | ||
| M.B. Cai, N. Schuck, J. Pillow, Y. Niv, | ||
| Neural Information Processing System 29, 2016. |
There was a problem hiding this comment.
small typo: Neural Information Processing Systems
…uced the tolerance for gradient test of brsa.
|
@mihaic I cannot tell what causes a failure in Travid CI test: |
|
@lcnature No clue. The error was only on MacOS. The tests passed on MacOS on Jenkins, so I restarted the Travis test. |
|
@lcnature More evidence that the best approach for solving computer errors is restarting. :) |
mshvartsman
left a comment
There was a problem hiding this comment.
I think ready to go out there and get its tires kicked.
| self.design_used = np.insert(self.design_used, 0, 1, axis=1) | ||
| self.n_TR = np.size(self.design_used, axis=0) | ||
|
|
||
| def readAFNI(self, fname): |
There was a problem hiding this comment.
Method names should be lowercase, with optional underscores for readability:
https://www.python.org/dev/peps/pep-0008/#method-names-and-instance-variables
| return corr | ||
|
|
||
|
|
||
| class read_design: |
There was a problem hiding this comment.
Class names should be use the CapWords capitalization convention:
https://www.python.org/dev/peps/pep-0008/#class-names
While for the code that closely follows the formulas in the paper we can disregard this rule, please follow it here.
|
This time travis-CI throws some details for its failure. It looks like some problem with pytest iteslf. |
|
@lcnature Please update the branch so I can merge it. |
|
@mihaic Thanks Mihai, I updated and travis-CI failed again... |
|
@lcnature Thank you very much for your contribution and for addressing the issues that came up during the review (thanks for pointing them out, @mshvartsman). |
This reverts commit 669f041.
Bayesian RSA stands for Bayesian representational similarity analysis.
The code should be functional.
Planned updates for the upcoming weeks:
(1) add an example as a python notebook
(2) upload supplementary file including the derivation of the gradient of log likelihood
(3) capability of modeling the spatial correlation of noise