Skip to content

Conversation

mgxd
Copy link
Collaborator

@mgxd mgxd commented May 10, 2023

The anatomical workflow was starting to become a rats nest, with a variety of workflows that may or may not be called depending on the presence of precomputed derivatives (--derivatives). This is an attempt to untangle the workflow into clearer steps. A top-level breakdown of the major reorganization this PR addresses:

Template workflows (T1w / T2w)

Create a canonically-oriented, conformed reference image. If multiple runs are found, intensity-nonuniform (INU) correction is run on a each volume, and are used to create a single volume structural reference. Additionally, if a precomputed mask / aseg are provided (only if a single T1w is found), they are reoriented and conformed as well.

Preproc workflows (T1w/T2w)

Clean up anatomical reference by performing intensity clipping, denoising, and INU correction.

Brain Extraction workflow (T2w)

If a precomputed mask is not available, register the T2w (higher contrast) to a skull-stripping template to create a brainmask.

Coregistration workflow (T2w -> T1w)

Align the T2w to T1w space, and optionally the mask (if the brain extraction workflow was needed).

No remarkable changes to the normalization, segmentation, and surface generation workflows.

@codecov-commenter
Copy link

codecov-commenter commented May 10, 2023

Codecov Report

Patch coverage: 14.17% and project coverage change: +0.11 🎉

Comparison is base (9d070eb) 32.07% compared to head (57f3ca6) 32.18%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #286      +/-   ##
==========================================
+ Coverage   32.07%   32.18%   +0.11%     
==========================================
  Files          53       54       +1     
  Lines        4727     4748      +21     
==========================================
+ Hits         1516     1528      +12     
- Misses       3211     3220       +9     
Impacted Files Coverage Δ
nibabies/workflows/anatomical/segmentation.py 19.23% <0.00%> (+1.37%) ⬆️
nibabies/workflows/anatomical/brain_extraction.py 9.41% <6.66%> (+1.16%) ⬆️
nibabies/workflows/anatomical/template.py 8.88% <8.88%> (ø)
nibabies/workflows/anatomical/registration.py 17.14% <9.09%> (-0.72%) ⬇️
nibabies/workflows/anatomical/base.py 11.39% <18.51%> (+6.05%) ⬆️
nibabies/utils/misc.py 55.35% <21.42%> (-12.09%) ⬇️
nibabies/workflows/anatomical/preproc.py 26.66% <30.76%> (+17.77%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@mgxd mgxd marked this pull request as ready for review May 11, 2023 00:20
@mgxd mgxd merged commit d068bff into master May 11, 2023
@mgxd mgxd deleted the rf/anatomical-processing branch May 11, 2023 01:49
Copy link
Member

@effigies effigies left a comment

Choose a reason for hiding this comment

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

Some initial thoughts. Overall things look quite clean. It might be worth a detailed walkthrough of differences between infant and adult smriprep once MCRIBs is in and see if we can unify.

return os.path.join(base, basename)


def get_file(pkg: str, src_path: Union[str, Path]) -> str:
Copy link
Member

Choose a reason for hiding this comment

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

I like this. But I would consider using a module-local ExitStack and cached responses to avoid unnecessary duplicates.

Example:

import atexit
from contextlib import ExitStack
from functools import cache
try:
    from importlib.resources import as_file, files
except ImportError:
    from importlib_resources import as_file, files

# Module-scoped
exit_stack = ExitStack()
atexit.register(exit_stack.close)

@cache
def get_file(pkg: str, src_path: Union[str, Path]) -> str:
    file_ref = files(pkg) / src_path
    pathlike = exit_stack.enter_context(as_file(file_ref))
    return str(pathlike)

from nipype.interfaces import utility as niu
from nipype.pipeline import engine as pe
from niworkflows.engine.workflows import LiterateWorkflow
from niworkflows.utils.spaces import Reference, SpatialReferences
Copy link
Member

Choose a reason for hiding this comment

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

This can be an expensive import. I might use a from __future__ import annotations to avoid runtime resolution of annotations, and then put this in an if ty.TYPE_CHECKING: block.

Comment on lines +236 to +237
t1w_preproc_wf = init_anat_preproc_wf(name="t1w_preproc_wf")
t2w_preproc_wf = init_anat_preproc_wf(name="t2w_preproc_wf")
Copy link
Member

Choose a reason for hiding this comment

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

Are we making strong assumptions that num_t1w and num_t2w >0? Even if it's true for now, I would consider guarding these.

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.

3 participants