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

fix(brain_extraction): Remove duplicated code, problematic deviation from antsBrainExtraction #931

Merged
merged 7 commits into from
Mar 7, 2025

Conversation

effigies
Copy link
Member

@effigies effigies commented Mar 3, 2025

As discussed in #928, running a masked N4 before calling Atropos can lead to failures, although I don't fully understand why. Nonetheless, passing the output of the first N4 correction resolves the issue in the case I'm looking at as well as brings the workflow into line with antsBrainExtraction.sh, which does not do this step.

Furthermore, if atropos_wf is called, then N4 is called using the white matter mask at the end, which means this is duplicated effort. This PR rearranges the workflow so that the second top-level N4 is only called if atropos_wf isn't created. It also shifts from the pattern:

wf.connect(A)
if condition:
  wf.disconnect(A)
  wf.connect(B)

to:

if condition:
  wf.connect(B)
else:
  wf.connect(A)

I personally find this logic easier to follow and compare between the conditions.

One thing that I discovered was that the lap_{tmpl,target} and mrg_{tmpl,target} nodes were defined twice, but only connected if use_laplacian. Here we always define and connect mrg_{tmpl,target}, and only create and connect lap_{tmpl,target} if use_laplacian.

Copy link

codecov bot commented Mar 3, 2025

Codecov Report

Attention: Patch coverage is 92.30769% with 2 lines in your changes missing coverage. Please review.

Project coverage is 71.12%. Comparing base (b55d82d) to head (36e5955).
Report is 11 commits behind head on master.

Files with missing lines Patch % Lines
niworkflows/anat/ants.py 84.61% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #931      +/-   ##
==========================================
+ Coverage   69.37%   71.12%   +1.74%     
==========================================
  Files          86       87       +1     
  Lines        8474     8477       +3     
  Branches     1057     1057              
==========================================
+ Hits         5879     6029     +150     
+ Misses       2373     2220     -153     
- Partials      222      228       +6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@effigies
Copy link
Member Author

effigies commented Mar 5, 2025

@mgxd I would appreciate a review here.

Copy link
Contributor

@mgxd mgxd left a comment

Choose a reason for hiding this comment

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

Looks reasonable to me - tests inspire confidence.

I personally find this logic easier to follow and compare between the conditions.
Agreed, though I noticed there are still 2 disconnects remaining. Do you want to tackle that refactor in this PR as well?

effigies added 2 commits March 6, 2025 07:17
antsBrainExtraction.sh passes the original N4 corrected image to
Atropos, where we have been running a masked N4 before Atropos.
We have examples where this difference leads to failures that do not
occur in the original ANTs workflow. Further, if we do run Atropos,
then this result is discarded and N4 is run using a white-matter mask.

This patch therefore simply passes the original N4 image to the Atropos
workflow, but otherwise leaves the workflow unchanged.
@effigies effigies force-pushed the fix/brain-extraction-flow branch from 0ba6838 to f8a20dc Compare March 6, 2025 12:46
@effigies
Copy link
Member Author

effigies commented Mar 6, 2025

Agreed, though I noticed there are still 2 disconnects remaining. Do you want to tackle that refactor in this PR as well?

Sure. I also changed inu_n4_final to inu_n4 in the workflow where it's only done once.

@effigies effigies force-pushed the fix/brain-extraction-flow branch from f8a20dc to 5aa974d Compare March 6, 2025 13:06
@effigies effigies force-pushed the fix/brain-extraction-flow branch from 5aa974d to 4483782 Compare March 6, 2025 13:10
Comment on lines 436 to 445
# Check ANTs version
try:
inu_n4_final.inputs.rescale_intensities = True
except ValueError:
warn(
"N4BiasFieldCorrection's --rescale-intensities option was added in ANTS 2.1.0 "
f'({inu_n4.interface.version} found.) Please consider upgrading.',
UserWarning,
stacklevel=1,
)
Copy link
Member Author

Choose a reason for hiding this comment

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

Rebased to re-add this check, which was removed in passing. If we want to remove these, that can be a separate PR.

@effigies
Copy link
Member Author

effigies commented Mar 7, 2025

@mgxd Good to go?

Copy link
Contributor

@mgxd mgxd left a comment

Choose a reason for hiding this comment

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

very minor commment, LGTM

Co-authored-by: Mathias Goncalves <[email protected]>
@effigies effigies merged commit 2a43461 into nipreps:master Mar 7, 2025
7 of 10 checks passed
@effigies effigies deleted the fix/brain-extraction-flow branch March 7, 2025 15:59
effigies added a commit that referenced this pull request Mar 20, 2025
1.13.0 (March 20, 2025)

New feature release in the 1.13.x series.

This release restructures the ANTs brain extraction workflow.
This should be a backwards compatible change, but a minor release is used out of caution.

* FIX: Remove duplicated code, problematic deviation from antsBrainExtraction (#931)
* FIX: Listify numbers (#925)
* ENH: Allow + symbol in label entities (#926, #927)
* STY: Apply various ruff checks (#913, #915, #917, #918, #919, #930)
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.

2 participants