nipy / heudiconv

Flexible DICOM conversion into structured directory layouts
https://heudiconv.readthedocs.io
Other
237 stars 126 forks source link

[WIP] Handle dcm2niix not compressing nifti files #802

Open octomike opened 2 weeks ago

octomike commented 2 weeks ago

So, we were hit by https://github.com/rordenlab/dcm2niix/issues/124, when converting a very long epi scan.

It seems that:

I added some code that tries to catch these corner cases in convert.py

I think this should also fix #365 and #576.

Tests were only run manually with a dcmconfig file and without, and with source data of mixed file length (>4GB and <4GB). Conversion works fine, validator is happy and scans.tsv now contain .nii and .nii.gz respectively.

But I am very nervous about this, because I assume other side effects by just switch outtype from nii.gz to nii mid conversion.

codecov[bot] commented 2 weeks ago

Codecov Report

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

Project coverage is 81.76%. Comparing base (900ccdc) to head (bed3f11).

Files with missing lines Patch % Lines
heudiconv/convert.py 58.82% 7 Missing :warning:
heudiconv/bids.py 85.71% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #802 +/- ## ========================================== - Coverage 82.48% 81.76% -0.73% ========================================== Files 42 42 Lines 4323 4344 +21 ========================================== - Hits 3566 3552 -14 - Misses 757 792 +35 ```

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

yarikoptic commented 2 weeks ago

Hi @octomike I totally forgot about that

I wonder -- did you try dcm2niix built with -dmyDisableGzSizeLimits as @neurolabusc suggested us to try (in 2017!)? I think I might just try to enable that flag for neurodebian builds of dcm2niix

yarikoptic commented 2 weeks ago

as for this particular PR: have you considered simply adding code which would just add a check that produced by dcm2niix .nii.gz is actually not compressed, and if so -- double check that it is ok'ish (opens up using nibabel and can access last volume? just to ensure that it is not totally borked conversion exiting early with 1... just to be safe), rename/compress from within heudiconv python process using gzip "battery included"?

octomike commented 2 weeks ago

Hi @octomike I totally forgot about that

* [(Silently) produces .nii (not .nii.gz) despite   -z i   switch rordenlab/dcm2niix#124](https://github.com/rordenlab/dcm2niix/issues/124)

I wonder -- did you try dcm2niix built with -dmyDisableGzSizeLimits as @neurolabusc suggested us to try (in 2017!)? I think I might just try to enable that flag for neurodebian builds of dcm2niix

I just did and the warning (Warning: Saving uncompressed data: internal compressor unable to process such large files.) went away, but it still produced uncompressed nifits (silently now).

After digging through the dcm2niix code a litte, it appears as if https://github.com/rordenlab/dcm2niix/commit/3c7f26be#diff-210e732ea7e4240e3ca012e3668da51f0e389a975fbc5b0edab430afbe27e314R9926 disabled pigz compression altogether, but that's the only code path to compress something if this define is set.

What a mess :see_no_evil:

octomike commented 2 weeks ago

rename/compress from within heudiconv python process using gzip "battery included"?

That sounds much more reasonable than passing uncompress nii forward. I'll try to create a different PR with this approach.