search

BIC-MNI / minc-tools

Basic minc-tools from former minc repository
Other
29 stars 25 forks source link

Make major modifications to dcm2mnc towards better support of a variety of datasets #113

Closed jpcoutu closed 1 year ago

jpcoutu commented 3 years ago

Better conversion support of various PET datasets, multi-echo acquisitions, mosaic DICOM datasets, GEMS diffusion MRI datasets, Philips ASL datasets, Siemens private headers, slice order of fMRI datasets, RealWorldValue rescaling and Philips private header quantitative rescaling.

As mentioned on the MINC-development mailing list, this is the result of 4 years of internal development on dcm2mnc (with contributions from Massine Yahia, Dany Chagnon, Jiaxing Li and others) that we couldn't feed back in a more piecemeal approach. This fixes a wide range of issues, and offering this as take it or leave it, but happy to answer questions, and feel free to modify as you see fit.

vfonov commented 3 years ago

Thank you for the contribution! Do you happen to have examples of the dicom files that are now supported ? Even better would be to add some unit tests for dcm2mnc

jpcoutu commented 3 years ago

Unfortunately the DICOM data and associated unit/regression tests are proprietary and we are unable to share them.

vfonov commented 3 years ago

Well, this change did not help with the sample dataset I got from @cmadjar: output of old version on the left, new version is on the right.

Screenshot at 2021-05-18 23-27-40

Screenshot from 2021-05-18 23-49-24

vfonov commented 3 years ago

I don't feel comfortable making a big change to the crucial minc tool without proper testing. Currently there are no tests for dcm2mnc. I started building a dataset of DICOM images that could be used for testing , but it is taking time. So, I suggest that interested parties, @cmadjar for example, help in creating a test suite for this tool.

jpcoutu commented 3 years ago

I looked again through our internal regression test dataset and a small subset of data we use are public and can be found here if that helps (not only under that section, but elsewhere on that page as well): https://www.nitrc.org/plugins/mwiki/index.php/dcm2nii:MainPage#Archival_MRI However, I don't recall if we ended up fixing dcm2mnc to handle some of these, or whether we simply used it to make sure we did not introduce any bugs.

vfonov commented 3 years ago

Having a dataset to test is good, now somebody actually have to make regression tests.

jpcoutu commented 1 year ago

Closing this PR as it does introduce issues with the existing multi-frame conversion. We have corrected those issues internally and improved support for multi-frame conversion. I can open a new PR if desired but it would come with the same caveat as before, without test data, and with more changes than this PR.

vfonov commented 1 year ago

well, create a pull request, maybe sometime in the future somebody will create a proper test set to run regression tests.