Iterating over multiple pairs of PET and T1w files #13
Conversation
There was a problem hiding this comment.
Many thanks for making this PR, @bilgelm! I am still not quite convinced that the updated data grabbing is the most ideal (I would rather try to make a main workflow grabbing subjects, and then a subsequent subject workflow grabbing all sessions. Because there you can grab just a single MR for a given subject, and then use this to iterate over multiple PET sessions. It also accommodates the inheritance principle). Nevertheless, I am quite happy with the remaining parts of your PR, so many thanks for these edits/comments!
petdeface/petdeface.py
Outdated
| ) # better solution would be to add this properly in niworkflows collect_data queries | ||
|
|
||
| # Check that PET and T1w files are matched | ||
| match_error = "Make sure that each PET has a corresponding T1w in the same session." |
There was a problem hiding this comment.
I guess this goes against the inheritance principle, as you will often see just one anat directory above several session directories with PET data in them. Or if only one MRI is present in a test-retest setting, we should just grab that MRI and use it for both PET files.
There was a problem hiding this comment.
I think my latest commit achieves the desired behavior. My testing has been limited, so hopefully the code covers all possible situations 🤞.
There are ambiguous cases. Let's say a subject has t1w for sessions 1, 2, and 4. Which t1w should be used for pet in session 3? I didn't add anything in my code to "break ties" and favor one of these sessions over the others. (In current implementation, all of these t1ws are tied for best match, and the code would pick ses-01_t1w to process ses-03_pet just because it comes first in the list.)
There was a problem hiding this comment.
Awesome - thanks @bilgelm! @bendhouseart we should go ahead and make the test cases to verify that this indeed covers the different cases. Regarding the ambiguous cases, workflows like fmriprep and smriprep go ahead an create a single anatomical image using mri_robust_register if more than one anatomical file is available. Difficult to say what would be the optimal usage here, but I would be fine if there is match within a session between PET and anatomical then use that, and if there is no anatomical present, then use the first available anatomical.
|
I'm happy with this barring the committed changes to the |
|
This also looks good to me! Thank you so much, Murat! We should, however, still move ahead and create the necessary tests and include them in the repo. |
Do you want to merge this or create the tests first? We're going to be limited to really simple unit tests unless we do one or more of the following:
|
Fixes #6 by throwing
RuntimeErrorif there are no T1w images for a given participant.Resolves #8 by implementing
init_single_subject_wf. (This also fixes #5.) I couldn't get this approach to work withcreate_weighted_average_petandcreate_apply_str, so I wrote the wrappersWeightedAverageandApplyMideface. I replacedshutilfile copying with nipype'sDataSink, but only the "derivatives" option ofPetDeface'splacementargument is currently implemented.Minor bug fix for weighted average PET: previous implementation used the sum of mid time points in the denominator, but this should instead be the range of mid time points (i.e., the duration over which TAC is specified). This change should not affect PET-MR coregistration as mutual information is invariant to scaling.
This pull request simplifies the dependency list by omitting redundant modules from
pyproject.toml.This pull request also implements code formatting with black, flake8, and isort, and sets up pre-commit hooks for these formatting checks.