InsightSoftwareConsortium / ITK

Insight Toolkit (ITK) -- Official Repository. ITK builds on a proven, spatially-oriented architecture for processing, segmentation, and registration of scientific images in two, three, or more dimensions.
https://itk.org
Apache License 2.0
1.42k stars 664 forks source link

Write to NRRD includes ITK_non_uniform_sampling_deviation key #3940

Open paul-thomson opened 1 year ago

paul-thomson commented 1 year ago

This was tested using SimpleITK 2.1.0 which I believe uses ITK 5.2.0. I don't use ITK directly but I've raised this here because I'd guess the behaviour is coming from ITK.

Steps to reproduce

When a (DICOM) series with non-uniform spacing is loaded, the ImageSeriesReader adds a metadata key "ITK_non_uniform_sampling_deviation" to the loaded image. When this image is written out using NRRD (haven't checked other formats) the NRRD header includes the key with what looks like an incorrect value.

NRRD0004
# Complete NRRD file format specification at:
# http://teem.sourceforge.net/nrrd/format.html
type: short
dimension: 3
space: left-posterior-superior
sizes: 5 4 3
space directions: (2.5,0,0) (0,3.5,0) (0,0,4.5)
kinds: domain domain domain
endian: little
encoding: raw
space origin: (0,0,0)
ITK_non_uniform_sampling_deviation:=

To make it easy to reproduce I have attached some SimpleITK code based on https://simpleitk.readthedocs.io/en/next/Examples/DicomSeriesFromArray/Documentation.html nrrdtest.zip

Expected behaviour

As this is ITK-specific metadata I would not expect it to be written out to the file. If that is intended though I would expect it to at least be written out in a meaningful way.

github-actions[bot] commented 1 year ago

Thank you for contributing an issue! 🙏

Welcome to the ITK community! 🤗👋☀️

We are glad you are here and appreciate your contribution. Please keep in mind our community participation guidelines. 📜 Also, please check existing open issues and consider discussion on the ITK Discourse. 📖

dzenanz commented 1 year ago

@vfonov I think you added this feature. Could you look into this?

vfonov commented 1 year ago

sure, it is strange though. It is supposed to be a simple double value showing standard deviation of slice thicknesses

vfonov commented 1 year ago

So, NRRD writer doesn't properly interpret Metadata records which are not strings and which are not produced by NRRD reader( https://github.com/InsightSoftwareConsortium/ITK/blob/master/Modules/IO/NRRD/src/itkNrrdImageIO.cxx#L1069 ). To fix this behaviour I can change NRRD writer to ignore all records that are not string or make it convert values to string. What would you prefer?

dzenanz commented 1 year ago

I guess that conversion to string would be preferred. @lassoan do you have some insight, or opinions about this?

vfonov commented 1 year ago

Something like this can be done: https://github.com/InsightSoftwareConsortium/ITK/blob/master/Modules/IO/MINC/src/itkMINCImageIO.cxx#L1285

lassoan commented 1 year ago

If we convert that field from float to string when we write to file, then it would be nice to convert from string to float when we read the file. This would require making the NRRD IO aware of this field. It would be simpler to store this field as string metadata, then we would not need any conversion when writing/reading.

Whether we should store this field in the NRRD (and other) file formats is another question. Since it starts with the ITK_ prefix it is clear that it is some additional data for ITK, it probably won't cause any trouble for anyone. However, if this field is not used for anything anywhere then it would be better not to write it out to file. Instead of storing in the metadata dictionary, it could be stored in a member variable of the file reader and then it would not get written into the output file.

vfonov commented 1 year ago

and how do we figure out when to convert and when not to convert? There are also several other potential types: ints, arrays of ints/floats , etc.

lassoan commented 1 year ago

The simplest is if we never convert, but store the metadata as a string. I remember seeing this in NIFTI header reading/writing. I was wondering why all numbers are stored as strings, but now I understand the reason: it allows any IO class to write/read the metadata.

If we want to store the field as float then probably the NRRD IO would need to know about the field. It could be possible to implement some type system in the NRRD writer (e.g., we would save not just the metadata value in the file but also the type) - but this would be quite complicated, probably not necessary.