Add recent work to BRSA by lcnature · Pull Request #194 · brainiak/brainiak

lcnature · 2017-03-04T05:16:11Z

Add group BRSA: shared covariance matrix across subjects.
Add transform and score function
Modify utils.gen_design to make the generated design matrix approximately scaled in amplitude.
Add automatic determination of number of nuisance regressor based on method by Gavish and Donoho 2013

Changes to be committed: new file: brainiak/brsa/test.txt

Resolved conflict in examples/reprsimil/brsa_representational_similarity_estimate_example.ipynb.

Merge brainiak/master and resolve conflict

mshvartsman · 2017-07-19T19:23:24Z

-            current_GP, X0
+            current_GP, X_res
+
+    def _fit_null(self, Y, X_base, scan_onsets=None):


For future refactoring it may be possible to just call fit here with a design matrix of 1's rather than have separate method (though it will require changing the error checking in fit also).

I have not verified that such calling would replace _fit_null. But there is no urgency for this.

mshvartsman · 2017-07-19T20:35:00Z

+    assert np.all(SNR_grids >= 0) and np.isclose(np.sum(SNR_weights), 1) and np.all(SNR_weights > 0) \
+        and np.all(np.diff(SNR_grids) > 0), \
+        'SNR_grids or SNR_weights not correct for exponential prior'
+


Somewhere around here there need to be tests of the numerical integration (against known closed-form, or against code validated elsewhere like from scipy.integrate), and/or there should be math somewhere showing that the numerical integration tricks are correct.

Thanks @mshvartsman This is now added as test_grid_flatten_num_int() in test_gbrsa.py. Unfortunately this is a long test function. Additional check for flattening 2-D grids to 1-D grid is included in this function as well.

mshvartsman · 2017-07-19T21:13:00Z

With addition of tests of the numerical integration code, plus some changes to the eigenvalue cutoff code @lcnature is doing now, I am happy to have this merged. Sorry it took so long.

…ng rho_grids and rho_weights, and flattening 2-D grids to 1-D grid to two new functions. Added test for numerical integration

… of setting up some variables that are reused for each test to a function

mshvartsman

I'm sure more issues will come up as this gets out in the world, but looks good enough to push for me.

mihaic · 2017-08-02T17:32:42Z

Thank you, @mshvartsman.

@lcnature, now we only need a blurb for the release notes in a new RST file named "docs/newsfragments/194.feature". It can be as simple as single sentence naming the new method added.

lcnature · 2017-08-02T18:17:56Z

@mihaic Done. Please check it out. Thanks!
Thanks for the careful review @mshvartsman !

mihaic

Hopefully this is the last you will hear from me on this. Mostly minor changes to make the code fit better with BrainIAK. However, I think you must address the comment about differences from the NIPS paper.

mihaic · 2017-08-04T17:36:05Z

+    Advances in Neural Information Processing Systems 29, 2016, 4952--4960
+    Available at:
+    http://papers.nips.cc/paper/6131-a-bayesian-method-for-reducing-bias-in-neural-representational-similarity-analysis.pdf
+    Some extensions not described in the paper have been made here.


Could you please describe the extensions as much as possible?

mihaic · 2017-08-04T17:42:09Z

+    return ncomp
+
+
+def CoM_exp(a, b, scale=1.0):


Please follow naming conventions (e.g., lower case). Add to __all__ if it is not private; better yet, move it to brainiak.utils.utils given that it is not connected to BRSA.

mihaic · 2017-08-04T17:48:47Z

-    n_nureg: int, default: 6
+        not provided, DC components for each run will be included as nuisance
+        regressor regardless of the auto_nuisance parameter.
+    n_nureg: int or string. Default: 'opt'


Instead of allowing str, you should have None as the default, in which case you compute it from the data. If in the future you want to add other methods for computing it than Gavish & Donoho, you can add another parameter specifying the method.

mihaic · 2017-08-04T17:51:18Z

+        this parameter is used in a half-Cauchy prior
+        on the standard deviation, or an inverse-Gamma prior
+        on the variance of the GP.
+    tau2_prior: callable[[float, int, float], [float, float]],


Capital C:
https://docs.python.org/3/library/typing.html#typing.Callable

Thanks @mihaic I forgot how I wrote this line. It looks like it only takes [float, int, float]. So I will delete the second part of the line as well.

mihaic · 2017-08-04T17:56:41Z

+        if auto_nuisance:
+            assert (type(n_nureg) is str and
+                    n_nureg in ['opt']) \
+                or (type(n_nureg) is int and n_nureg > 0), \


These type tests should go away when making the suggested change to None as default, but for future reference:

The isinstance() built-in function is recommended for testing the type of an object, because it takes subclasses into account.

https://docs.python.org/3/library/functions.html#type

fixed. But the test for being positive integer is still kept.

…il.brsa to utils.utils, and changed the default of n_nureg from opt to None, and added description of the extension of BRSA work from that of the NIPS paper.

mihaic

Just a couple of typos and nitpicking.

mihaic · 2017-08-09T16:21:40Z

+    More specifically:
+    (1) spatial noise correlation (or alternatively
+    considered as signals of intrinsic fluctuation not related to tasks);
+    (2) new fitting procedure which marginalize all voxel-specific


Typo: marginalizes.

mihaic · 2017-08-09T19:59:52Z

        not provided, DC components for each run will be included as nuisance
        regressor regardless of the auto_nuisance parameter.
-    n_nureg: int or string. Default: 'opt'
+    n_nureg: int or None. Default: None


You do not really need to address this; only commenting to remind you of the format for type annotations.

There is no need to mention None here because it is implied by the default value. If you do want to mention it anyway, please use the Optional[int] notation from PEP 484:
https://docs.python.org/3/library/typing.html#typing.Optional

Do you mean, writing it as:
n_nureg: Optional[int], period?
Does it also apply to other parameters whose defaults are None, such as rank, random_state, space_smooth_range and inten_smooth_range?

I would write:
n_nureg: int

This is also what the documentation I linked says ("An optional argument with a default needn’t use the Optional qualifier on its type annotation (although it is inferred if the default is None).") And also what you have already for inten_smooth_range and space_smooth_range.

mihaic · 2017-08-09T20:41:02Z

        on the standard deviation, or an inverse-Gamma prior
        on the variance of the GP.
-    tau2_prior: callable[[float, int, float], [float, float]],
+    tau2_prior: Callable[[float, int, float]],


The return type of the Callable is missing:
https://docs.python.org/3/library/typing.html#typing.Callable

Sorry, fixed

mihaic · 2017-08-09T20:43:41Z

+        depending on the data and SNR_bins, but many should have low
+        values with few voxels with high values.
+        Note that this attribute can not be interpreted as true SNR,
+        but the relative ratios between voxel indicates the contribution


Typo: voxels.

mihaic · 2017-08-09T20:51:52Z

+        is calculated for exponential distribution.
+    b: float
+        The ending point of the interval in which the center of mass
+        is calculated for exponential distribution.


scale is missing.

mihaic · 2017-08-09T23:48:40Z

Thank you very much for this massive contribution, @lcnature! After keeping you waiting so long for the merge, I hope you find some consolation in knowing you helped us improve the BrainIAK contribution process.

lcnature and others added 30 commits September 2, 2016 10:12

a

e4ab872

Changes to be committed: new file: brainiak/brsa/test.txt

deleted: brsa/test.txt

06c869a

Merge remote-tracking branch 'upstream/master'

9b0819c

Merge remote-tracking branch 'upstream/master'

7766c2b

Merge remote-tracking branch 'upstream/master'

89dc1e1

Merge remote-tracking branch 'upstream/master'

ea5b572

Merge remote-tracking branch 'upstream/master'

f65a42f

a

ea7b002

Changes to be committed: new file: brainiak/brsa/test.txt

deleted: brsa/test.txt

ad3af51

Merge branch 'master' of https://github.com/lcnature/brainiak

4fec06f

a

66277e6

Changes to be committed: new file: brainiak/brsa/test.txt

deleted: brsa/test.txt

1b6127a

a

4b1a794

Changes to be committed: new file: brainiak/brsa/test.txt

deleted: brsa/test.txt

2ded8db

Merge remote-tracking branch 'refs/remotes/origin/master'

9ac940d

a

66ebf5f

Changes to be committed: new file: brainiak/brsa/test.txt

deleted: brsa/test.txt

6a24344

a

746f66e

Changes to be committed: new file: brainiak/brsa/test.txt

deleted: brsa/test.txt

9fad12b

a

92a199b

Changes to be committed: new file: brainiak/brsa/test.txt

deleted: brsa/test.txt

5265476

a

3acf09f

Changes to be committed: new file: brainiak/brsa/test.txt

deleted: brsa/test.txt

bc49c16

Merge remote-tracking branch 'refs/remotes/origin/master'

973bd92

forcing scan_onsets to be integer

f288c31

make scan_onsets integers as well in example

accb974

Merge branch 'master' into add_transform_brsa

eed8ee8

Resolved conflict in examples/reprsimil/brsa_representational_similarity_estimate_example.ipynb.

Merge pull request #1 from mihaic/add_transform_brsa

0e7029f

Merge brainiak/master and resolve conflict

Add transform function

057deb0

Adding marginalized version of BRSA

e02d5a0

Mingbo Cai added 6 commits July 12, 2017 13:19

add checking for SNR_prior

0c47ede

add comment in _set_SNR_grids

37f4b22

corrected for formatting of the comment

60593af

Merge remote-tracking branch 'upstream/master'

5c87f1e

Merge remote-tracking branch 'upstream/master'

34af3ac

merged the change in upstream/master

b918ccb

mshvartsman reviewed Jul 19, 2017

View reviewed changes

Mingbo Cai added 6 commits July 20, 2017 01:02

fixed some docstring according to the PR. Isolated the work of creati…

c34745c

…ng rho_grids and rho_weights, and flattening 2-D grids to 1-D grid to two new functions. Added test for numerical integration

renamed test_grid_flatten to test_grid_flatten_num_int

2e4bb88

in test_grid_flatten_num_int() of test_gbrsa.py(), condense all codes…

1545313

… of setting up some variables that are reused for each test to a function

Merge remote-tracking branch 'remotes/upstream/master'

8dd02ca

Merge branch 'master' into add_transform_brsa

5177839

added feature description for this pull-request

9b6ed41

mshvartsman approved these changes Aug 2, 2017

View reviewed changes

mihaic requested changes Aug 4, 2017

View reviewed changes

Mingbo Cai added 4 commits August 9, 2017 10:54

addressed recent comment of brsa, moved CoM_exp function from reprsim…

af9fe57

…il.brsa to utils.utils, and changed the default of n_nureg from opt to None, and added description of the extension of BRSA work from that of the NIPS paper.

added one sentence

e9b41cc

changing one line to pass run-checks

d5872a2

replaced the phrase "refreshing noise" by "innovation noise"

60f04e5

mihaic requested changes Aug 9, 2017

View reviewed changes

Mingbo Cai and others added 2 commits August 9, 2017 19:11

changed some typo and docstrings

c131485

Merge branch 'master' into add_transform_brsa

b665e6b

mihaic approved these changes Aug 9, 2017

View reviewed changes

mihaic merged commit 9844e65 into brainiak:master Aug 9, 2017

danielsuo pushed a commit that referenced this pull request Nov 16, 2017

C++ does not guarantee order of argument evaluation (#194)

5821851

Conversation

lcnature commented Mar 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

lcnature Jul 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mshvartsman commented Jul 19, 2017

Uh oh!

mshvartsman left a comment

Choose a reason for hiding this comment

Uh oh!

mihaic commented Aug 2, 2017

Uh oh!

lcnature commented Aug 2, 2017

Uh oh!

mihaic 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!

mihaic 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!

mihaic commented Aug 9, 2017

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

lcnature commented Mar 4, 2017 •

edited

Loading

lcnature Jul 20, 2017 •

edited

Loading