DIAGNijmegen / rse-panimg

Conversion of medical images to MHA and TIFF.
Apache License 2.0
13 stars 5 forks source link

Restructure DICOM reader to split complex functions up into smaller units #71

Closed nlessmann closed 2 years ago

nlessmann commented 2 years ago

This is a proposal for restructuring the DICOM reader code a bit by splitting the _process_dicom_file() function up into smaller units. In a next step, we can then add tests for the individual functions.

I've moved this from functions into a DicomDataset class, which I personally like better but which is not the style of most of the image builder code - so this could also be changed back to a functional style if that's preferred.

Closes #74

jmsmkn commented 2 years ago

I have a small preference for keeping the functional style. If you're still making changes could you convert it to a draft? If you're not then we can review.

nlessmann commented 2 years ago

My bad! Thanks for the feedback.

nlessmann commented 2 years ago

This is ready for review - I don't have time right now to convert this back to a functional style and to add tests for the individual functions, but this could be done later.

Next to reorganizing the code, this PR adds a test with an X-ray image (our own data, all values set to 1, header checked for sensitive data) and changes the way the pixel data is cast (deducing the appropriate dtype from the first image instead of always casting to int16, inverting intensities of MONOCHROME1 images).

chrisvanrun commented 2 years ago

Still checking but I think the _find_valid_dicom_files() docstring is no longer accurate.

nlessmann commented 2 years ago

Still checking but I think the _find_valid_dicom_files() docstring is no longer accurate.

Thanks, fixed!

nlessmann commented 2 years ago

Thanks for the thorough review and constructive comments, very helpful for me!