-
Notifications
You must be signed in to change notification settings - Fork 131
[FIX] remove XA partial volumes, generated by dcm2niix as derived data #817
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
base: master
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #817 +/- ##
==========================================
- Coverage 82.48% 82.27% -0.22%
==========================================
Files 42 42
Lines 4323 4344 +21
==========================================
+ Hits 3566 3574 +8
- Misses 757 770 +13 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
could it be some really minimal phantom data, like masterfully interrupted 2 volumes with 1 having 2 slices and another 1? ;) if not -- may be we should add to https://datasets-tests.datalad.org/ -- just share somewhere and I would add there |
@@ -1040,7 +1067,7 @@ def save_converted_files( | |||
safe_movefile(res_files, outname, overwrite) | |||
if isdefined(res.outputs.bids): | |||
try: | |||
safe_movefile(res.outputs.bids, outname_bids, overwrite) | |||
safe_movefile(bids_files, outname_bids, overwrite) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would need to double-check on what is the effect here and why e.g. we did not use bids_files to start with here, or why checking isdefined
for res.output.bids and not bids_files to be defined...
thank you for this @bpinsard . I tried to run it on the dataset that gave the error, but it still fails. This is a multiecho (3 echoes) dataset, and the weird thing is that the partial volumes for echoes 1 and 2 get recognized as partial with your PR, but not echo 3. unfortunately I cannot share the dataset and it will take me a while to get some phantom data |
Thanks for the feedback, I will acquire some phantom multi-echo tomorrow to have test data. Are you using the CMRR sequence? |
yep. thanks @bpinsard ! |
Well obviously removing items from the lists while iterating on it was a bad idea, but was only a problem with more than 2 converted files, such as with multi-echo. Works on the multi-echo phantom data I got this morning. |
I just tried on my dataset, it works great 💯 |
|
||
""" | ||
partial_volumes = [not metadata.get("RawImage", True) for metadata in bids_metas] | ||
no_partial_volumes = not any(partial_volumes) or all(partial_volumes) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry, my had hurts a little trying to do this bool
and need for that or
. Wouldn't it make it easier to talk in terms of
no_partial_volumes = not any(partial_volumes) or all(partial_volumes) | |
has_partial_volumes = any(partial_volumes) |
? (and then flip if
below) or it wouldn't be sufficient?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure if "RawImage":False is also present for other derived data, so I tried to narrow down the case where a series produced niftis with both False and not present (or True if ever).
There might still be derived data that we want to convert.
filtered BIDS metadata | ||
|
||
""" | ||
partial_volumes = [not metadata.get("RawImage", True) for metadata in bids_metas] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so, a partial volume is iff RawImage = False
? i.e. is every volume with RawImage = False
is a partial volume?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's the way that it's implemented in dcm2niix for now, but as mentionned above I am not sure if that BIDS tag is used for other cases such as true derived data.
adding tests data, single-echo to make it smaller. |
Attempt at fixing #814 , following new dcm2niix strategy to fix rordenlab/dcm2niix#742
TODO: