PennLINC / qsirecon

Reconstruction of preprocessed q-space images (dMRI)
https://qsirecon.readthedocs.io/
BSD 3-Clause "New" or "Revised" License
5 stars 3 forks source link

Support output spaces to arbitrary templates and cohorts #184

Closed mattcieslak closed 4 days ago

mattcieslak commented 4 days ago

The HBCD data will be registered to MNIInfant+cohort templates. We want to make sure these file names are used in the qsirecon scalar mappers

tsalo commented 4 days ago

This looks like a really clean solution. Do you know what would happen if there were multiple transforms available?

mattcieslak commented 4 days ago

there would be a crash. Probably better to exit with an error than let it crash downstream?

tsalo commented 4 days ago

Oh I see that now:

https://github.com/PennLINC/qsirecon/blob/0b53f5338e635e307d3b21d1f91b9e6ea7328eb6/qsirecon/utils/bids.py#L204-L217

Users would get a pretty clear error message. Something like:

More than one acpc_to_template_xfm found.
Files found:
    /path/to/sub-01_from-ACPC_to-MNIInfant+1_mode-image_xfm.h5
    /path/to/sub-01_from-ACPC_to-MNIInfant+2_mode-image_xfm.h5
Query: {'datatype': 'anat', 'from': ['T1w', 'T2w', 'ACPC'], 'to': 'MNI152NLin2009cAsym', 'mode': 'image', 'suffix': 'xfm', 'extension': '.h5'}
mattcieslak commented 4 days ago

I'm happy with that error message!

tsalo commented 4 days ago

Actually, since the to value is set by QSIRecon as either MNI152NLin2009cAsym (default) or MNIInfant (infant mode), I think (1) you wouldn't get an error and (2) you won't catch MNIInfant+<cohort>. Does that sound right?

codecov-commenter commented 4 days ago

Codecov Report

Attention: Patch coverage is 46.66667% with 8 lines in your changes missing coverage. Please review.

Project coverage is 47.90%. Comparing base (0b53f53) to head (f63f7ea).

Files with missing lines Patch % Lines
qsirecon/interfaces/anatomical.py 20.00% 4 Missing :warning:
qsirecon/interfaces/scalar_mapping.py 33.33% 2 Missing :warning:
qsirecon/utils/bids.py 66.66% 1 Missing and 1 partial :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #184 +/- ## ========================================== + Coverage 47.64% 47.90% +0.26% ========================================== Files 57 56 -1 Lines 7231 7198 -33 Branches 986 978 -8 ========================================== + Hits 3445 3448 +3 + Misses 3581 3544 -37 - Partials 205 206 +1 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.


🚨 Try these New Features:

mattcieslak commented 4 days ago

In my local testing with the cohorts it still finds them

tsalo commented 4 days ago

Oh... maybe that's related to how parse_file_entities doesn't find the +<cohort> in the entities?

mattcieslak commented 4 days ago

Right, the query finds it while ignoring the cohort part

tsalo commented 4 days ago

I worry that's more of a convenient bug than a feature, but I also think we'll have plenty of notice if pybids starts parsing these plus signs correctly and searching 'MNIInfant' stops finding 'MNIInfant+1' etc.

mattcieslak commented 4 days ago

We'll definitely need to add a test for MNIInfant inputs, but it will be a big task to get simulated data that looks like HBCD

tsalo commented 4 days ago

I think SDCFlows has a few tests where they just generate a dataset layout from a dictionary and make sure that the queries work as expected. We could probably add something like that just to make sure the collection steps are working.