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.38k stars 661 forks source link

BUG: Missing modality tag when reading meta-image format. #4549

Closed zivy closed 3 months ago

zivy commented 3 months ago

The modality of a meta image is not stored as part of the metadata dictionary and was not copied into the ITK metadata dictionary. This change addresses the issue.

N-Dekker commented 3 months ago

Thanks @zivy I'm sorry I'm not very familiar with this part of ITK! So a few questions:

zivy commented 3 months ago

Hello @N-Dekker,

The "Modality" tag is treated in a unique manner by the metaIO module, converted from string and stored as enum here, so it is separate from the additional read fields stored here. As a result a MetaImage always has a modality (often MET_MOD_UNKNOWN) (see values here) and we can expect to always get a value even if it just means "don't know".

I have no idea why the modality was singled out in this way, possibly @aylward may remember though looks like the decision was made 22 years ago which is amazing in terms of software longevity but the rational may be lost in the mists of time :foggy: .

aylward commented 3 months ago

The basis was that every image has a modality. So, it shouldn't be optional meta data.

On Thu, Mar 28, 2024 at 4:25 PM Ziv Yaniv @.***> wrote:

Hello @N-Dekker https://github.com/N-Dekker,

The "Modality" tag is treated in a unique manner by the metaIO module, converted from string and stored as enum here https://github.com/InsightSoftwareConsortium/ITK/blob/1780a31476d63a3f67b97c50037bf8880224f3b9/Modules/ThirdParty/MetaIO/src/MetaIO/src/metaImage.h#L354, so it is separate from the additional read fields stored here https://github.com/InsightSoftwareConsortium/ITK/blob/1780a31476d63a3f67b97c50037bf8880224f3b9/Modules/ThirdParty/MetaIO/src/MetaIO/src/metaObject.h#L44. As a result a MetaImage always has a modality (often MET_MOD_UNKNOWN) (see values here https://github.com/InsightSoftwareConsortium/ITK/blob/1780a31476d63a3f67b97c50037bf8880224f3b9/Modules/ThirdParty/MetaIO/src/MetaIO/src/metaImageTypes.h#L22-L40) and we can expect to always get a value even if it just means "don't know".

I have no idea why the modality was singled out in this way, possibly @aylward https://github.com/aylward may remember though looks like the decision was made 22 years ago which is amazing in terms of software longevity but the rational may be lost in the mists of time 🌁 .

— Reply to this email directly, view it on GitHub https://github.com/InsightSoftwareConsortium/ITK/pull/4549#issuecomment-2026053260, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACEJLYWPPOC2ASGU3WFYR3Y2R4DJAVCNFSM6AAAAABFNJTK2OVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMRWGA2TGMRWGA . You are receiving this because you were mentioned.Message ID: @.***>

-- Stephen R. Aylward, Ph.D. Chair, MONAI Advisory Board Senior Director, Strategic Initiatives, Kitware

nicolascedilnik commented 3 months ago

For reference, this is the post that triggered this response. The problem is not necessarily that it is not part of the metadata dict, but rather that there is no method to actually get this info after reading the image, at least in SimpleITK.

N-Dekker commented 3 months ago

The basis was that every image has a modality. So, it shouldn't be optional meta data.

Thanks for your feedback, Stephen. But isn't the concept of image modality very specific to medical images? And isn't the meta image format more generally applicable, also outside the medical field?

I noticed that the list of supported modalities is very short: CT, MR, NM, US, and OTHER https://github.com/InsightSoftwareConsortium/ITK/blob/1780a31476d63a3f67b97c50037bf8880224f3b9/Modules/ThirdParty/MetaIO/src/MetaIO/src/metaImageTypes.h#L24-L29

Compared to the list of 51 modalities listed as Acquisition Modality: https://dicom.nema.org/MEDICAL/Dicom/current/output/chtml/part16/sect_CID_29.html

In order to preserve the value of an image modality, I think it would be better to store it as a text string. That would allow MetaIO to be future prove.

aylward commented 3 months ago

Meh. Seems like this should be an enum, unless we adopt a standard naming convention and check that it is being followed...which is then equivalent to an enum.

It is easy to add modalities to the enum list, and that approach maintains backward compatibility. MetaIO is primarily medical...but we could add others to the enum list. As @N-Dekker pointed out, DICOM is already using an enum list - so we should adopt it.

Also, if folks want to add something not in the enum list, the expected way is to mark it as "OTHER" and then create a user-defined tag that specifies what is meant by other. This handles the 1% case...allowing application-specific use in those rare cases.

aylward commented 3 months ago

One thing...these changes should go in the MetaIO repo, not in ITK. The MetaIO repo is automatically pulled into ITK using a script. If the changes aren't upstreamed, they will be overwritten with the next update.

N-Dekker commented 3 months ago

Thanks for reviewing, @aylward I don't think this PR is meant to change Kitware's MetaIO library. It only adjusts the ITK wrapper, itk::MetaImageIO.

aylward commented 3 months ago

Ah. My mistake. Approved!

Stephen R. Aylward, Ph.D. Chair, MONAI Advisory Board Senior Director, Strategic Initiatives, Kitware

On Fri, Mar 29, 2024 at 12:38 PM Niels Dekker @.***> wrote:

Thanks for reviewing, @aylward https://github.com/aylward I don't think this PR is meant to change Kitware's MetaIO https://github.com/Kitware/MetaIO library. It only adjusts the ITK wrapper, itk::MetaImageIO.

— Reply to this email directly, view it on GitHub https://github.com/InsightSoftwareConsortium/ITK/pull/4549#issuecomment-2027466103, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACEJL65LHHUNVRAJMC3LMDY2WDHZAVCNFSM6AAAAABFNJTK2OVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMRXGQ3DMMJQGM . You are receiving this because you were mentioned.Message ID: @.***>

zivy commented 3 months ago

@aylward thanks for approving, will merge the fix now.