Remove sdcflows and model ASLPrep on fMRIPrep/next #338

tsalo · 2023-11-07T21:54:15Z

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 · 2023-11-08T16:27:56Z

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 · 2023-11-10T15:58:48Z

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

aslprep/aslprep/workflows/asl/qc.py

Line 182 in 3a34405

    
           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 · 2023-11-11T14:50:55Z

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:

Rename the input file(s) so they're called "bold" instead of "asl" to trick the fMRIPrep DerivativesDataSink.
Open an issue in niworkflows about adding asl to the list of valid suffixes for figures.
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.
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 · 2023-11-13T16:24:25Z

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 · 2023-11-14T14:18:32Z

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:

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.
Open an issue in fMRIPrep requesting that init_carpetplot_wf make detrend a parameter.
- The fMRIPrep devs may not want to do this.
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 · 2023-11-14T14:56:56Z

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 · 2023-11-15T14:48:49Z

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 · 2023-11-15T15:27:07Z

I think I might be stuck.

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.
I can't pin to a modified branch on my fork because CircleCI refuses to install dependencies from GitHub repos.
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:

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.
I just gave up on this.
Local testing has been a useful workaround. I also ended up fully adopting the fit/apply structure.

The Philips M0 scan has values of zero for some in-mask voxels, which, when used to calculate CBF, results in Infs.

tsalo · 2023-12-05T21:37:52Z

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 · 2023-12-06T17:58:42Z

😭 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 added 22 commits November 15, 2023 12:39

Try this.

e2e9ee0

keep working.

3f6f077

Update plotting.py

76ad6f9

Update base.py

4833b89

Update base.py

8fa2fd2

Update.

fc854ab

Update plotting.py

6b9677d

Update plotting.py

abc4e99

fix.

1f079de

Update plotting.py

7c9ef8b

Update plotting.py

4a574e2

Update plotting.py

f3451ee

Update.

1b1927f

Update cbf.py

134f628

Update cbf.py

ec2063b

Update cbf.py

deab5f3

Update cbf.py

54539a6

Set Infs to 0.

fa8dc13

The Philips M0 scan has values of zero for some in-mask voxels, which, when used to calculate CBF, results in Infs.

Try just using standard ASL preproc pipeline.

0210e74

Try that.

3ce5d5b

Update tests.py

cf48a13

Update outputs.py

a415c2f

tsalo added 9 commits December 5, 2023 10:49

Remove another unused function.

0efc264

Remove unused interface function.

83bb639

Bump anatomical test data version.

7103770

Fix import.

983f83e

Update expected outputs.

980c523

Create expected_outputs_qtab.txt

ac08292

Grab MNI152NLin2009cAsym xfm for resampling.

f4582e5

Update expected outputs.

6bca96d

Add topup to boilerplate BibTeX file.

2207e4b

tsalo added 10 commits December 5, 2023 16:55

Revert some changes.

7ffa99d

Update expected outputs.

4aa560d

Move asl_anat_wf from resampling to full.

9636209

Try outputting CIFTI for one test.

9b456b6

Add updated fsLR workflow from fMRIPrep.

6885ac8

Update resampling.py

63c7b7a

Speed up test_003.

2029334

Use internal version of init_bold_surf_wf.

d327413

Fix style issues.

55888a6

Remove unused script.

df33d20

This was referenced Dec 6, 2023

Support GIFTI CBF outputs #356

Closed

Replace sdcflows calls with dependency calls #219

Closed

Fix typos.

1fc6288

Drop CIFTI test and sort expected outputs.

d3b027c

tsalo mentioned this pull request Dec 6, 2023

CBF CIFTI project failing #360

Closed

tsalo merged commit 8c80d31 into PennLINC:main Dec 6, 2023
25 checks passed

tsalo deleted the remove-sdcflows-2 branch December 6, 2023 19:42

tsalo mentioned this pull request Dec 8, 2023

Freesurfer for cifti #179

Closed

tsalo added the nipreps Maintains consistency with the rest of the nipreps ecosystem (especially fMRIPrep). label Jan 15, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove sdcflows and model ASLPrep on fMRIPrep/next #338

Remove sdcflows and model ASLPrep on fMRIPrep/next #338

tsalo commented Nov 7, 2023 •

edited

Loading

tsalo commented Nov 8, 2023 •

edited

Loading

tsalo commented Nov 10, 2023

tsalo commented Nov 11, 2023 •

edited

Loading

tsalo commented Nov 13, 2023

tsalo commented Nov 14, 2023 •

edited

Loading

tsalo commented Nov 14, 2023 •

edited

Loading

tsalo commented Nov 15, 2023

tsalo commented Nov 15, 2023 •

edited

Loading

tsalo commented Dec 5, 2023 •

edited

Loading

tsalo commented Dec 6, 2023 •

edited

Loading

Remove sdcflows and model ASLPrep on fMRIPrep/next #338

Remove sdcflows and model ASLPrep on fMRIPrep/next #338

Conversation

tsalo commented Nov 7, 2023 • edited Loading

Changes proposed in this pull request

Breaking changes

tsalo commented Nov 8, 2023 • edited Loading

tsalo commented Nov 10, 2023

tsalo commented Nov 11, 2023 • edited Loading

tsalo commented Nov 13, 2023

tsalo commented Nov 14, 2023 • edited Loading

tsalo commented Nov 14, 2023 • edited Loading

tsalo commented Nov 15, 2023

tsalo commented Nov 15, 2023 • edited Loading

tsalo commented Dec 5, 2023 • edited Loading

tsalo commented Dec 6, 2023 • edited Loading

tsalo commented Nov 7, 2023 •

edited

Loading

tsalo commented Nov 8, 2023 •

edited

Loading

tsalo commented Nov 11, 2023 •

edited

Loading

tsalo commented Nov 14, 2023 •

edited

Loading

tsalo commented Nov 14, 2023 •

edited

Loading

tsalo commented Nov 15, 2023 •

edited

Loading

tsalo commented Dec 5, 2023 •

edited

Loading

tsalo commented Dec 6, 2023 •

edited

Loading