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: An initial implementation of SD estimation. #97

Merged
merged 3 commits into from
Jun 8, 2020

Conversation

oesteban
Copy link
Member

@oesteban oesteban commented Mar 24, 2020

Adds a new subworkflow based on FSL TOPUP to integrate SD estimation
for the ds001771 dataset.

Resolves: #92

@pull-assistant
Copy link

pull-assistant bot commented Mar 24, 2020

Score: 0.94

Best reviewed: commit by commit


Optimal code review plan

     feat(TOPUP): an initial implementation of SD estimation.

     fix: add a --sloppy mode for topup to fit in Circle

     enh: use sdcflows.interfaces.fmap.get_trt instead of custom functi...

Powered by Pull Assistant. Last update a05e701 ... cb11a51. Read the comment docs.

@codecov
Copy link

codecov bot commented Apr 13, 2020

Codecov Report

Merging #97 into master will decrease coverage by 1.25%.
The diff coverage is 19.60%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #97      +/-   ##
==========================================
- Coverage   51.37%   50.11%   -1.26%     
==========================================
  Files          20       21       +1     
  Lines        1238     1289      +51     
  Branches      162      169       +7     
==========================================
+ Hits          636      646      +10     
- Misses        590      631      +41     
  Partials       12       12              
Impacted Files Coverage Δ
dmriprep/workflows/fmap/base.py 18.75% <18.75%> (ø)
dmriprep/workflows/base.py 22.72% <33.33%> (+0.37%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6792e96...cb11a51. Read the comment docs.

@pep8speaks
Copy link

pep8speaks commented Apr 22, 2020

Hello @oesteban, Thank you for updating!

Cheers! There are no style issues detected in this Pull Request. 🍻 To test for issues locally, pip install flake8 and then run flake8 dmriprep.

Comment last updated at 2020-06-02 23:43:34 UTC

@oesteban oesteban force-pushed the enh/init-sdc branch 2 times, most recently from e8ec194 to d390517 Compare June 2, 2020 16:17
oesteban added a commit to oesteban/dmriprep that referenced this pull request Jun 2, 2020
oesteban added a commit that referenced this pull request Jun 2, 2020
ENH: Minor refactor reorganizing base workflows, in prep for #97
@@ -92,6 +92,9 @@ ENV FSLDIR="/usr/share/fsl/5.0" \
AFNI_PLUGINPATH="/usr/lib/afni/plugins"
ENV PATH="/usr/lib/fsl/5.0:/usr/lib/afni/bin:$PATH"

COPY .docker/fsl-6.0/bin/topup /usr/share/fsl/5.0/bin/topup
Copy link
Collaborator

Choose a reason for hiding this comment

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

FYI-- if you want to bump up the fsl version (I've recently encountered a number of issues with the neurodebian fsl-5.0 libraries), I've worked out the following:

RUN apt-get update -qq \
    && apt-get install -y --no-install-recommends curl libquadmath0 \
    && curl -sSL https://fsl.fmrib.ox.ac.uk/fsldownloads/fsl-6.0.2-centos7_64.tar.gz | tar xz -C /usr/local \
       --exclude='fsl/doc' \
       --exclude='fsl/data/first' \
       --exclude='fsl/data/atlases' \
       --exclude='fsl/data/possum' \
       --exclude='fsl/src' \
       --exclude='fsl/extras/src' \
       --exclude='fsl/bin/fslview*' \
       --exclude='fsl/bin/FSLeyes' \
       --exclude='fsl/bin/*_gpu*' \
       --exclude='fsl/bin/*_cuda*' \
    && chmod 775 -R /usr/local/fsl/bin \
    && chown -R neuro /usr/local/fsl

It does add some GB to the build though...

Copy link
Member Author

Choose a reason for hiding this comment

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

bumping fsl will definitely break nipype at points... so probably not the rabbit hole I'd like to explore right this minute... I agree in some time we will be updating to 6.0 (and then your code will be very useful).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, probably best to wait then. I wish there was an organized bookmarking system on github so that we could more easily remember to revisit/ priority-mark these kinds of things!

oesteban added 3 commits June 2, 2020 16:43
Adds a new subworkflow based on FSL TOPUP to integrate SD estimation
for the ds001771 dataset.

- [x] Pin niworkflows to current master (while I release 1.2.0rc5
  containing nipreps/niworkflows#503, nipreps/niworkflows#504, which
  are used here).
- [x] Create a new sdc estimation workflow, with the expectation of
  upstreaming it to SDCFlows.
- [x] Implement the barebones of how nipreps/sdcflows#101 could look
  like. Also to be upstreamed to SDCFlows when mature.
- [x] Stick TOPUP from FSL 6.0.3 in the Docker image, since topup from
  FSL 5.0.x is really unstable (for instance, it fails with a
  segmentation fault on the workflow of ds001771)

Resolves: nipreps#92
and fix THP002, and pin latest rc of niworkflows
@oesteban oesteban marked this pull request as ready for review June 3, 2020 00:25
@oesteban
Copy link
Member Author

oesteban commented Jun 3, 2020

Alright, the report with TOPUP-estimated field is here - https://460-233429127-gh.circle-artifacts.com/0/tmp/ds001771/derivatives/dmriprep/sub-36.html

@oesteban oesteban requested a review from dPys June 3, 2020 00:27
@oesteban
Copy link
Member Author

oesteban commented Jun 3, 2020

@dPys
Copy link
Collaborator

dPys commented Jun 8, 2020

@dPys @mattcieslak @josephmje how does this look?

https://460-233429127-gh.circle-artifacts.com/0/tmp/ds001771/derivatives/dmriprep/sub-36.html

Hi @oesteban -- this looks REALLY nice to me. Question-- do the reports need to visualize the inhomogeneity outside of the brain or do you think we should just mask it so that users don't get alarmed by the amount of out-of-brain field inhomogeneity?

@oesteban
Copy link
Member Author

oesteban commented Jun 8, 2020

Question-- do the reports need to visualize the inhomogeneity outside of the brain or do you think we should just mask it so that users don't get alarmed by the amount of out-of-brain field inhomogeneity?

This question is THE QUESTION. On the one hand, I do agree it is alarming (especially to the non-familiar eye). On the other hand, I think it is nice to compare how different methods regularize outside the brain differently. For instance, the traditional FUGUE correction (the infamous epiunwarp.fsl script) extrapolates (constant) the values at brain edges along the PE axis. That produces this kind of "rays":
Screen Shot 2020-02-06 at 2 17 52 PM (1)

Would you mind filing an issue about this at SDCflows?

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.

Implement a base SDC workflow using SDCFlows
3 participants