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: Generate the eddy QC report #155

Merged
merged 23 commits into from
Nov 13, 2019

Conversation

MichielCottaar
Copy link
Contributor

This should add the single-subject QC report to the HCP pipelines. I am still testing this locally, but that requires rerunning eddy, so it will probably not be finished before the AHM.

@MichielCottaar
Copy link
Contributor Author

One thing to think about is whether we want the QC report to be in its default location (${subject}/Diffusion/eddy/eddy_unwarped_images.qc/). It might be nice to move it to a location easier to find (e.g. ${subject}/QC/Diffusion/).

Copy link
Contributor

@glasserm glasserm left a comment

Choose a reason for hiding this comment

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

I would move it to an easier to find location. Thanks for the pull request.

@mharms
Copy link
Contributor

mharms commented Oct 30, 2019

It's not technically in the "T1w" space, but I wonder if we shouldn't copy it to ${subject}/T1w/Diffusion/eddylogs since that is a directory that we are already exposing publicly.

@glasserm
Copy link
Contributor

Is it images or a QC report? If it is images, why not resample them to T1w space and do that?

@MichielCottaar
Copy link
Contributor Author

It is just a QC report (no NIFTI files). The report does contain pngs of the brain, which would of course not be showing the brain in T1-weighted space, but that should not be an issue. So I think the eddylogs is a good place to put the report.

Another issue is: there are some text files with the movement, average residual, etc. per volume. Due to the resampling of two volumes into one after eddy, these can no longer be used as confound regressors (that is true both in the original diffusion space and the T1-weighted space). I don't see an obvious fix for this (and of course having the report itself would still be useful anyway).

@mharms
Copy link
Contributor

mharms commented Oct 30, 2019

That said, while eddy_quad is more "report" (with png's) than images, perhaps we should be resampling the cnr maps generated by eddy (--cnr_maps flag) into the T1w space? I believe that @MichielCottaar raised as a possible addition previously?

@MichielCottaar
Copy link
Contributor Author

Transforming some of the output maps from eddy to T1-weighted space (in particular the CNR maps) would be easy to do. I agree with Jesper that it is probably not very useful, but we might as well.

@mharms
Copy link
Contributor

mharms commented Nov 1, 2019

One other random thought: I don't suppose that eddy --json generates an output text file of the form that would be appropriate for the slspec format? If so, then obviously we could use that file as the slspec file for QUAD.

@MichielCottaar
Copy link
Contributor Author

MichielCottaar commented Nov 1, 2019

TBH, it would not be very hard to include a python script to do that conversion from json to slspec ourselves (I don't think eddy does so).

@MichielCottaar MichielCottaar marked this pull request as ready for review November 5, 2019 16:32
@MichielCottaar
Copy link
Contributor Author

Ok, this version works. I've attached the produced QC report for reference.

qc.pdf

What I haven't implemented:

  • passing in slspec (either from user or converted from json)
  • converting CNR maps. There seemed to be a lot of steps to do this properly (first convert to undistorted diffusion space in eddy_postproc, keeping the unwarped version and then converting to Structural space in Diffusion2Structural.sh). It does not seem worth it to me to add so much code for a file that people probably won't use. There are already CNR summary measures in the report.

@glasserm glasserm requested review from coalsont and mharms November 5, 2019 20:23
Copy link
Contributor

@mharms mharms left a comment

Choose a reason for hiding this comment

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

Perhaps I'm missing it, but where does the output of QUAD get copied/moved to T1w/Diffusion/eddylogs ?

@mharms
Copy link
Contributor

mharms commented Nov 5, 2019

Now that we've figured out the spline resampling/masking issues, isn't it just a matter of a couple lines in eddy_postproc.sh and DiffusionToStructural.sh to get the CNR maps into the T1w space? I actually think they would be quite a nice addition.

@glasserm
Copy link
Contributor

glasserm commented Nov 6, 2019

I agree with Mike about the CNR maps.

@MichielCottaar
Copy link
Contributor Author

MichielCottaar commented Nov 7, 2019

Ok, this version appears to work both with and without gradient distortion correction

${FSLDIR}/bin/immv ${datadir}/data ${datadir}/data_warped
# Note: data in the warped directory is eddy-current and suspectibility distortion corrected (via 'eddy'), but prior to gradient distortion correction
# i.e., "data_posteddy_preGDC" would be another way to think of it
mkdir -p ${datadir}/warped
Copy link
Contributor

Choose a reason for hiding this comment

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

If we want to consolidate the code a bit, we could define warpedDir=${datadir}/warped and then use ${warpedDir} throughout the following.


# Transform CNR maps
${CARET7DIR}/wb_command -volume-dilate ${datadir}/warped/cnr_maps_warped.nii.gz $DilateDistance NEAREST ${datadir}/warped/cnr_maps_dilated.nii.gz
${FSLDIR}/bin/applywarp --rel --interp=trilinear -i ${datadir}/warped/cnr_maps_dilated -r ${datadir}/warped/cnr_maps_dilated -w ${datadir}/warped/fullWarp -o ${datadir}/cnr_maps
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you intentionally choose trilinear rather than spline interpolation here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, that should have been spline interpolation

Copy link
Contributor

@mharms mharms left a comment

Choose a reason for hiding this comment

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

Thanks for adding the CNR maps into the T1w space.

Copy link
Contributor

@mharms mharms left a comment

Choose a reason for hiding this comment

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

Looks good to me. Are you running one more quick test, just to make sure the latest changes didn't break anything?

@MichielCottaar
Copy link
Contributor Author

Indeed, the test will probably not finish today, but I'll let you know if it works.

@MichielCottaar
Copy link
Contributor Author

This version seems to work both with and without gradient-distortion correction

@glasserm
Copy link
Contributor

I would merge this when the conflicts are resolved

@mharms
Copy link
Contributor

mharms commented Nov 11, 2019

What conflicts? The PR is reporting that it has no conflicts and merging can be performed automatically.

@glasserm
Copy link
Contributor

This branch cannot be rebased due to conflicts
Rebasing the commits of this branch on top of the base branch cannot be performed automatically due to conflicts encountered while reapplying the individual commits from the head branch.

@coalsont
Copy link
Member

It conflicts for the rebasing merge mode, which I asked him to use. If @MichielCottaar can rebase this branch onto master and manually resolve any conflicts, that should fix it. However, if he has been merging in the latest master at several points, then we may have to use the merge commit mode.

@glasserm
Copy link
Contributor

I am letting Tim handle any PR that does not Rebase and merge automatically since he has asked me not to use the other modes.

@MichielCottaar
Copy link
Contributor Author

Sorry, I did use merge onto master. If you prefer, I could just squash all of the history, which would allow a rebase to work. However, that would hide the history anyway, so I think it makes more sense to just use merge commit mode for this branch.

@coalsont coalsont merged commit 2744b1b into Washington-University:master Nov 13, 2019
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.

5 participants