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

Add Bayesian RSA to the package (reprsimil) #98

Merged
merged 63 commits into from Sep 22, 2016
Merged

Add Bayesian RSA to the package (reprsimil) #98

merged 63 commits into from Sep 22, 2016

Conversation

@lcnature
Copy link
Contributor

@lcnature lcnature commented Sep 7, 2016

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

@buildbot-princeton
Copy link
Collaborator

@buildbot-princeton buildbot-princeton commented Sep 7, 2016

Can one of the admins verify this patch?

1 similar comment
@buildbot-princeton
Copy link
Collaborator

@buildbot-princeton buildbot-princeton commented Sep 7, 2016

Can one of the admins verify this patch?

@mihaic
Copy link
Contributor

@mihaic mihaic commented Sep 7, 2016

Add to whitelist.

@mihaic
Copy link
Contributor

@mihaic mihaic commented Sep 7, 2016

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 pr-check passing for you? I tried on my computer and run-checks is failing just like on Jenkins because of trailing whitespace:

brainiak/reprsimil/brsa.py:23:31: W291 trailing whitespace
brainiak/reprsimil/brsa.py:123:14: W291 trailing whitespace
lcnature
@lcnature
Copy link
Contributor Author

@lcnature lcnature commented Sep 8, 2016

Sorry @mihaic . I made some changes to the docstring after it passed the last time.
The trailing whitespace was fixed but now I have errors about unexpected indentation in docstring, which I cannot find why.

@mihaic
Copy link
Contributor

@mihaic mihaic commented Sep 8, 2016

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".

@lcnature
Copy link
Contributor Author

@lcnature lcnature commented Sep 8, 2016

Thanks @mshvartsman who has kindly agreed to review. I will let you know after I upload more documentation of the maths.

@lcnature
Copy link
Contributor Author

@lcnature lcnature commented Sep 8, 2016

@mihaic How can I know why the check with macos failed?

@yidawang
Copy link
Member

@yidawang yidawang commented Sep 8, 2016

@lcnature Within Princeton network, you can click the Details to see. It looks like it is an import error related to FCMA, which is weird because FCMA is not yet merged. @mihaic may have some idea about this?

@lcnature
Copy link
Contributor Author

@lcnature lcnature commented Sep 8, 2016

@yidawang In fact I do not have read permission -- access denied even after logging in.

@yidawang
Copy link
Member

@yidawang yidawang commented Sep 8, 2016

@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.

@mihaic
Copy link
Contributor

@mihaic mihaic commented Sep 8, 2016

Retest this please.

@mihaic
Copy link
Contributor

@mihaic mihaic commented Sep 8, 2016

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.

This comment has been minimized.

@mshvartsman

mshvartsman Sep 8, 2016
Contributor

small typo: Neural Information Processing Systems

Spatial correlation will be included in a future version.
"""

def __init__(

This comment has been minimized.

@mshvartsman

mshvartsman Sep 8, 2016
Contributor

Looks like there are some parameters here (intern_smooth_range, space_smooth_range, etc) that are not documented up in the docstring. If I understand the standard correctly, all exposed parameters (e.g. arguments to __init__) should be documented.

This comment has been minimized.

@lcnature

lcnature Sep 8, 2016
Author Contributor

Thanks Mike! I will add them! Those are parameters added later and I forgot
to add documentation.

Mingbo Cai

On Thu, Sep 8, 2016 at 1:39 PM, Michael Shvartsman <notifications@github.com

wrote:

In brainiak/reprsimil/brsa.py
#98 (comment):

  • lGPspace_ : number, only if GP_space or GP_inten is True
  •    the length scale of Gaussian Process prior of log(SNR)
    
  • bGP_ : number, only if GP_space or GP_inten is True.
  •    the standard deviation of the GP prior
    
  • lGPinten_: number, only if GP_inten is True
  •    the length scale in fMRI intensity of the GP prior of log(SNR)
    
  • Notes

  • The current version assumes noise is independent across voxels.
  • Real data typically has spatial correlation in noise.
  • This assumption might still introduce some bias in the result.
  • Spatial correlation will be included in a future version.
  • """
  • def init(

Looks like there are some parameters here (intern_smooth_range,
space_smooth_range, etc) that are not documented up in the docstring. If I
understand the standard correctly, all exposed parameters (e.g. arguments
to init_) should be documented.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/IntelPNI/brainiak/pull/98/files/1f5775a6846235c51d7c65be2e22eae6cdec5444#r78053771,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AHZ3TXs1FwywlNuQwu1-cgF-TbXEiMklks5qoEg_gaJpZM4J3eAD
.

This comment has been minimized.

@mshvartsman

mshvartsman Sep 16, 2016
Contributor

👍

Parameters
----------
X: 2-D numpy array, shape=[time_points, voxels]
In the equation, this is actually Y.

This comment has been minimized.

@mshvartsman

mshvartsman Sep 8, 2016
Contributor

This is confusing because of the shift from generative to descriptive perspective -- but I am not sure what the best solution is. At the very least, the derivation that ships with brainiak should match notation with the code and also show the relationship to the NIPS paper notation. But maybe there is a better option, maybe someone else can think of it?

This comment has been minimized.

@lcnature

lcnature Sep 8, 2016
Author Contributor

Yeah, I also do not have a good idea of what to do.
@mihaic Basically the issue is in our model, we call X the design matrix (things we believe we know to some extent) and Y the data (which from our generative model is generated from X). But in scikit-learn, the input data is called X. So I have to make the explanation that what I write in the wrapper function fit() as X is actually Y in my major fitting code and the paper.

This comment has been minimized.

@mihaic

mihaic Sep 8, 2016
Contributor

The code should match the rest of BrainIAK and Scikit-learn, so the variable representing input data in the fit method should be called X. Furthermore, y is reserved for labels. Perhaps it is best to rename Y to X and use design throughout the code, even if it creates a mismatch with the paper.

This comment has been minimized.

@lcnature

lcnature Sep 13, 2016
Author Contributor

The new version looks more like what Mike suggested: in the wrapper, X and y are used the same as in scikit-learn. In other places, it follows the paper and common notation in fMRI literature

@lcnature
Copy link
Contributor Author

@lcnature lcnature commented Sep 20, 2016

@mihaic I cannot tell what causes a failure in Travid CI test:
https://travis-ci.org/IntelPNI/brainiak/jobs/161177341

@mihaic
Copy link
Contributor

@mihaic mihaic commented Sep 20, 2016

@lcnature No clue. The error was only on MacOS. The tests passed on MacOS on Jenkins, so I restarted the Travis test.

@mihaic
Copy link
Contributor

@mihaic mihaic commented Sep 20, 2016

@lcnature More evidence that the best approach for solving computer errors is restarting. :)

@lcnature lcnature closed this Sep 20, 2016
@lcnature lcnature reopened this Sep 20, 2016
Copy link
Contributor

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

This comment has been minimized.

@mihaic

mihaic Sep 20, 2016
Contributor

Method names should be lowercase, with optional underscores for readability:
https://www.python.org/dev/peps/pep-0008/#method-names-and-instance-variables

This comment has been minimized.

@lcnature

lcnature Sep 21, 2016
Author Contributor

Thanks. Done

return corr


class read_design:

This comment has been minimized.

@mihaic

mihaic Sep 20, 2016
Contributor

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

@lcnature

lcnature Sep 21, 2016
Author Contributor

Done.

@lcnature
Copy link
Contributor Author

@lcnature lcnature commented Sep 21, 2016

This time travis-CI throws some details for its failure. It looks like some problem with pytest iteslf.

@mihaic
Copy link
Contributor

@mihaic mihaic commented Sep 21, 2016

@lcnature Please update the branch so I can merge it.

@mihaic
mihaic approved these changes Sep 21, 2016
@lcnature
Copy link
Contributor Author

@lcnature lcnature commented Sep 22, 2016

@mihaic Thanks Mihai, I updated and travis-CI failed again...

@mihaic mihaic merged commit 669f041 into brainiak:master Sep 22, 2016
2 of 3 checks passed
2 of 3 checks passed
continuous-integration/travis-ci/pr The Travis CI build failed
Details
linux Build finished.
Details
macos Build finished.
Details
@mihaic
Copy link
Contributor

@mihaic mihaic commented Sep 22, 2016

@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).
We will be looking into the Travis failure.

@lcnature lcnature deleted the lcnature:add_brsa branch Sep 22, 2016
yidawang added a commit to yidawang/brainiak that referenced this pull request Sep 27, 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

6 participants