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

SDC-pepolar: topup implementation #76

Closed
wants to merge 5 commits into from

Conversation

markushs
Copy link
Contributor

Here's a solution that works for us (ref #37).

topup.py is a modification of pepolar.py and reuses many of the functions/workflows found in pepolar.py.

Note, I had to decrease the bet threshold in gre.py as the epis (which are used as magnitude inputs to fmap_wf) already got skullstripped in topup/init_prepare_epi_wf. This is probably not the best solution, as the relevant BET node is also used by other workflows with non-skullstripped mag-images. However I see that there's an ongoing discussion about changing the approach to something more robust than BET (#23).

@pep8speaks
Copy link

pep8speaks commented Dec 13, 2019

Hello @markushs, 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 sdcflows.

Comment last updated at 2019-12-18 08:45:03 UTC

@markushs markushs changed the title Topup implementation of PEPOLAR flow SDC-pepolar: topup implementation Dec 13, 2019
@mattcieslak
Copy link
Collaborator

Thank you for starting this PR! I was wondering if you have run into any left-right flips between the fieldmaps and the epi data. Topup in some versions of FSL assumes that the image orientation is LAS+. Since fmriprep uses RAS+ this might be difficult to spot.

@markushs
Copy link
Contributor Author

@mattcieslak I haven't seen any left-right flips, no. And visually inspecting epi data pre-post topup (i.e. distorted epi input and undistorted epi output) + inspecting headers suggest no flips have occured. Any bullet-proof way to test if something like this has occured?

@oesteban
Copy link
Member

I was wondering if you have run into any left-right flips between the fieldmaps and the epi data.

Are you referring to inconsistency between them? i.e., one of them has an x-form header where there is an X-flip and the data are therefore interpreted in reversed way.

Topup in some versions of FSL assumes that the image orientation is LAS+.

Assumes LAS+ or assumes the same orientation information for both images? The former would be really dangerous, the latter would be just unfortunate but understandable from a historical point of view.

@markushs
Copy link
Contributor Author

I did some additional testing, and with our data it looks like topup works better when epi fieldmaps are not skullstripped. The latest commit to my branch reflects this. As the "magnitude" input to fmap.py now contains skull, the frac-threshold in gre.py could be reverted to its original value (0.6).

@oesteban
Copy link
Member

Hi @markushs, is there anything we can do to help you finalize this?

@markushs
Copy link
Contributor Author

Hi @oesteban, have you had a chance to look at the draft? If the approach seems ok I can try to fix the remaining problems (some tests that need to be updated and an issue with the _desc_fieldmap_bold.svg, If I recall correctly). The skullstripping could also be refined, but that seems to be work in progress (ref #23)

Copy link
Member

@oesteban oesteban left a comment

Choose a reason for hiding this comment

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

Getting there! Very looking forward to seeing this finalized :)

Based on the estimated susceptibility distortion, a corrected
EPI (echo-planar imaging) reference was calculated for a more
accurate co-registration with the anatomical reference.
Based on the estimated susceptibility distortion, a corrected EPI (echo-planar imaging) reference
Copy link
Member

Choose a reason for hiding this comment

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

Please rebase or merge with current upstream so differences in this file are real.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As you can see I've opened a new PR (#106 based on the current master). Will respond to your comments here, but the perhaps better to continue the discussion in the new PR? Sorry for the mess, far from fluent in git...

if 'epi' in fmaps:
from .pepolar import init_pepolar_unwarp_wf, check_pes
from .topup import init_topup_wf, check_pes
Copy link
Member

Choose a reason for hiding this comment

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

I don't love the topup name of the module. I would prefer this workflow to be within the pepolar module.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in #106

Comment on lines +97 to +98
echo-planar imaging (EPI) references with opposing phase-encoding
directions, with `Topup`).
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
echo-planar imaging (EPI) references with opposing phase-encoding
directions, with `Topup`).
echo-planar imaging (EPI) references of opposed phase-encoding
directions with `topup` @topup (FSL {fsl_ver}).

We will need to add a topup entry to the bib file, referencing

J.L.R. Andersson, S. Skare, J. Ashburner. How to correct susceptibility distortions in spin-echo echo-planar images: application to diffusion tensor imaging. NeuroImage, 20(2):870-888, 2003.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

FSL version gets returned in #106, but not sure where I find the bib-file?

return workflow


def init_prepare_epi_wf(omp_nthreads, matched_pe=False,
Copy link
Member

Choose a reason for hiding this comment

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

Can you unpack for me how much of this workflow overlaps the existing analog?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In #106 all edits were done in pepolar.py, so should be easier to evaluate the overlap

return inlist


def check_pes(epi_fmaps, pe_dir):
Copy link
Member

Choose a reason for hiding this comment

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

Ditto

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ditto

return matched_pe, matched_pe_dir, opposed_pe_dir


def _split_epi_lists(in_files, pe_dir, max_trs=50):
Copy link
Member

Choose a reason for hiding this comment

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

Ditto

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ditto

Comment on lines +167 to +169
sdc_unwarp_wf = init_sdc_unwarp_wf(
omp_nthreads=omp_nthreads,
debug=debug)
Copy link
Member

Choose a reason for hiding this comment

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

This is not a responsibility of this workflow. Instead, the warpfield should be converted to an ITK-type of transform and generate a new (unwarped) reference, as in:

to_ants = pe.Node(niu.Function(function=_fix_hdr), name='to_ants',
mem_gb=0.01)
cphdr_warp = pe.Node(CopyHeader(), name='cphdr_warp', mem_gb=0.01)
unwarp_reference = pe.Node(ANTSApplyTransformsRPT(dimension=3,
generate_report=False,
float=True,
interpolation='LanczosWindowedSinc'),
name='unwarp_reference')
enhance_and_skullstrip_bold_wf = init_enhance_and_skullstrip_bold_wf(
omp_nthreads=omp_nthreads)

But that's going to be tricky while https://github.com/poldracklab/nitransforms does not support FSL's bspline parametric fields.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure what you mean by this not being a responsibility of this workflow. I tried to summarize the approach in the comment to (#106), but essentially the topup-estimated fieldmap is fed into the workflows of the fmap-module.

@markushs
Copy link
Contributor Author

Hi,
Sorry for the delay here (has been crazy times...)
I have some time now to finalize this, if it's still of interest @oesteban?
As I have to re-familiarize myself with the code (including my own) I was wondering if it would be cleaner to open a new PR based on the current master?

@oesteban
Copy link
Member

I'll have a closer look later today or tomorrow. Since this PR seemed a bit abandoned, I made some progress on this front here: https://github.com/nipreps/dmriprep/pull/97/files. Hopefully, we can find the common points of that PR to dmriprep and this one to come up with a solution that works well regardless of the input dataset.

@oesteban
Copy link
Member

Let's move the conversation to #106.

@oesteban oesteban closed this Nov 18, 2020
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.

4 participants