Skip to content

[ENH] Include repetition time in functional summary - #1508

Merged
oesteban merged 3 commits into
nipreps:masterfrom
wiheto:master
Feb 15, 2019
Merged

[ENH] Include repetition time in functional summary#1508
oesteban merged 3 commits into
nipreps:masterfrom
wiheto:master

Conversation

@wiheto

@wiheto wiheto commented Feb 13, 2019

Copy link
Copy Markdown
Contributor

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

@welcome

welcome Bot commented Feb 13, 2019

Copy link
Copy Markdown

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.
We invite you to list yourself as a fMRIPrep contributor, so if your name is not already mentioned, please modify the .zenodo.json file with your data right above Russ' entry. Example:

{
   "name": "Contributor, New FMRIPrep",
   "affiliation": "Department of fMRI prep'ing, Open Science Made-Up University",
   "orcid": "<your id>"
},
{
   "name": "Poldrack, Russell A.",
   "affiliation": "Department of Psychology, Stanford University",
   "orcid": "0000-0001-6755-0259"
},

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.

@effigies effigies left a comment

Copy link
Copy Markdown
Member

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.

Comment thread fmriprep/interfaces/reports.py Outdated
Comment thread fmriprep/interfaces/reports.py Outdated
@effigies effigies changed the title [ENH] adding repetition time to reports [ENH] Include repetition time in functional summary Feb 13, 2019
@effigies

Copy link
Copy Markdown
Member

@wiheto I'm planning to release 1.3.1 today. Do you have time to finish this off?

@wiheto

wiheto commented Feb 14, 2019 via email

Copy link
Copy Markdown
Contributor Author

@oesteban

Copy link
Copy Markdown
Member

@effigies, why don't we release a 1.3.0-2? after all we needed to fix 1.3.0. That would also give @wiheto some extra time for these fixes (and release 1.3.1 asap).

@effigies

Copy link
Copy Markdown
Member

Because we have an actual change in functionality (#1467) already in master.

@oesteban

Copy link
Copy Markdown
Member

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.

@effigies

Copy link
Copy Markdown
Member

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.

@oesteban

Copy link
Copy Markdown
Member

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

effigies and others added 2 commits February 14, 2019 09:18
Co-Authored-By: wiheto <wiheto@users.noreply.github.com>
Co-Authored-By: wiheto <wiheto@users.noreply.github.com>
@wiheto

wiheto commented Feb 14, 2019

Copy link
Copy Markdown
Contributor Author

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

from fmriprep.interfaces.reports import FunctionalSummary
summary = FunctionalSummary(slice_timing=True,
                        registration='FreeSurfer',
                        registration_dof=6,
                        pe_direction='i',
                        tr=2,
                        distortion_correction='a string',
                        fallback=False,
                        output_spaces=['MNI152NLin2009cAsym'],
                        confounds_file='tmp.tsv')
summary.run()

Output file in ./reports.html is:

                <h3 class="elem-title">Summary</h3>
                <ul class="elem-desc">
                        <li>Repetition time (TR): 2s</li>
                        <li>Phase-encoding (PE) direction: Left-Right</li>
                        <li>Slice timing correction: Applied</li>
                        <li>Susceptibility distortion correction: a string</li>
                        <li>Registration: FreeSurfer <code>bbregister</code> (b$
                        <li>Functional series resampled to spaces: MNI152NLin20$
                        <li>Confounds collected: a, b, c</li>
                </ul>

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?

@effigies

Copy link
Copy Markdown
Member

The only thing I was a slight unsure about is if the flag Mandetory=True should be there. Is there always able to get RepetitionTime in metadata?

Yes. This is required by BIDS.

@wiheto

wiheto commented Feb 14, 2019

Copy link
Copy Markdown
Contributor Author

Good, then this won't break anything.

@effigies effigies left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM!

@oesteban
oesteban merged commit 4470ffb into nipreps:master Feb 15, 2019
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.

Add TR to functional summary reports

3 participants