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 recent work to BRSA #194

Merged
merged 154 commits into from Aug 9, 2017
Merged

Add recent work to BRSA #194

merged 154 commits into from Aug 9, 2017

Conversation

@lcnature
Copy link
Contributor

@lcnature lcnature commented Mar 4, 2017

  1. Add group BRSA: shared covariance matrix across subjects.
  2. Add transform and score function
  3. Modify utils.gen_design to make the generated design matrix approximately scaled in amplitude.
  4. Add automatic determination of number of nuisance regressor based on method by Gavish and Donoho 2013
lcnature and others added 30 commits Sep 2, 2016
a
 Changes to be committed:
	new file: brainiak/brsa/test.txt
a
 Changes to be committed:
	new file: brainiak/brsa/test.txt
a
 Changes to be committed:
	new file: brainiak/brsa/test.txt
a
 Changes to be committed:
	new file: brainiak/brsa/test.txt
a
 Changes to be committed:
	new file: brainiak/brsa/test.txt
a
 Changes to be committed:
	new file: brainiak/brsa/test.txt
a
 Changes to be committed:
	new file: brainiak/brsa/test.txt
a
 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
current_GP, X0
current_GP, X_res

def _fit_null(self, Y, X_base, scan_onsets=None):
Copy link
Contributor

@mshvartsman mshvartsman Jul 19, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@lcnature lcnature Jul 20, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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'

Copy link
Contributor

@mshvartsman mshvartsman Jul 19, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@lcnature lcnature Jul 20, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor

@mshvartsman mshvartsman commented Jul 19, 2017

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.

Copy link
Contributor

@mshvartsman mshvartsman left a comment

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

@mihaic
Copy link
Contributor

@mihaic mihaic commented Aug 2, 2017

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
Copy link
Contributor Author

@lcnature lcnature commented Aug 2, 2017

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

Copy link
Contributor

@mihaic mihaic left a comment

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.

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.
Copy link
Contributor

@mihaic mihaic Aug 4, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please describe the extensions as much as possible?

Copy link
Contributor Author

@lcnature lcnature Aug 9, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed.

return ncomp


def CoM_exp(a, b, scale=1.0):
Copy link
Contributor

@mihaic mihaic Aug 4, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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'
Copy link
Contributor

@mihaic mihaic Aug 4, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@lcnature lcnature Aug 9, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed.

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]],
Copy link
Contributor

@mihaic mihaic Aug 4, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

@lcnature lcnature Aug 9, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

@mihaic mihaic Aug 4, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@lcnature lcnature Aug 9, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Mingbo Cai added 4 commits Aug 9, 2017
…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.
Copy link
Contributor

@mihaic mihaic left a comment

Just a couple of typos and nitpicking.

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
Copy link
Contributor

@mihaic mihaic Aug 9, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo: marginalizes.

Copy link
Contributor Author

@lcnature lcnature Aug 9, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

@@ -284,12 +270,12 @@ class BRSA(BaseEstimator, TransformerMixin):
Note that nuisance regressor is not required from user. If it is
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
Copy link
Contributor

@mihaic mihaic Aug 9, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@lcnature lcnature Aug 9, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

@mihaic mihaic Aug 9, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@@ -368,7 +354,7 @@ class BRSA(BaseEstimator, TransformerMixin):
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]],
tau2_prior: Callable[[float, int, float]],
Copy link
Contributor

@mihaic mihaic Aug 9, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@lcnature lcnature Aug 9, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, fixed

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
Copy link
Contributor

@mihaic mihaic Aug 9, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo: voxels.

Copy link
Contributor Author

@lcnature lcnature Aug 9, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

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

@mihaic mihaic Aug 9, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

scale is missing.

Copy link
Contributor Author

@lcnature lcnature Aug 9, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

mihaic
mihaic approved these changes Aug 9, 2017
@mihaic mihaic merged commit 9844e65 into brainiak:master Aug 9, 2017
2 of 3 checks passed
@mihaic
Copy link
Contributor

@mihaic mihaic commented Aug 9, 2017

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants