dwi preprocessing using metadata #15
Conversation
|
Does this one replace #13? |
|
Yes. |
There was a problem hiding this comment.
Overall looks good, but the code for finding bvecs seem overly complex. Hopefully after this will get implemented: bids-standard/pybids#9 things can be cleaned up (in another PR).
Dockerfile
Outdated
| @@ -52,10 +52,10 @@ ENV PATH /opt/freesurfer/bin:/opt/freesurfer/fsfast/bin:/opt/freesurfer/tktools: | |||
| # Install FSL 5.0.9 | |||
| RUN apt-get update && \ | |||
| apt-get install -y --no-install-recommends curl && \ | |||
| curl -sSL http://neuro.debian.net/lists/trusty.us-ca.full >> /etc/apt/sources.list.d/neurodebian.sources.list && \ | |||
| curl -sSL http://neuro.debian.net/lists/xenial.us-ca.full >> /etc/apt/sources.list.d/neurodebian.sources.list && \ | |||
There was a problem hiding this comment.
Why did you need to change the base Ubuntu distribution?
There was a problem hiding this comment.
Hm. I actually wasn't aware of this change. There wasn't any need - sorry about that.
run.py
Outdated
| @@ -193,7 +194,7 @@ def run_diffusion_processsing(**args): | |||
| 'fMRISurface', 'DiffusionPreprocessing'], | |||
| default=['PreFreeSurfer', 'FreeSurfer', 'PostFreeSurfer', | |||
| 'fMRIVolume', 'fMRISurface', | |||
| 'DiffusionPreprocessing']) | |||
| 'DiffusionPreprocessing', 'TaskfMRIAnalysis']) | |||
There was a problem hiding this comment.
Leftover from another PR? This one should be just about DWI right?
run.py
Outdated
| assert all(len(dwi_dict[k]) for k,v in dwi_dict.items()) | ||
|
|
||
| for dirnum in set(dwi_dict['bval']): | ||
| idxs = { i for k,v in dwi_dict.iteritems() for i in range(0,len(dwi_dict['bval'])) if v[i] == dirnum } |
There was a problem hiding this comment.
Could you add a comment explaining what this line does?
There was a problem hiding this comment.
Line 450: I made a mistake - it's supposed to be
assert all(len(dwi_dict[k]) ==n for k,v in dwi_dict.items())
Line 453: extracts the index values in dwi_dict['bval'] if the value matches "dirnum", which is the number of directions (i.e. 98 or 99). These index values gets used to find the PE directions, dwi files names, etc. in the dictionary.
There was a problem hiding this comment.
If you could add this comment to the code it would be easier for people in the future to understand this code. Thanks!
run.py
Outdated
| for stage, stage_func in dwi_stage_dict.iteritems(): | ||
| if stage in args.stages: | ||
| stage_func() | ||
| except: |
There was a problem hiding this comment.
Please replace this with a more specific catch (narrower in the amount of code it encomapses and types of exceptions it is suppose to catch). Right now it will catch all exceptions making debugging really hard.
run.py
Outdated
| onerun= True | ||
| numruns = {'run-01'} | ||
| if numruns: | ||
| for session in numruns: |
There was a problem hiding this comment.
run is a function - I believe. Would you like me to rename the variable session?
There was a problem hiding this comment.
I think this would make more sense since you are iterating over runs not sessions.
run.py
Outdated
| @@ -485,6 +491,6 @@ def run_diffusion_processsing(**args): | |||
| for stage, stage_func in dwi_stage_dict.iteritems(): | |||
| if stage in args.stages: | |||
| stage_func() | |||
| except: | |||
| except NameError: | |||
| print("You may have missing diffusion data in the positive phase encoding direction.") | |||
There was a problem hiding this comment.
This tra/except statement still encompases a huge chunk of code that could produce "NameError" that does not necessarily mean that there is missing diffusion data in the positive phase encoding direction. This should be made more specific. Furthermore we should exit with a non zero code to indicate that something did not work correctly.
| @@ -55,7 +55,7 @@ RUN apt-get update && \ | |||
| curl -sSL http://neuro.debian.net/lists/trusty.us-ca.full >> /etc/apt/sources.list.d/neurodebian.sources.list && \ | |||
| apt-key adv --recv-keys --keyserver hkp://pgp.mit.edu:80 0xA5D32F012649A5A9 && \ | |||
| apt-get update && \ | |||
| apt-get install -y fsl-core=5.0.9-3~nd14.04+1 && \ | |||
| apt-get install -y fsl-core=5.0.9-1~nd+1+nd16.04+1 && \ | |||
There was a problem hiding this comment.
are those changes still necessary applicable to this PR? (applies to connectome-workbench as well)
Diffusion pre-processing using metadata from sidecar files.