HCP Pipelines Improvements (ICAFIX, PostFix, RestingStateStats, TaskfMRIAnalysis, londitudinal/cross-sectional logic) #31

tjhendrickson · 2018-06-21T20:48:12Z

This pull request extends the existing bids apps HCP Pipeline by adding on ICAFIX, PostFix, and RestingStateStats.

The TaskfMRIAnalysis portion has been started but has yet to be completed.

Logic was added in order to account for a bids format that meets either a standard cross sectional format such as:
sub-01
anat
func
And also a longitudinal format:
sub-01
ses-01
anat
func
ses-02
anat
func

… within fieldmap json sidecars

…ndedFor field at time of execution

…sn't exist

…d maps if they exist

…en fieldmap and func shim settings

… run ICA-FIX pipeline

…lved within BIDS structure

Add Longitudinal to Extend Stream

…lly placed files that needed modifying into separate directory named modified_files

…erly within run.py

…ame instead of fmritcs

…ally made /bids_dir and /output_dir directories so this can container will work on singularity.

…longitudinal data. This has been corrected.

…ave to think about lvl2_fsf for TaskfMRIAnalysis.

chrisgorgo · 2018-06-21T21:34:38Z

Thanks for sending the PR!

Could you remove the .idea folder?

Also could you remove the TaskfMRIAnalysis folder since this work is not finished and those scripts are already part of HCP Pipelines installed in the docker container?

chrisgorgo · 2018-06-21T21:35:58Z

IntendedFor.py

+import numpy
+
+"""
+This script is designed to determine which field maps apply to discrete fMRI scans


This looks very similar to get_fieldmap implemented in pybids https://github.com/INCF/pybids/tree/master/bids/grabbids#get-a-fieldmap-files-intended-for-a-given-map. Maybe we should use that one?

Hmm. It has been a while since I looked at the get_fieldmap function implemented in pybids so perhaps it has changed, but doesn't the get_fieldmap function just retrieve the fieldmap in which the fmriname is within the "IntendedFor" field? I was under the impression that the "IntendedFor" field does not automatically generate through the BIDS conversion, thus it has to be added manually.

What am I missing?

Correct. Maybe I misunderstood what your function does. If it's part of a conversion process this repo might not be the best place for it. Maybe it would be better to put it in pybids?

I can see why you may have been confused. The comments I provide I very unclear. Obviously I created them when I did not understand what the script was doing myself, and forgot to come through and change them. It is added the "IntendedFor" fields to the fmap json sidecars, so yes I suppose it has more to do with the BIDS conversion. I can propose incorporation to pybids, but perhaps as a catch-all let's leave IntendedFor..py as is for now.

tjhendrickson · 2018-06-21T21:59:53Z

I can certainly remove the .idea and TaskfMRIAnalysis folders. Is there a way that can be done without deleting it on my fork and resending the pull request (sorry this is my first pull request!)?

chrisgorgo · 2018-06-21T22:09:49Z

You just need to delete it on the branch from this PR and puszh it to GitHub. The PR will update automatically.

tjhendrickson · 2018-06-21T22:59:35Z

I'll also correct merge conflicts within Dockerfile

chrisgorgo

Thanks again for sending this. I would love to merge it, but there are a few outstanding issues.. I had a first pass of the code, but since your changes are so extensive it's hard to evaluate everything. Tip for future contributions: keep PRs small and focused on one feature :)

chrisgorgo · 2018-06-22T08:38:57Z

version

@@ -1 +1 @@
-dev
+v0.3


please revert this change - this file is automatically updated when we trigger a release on GitHub

chrisgorgo · 2018-06-22T08:40:48Z

Dockerfile

+ENV LD_LIBRARY_PATH=/usr/lib/fsl/5.0
+ENV FSLTCLSH=/usr/bin/tclsh
+ENV FSLWISH=/usr/bin/wish
+ENV FSLOUTPUTTYPE=NIFTI_GZ


Was there a reason for moving this here?

chrisgorgo · 2018-06-22T08:42:01Z

Dockerfile

-	echo 'deb http://neuro.debian.net/debian trusty main'; \
-	echo 'deb http://neuro.debian.net/debian data main'; \
-	echo '#deb-src http://neuro.debian.net/debian-devel trusty main'; \
-} > /etc/apt/sources.list.d/neurodebian.sources.list


Why do you want to remove this? It's an important workaround for flaky key server.

Obviously I did not understand the purpose of this. Feel free to leave it in!

chrisgorgo · 2018-06-22T08:42:10Z

Dockerfile

-    apt-key adv --recv-keys --keyserver hkp://pgp.mit.edu:80 0xA5D32F012649A5A9 && \
-    apt-get update && \
-    apt-get install -y fsl-core=5.0.9-4~nd14.04+1
+    curl -sSL http://neuro.debian.net/lists/trusty.us-ca.full >> /etc/apt/sources.list.d/neurodebian.sources.list


See comment above.

See my comment above.

chrisgorgo · 2018-06-22T08:43:39Z

Dockerfile

-    tar zxf MSMs.tar.gz && \
-    rm MSMs.tar.gz && \
-    mv MSM_HOCR_v1/Ubuntu /opt/HCP-Pipelines/MSMBinaries && \
-    rm -rf MSM_HOCR_v1


Is there a reason for not installing MSM?

No reason, let's go with your version.

chrisgorgo · 2018-06-22T08:48:09Z

Dockerfile


 COPY version /version
+COPY IntendedFor.py /IntendedFor.py
+COPY modified_files/fsl_sub /usr/lib/fsl/5.0/fsl_sub


What changes are required to this file? It would be better to apply them via diff

chrisgorgo · 2018-06-22T08:48:55Z

Dockerfile

+COPY modified_files/PostFix.sh /opt/HCP-Pipelines/PostFix/PostFix.sh
+COPY modified_files/run_prepareICAs.sh /opt/HCP-Pipelines/PostFix/Compiled_prepareICAs/distrib/run_prepareICAs.sh
+COPY modified_files/RestingStateStats.sh /opt/HCP-Pipelines/RestingStateStats/RestingStateStats.sh
+COPY modified_files/run_RestingStateStats.sh /opt/HCP-Pipelines/RestingStateStats/Compiled_RestingStateStats/distrib/run_RestingStateStats.sh


I would like to avoid departing from the original HCP Pipelines as much as possible. Why those files need to be replaced?

Generally I absolutely agree with you, however, I was required to make mods to the RestingStateStats and PostFix scripts because the paths to the matlab compiler runtime are hardcoded to paths that fall within WashU's server systems.

chrisgorgo · 2018-06-22T08:50:08Z

run.py

@@ -8,9 +8,13 @@
 from subprocess import Popen, PIPE
 from shutil import rmtree
 import subprocess
+import nibabel as nip


this will be confusing for a lot of people - why not nib or nb?

chrisgorgo · 2018-06-22T08:50:54Z

run.py

-lowresmesh = 32
+    #change ownership of completed processing
+    #change_owner ='chown -R `stat -c "%u:%g" ' + args.output_dir + '` ' + args.output_dir
+    #os.system(change_owner)


Please remove commented out code.

chrisgorgo · 2018-06-22T08:56:11Z

run.py

-                    if "-" in enc_dir:
-                        SEPhaseNeg = fieldmap
+        # before do anything else add IntendedFor field to fieldmap
+        setup(os.path.join(args.bids_dir, "sub-"+subject_label))


Modifying input dataset in place is a big red flag. This bahaviour will not be expected by users and can cause a lot of trouble. We actually recommend to pass bids_dir as a read-only volume which would break this.

Handling of missing metadata (such as IntendedFor) should be done before running BIDS Apps.

tjhendrickson · 2018-06-22T14:08:46Z

Yes I am learning my lesson that PRs should be small. I will definitely create small PRs in the future.

tjhendrickson and others added 30 commits March 28, 2018 16:30

first commit to github.umn.edu of bids apps hcppipeline app

ce2c6db

made changes to run.py in order to account for my BIDS infrastructure

56f01cb

last commit prior to removing branch

8f46b5b

Added IntendedFor.py to create IntendedFor field if it does not exist…

53d615b

… within fieldmap json sidecars

Merge branch 'dev' of github.umn.edu:hendr522/HCPPipelines into extend

3b23b48

added IntendedFor scrit to run.py so it will take care of adding Inte…

4f98e77

…ndedFor field at time of execution

changed run.py for Dockerfile implementation

8ac3dad

added intendedfor script to run.py to add intendedfor field if it doe…

6c61788

…sn't exist

changed set_fieldmap with return_list=True to allow for multiple fiel…

bbab867

…d maps if they exist

merged changes from extend to allow for a list of fieldmaps

b91946f

made changes to IntendedFor.py for user determined fudge factor betwe…

4696caa

…en fieldmap and func shim settings

added /bin/bash -c to entrypoint to fix bit set problem with udocker

197e040

fixed conflicts between local and remote branch

937bc62

remove entrypoint command so udocker will run

35e86d3

remove entrypoint command so udocker will run

ae79bb6

merged conflicts with master

8b5e288

added diffusion pieces from jokedurnez fork commit 2c44cc0

3f0c1ea

Added function to run ICA-FIX processing stream

6378f36

install FIX along with dependencies to Dockerfile

dc33ab0

added FIX along with dependencies such as R to dockerfile in order to…

5fbf323

… run ICA-FIX pipeline

added install for pcre for R install

d52473d

made several changes to download and incorporate FIX into container

f47ac58

created a TODO to determine input_file variable for ICAFIX

16861b9

ensure clean working tree

fdc426f

added v0.1 to master for versioning

ab32ac1

added if-else statement to run pipelines when session labels are invo…

1cc0c75

…lved within BIDS structure

changes made to run.py to ensure session label conditional would work

5118fd3

Merge branch 'extend' into master

eaead66

Merge pull request bids-apps#2 from hendr522/master

07d8b30

Add Longitudinal to Extend Stream

Finished writing ICAFIX portion of pipeline, can be tested

8b06f71

tjhendrickson and others added 21 commits June 1, 2018 18:58

made changes to README.md to reflect current capabilities of release

782df47

Update README.md

86e10e2

Update README.md

943afa0

Update run.py

856e95c

playing around with R packages to meet FIX dependencies

b939175

finished testing of ICAFIX, RestingStateStats, and PostFix. Additiona…

dd17549

…lly placed files that needed modifying into separate directory named modified_files

moved IntendedFor.py back to top level directory so it would run prop…

5401cc6

…erly within run.py

removed pdb.set_trace() commands from run.py

8b5c071

made a small change to run.py, for resting state processing use fmrin…

9ac1544

…ame instead of fmritcs

finish out session label vs subject label if else condition. Addition…

a8675c3

…ally made /bids_dir and /output_dir directories so this can container will work on singularity.

session label logic was incorrect resulting in failed processing for …

35cbd7d

…longitudinal data. This has been corrected.

created TaskfMRIAnalysis directory within container

ad5fc48

incremental progress on TaskfMRI implementation

156b66a

Reverted TaskfMRIAnalysis pipeline to v3.17.0

ce466ce

added generate_level1_fsf.sh to docker image

23a4e6f

place function and resting state underneath for loop

2048ab4

making sure odin repo is up to date

0d40451

backed out of user input section within intendedfor script

2e3a4c9

added necessary features to generatefsf and TaskfMRIAnalysis. Still h…

3b1b3dc

…ave to think about lvl2_fsf for TaskfMRIAnalysis.

added Singularity usage to README.md

e67b1cd

resolved merge conflicts

cec30da

chrisgorgo reviewed Jun 21, 2018

View reviewed changes

removed TaskfMRIAnalysis and .idea

5a0b02f

correct merge conflict with Dockerfile

6b0502d

chrisgorgo suggested changes Jun 22, 2018

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HCP Pipelines Improvements (ICAFIX, PostFix, RestingStateStats, TaskfMRIAnalysis, londitudinal/cross-sectional logic) #31

HCP Pipelines Improvements (ICAFIX, PostFix, RestingStateStats, TaskfMRIAnalysis, londitudinal/cross-sectional logic) #31

tjhendrickson commented Jun 21, 2018

chrisgorgo commented Jun 21, 2018

chrisgorgo Jun 21, 2018

tjhendrickson Jun 21, 2018

chrisgorgo Jun 21, 2018

tjhendrickson Jun 21, 2018

tjhendrickson commented Jun 21, 2018

chrisgorgo commented Jun 21, 2018

tjhendrickson commented Jun 21, 2018

chrisgorgo left a comment

chrisgorgo Jun 22, 2018

chrisgorgo Jun 22, 2018

chrisgorgo Jun 22, 2018

tjhendrickson Jun 22, 2018

chrisgorgo Jun 22, 2018

tjhendrickson Jun 22, 2018

chrisgorgo Jun 22, 2018

tjhendrickson Jun 22, 2018

chrisgorgo Jun 22, 2018

chrisgorgo Jun 22, 2018

tjhendrickson Jun 22, 2018

chrisgorgo Jun 22, 2018

chrisgorgo Jun 22, 2018

chrisgorgo Jun 22, 2018

tjhendrickson commented Jun 22, 2018

		@@ -1 +1 @@
		dev
		v0.3

HCP Pipelines Improvements (ICAFIX, PostFix, RestingStateStats, TaskfMRIAnalysis, londitudinal/cross-sectional logic) #31

Are you sure you want to change the base?

HCP Pipelines Improvements (ICAFIX, PostFix, RestingStateStats, TaskfMRIAnalysis, londitudinal/cross-sectional logic) #31

Conversation

tjhendrickson commented Jun 21, 2018

chrisgorgo commented Jun 21, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tjhendrickson commented Jun 21, 2018

chrisgorgo commented Jun 21, 2018

tjhendrickson commented Jun 21, 2018

chrisgorgo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tjhendrickson commented Jun 22, 2018