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

Remove sdcflows and model ASLPrep on fMRIPrep/next #338

Merged
merged 380 commits into from
Dec 6, 2023

Conversation

tsalo
Copy link
Member

@tsalo tsalo commented Nov 7, 2023

Closes #219, closes #297, closes #346, closes #340, closes #341, and closes #199.

Changes proposed in this pull request

  • Model ASLPrep on fMRIPrep 23.2.0a2.
  • Outsource everything I can to fMRIPrep.
  • New parameters: --bids-database-dir, --level (which is mostly untested), --medial-surface-nan, --project-goodvoxels, --cifti-output, --no-msm, --no-submm-recon, --fs-subjects-dir, --fs-no-reconall, --config-file.

Breaking changes

  • Users must point to the actual output directory. aslprep will no longer be appended automatically.
  • The parameter --anat-derivatives is replaced with --derivatives, which takes one or more paths to anatomical or ASLPrep derivatives.
  • The parameter --dummy-vols is now --dummy-scans to match fMRIPrep.
  • Native-space aslref image split into desc-hmc and desc-coreg versions.
  • Add coverage files for mean CBF and basil CBF parcellations.
  • desc-pvGM_cbf renamed to desc-basilGM_cbf
  • desc-pvWM_cbf renamed to desc-basilWM_cbf
  • desc-confounds_regressors.tsv renamed to desc-confounds_timeseries.tsv
  • from-T1w_to-scanner_mode-image_xfm.txt removed. The inverse is invertible, so it's fine.
  • from-scanner_to-T1w_mode-image_xfm.txt split into from-orig_to-aslref_mode-image_xfm.txt and from-aslref_to-T1w_mode-image_xfm.txt

@tsalo
Copy link
Member Author

tsalo commented Nov 8, 2023

I'm getting the following error now that it's installing a version of niworkflows from GitHub:

Collecting niworkflows@ git+https://github.com/nipreps/niworkflows.git@master
  Cloning https://github.com/nipreps/niworkflows.git (to revision master) to /tmp/pip-install-enby527a/niworkflows_56e388d470f74c29b63b04d7b4f3bdab
  Running command git clone --filter=blob:none --quiet https://github.com/nipreps/niworkflows.git /tmp/pip-install-enby527a/niworkflows_56e388d470f74c29b63b04d7b4f3bdab
The authenticity of host 'github.com (140.82.112.3)' can't be established.

My solution: Use the most recent releases from each of the dependencies and remove anything that requires the pinned GitHub versions (e.g., the MSMSulc elements).

@tsalo
Copy link
Member Author

tsalo commented Nov 10, 2023

Next problem:

/usr/local/miniconda/lib/python3.9/site-packages/nipype/pipeline/engine/workflows.py:628: in run
    execgraph = generate_expanded_graph(deepcopy(flatgraph))
/usr/local/miniconda/lib/python3.9/site-packages/nipype/pipeline/engine/utils.py:967: in generate_expanded_graph
    graph_in = _remove_nonjoin_identity_nodes(graph_in, keep_iterables=True)
/usr/local/miniconda/lib/python3.9/site-packages/nipype/pipeline/engine/utils.py:847: in _remove_nonjoin_identity_nodes
    _remove_identity_node(graph, node)
/usr/local/miniconda/lib/python3.9/site-packages/nipype/pipeline/engine/utils.py:873: in _remove_identity_node
    _propagate_internal_output(graph, node, field, connections, portinputs)
/usr/local/miniconda/lib/python3.9/site-packages/nipype/pipeline/engine/utils.py:947: in _propagate_internal_output
    value = evaluate_connect_function(src[1], src[2], value)
/usr/local/miniconda/lib/python3.9/site-packages/nipype/pipeline/engine/utils.py:697: in evaluate_connect_function
    output_value = func(first_arg, *list(args))
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

lst = <undefined>

>   ???
E   TypeError: '_Undefined' object is not subscriptable

<string>:3: TypeError

So far my debugging process is to mount my local clone of Nipype within run_local_tests.py. I then added some print/raise statements into Nipype to improve the error messages, which let me pin down the problem to

workflow.connect([(inputnode, resample, [(("asl_mask_std", _select_last_in_list), "master")])])

Basically, I had accidentally removed the connection of asl_mask_std from asl_std_trans_wf to compute_cbf_qc_wf, so that input was _Undefined, which the connection function _select_last_in_list could not handle. This would have been much easier to debug if Nipype printed out the connection where the error occurred, or even the connection function's name.

@tsalo
Copy link
Member Author

tsalo commented Nov 11, 2023

The current problem is that fMRIPrep's init_bold_reg_wf (which I'm using now because it's basically the same as ASLPrep's copy) writes out a report file, but the fMRIPrep DerivativesDataSink config file doesn't allow for the asl suffix. I think I have a few options:

  1. Rename the input file(s) so they're called "bold" instead of "asl" to trick the fMRIPrep DerivativesDataSink.
  2. Open an issue in niworkflows about adding asl to the list of valid suffixes for figures.
  3. Set write_report in the init_bold_reg_wf call to False and write out the report within init_asl_preproc_wf instead.
    • This is the one I'll probably go with for this PR, but I think modifying niworkflows' config file is a good idea in the long run.
  4. Open an issue in fMRIPrep about moving some of the more generic workflows (e.g., init_bold_reg_wf, init_bold_t1_trans_wf, init_bold_preproc_trans_wf) from fMRIPrep to niworkflows and making them even more generic.

@tsalo
Copy link
Member Author

tsalo commented Nov 13, 2023

fMRIPrep's init_carpetplot_wf uses the fMRIPrep DerivativesDataSink, which has the same suffix problem I mentioned in #338 (comment). However, unlike init_bold_reg_wf, there is no write_report parameter. It seems like I should open an issue to add asl to the fMRIPrep/niworkflows datasink config file.

@tsalo
Copy link
Member Author

tsalo commented Nov 14, 2023

The carpetplots from fMRIPrep look bad for ASL data because detrend is automatically enabled in the fMRIPrep fMRISummary (which is what is used in init_carpetplot_wf. A few options:

  1. Try using a context manager to swap out the fMRISummary version using by init_carpetplot_wf.
    • Given the simplicity of the workflow, this seems like overkill.
  2. Open an issue in fMRIPrep requesting that init_carpetplot_wf make detrend a parameter.
    • The fMRIPrep devs may not want to do this.
  3. Use a copy of init_carpetplot_wf in ASLPrep. I don't love this idea, but it is a very simple workflow...
    • I'll need an updated clone of fMRISummary.

@tsalo
Copy link
Member Author

tsalo commented Nov 14, 2023

Here's a trick ChatGPT gave me to patch in ASLPrep's DerivativesDataSink into an fMRIPrep workflow with a context manager. I'm not using it for init_carpetplot_wf because another parameter (detrend) is buried too deeply into the workflow to make it useful to call directly.

class OverrideDerivativesDataSink:
    """A context manager for temporarily overriding the definition of SomeClass.

    Parameters
    ----------
    None

    Attributes
    ----------
    original_class (type): The original class that is replaced during the override.

    Methods
    -------
    __init__()
        Initialize the context manager.
    __enter__()
        Enters the context manager and performs the class override.
    __exit__(exc_type, exc_value, traceback)
        Exits the context manager and restores the original class definition.
    """

    def __init__(self, module):
        """Initialize the context manager with the target module.

        Parameters
        -----------
        module
            The module where SomeClass should be overridden.
        """
        self.module = module

    def __enter__(self):
        """Enter the context manager and perform the class override.

        Returns
        -------
        OverrideConfoundsDerivativesDataSink
            The instance of the context manager.
        """
        # Save the original class
        self.original_class = self.module.DerivativesDataSink
        # Replace SomeClass with YourOwnClass
        self.module.DerivativesDataSink = DerivativesDataSink
        return self

    def __exit__(self, exc_type, exc_value, traceback):  # noqa: U100
        """Exit the context manager and restore the original class definition.

        Parameters
        ----------
        exc_type : type
            The type of the exception (if an exception occurred).
        exc_value : Exception
            The exception instance (if an exception occurred).
        traceback : traceback
            The traceback information (if an exception occurred).

        Returns
        -------
        None
        """
        # Restore the original class
        self.module.DerivativesDataSink = self.original_class

Usage:

with OverrideDerivativesDataSink(package.module):
    # do some stuff

@tsalo
Copy link
Member Author

tsalo commented Nov 15, 2023

It looks like fMRIPrep is currently undergoing a massive refactor, and that's going to have upstream impacts on SDCFlows, (probably) niworkflows, and sMRIPrep. Basically everything I'm doing is based on the version prior to this refactor, which unfortunately doesn't have any releases related to it.

SDCFlows has a bunch of backwards-incompatible changes, and I also need to make a bunch of changes to support ASL data because SDCFlows relies on hardcoded suffixes and datatypes.

@tsalo
Copy link
Member Author

tsalo commented Nov 15, 2023

I think I might be stuck.

  1. I can't pin to an older version of SDCFlows because I need to make changes to the codebase to support ASL and M0scan files for fieldmap-less and PEPOLAR distortion correction.
  2. I can't pin to a modified branch on my fork because CircleCI refuses to install dependencies from GitHub repos.
  3. I can update SDCFlows and wait for a new release, but (1) that will delay this PR substantially and (2) I don't have good example code to use because the version of fMRIPrep that uses the new version of SDCFlows doesn't appear to be compatible with ASLPrep (e.g., the fit/apply structure won't work with ASL since we need to calculate CBF and the code was completely rewritten).
    • Actually, since fMRIPrep does include one boldref-space non-BOLD derivatives (i.e., the T2* map for multi-echo data), its new structure may work reasonably well with ASL data.
    • However, other than SDCFlows, fMRIPrep's next branch relies on a bunch of unreleased versions (e.g., niworkflows and smriprep). I can't pin to these versions.

UPDATE:

  1. I've been mounting my SDCFlows branch in my local tests. I'll need to open a PR to add ASL to SDCFlows and wait for a release before this refactor is fully buttoned-up.
  2. I just gave up on this.
  3. Local testing has been a useful workaround. I also ended up fully adopting the fit/apply structure.

@tsalo
Copy link
Member Author

tsalo commented Dec 5, 2023

It looks like the only thing I need to get working is the resampling workflow!

EDIT: Also, there aren't workflow plots for the workflows in aslprep.workflows.asl.outputs or aslprep.workflows.asl.apply.init_asl_cifti_resample_wf.

@tsalo
Copy link
Member Author

tsalo commented Dec 6, 2023

😭 I got an error with the CIFTI resampling:

While running:
        wb_command -volume-to-surface-mapping /src/aslprep/.circleci/work/test_003_full/aslprep_0_6_wf/sub_01_wf/asl_preproc_ses_1_wf/ds_asl_cifti_wf/warp_mean_cbf_to_anat/sub-01_ses-1_asl_DeltaMOrCBF_meancbf_trans.nii.gz /src/aslprep/.circleci/data/anatomical/smriprep/sub-01/anat/sub-01_hemi-L_midthickness.surf.gii sub-01_hemi-L_midthickness.surf_mapped.func.gii -ribbon-constrained /src/aslprep/.circleci/data/anatomical/smriprep/sub-01/anat/sub-01_hemi-L_white.surf.gii /src/aslprep/.circleci/data/anatomical/smriprep/sub-01/anat/sub-01_hemi-L_pial.surf.gii -volume-roi /src/aslprep/.circleci/work/test_003_full/aslprep_0_6_wf/sub_01_wf/asl_preproc_ses_1_wf/asl_cifti_resample_wf/goodvoxels_bold_mask_wf/goodvoxels_mask/sub-01_ses-1_asl_reduced_std_maths_maths_maths_thresh_maths.nii.gz

        ERROR: roi volume is not in the same volume space as input volume

I think I need the working directory to figure out what's going wrong, but that's not really feasible with local testing or using CircleCI, so I'm leaning toward merging this once I have CI passing, building the Singularity image on CUBIC, and testing there.

@tsalo tsalo mentioned this pull request Dec 6, 2023
@tsalo tsalo merged commit 8c80d31 into PennLINC:main Dec 6, 2023
25 checks passed
@tsalo tsalo deleted the remove-sdcflows-2 branch December 6, 2023 19:42
@tsalo tsalo mentioned this pull request Dec 8, 2023
@tsalo tsalo added the nipreps Maintains consistency with the rest of the nipreps ecosystem (especially fMRIPrep). label Jan 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change PRs that change results or interfaces. enhancement New feature or request nipreps Maintains consistency with the rest of the nipreps ecosystem (especially fMRIPrep).
Projects
None yet
2 participants