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

Support output spaces to arbitrary templates and cohorts #184

Merged
merged 2 commits into from
Nov 21, 2024

Conversation

mattcieslak
Copy link
Contributor

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
Copy link
Member

tsalo commented Nov 21, 2024

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

@mattcieslak
Copy link
Contributor Author

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

@tsalo
Copy link
Member

tsalo commented Nov 21, 2024

Oh I see that now:

for name, query in queries.items():
files = layout.get(
return_type="file",
subject=subject_id,
session=session_id,
**query,
)
if len(files) == 1:
anat_data[name] = files[0]
elif len(files) > 1:
files_str = "\n\t".join(files)
raise ValueError(
f"More than one {name} found.\nFiles found:\n\t{files_str}\nQuery: {query}"
)

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
Copy link
Contributor Author

I'm happy with that error message!

@tsalo
Copy link
Member

tsalo commented Nov 21, 2024

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
Copy link

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 ⚠️
qsirecon/interfaces/scalar_mapping.py 33.33% 2 Missing ⚠️
qsirecon/utils/bids.py 66.66% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            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     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.


🚨 Try these New Features:

@mattcieslak
Copy link
Contributor Author

In my local testing with the cohorts it still finds them

@tsalo
Copy link
Member

tsalo commented Nov 21, 2024

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

@mattcieslak
Copy link
Contributor Author

Right, the query finds it while ignoring the cohort part

@tsalo
Copy link
Member

tsalo commented Nov 21, 2024

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
Copy link
Contributor Author

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
Copy link
Member

tsalo commented Nov 21, 2024

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.

Copy link
Member

@tsalo tsalo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think things will change once we let users specify the output spaces they want, but this is a huge step in that direction. Everything looks good to me!

@mattcieslak mattcieslak merged commit 619341b into main Nov 21, 2024
25 checks passed
@mattcieslak mattcieslak deleted the fix/generic-output-space branch November 21, 2024 22:11
@tsalo tsalo added the enhancement New feature or request label Nov 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants