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

Unified fieldmap #194

Merged

Conversation

axkralj990
Copy link
Contributor

Hi,

This pull request improves the performance of the distortion correction for Philips B0 map fieldmap scans, by taking the echo difference (deltaTE) into account when computing the fieldmaps. We have also included phase unwrapping and image de-meaning. We followed exactly the same steps as are implemented in the fsl_prepare_fieldmap function for Siemens scanners. The only difference is the additional brain mask erosion step for Philips scans, which results in a cleaner preprocessed fieldmap. A comparison between the current Philips and Siemens steps for fieldmap preparation is shown on the attached figure.
The code has been tested successfully on both structural and functional images from Siemens and Philips scanners. It does not change any other parts of the processing pipelines.

With kind regards,

Grega & Aleksij

Fieldmaps

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.

This seems to have conflicts with master. Please resolve so that this can be merged. Thanks.

@sivcek
Copy link
Contributor

sivcek commented Sep 7, 2020

@glasserm master has been pulled into the unified_fieldmap feature branch and all conflicts resolved. Also, GitHub reports "This branch has no conflicts with the base branch. Merging can be performed automatically." The latest master change that the PR includes was committed on Aug. 25, by @coalsont, "update version.txt".

@glasserm
Copy link
Contributor

glasserm commented Sep 7, 2020

I get this message "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." We may need @coalsont to weigh in on how to fix.

@sivcek
Copy link
Contributor

sivcek commented Sep 7, 2020

@glasserm How about a regular merge? Or a "squash and merge"?

@glasserm
Copy link
Contributor

glasserm commented Sep 7, 2020

Tim has asked that we do rebase and merge, so I'd like his input at this point.

@mharms
Copy link
Contributor

mharms commented Sep 7, 2020

Why the need for the "erode" step specifically in the Philips case? I wonder if Oxford will want to include that in fsl_prepare_fieldmap if it isn't part of the processing for the other platforms...

@axkralj990
Copy link
Contributor Author

I included the erode step for Philips data, since fsl_prepare_fieldmap function calls an additional internal function clean_up_edge() on Siemens data, which reduces the noise at the outer edge of the masked fieldmap image. I followed a recommendation to make the brain mask tighter by excluding the outermost edge of the fieldmap, which usually contains some noise. I've compared a preprocessed Philips B0 map with the modified fsl_prepare_fieldmap function, where clean_up_edge() is called, to a preprocessed scan with an eroded brain mask, and it looks like the former still contained some noise at the rim, despite calling clean_up_edge(). Because the code from this pull request does not have clean_up_edge() functionality and the scans seem to contain quite some noise at the rim, I decided to leave the erosion step in.

@mharms
Copy link
Contributor

mharms commented Sep 8, 2020

@axkralj990 Makes sense. Thanks for the explanation.

@coalsont
Copy link
Member

coalsont commented Sep 8, 2020

Tim has asked that we do rebase and merge, so I'd like his input at this point.

Looks like he pulled the master with a merge commit to resolve conflicts, after all his changes, instead of doing a rebase. I'll look at what the conflicts would be for a rebase.

@coalsont
Copy link
Member

coalsont commented Sep 8, 2020

Looks like the philips_fieldmap branch this is based on has pulled from master, so this is problematic to convert to a rebase. We can do this as a normal merge, but I would prefer that branches intended to become pull requests be rebased on master when you want to catch up, and avoid ever doing a pull merge from master.

@glasserm
Copy link
Contributor

glasserm commented Sep 8, 2020

@coalsont and @mharms if there is no other review needed, I'm fine with Tim merging this.

@mharms
Copy link
Contributor

mharms commented Sep 8, 2020

I'm not planning on any further review.

@coalsont
Copy link
Member

coalsont commented Sep 8, 2020

This is only intended to be in a future 4.3.1 release, right? I haven't had confirmation of the current 4.3.0 candidate completing whatever testing is needed before making it the official release.

@mharms
Copy link
Contributor

mharms commented Sep 8, 2020

Yes. So, we'll need to update version.txt.

@coalsont coalsont merged commit cd8f164 into Washington-University:master Sep 8, 2020
@axkralj990 axkralj990 deleted the unified_fieldmap branch December 15, 2021 10:55
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