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

Replace --anatomical-template and --output-resolution with --output-spaces #681

Open
tsalo opened this issue Jan 26, 2024 · 10 comments
Open
Assignees
Labels
breaking-change Issues/PRs that change results or interfaces. enhancement New feature or request
Milestone

Comments

@tsalo
Copy link
Member

tsalo commented Jan 26, 2024

This would bring QSIPrep more in line with other preps, and would maybe make it easier to support a res-native option for cases where resolution varies across acquisitions, sessions, etc.

@mattcieslak pointed out that most recon methods require isotropic voxels, so we might need something like res-nativemax and res-nativemin to use the largest or smallest voxel dimension, respectively.

For example, if voxels are 1.7 x 1.7 x 1.718, res-nativemax would produce outputs with 1.718 x 1.718 x 1.718 voxels and res-nativemin would produce outputs with 1.7 x 1.7 x 1.7 voxels.

@tsalo tsalo added the enhancement New feature or request label Jan 26, 2024
@tsalo tsalo added the breaking-change Issues/PRs that change results or interfaces. label Feb 3, 2024
@tsalo
Copy link
Member Author

tsalo commented Mar 2, 2024

Satra proposed something like res-isoXmm for arbitrary isotropic resolutions in nipreps/fmriprep#2424 (comment).

@tsalo
Copy link
Member Author

tsalo commented Aug 12, 2024

I've opened nipreps/niworkflows#889 to request this on niworkflows' side.

@tsalo
Copy link
Member Author

tsalo commented Nov 18, 2024

We should require ACPC as an output space, and other output spaces won't use the resolution component.

So you could do something like:

--output-spaces ACPC:res-2:res-nativemin:res-nativemax MNI152NLin2009cAsym MNI152NLin6Asym

That would output preprocessed DWI in ACPC with three different resolutions, and would also generate transforms to MNI152NLin6Asym and MNI152NLin2009cAsym.

@tsalo
Copy link
Member Author

tsalo commented Nov 19, 2024

I think this proposal can take over for #620. We could allow other output spaces than ACPC and raise a warning/error if someone wants something that doesn't make sense.

@tsalo tsalo added this to the 1.1.0 milestone Nov 21, 2024
@tsalo
Copy link
Member Author

tsalo commented Nov 21, 2024

A note on infant processing- MNIInfant without a cohort entity should resolve to the appropriate cohort based on the subject age. If the cohort entity is present, then we use that. Maybe we should have an 'auto' option as well? E.g.,

--output-spaces MNIInfant:cohort-1:cohort-auto

That would produce transforms to MNIInfant:cohort-1 and transforms to whatever cohort is most appropriate for the participant based on their age.

@mattcieslak
Copy link
Collaborator

One other thing I was thinking about - what if we removed options for spatial normalization from qsiprep entirely? The --output-spaces argument is misleading because the only space you'll get out is ACPC. It really just computes the registrations to each of those spaces, and those later get used by qsirecon.

It seems like it would make sense to have the normalization happen in qsirecon, where there will actually be scalars getting warped to that output space.

I had this thought during The Great Split, but decided against it because there are some practical problems with it. The first is, if I want to run separate qsirecon workflows on the same input data, I would end up computing the normalization multiple times, which would introduce error/variability across the qsirecon runs. The other issue is that for synb0-disco to work correctly we need a normalization to one of the MNIs.

@tsalo
Copy link
Member Author

tsalo commented Nov 21, 2024

It would simplify the interface and make QSIRecon's data ingression easier. We can always allow QSIRecon to ingress derivatives from previous QSIRecon runs (see PennLINC/qsirecon#111), so the transforms wouldn't need to be recalculated.

I just worry about compatibility with other post-processing workflows. Minimal preprocessing should preferably produce self-contained outputs. It would be good to check with some of the other dMRI folks to make sure QSIPrep derivatives would still be useful without thoe transforms.

@mattcieslak
Copy link
Collaborator

As of now, those transforms are only used for the connectivity matrices. They used to be used for PyAFQ as well, but a recent PyAFQ update made the format incompatible with PyAFQ, so it just recalculates its own transform.

@mattcieslak
Copy link
Collaborator

correction: connectivity matrices and getting the ROIs for the ODF plots in the QSIRecon reports

@tsalo
Copy link
Member Author

tsalo commented Nov 21, 2024

One reason to keep it in QSIPrep is that we can probably integrate sMRIPrep at some point. QSIPrep could do the native-to-ACPC transform and then pass the ACPC-space anats to sMRIPrep for full processing, right? sMRIPrep would produce the transforms to whatever output spaces we might want. Of course that could be done on QSIRecon's side, but I think it makes more sense to run Freesurfer in the preprocessing stage than the reconstruction stage.

This is kind of a dumb question, but how do you get tracts in the same space to look at patterns across subjects? Does that not use the ACPC-to-MNI transform?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change Issues/PRs that change results or interfaces. enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants