From e7b504b5cba3b2d2a7ed358ce17252193283e893 Mon Sep 17 00:00:00 2001
From: Ziv Yaniv <zivyaniv@nih.gov>
Date: Fri, 1 Nov 2024 09:12:05 -0400
Subject: [PATCH] ENH: Support both comma and pipe separators in tag
 specification.

DICOM group-element separator can be either comma or pipe, accept
both. Also, when non DICOM and non standard ITK tags are in the metadata
dictionary, issue a single warning per file.
---
 Modules/IO/GDCM/src/itkGDCMImageIO.cxx       | 17 +++++--
 Modules/IO/GDCM/test/itkGDCMImageIOTest2.cxx | 52 ++++++++++++++++++--
 2 files changed, 61 insertions(+), 8 deletions(-)

diff --git a/Modules/IO/GDCM/src/itkGDCMImageIO.cxx b/Modules/IO/GDCM/src/itkGDCMImageIO.cxx
index 0662a4a0f83..70b079448f4 100644
--- a/Modules/IO/GDCM/src/itkGDCMImageIO.cxx
+++ b/Modules/IO/GDCM/src/itkGDCMImageIO.cxx
@@ -865,14 +865,19 @@ GDCMImageIO::Write(const void * buffer)
   gdcm::StringFilter sf;
   sf.SetFile(writer.GetFile());
 
+  std::vector<std::string> problematicKeys;
+
   while (itr != end)
   {
     std::string         value;
     const std::string & key = itr->first; // Needed for bcc32
     ExposeMetaData<std::string>(dict, key, value);
 
-    // Convert DICOM name to DICOM (group,element)
-    bool b = tag.ReadFromPipeSeparatedString(key.c_str());
+    // Convert DICOM name to DICOM (group,element). Corner case
+    // currently ignored, same tag appears twice in the dictionary
+    // once with comma separator and once with pipe. The last one
+    // encountered is the one used to set the tag value.
+    bool b = tag.ReadFromPipeSeparatedString(key.c_str()) || tag.ReadFromCommaSeparatedString(key.c_str());
 
     // Anything that has been changed in the MetaData Dict will be pushed
     // into the DICOM header:
@@ -1008,13 +1013,19 @@ GDCMImageIO::Write(const void * buffer)
       }
       else
       {
-        itkDebugMacro("GDCMImageIO: non-DICOM and non-ITK standard key = " << key);
+        problematicKeys.push_back(key);
       }
     }
 
     ++itr;
   }
 
+  if (!problematicKeys.empty())
+  {
+    itkWarningMacro("ignoring non-DICOM and non-ITK standard keys = "
+                    << problematicKeys[0] +
+                         std::accumulate(problematicKeys.begin() + 1, problematicKeys.end(), std::string(", ")));
+  }
   gdcm::SmartPointer<gdcm::Image> simage = new gdcm::Image;
   gdcm::Image &                   image = *simage;
   image.SetNumberOfDimensions(2); // good default
diff --git a/Modules/IO/GDCM/test/itkGDCMImageIOTest2.cxx b/Modules/IO/GDCM/test/itkGDCMImageIOTest2.cxx
index 7349daabe94..3f0e9b94926 100644
--- a/Modules/IO/GDCM/test/itkGDCMImageIOTest2.cxx
+++ b/Modules/IO/GDCM/test/itkGDCMImageIOTest2.cxx
@@ -59,7 +59,7 @@ itkGDCMImageIOTest2(int argc, char * argv[])
   // reader->GetOutput()->Print(std::cout);
 
   itk::MetaDataDictionary & dict = dicomIO->GetMetaDataDictionary();
-  std::string               tagkey, value;
+  std::string               tagkey, value, commatagkey, commavalue;
   tagkey = "0002|0002";
   value = "1.2.840.10008.5.1.4.1.1.4"; // Media Storage SOP Class UID
   itk::EncapsulateMetaData<std::string>(dict, tagkey, value);
@@ -78,9 +78,9 @@ itkGDCMImageIOTest2(int argc, char * argv[])
   tagkey = "0020|1040"; // Position Reference Indicator
   value = "";
   itk::EncapsulateMetaData<std::string>(dict, tagkey, value);
-  tagkey = "0018|0020"; // Scanning Sequence
-  value = "GR";
-  itk::EncapsulateMetaData<std::string>(dict, tagkey, value);
+  commatagkey = "0018,0020"; // Scanning Sequence
+  commavalue = "GR";
+  itk::EncapsulateMetaData<std::string>(dict, commatagkey, commavalue);
   tagkey = "0018|0021"; // Sequence Variant
   value = "SS\\SP";
   itk::EncapsulateMetaData<std::string>(dict, tagkey, value);
@@ -155,6 +155,48 @@ itkGDCMImageIOTest2(int argc, char * argv[])
 
   ITK_TRY_EXPECT_NO_EXCEPTION(writer->Update());
 
-
+  // Try to read tags from the written file
+  auto outputReader = ReaderType::New();
+  outputReader->SetFileName(writer->GetFileName());
+  try
+  {
+    outputReader->Update();
+  }
+  catch (const itk::ExceptionObject & error)
+  {
+    std::cerr << "Error: exception in file reader " << std::endl;
+    std::cerr << error << std::endl;
+    return EXIT_FAILURE;
+  }
+  auto & inputDict = outputReader->GetOutput()->GetMetaDataDictionary();
+  // DICOM tags can be specified using pipe or comma separators for
+  // writing in the metadata dictionary, but are always read back with
+  // the pipe separator.
+  commatagkey[4] = '|';
+  auto tagIt = inputDict.Find(commatagkey);
+  if (tagIt == inputDict.End())
+  {
+    std::cerr << "Error: Tag " << commatagkey << " expected to be in file but missing" << std::endl;
+    return EXIT_FAILURE;
+  }
+  else
+  {
+    itk::MetaDataObject<std::string>::ConstPointer tagvalue =
+      dynamic_cast<const itk::MetaDataObject<std::string> *>(tagIt->second.GetPointer());
+    if (tagvalue)
+    {
+      if (tagvalue->GetMetaDataObjectValue() != commavalue)
+      {
+        std::cerr << "Error: Written tag value was (" << commavalue << "), read value was ("
+                  << tagvalue->GetMetaDataObjectValue() << ")" << std::endl;
+        return EXIT_FAILURE;
+      }
+    }
+    else
+    {
+      std::cerr << "Error: Tag value for tag (" << commatagkey << ") is missing" << std::endl;
+      return EXIT_FAILURE;
+    }
+  }
   return EXIT_SUCCESS;
 }