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

Updated how drift is calculated and how masking is done (Ready for review) #244

Merged
merged 53 commits into from Nov 10, 2017

Conversation

Projects
None yet
4 participants
@CameronTEllis
Contributor

CameronTEllis commented Jul 21, 2017

No description provided.

CameronTEllis added some commits Jul 21, 2017

@mihaic mihaic requested a review from cbaldassano Jul 21, 2017

CameronTEllis added some commits Jul 22, 2017

Updated doc string of most functions to be more clear. Combined _calc…
…_sfnr and _calc_temporal_noise and then edited accordingly
Simplified masking by distinguishing between a binary mask and a cont…
…inuous template. Both are 3d (and made into 4d volumes where necessary)
@mihaic

This comment has been minimized.

Show comment
Hide comment
@mihaic

mihaic Jul 25, 2017

Contributor

@CameronTEllis, if you plan to continue making changes to this PR, it would be useful to add a tag to the title, such as "WIP" (work in progress), to signal to the reviewers it is not ready for review. I will add this suggestion to CONTRIBUTING.rst.

Contributor

mihaic commented Jul 25, 2017

@CameronTEllis, if you plan to continue making changes to this PR, it would be useful to add a tag to the title, such as "WIP" (work in progress), to signal to the reviewers it is not ready for review. I will add this suggestion to CONTRIBUTING.rst.

@CameronTEllis CameronTEllis changed the title from Updated how drift is calculated and how masking is done to Updated how drift is calculated and how masking is done (Work in Progress) Jul 25, 2017

CameronTEllis added some commits Jul 28, 2017

@CameronTEllis CameronTEllis changed the title from Updated how drift is calculated and how masking is done (Work in Progress) to Updated how drift is calculated and how masking is done (ready for review) Jul 28, 2017

@mihaic

This comment has been minimized.

Show comment
Hide comment
@mihaic

mihaic Aug 8, 2017

Contributor

@CameronTEllis, feel free to contact @cbaldassano or ask others to review if you want this merged faster.

Contributor

mihaic commented Aug 8, 2017

@CameronTEllis, feel free to contact @cbaldassano or ask others to review if you want this merged faster.

@CameronTEllis CameronTEllis changed the title from Updated how drift is calculated and how masking is done (ready for review) to Updated how drift is calculated and how masking is done (work in progress) Sep 18, 2017

CameronTEllis added some commits Sep 21, 2017

Extensive update. Changed how noise is calculated, now it is focused …
…around SNR and SFNR. Made stimfunction and signal function more useable and extensible. Added more documentation
Updated fmrisim to accept any specified hrf. Changed utils to deal wi…
…th the new procedure for generating functions

CameronTEllis added some commits Oct 18, 2017

@CameronTEllis CameronTEllis changed the title from Updated how drift is calculated and how masking is done (work in progress) to Updated how drift is calculated and how masking is done (Ready for review) Oct 22, 2017

@CameronTEllis

This comment has been minimized.

Show comment
Hide comment
@CameronTEllis

CameronTEllis Oct 22, 2017

Contributor

@mihaic I am unsure what the error with Travis is. It says I am using the wrong version of Brew. Please advise the appropriate next steps.

Otherwise, this is ready for review e.g. @cbaldassano @lcnature

Contributor

CameronTEllis commented Oct 22, 2017

@mihaic I am unsure what the error with Travis is. It says I am using the wrong version of Brew. Please advise the appropriate next steps.

Otherwise, this is ready for review e.g. @cbaldassano @lcnature

@mihaic

This comment has been minimized.

Show comment
Hide comment
@mihaic

mihaic Oct 23, 2017

Contributor

@CameronTEllis, thanks for pointing out the Travis error. It not related to your PR. I am looking for a workaround (issue #274), but we can merge your PR irrespective of it, as soon as we get a review.

Contributor

mihaic commented Oct 23, 2017

@CameronTEllis, thanks for pointing out the Travis error. It not related to your PR. I am looking for a workaround (issue #274), but we can merge your PR irrespective of it, as soon as we get a review.

@cbaldassano

I think my comments cover most of the main changes. I also looked at the notebook and that looks good to me. Congrats Cameron on finally getting this PR together!

Show outdated Hide outdated brainiak/utils/fmrisim.py
Show outdated Hide outdated brainiak/utils/fmrisim.py
Show outdated Hide outdated brainiak/utils/fmrisim.py
Show outdated Hide outdated brainiak/utils/fmrisim.py
Show outdated Hide outdated brainiak/utils/fmrisim.py
Show outdated Hide outdated brainiak/utils/fmrisim.py
Show outdated Hide outdated brainiak/utils/fmrisim.py
# Since you are combining spatial and temporal noise, you need to
# subtract the variance of the two to get the spatial sd
if spatial_sd > temporal_sd:
spatial_sd = np.sqrt(spatial_sd ** 2 - temporal_sd ** 2)

This comment has been minimized.

@cbaldassano

cbaldassano Nov 6, 2017

Collaborator

Is there a reference for this? It makes intuitive sense but it is not totally obvious to me, e.g. should there be a factor here for the fact that there are 3 spatial dims and only 1 temporal?

@cbaldassano

cbaldassano Nov 6, 2017

Collaborator

Is there a reference for this? It makes intuitive sense but it is not totally obvious to me, e.g. should there be a factor here for the fact that there are 3 spatial dims and only 1 temporal?

This comment has been minimized.

@CameronTEllis

CameronTEllis Nov 9, 2017

Contributor

No there is no reference here. This is part of the tool that I am least sure about. It was a very difficult problem to model noise that corresponded to the brain (things like autoregression and spatial noise) while independently manipulating system noise. The solution I had was to distinguish them as here but I am not confident this is the optimal way. I played with a number of alternative procedures and they all seemed sub optimal but I am open to suggestions.

@CameronTEllis

CameronTEllis Nov 9, 2017

Contributor

No there is no reference here. This is part of the tool that I am least sure about. It was a very difficult problem to model noise that corresponded to the brain (things like autoregression and spatial noise) while independently manipulating system noise. The solution I had was to distinguish them as here but I am not confident this is the optimal way. I played with a number of alternative procedures and they all seemed sub optimal but I am open to suggestions.

@@ -1444,42 +1789,56 @@ def _generate_noise_temporal(stimfunction_tr,
# Finally, z score each voxel so things mix nicely
noise_temporal = stats.zscore(noise_temporal, 3)

This comment has been minimized.

@cbaldassano

cbaldassano Nov 6, 2017

Collaborator

zscoring when some variables have 0 variance may cause some warnings to be thrown - not necessarily a problem, but could be solved by using a masked array

@cbaldassano

cbaldassano Nov 6, 2017

Collaborator

zscoring when some variables have 0 variance may cause some warnings to be thrown - not necessarily a problem, but could be solved by using a masked array

This comment has been minimized.

@CameronTEllis

CameronTEllis Nov 9, 2017

Contributor

Yes good point. I will put this on the list for future things to fix

@CameronTEllis

CameronTEllis Nov 9, 2017

Contributor

Yes good point. I will put this on the list for future things to fix

Show outdated Hide outdated brainiak/utils/fmrisim.py
@lcnature

Hi @CameronTEllis Sorry for my delayed review.
All these are great works! Just a few comments for your consideration.

generate_stimfunction
Create a function to describe the stimulus onsets/durations
Create a timecourse of the signal activation. This can be specified using event

This comment has been minimized.

@lcnature

lcnature Nov 7, 2017

Contributor

Maybe worth mentioning this function is before convolution with HRF, and has higher temporal resolution than TR.

@lcnature

lcnature Nov 7, 2017

Contributor

Maybe worth mentioning this function is before convolution with HRF, and has higher temporal resolution than TR.

This comment has been minimized.

@CameronTEllis

CameronTEllis Nov 9, 2017

Contributor

Firstly, thank you so so much for all of these sophisticated and in depth comments. You really dived deep into the code and I appreciate the scrutiny greatly!

Secondly, good suggestion, added

@CameronTEllis

CameronTEllis Nov 9, 2017

Contributor

Firstly, thank you so so much for all of these sophisticated and in depth comments. You really dived deep into the code and I appreciate the scrutiny greatly!

Secondly, good suggestion, added

double_gamma_hrf
Convolve the stimulus function with the HRF to model when events are expected.
export_epoch_file:

This comment has been minimized.

@lcnature

lcnature Nov 7, 2017

Contributor

might be worth explaining what an epoch file is.

@lcnature

lcnature Nov 7, 2017

Contributor

might be worth explaining what an epoch file is.

This comment has been minimized.

@CameronTEllis

CameronTEllis Nov 9, 2017

Contributor

Added more detail

@CameronTEllis

CameronTEllis Nov 9, 2017

Contributor

Added more detail

Show outdated Hide outdated brainiak/utils/fmrisim.py
Show outdated Hide outdated brainiak/utils/fmrisim.py
Show outdated Hide outdated brainiak/utils/fmrisim.py
@@ -1189,6 +1510,7 @@ def _generate_noise_temporal_phys(timepoints,
----------
one dimensional array, float
Generates the physiological temporal noise timecourse
"""
noise_phys = [] # Preset

This comment has been minimized.

@lcnature

lcnature Nov 7, 2017

Contributor

suggestion (not critical): random phases may be added to the respiratory radian and heart radian.
I am personally against z-scoring. Although it makes the sample variance equal across any time series generated, essentially every time you generate noise, a different coefficient is multiplied due to the normalization. So there is no guarantee that the sinusoidal component has the same magnitude across different samples drawn. But this is personal flavor.
I also wonder whether the addition of Gaussian noise here is necessary. Isn't Gaussian noise also added somewhere else?

@lcnature

lcnature Nov 7, 2017

Contributor

suggestion (not critical): random phases may be added to the respiratory radian and heart radian.
I am personally against z-scoring. Although it makes the sample variance equal across any time series generated, essentially every time you generate noise, a different coefficient is multiplied due to the normalization. So there is no guarantee that the sinusoidal component has the same magnitude across different samples drawn. But this is personal flavor.
I also wonder whether the addition of Gaussian noise here is necessary. Isn't Gaussian noise also added somewhere else?

This comment has been minimized.

@CameronTEllis

CameronTEllis Nov 9, 2017

Contributor

Aha, good find, that was actually what I intended to do but did it wrong. In terms of z scoring, I do it in order to make the combination of noise types more tractable. When they are z scored I know how to mix their relative values in order to fit SFNR and SNR

@CameronTEllis

CameronTEllis Nov 9, 2017

Contributor

Aha, good find, that was actually what I intended to do but did it wrong. In terms of z scoring, I do it in order to make the combination of noise types more tractable. When they are z scored I know how to mix their relative values in order to fit SFNR and SNR

This comment has been minimized.

@lcnature

lcnature Nov 9, 2017

Contributor

For periodic signal, you might get away with analytically calculating the expected standard deviation with magnitude/sqrt(2) according to this wiki page:
https://en.wikipedia.org/wiki/Root_mean_square
That way you do not need to z-score. But of course there may be issue of aliasing. But I think as long as ratio between the frequency of breathing and the fMRI sampling frequency is not a regular number, this estimate should OK.

@lcnature

lcnature Nov 9, 2017

Contributor

For periodic signal, you might get away with analytically calculating the expected standard deviation with magnitude/sqrt(2) according to this wiki page:
https://en.wikipedia.org/wiki/Root_mean_square
That way you do not need to z-score. But of course there may be issue of aliasing. But I think as long as ratio between the frequency of breathing and the fMRI sampling frequency is not a regular number, this estimate should OK.

This comment has been minimized.

@CameronTEllis

CameronTEllis Nov 9, 2017

Contributor

I am not sure what the current issues are with zscoring: at the moment it doesn't really matter that I do it because already the mean is ~0 and std is ~1 but I could remove it if you think it is necessary

@CameronTEllis

CameronTEllis Nov 9, 2017

Contributor

I am not sure what the current issues are with zscoring: at the moment it doesn't really matter that I do it because already the mean is ~0 and std is ~1 but I could remove it if you think it is necessary

This comment has been minimized.

@lcnature

lcnature Nov 9, 2017

Contributor

It is more for reproducibility concern - that you always have the same expected summary statistics for any given type of noise. (i.e., when a user say I simulated certain noise from certain generative model with certain parameters, the parameters that the user put in indeed governs how the samples drawn). I agree it is not a critical issue. So it is up to you.

@lcnature

lcnature Nov 9, 2017

Contributor

It is more for reproducibility concern - that you always have the same expected summary statistics for any given type of noise. (i.e., when a user say I simulated certain noise from certain generative model with certain parameters, the parameters that the user put in indeed governs how the samples drawn). I agree it is not a critical issue. So it is up to you.

This comment has been minimized.

@CameronTEllis

CameronTEllis Nov 9, 2017

Contributor

I think I will leave it as is but I appreciate the concern. Given that the critical output of this is the summed response of a respiratory and heart rate oscillation I think the code does what is necessary

@CameronTEllis

CameronTEllis Nov 9, 2017

Contributor

I think I will leave it as is but I appreciate the concern. Given that the critical output of this is the summed response of a respiratory and heart rate oscillation I think the code does what is necessary

Show outdated Hide outdated brainiak/utils/fmrisim.py
Show outdated Hide outdated brainiak/utils/fmrisim.py
Show outdated Hide outdated brainiak/utils/fmrisim.py
noise_system = _generate_noise_system(dimensions_tr=dimensions_tr)
# Calculate the sd that is necessary to be combined with itself in order
# to generate the temporal_sd
temporal_sd_element = np.sqrt(temporal_sd ** 2 / 2)

This comment has been minimized.

@lcnature

lcnature Nov 7, 2017

Contributor

There is no concrete suggestion in this comment. It is just suggestion for future consideration.
It seems intuitive here to divide half of the temporal noise variance to noise_temporal and half to the temporal part of noise_system if we do not have much information. But this division of half and half seems a bit arbitrary. Is it helpful to actually allow the user to specify the magnitudes of the noise_temporal and noise_system separately? Maybe very few users would have a good intuition of what number they should put in if they have a choice. For future development, I think it may be possible to actually estimate these magnitudes from data. I think if you specify the belief of the distribution of the two types of noise, there may be ways to estimate their magnitudes.

@lcnature

lcnature Nov 7, 2017

Contributor

There is no concrete suggestion in this comment. It is just suggestion for future consideration.
It seems intuitive here to divide half of the temporal noise variance to noise_temporal and half to the temporal part of noise_system if we do not have much information. But this division of half and half seems a bit arbitrary. Is it helpful to actually allow the user to specify the magnitudes of the noise_temporal and noise_system separately? Maybe very few users would have a good intuition of what number they should put in if they have a choice. For future development, I think it may be possible to actually estimate these magnitudes from data. I think if you specify the belief of the distribution of the two types of noise, there may be ways to estimate their magnitudes.

This comment has been minimized.

@CameronTEllis

CameronTEllis Nov 9, 2017

Contributor

This is an astute point to bring up. This is related to my last comment from Chris above. Basically I agree with you and think that what I am doing is definitely a massive simplification. Assuming equality between the system and the non-system noise is a big (fallacious assumption). This is something I would love to improve upon in the future

@CameronTEllis

CameronTEllis Nov 9, 2017

Contributor

This is an astute point to bring up. This is related to my last comment from Chris above. Basically I agree with you and think that what I am doing is definitely a massive simplification. Assuming equality between the system and the non-system noise is a big (fallacious assumption). This is something I would love to improve upon in the future

@mihaic

This comment has been minimized.

Show comment
Hide comment
@mihaic

mihaic Nov 8, 2017

Contributor

@CameronTEllis, I assume you will need some time to address the helpful comments you received, so I am already preparing a new release in PR #295, without this code. Please let me know if I'm making a wrong assumption.

Contributor

mihaic commented Nov 8, 2017

@CameronTEllis, I assume you will need some time to address the helpful comments you received, so I am already preparing a new release in PR #295, without this code. Please let me know if I'm making a wrong assumption.

@CameronTEllis

This comment has been minimized.

Show comment
Hide comment
@CameronTEllis

CameronTEllis Nov 8, 2017

Contributor
Contributor

CameronTEllis commented Nov 8, 2017

@mihaic mihaic referenced this pull request Nov 8, 2017

Closed

Add release notes for v0.6 #295

Show outdated Hide outdated brainiak/utils/fmrisim.py
Show outdated Hide outdated brainiak/utils/fmrisim.py
Show outdated Hide outdated brainiak/utils/fmrisim.py
Show outdated Hide outdated brainiak/utils/fmrisim.py
Show outdated Hide outdated brainiak/utils/fmrisim.py
Show outdated Hide outdated brainiak/utils/fmrisim.py
@@ -1189,6 +1510,7 @@ def _generate_noise_temporal_phys(timepoints,
----------
one dimensional array, float
Generates the physiological temporal noise timecourse
"""
noise_phys = [] # Preset

This comment has been minimized.

@lcnature

lcnature Nov 9, 2017

Contributor

For periodic signal, you might get away with analytically calculating the expected standard deviation with magnitude/sqrt(2) according to this wiki page:
https://en.wikipedia.org/wiki/Root_mean_square
That way you do not need to z-score. But of course there may be issue of aliasing. But I think as long as ratio between the frequency of breathing and the fMRI sampling frequency is not a regular number, this estimate should OK.

@lcnature

lcnature Nov 9, 2017

Contributor

For periodic signal, you might get away with analytically calculating the expected standard deviation with magnitude/sqrt(2) according to this wiki page:
https://en.wikipedia.org/wiki/Root_mean_square
That way you do not need to z-score. But of course there may be issue of aliasing. But I think as long as ratio between the frequency of breathing and the fMRI sampling frequency is not a regular number, this estimate should OK.

@CameronTEllis

This comment has been minimized.

Show comment
Hide comment
@CameronTEllis

CameronTEllis Nov 10, 2017

Contributor
Contributor

CameronTEllis commented Nov 10, 2017

@lcnature

This comment has been minimized.

Show comment
Hide comment
@lcnature

lcnature Nov 10, 2017

Contributor

@mihaic Given the time pressure, I think it is fine to leave the code as is for the concerns I have raised and in the expectation that @CameronTEllis will improve it in future PR.

Contributor

lcnature commented Nov 10, 2017

@mihaic Given the time pressure, I think it is fine to leave the code as is for the concerns I have raised and in the expectation that @CameronTEllis will improve it in future PR.

@mihaic

This comment has been minimized.

Show comment
Hide comment
@mihaic

mihaic Nov 10, 2017

Contributor

@lcnature, thanks for your input. @cbaldassano, you also requested changes; what do you think about the code?

Contributor

mihaic commented Nov 10, 2017

@lcnature, thanks for your input. @cbaldassano, you also requested changes; what do you think about the code?

@cbaldassano

All my major concerns have been addressed, I'm happy to approve the current version of the PR.

@mihaic

This comment has been minimized.

Show comment
Hide comment
@mihaic

mihaic Nov 10, 2017

Contributor

@CameronTEllis, if you are done, push the "Update branch" button the PR website.

Contributor

mihaic commented Nov 10, 2017

@CameronTEllis, if you are done, push the "Update branch" button the PR website.

@mihaic mihaic merged commit 74dae88 into brainiak:master Nov 10, 2017

2 of 3 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
linux Build finished.
Details
macos Build finished.
Details
@mihaic

This comment has been minimized.

Show comment
Hide comment
@mihaic

mihaic Nov 10, 2017

Contributor

Thanks again, everyone. Please create issues based on the review comments.

Contributor

mihaic commented Nov 10, 2017

Thanks again, everyone. Please create issues based on the review comments.

@CameronTEllis CameronTEllis deleted the CameronTEllis:fmrisim_mean_mask branch Nov 10, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment