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

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

Closed
wants to merge 1 commit into from

Conversation

jpcoutu
Copy link

@jpcoutu jpcoutu commented May 4, 2021

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.

Better 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.
@jpcoutu jpcoutu changed the title Make major modifications towards better support of a variety of datasets Make major modifications to dcm2mnc towards better support of a variety of datasets May 4, 2021
@vfonov
Copy link
Member

vfonov commented May 4, 2021

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

jpcoutu commented May 6, 2021

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

@vfonov
Copy link
Member

vfonov commented May 19, 2021

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

vfonov commented Jun 14, 2021

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

jpcoutu commented Jun 14, 2021

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

vfonov commented Jun 14, 2021

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

@jpcoutu
Copy link
Author

jpcoutu commented Jul 4, 2023

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.

@jpcoutu jpcoutu closed this Jul 4, 2023
@vfonov
Copy link
Member

vfonov commented Jul 6, 2023

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants