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

Fix gen design #381

Merged
merged 7 commits into from Sep 14, 2018

Conversation

Projects
None yet
2 participants
@lcnature
Contributor

lcnature commented Sep 10, 2018

It was designed that an AFNI style stimulus timing onset file with a negative onset can result in an all-zero design matrix, but this actually raised error. This PR fixes it.

Mingbo Cai and others added some commits Sep 10, 2018

Mingbo Cai
Mingbo Cai
Mingbo Cai Mingbo Cai
removed the conditional clause in generate_stimfunction of fmrisim.py…
…, which shortens stim_function if its length is smaller than the product of total_time and temporal_resolution
lcnature
lcnature
changed two `floor` to `round` in utils.gen_design, because in princi…
…ple there is no reason to use floor. And in practice, due to precision of float number computation, this can sometime result in a smaller count of number of TRs. As long as the user provide the correct total_duration
@CameronTEllis

This comment has been minimized.

Contributor

CameronTEllis commented Sep 11, 2018

@lcnature, would you like me to go over it now?

@lcnature

This comment has been minimized.

Contributor

lcnature commented Sep 11, 2018

@CameronTEllis That would be awesome. One change I made was in fmrisim.py, which especially needs your eyes on it in case I make new bugs.
That change in fmrisim.py was primarily due to some strange corner case I encountered, in which machine precision in float number cause that conditional clause to execute when it is actually unnecessary (eg., sometimes total_time * temporal_resolution can be smaller than its integer version by a very small amount due to machine precision, and then int(total_time * temporal_resolution) somehow is smaller than int(round(total_time * temporal_resolution)) by 1 in very occasional case...).

And then I realized that the conditional clause in generate_stimfunction to shorten the stimfunction may not be necessary, because the lines above won't actually lengthen it as Matlab would do even if offset_idx happens to be larger than int(round(total_time * temporal_resolution)).
If you want to keep that clause, Line 528 might need to be changed to stimfunction = stimfunction[0:int(total_time * temporal_resolution), :], otherwise stimfunction can become 1-d nparray instead of a 2-d one, which convolve_hrf is not happy about.

@CameronTEllis

Looks good

@@ -523,10 +523,6 @@ def generate_stimfunction(onsets,
# Store the weights
stimfunction[onset_idx:offset_idx, 0] = [weights[onset_counter]]
# Shorten the data if it's too long

This comment has been minimized.

@CameronTEllis

CameronTEllis Sep 14, 2018

Contributor

Good point, this is unnecessary as you say

for duration in scan_duration]
else:
design = [np.empty([int(np.floor(scan_duration / TR)), n_C])]
design = [np.empty([int(np.round(scan_duration / TR)), n_C])]

This comment has been minimized.

@CameronTEllis

CameronTEllis Sep 14, 2018

Contributor

Makes sense

scan_duration=[48], TR=2, style='AFNI')
assert np.all(design7 == 0.0), (
'A negative stimulus onset of AFNI style should result in an all-zero'
+ ' design matrix')

This comment has been minimized.

@CameronTEllis

CameronTEllis Sep 14, 2018

Contributor

Good extra test to have

@lcnature lcnature merged commit 9889b80 into brainiak:master Sep 14, 2018

4 checks passed

codecov/patch 100% of diff hit (target 91.23%)
Details
codecov/project 91.39% (+0.16%) compared to 48a1b8b
Details
linux Build finished.
Details
macos Build finished.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment