Skip to content

Fix gen design - #381

Merged
lcnature merged 7 commits into
brainiak:masterfrom
lcnature:fix_gen_design
Sep 14, 2018
Merged

Fix gen design#381
lcnature merged 7 commits into
brainiak:masterfrom
lcnature:fix_gen_design

Conversation

@lcnature

Copy link
Copy Markdown
Contributor

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 7 commits September 9, 2018 23:45
…, which shortens stim_function if its length is smaller than the product of total_time and temporal_resolution
…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

Copy link
Copy Markdown
Contributor

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

@lcnature

lcnature commented Sep 11, 2018

Copy link
Copy Markdown
Contributor Author

@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 CameronTEllis left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good

Comment thread brainiak/utils/fmrisim.py
# Store the weights
stimfunction[onset_idx:offset_idx, 0] = [weights[onset_counter]]

# Shorten the data if it's too long

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Good point, this is unnecessary as you say

Comment thread brainiak/utils/utils.py
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])]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Makes sense

Comment thread tests/utils/test_utils.py
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')

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Good extra test to have

@lcnature
lcnature merged commit 9889b80 into brainiak:master Sep 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants