Skip to content
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: Conform confound regressor names to Derivatives RC2 #1343

Merged
merged 9 commits into from Oct 30, 2018

Conversation

effigies
Copy link
Member

@effigies effigies commented Oct 24, 2018

Changes proposed in this pull request

Updating confounds headers to match the RC2.

Known remaining tasks:

  • Update the cosine_* headers in nipype or update in a separate function in nipype.
  • Update DVARS and FD headers (might need to be in nipype)

Documentation that should be reviewed

Anywhere we discuss confounds. I believe we have example confounds that should be fixed, as well.

@effigies effigies added this to the 1.2.0 milestone Oct 24, 2018
@effigies effigies changed the title RF: Update confounds headers for Derivatives RC2 [WIP] RF: Update confounds headers for Derivatives RC2 Oct 24, 2018
@effigies
Copy link
Member Author

For simplicity, I put a camel_to_snake function in the confound gathering interface. We can update things in Nipype to be more BIDS-compliant if we want, but that can be a separate thing.

At this point, is there anything besides metadata left to update?

@@ -245,7 +255,7 @@ def _get_ica_confounds(ica_out_dir, skip_vols, newpath=None):
# add one to motion_ic_indices to match melodic report.
aroma_confounds = os.path.join(newpath, "AROMAAggrCompAROMAConfounds.tsv")
pd.DataFrame(aggr_confounds.T,
columns=['AROMAAggrComp%02d' % (x + 1) for x in motion_ic_indices]).to_csv(
columns=['aroma_motion_%02d' % (x + 1) for x in motion_ic_indices]).to_csv(
Copy link
Member Author

Choose a reason for hiding this comment

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

@jdkent I want your opinion on this change. We need to go to snake_case, and I didn't want to make it that much longer. I figured aroma_motion would indicate "components classified as motion by AROMA"...

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think that sounds good 👍, that's how I read it too.

@effigies effigies changed the title [WIP] RF: Update confounds headers for Derivatives RC2 RF: Update confounds headers for Derivatives RC2 Oct 26, 2018
@effigies effigies changed the title RF: Update confounds headers for Derivatives RC2 ENH: Conform confound regressor names to Derivatives RC2 Oct 26, 2018
@effigies
Copy link
Member Author

@oesteban I'm going to rebase this. I'm not sure what fixes you were going for, but these last merges look messed up.

@oesteban
Copy link
Member

Yes, I did a bad merge. Please go ahead 👍

@effigies
Copy link
Member Author

Assuming this passes, I think we should be good to go.

@effigies
Copy link
Member Author

@oesteban I'm seeing desc-aseg and label-aseg. I don't think we output label- anymore. Is this a thing where clearing the anatomical cache is required, or am I missing something?

@oesteban
Copy link
Member

Looking into this right now

@@ -6,6 +6,8 @@ fmriprep/logs/CITATION.md
fmriprep/logs/CITATION.tex
fmriprep/sub-01
fmriprep/sub-01/anat
fmriprep/sub-01/anat/sub-01_desc-aparcaseg_dseg.nii.gz
fmriprep/sub-01/anat/sub-01_desc-aseg_dseg.nii.gz
fmriprep/sub-01/anat/sub-01_desc-brain_mask.nii.gz
fmriprep/sub-01/anat/sub-01_desc-preproc_T1w.nii.gz
fmriprep/sub-01/anat/sub-01_dseg.nii.gz
Copy link
Member Author

Choose a reason for hiding this comment

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

Thought for another PR: Since we do sometimes have other dseg files, do we want to specify desc-fast or similar?

Copy link
Member

Choose a reason for hiding this comment

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

That would be a good idea. I'd rather go with desc-3tissues and leave the provenance for the json file.

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.

None yet

3 participants