-
Notifications
You must be signed in to change notification settings - Fork 296
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
[ENH] Include repetition time in functional summary #1508
Conversation
Thanks for opening this pull request! We have detected this is the first time for you to contribute to fMRIPrep. Please check out our contributing guidelines.
Of course, if you want to opt-out this time there is no problem at all with adding your name later. You will be always welcome to add it in the future whenever you feel it should be listed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Some cleanups, though.
@wiheto I'm planning to release 1.3.1 today. Do you have time to finish this off? |
Can do. I can sit this morning and fix all the stuff you have suggested (here and niworkflows). I’ll get to it in a few hours.
… 14 feb. 2019 kl. 06:19 skrev Chris Markiewicz ***@***.***>:
@wiheto I'm planning to release 1.3.1 today. Do you have time to finish this off?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
Because we have an actual change in functionality (#1467) already in master. |
Yeah, I was going to slack you right now. I had that situation as well before releasing 1.3.0-1. Let me make a backport. |
We have a patched Docker container people can use for now. I don't know that we need to rush beyond just getting it out today. |
It was just cherry-picking your commit based off 1.3.0.post1, it can't hurt. - https://github.com/poldracklab/fmriprep/releases/tag/1.3.0.post2 |
Co-Authored-By: wiheto <wiheto@users.noreply.github.com>
Co-Authored-By: wiheto <wiheto@users.noreply.github.com>
Committed suggestions. Also here is an example of the output of the changes (if making a tmp.tsv confounds file with the confounds a,b,c).
Output file in ./reports.html is:
The only thing I was a slight unsure about is if the flag Mandetory=True should be there. Is it always the case that RepetitionTime is in metadata? |
Yes. This is required by BIDS. |
Good, then this won't break anything. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Changes proposed in this pull request
Closes #1425. Small change that adds the repetition time to the functional reports.
(Was looking how the reports were generated and added it).
Documentation that should be reviewed