ENH: Let `MetaDataDictionary` support non-equality-comparable data by N-Dekker · Pull Request #3307 · InsightSoftwareConsortium/ITK

N-Dekker · 2022-03-17T10:53:15Z

No longer require operator== for the types of values stored in itk::MetaDataDictionary.

Follow-up to:
pull request #2246
commit 694bbc0
"ENH: Add operator==, operator!= to MetaDataObject and MetaDataDictionary"

Follow-up to: pull request InsightSoftwareConsortium#2246 commit 694bbc0 "ENH: Add operator==, operator!= to MetaDataObject and MetaDataDictionary"

N-Dekker · 2022-03-17T12:17:59Z

@Leengit Please have a look at my SFINAE 😸 You know, I always try to avoid it, but this time I just could not resist the magic of Substitution Failure Is Not An Error 😄 What do you think?

N-Dekker · 2022-03-17T16:25:05Z

Hmmm... 🤔 this pull request may not add much if we only want to support the MetaDataObject template instantiations declared at

ITK/Modules/Core/Common/include/itkMetaDataObject.h

Lines 298 to 321 in 24e7b45

    
           extern template class ITKCommon_EXPORT_EXPLICIT MetaDataObject<bool>; 
        
           extern template class ITKCommon_EXPORT_EXPLICIT MetaDataObject<unsigned char>; 
        
           extern template class ITKCommon_EXPORT_EXPLICIT MetaDataObject<char>; 
        
           extern template class ITKCommon_EXPORT_EXPLICIT MetaDataObject<signed char>; 
        
           extern template class ITKCommon_EXPORT_EXPLICIT MetaDataObject<unsigned short>; 
        
           extern template class ITKCommon_EXPORT_EXPLICIT MetaDataObject<short>; 
        
           extern template class ITKCommon_EXPORT_EXPLICIT MetaDataObject<unsigned int>; 
        
           extern template class ITKCommon_EXPORT_EXPLICIT MetaDataObject<int>; 
        
           extern template class ITKCommon_EXPORT_EXPLICIT MetaDataObject<unsigned long>; 
        
           extern template class ITKCommon_EXPORT_EXPLICIT MetaDataObject<long>; 
        
           extern template class ITKCommon_EXPORT_EXPLICIT MetaDataObject<unsigned long long>; 
        
           extern template class ITKCommon_EXPORT_EXPLICIT MetaDataObject<long long>; 
        
           extern template class ITKCommon_EXPORT_EXPLICIT MetaDataObject<float>; 
        
           extern template class ITKCommon_EXPORT_EXPLICIT MetaDataObject<double>; 
        
           extern template class ITKCommon_EXPORT_EXPLICIT MetaDataObject<std::string>; 
        
           extern template class ITKCommon_EXPORT_EXPLICIT MetaDataObject<std::vector<float>>; 
        
           extern template class ITKCommon_EXPORT_EXPLICIT MetaDataObject<std::vector<double>>; 
        
           extern template class ITKCommon_EXPORT_EXPLICIT MetaDataObject<std::vector<std::vector<float>>>; 
        
           extern template class ITKCommon_EXPORT_EXPLICIT MetaDataObject<std::vector<std::vector<double>>>; 
        
           extern template class ITKCommon_EXPORT_EXPLICIT MetaDataObject<Array<char>>; 
        
           extern template class ITKCommon_EXPORT_EXPLICIT MetaDataObject<Array<int>>; 
        
           extern template class ITKCommon_EXPORT_EXPLICIT MetaDataObject<Array<float>>; 
        
           extern template class ITKCommon_EXPORT_EXPLICIT MetaDataObject<Array<double>>; 
        
           extern template class ITKCommon_EXPORT_EXPLICIT MetaDataObject<Matrix<double>>;

Do we?

Leengit · 2022-03-17T17:08:21Z

  operator==(const Self & lhs, const Self & rhs)
  {
-    return lhs.m_MetaDataObjectValue == rhs.m_MetaDataObjectValue;
+    return EqualMetaDataObjects(&lhs.m_MetaDataObjectValue, &rhs.m_MetaDataObjectValue);


I would consider passing these in by reference rather than as const pointers. That mimics typical operator== convention. Also, it discourages nullptr as an argument to EqualMetaDataObjects.

@Leengit In general, I also prefer pass by reference rather than by pointer. But EqualMetaDataObjects is just an internal private helper function overload set, so it does not matter so much. And in this case it is actually essential that the parameters are pointer! The idea is that by providing an overload for void pointers, the compiler must do overload resolution. The overload for void pointers is less specific than the one for const TMetaDataObject *, so the compiler will always select the one for const TMetaDataObject *... except when the one for const TMetaDataObject * is SFINAE-ed away!

So only when SFINAE has removed the const TMetaDataObject * specific overload, the compiler will select the void pointer overload to be called. OK?

SFINAE removes the const TMetaDataObject * specific overload when the expression decltype(*lhs == *rh) is invalid ("substitution failure") for that specific TMetaDataObject type.

Leengit · 2022-03-17T17:29:48Z

+    return *lhs == *rhs;
+  }
+
+  /** Overload for MetaDataObject types that do not have an `operator==`.*/


I fear that in the case that operator== does exist, this code will produce two versions of EqualMetaDataObjects. If the testing code works on all platforms then those two versions are not considered redefinitions of each other, but it may be difficult to know which of the two the compiler is choosing. If the testing is working then maybe my fears are unfounded.

If EqualMetaDataObjects were a class instead of a function then we could put the second version here as the default implementation and the first version here as a template specialization.

If the testing code works on all platforms then those two versions are not considered redefinitions of each other, but it may be difficult to know which of the two the compiler is choosing. If the testing is working then maybe my fears are unfounded.

Just to be 100% sure, I did specifically extend the existing MetaDataDictionary.Equal unit tests to check comparing dictionaries after storing an std::string. Because std::string is an obvious example for which operator== is very different from a bitwise comparison (memcmp). Luckily, the tests are passing successfully 😃

ITK/Modules/Core/Common/test/itkMetaDataDictionaryGTest.cxx

Line 290 in d84e49d

itk::EncapsulateMetaData(metaDataDictionary, "key2", std::to_string(value));

Leengit · 2022-03-17T17:34:29Z

+  static bool
+  EqualMetaDataObjects(const void * const lhs, const void * const rhs)
+  {
+    return std::memcmp(lhs, rhs, sizeof(MetaDataObjectType)) == 0;


Apologies for my ignorance, but I see a hard-coded MetaDataObjectType here but a seemingly more flexible TMetaDataObject in the first implementation of EqualMetaDataObjects. I mention that just in case that is not what is intended.

Never mind. I see that MetaDataObjectType is the template parameter of the class. (Even though it does not start with T.)

N-Dekker · 2022-03-21T16:51:49Z

This PR was intended to more easily allow storing simple data structs into the itk::MetaDataDictionary, like mat44 from:

ITK/Modules/ThirdParty/NIFTI/src/nifti/niftilib/nifti1_io.h

Lines 58 to 60 in f372040

    
           typedef struct {                   /** 4x4 matrix struct **/ 
        
             float m[4][4] ; 
        
           } mat44 ;

The idea was that it would allow passing such a data struct directly to EncapsulateMetaData, instead of having to copy it to another type of object, like std::vector, before calling EncapsulateMetaData, as in pull request #3242 commit 9eb4c12:

ITK/Modules/IO/NIFTI/src/itkNiftiImageIO.cxx

Lines 1002 to 1010 in 9eb4c12

    
           std::vector<float> qto_xyz; 
        
           for (int row = 0; row < 4; ++row) 
        
           { 
        
             for (int column = 0; column < 4; ++column) 
        
             { 
        
               qto_xyz.push_back(nim->qto_xyz.m[row][column]); 
        
             } 
        
           } 
        
           EncapsulateMetaData<std::vector<float>>(thisDic, "qto_xyz", qto_xyz);

Those nine lines of code could then be replaced by one line, as follows:

EncapsulateMetaData(thisDic, "qto_xyz", nim->qto_xyz);

Of course, if such simple data structs are not meant to be supported by itk::MetaDataDictionary, and if all supported types already have an operator==, then this PR may be abandoned anyway.

dzenanz · 2022-03-21T18:23:12Z

This PR is probably useful, even if its immediate use no longer exists.

N-Dekker · 2022-03-21T19:52:43Z

This PR is probably useful, even if its immediate use no longer exists.

It would actually useful if we would add an instantiation of MetaDataObject<mat44> to the list of supported/exported instantiations, at:

ITK/Modules/Core/Common/include/itkMetaDataObject.h

Lines 298 to 321 in 24e7b45

    
           extern template class ITKCommon_EXPORT_EXPLICIT MetaDataObject<bool>; 
        
           extern template class ITKCommon_EXPORT_EXPLICIT MetaDataObject<unsigned char>; 
        
           extern template class ITKCommon_EXPORT_EXPLICIT MetaDataObject<char>; 
        
           extern template class ITKCommon_EXPORT_EXPLICIT MetaDataObject<signed char>; 
        
           extern template class ITKCommon_EXPORT_EXPLICIT MetaDataObject<unsigned short>; 
        
           extern template class ITKCommon_EXPORT_EXPLICIT MetaDataObject<short>; 
        
           extern template class ITKCommon_EXPORT_EXPLICIT MetaDataObject<unsigned int>; 
        
           extern template class ITKCommon_EXPORT_EXPLICIT MetaDataObject<int>; 
        
           extern template class ITKCommon_EXPORT_EXPLICIT MetaDataObject<unsigned long>; 
        
           extern template class ITKCommon_EXPORT_EXPLICIT MetaDataObject<long>; 
        
           extern template class ITKCommon_EXPORT_EXPLICIT MetaDataObject<unsigned long long>; 
        
           extern template class ITKCommon_EXPORT_EXPLICIT MetaDataObject<long long>; 
        
           extern template class ITKCommon_EXPORT_EXPLICIT MetaDataObject<float>; 
        
           extern template class ITKCommon_EXPORT_EXPLICIT MetaDataObject<double>; 
        
           extern template class ITKCommon_EXPORT_EXPLICIT MetaDataObject<std::string>; 
        
           extern template class ITKCommon_EXPORT_EXPLICIT MetaDataObject<std::vector<float>>; 
        
           extern template class ITKCommon_EXPORT_EXPLICIT MetaDataObject<std::vector<double>>; 
        
           extern template class ITKCommon_EXPORT_EXPLICIT MetaDataObject<std::vector<std::vector<float>>>; 
        
           extern template class ITKCommon_EXPORT_EXPLICIT MetaDataObject<std::vector<std::vector<double>>>; 
        
           extern template class ITKCommon_EXPORT_EXPLICIT MetaDataObject<Array<char>>; 
        
           extern template class ITKCommon_EXPORT_EXPLICIT MetaDataObject<Array<int>>; 
        
           extern template class ITKCommon_EXPORT_EXPLICIT MetaDataObject<Array<float>>; 
        
           extern template class ITKCommon_EXPORT_EXPLICIT MetaDataObject<Array<double>>; 
        
           extern template class ITKCommon_EXPORT_EXPLICIT MetaDataObject<Matrix<double>>;

Is itk::MetaDataDictionary really only supported for those exported instantiations? I guess itk::MetaDataDictionary would also work fine for non-exported types, at least for an internally used (non-exported) dictionary.

seanm · 2022-03-21T20:02:56Z

It would actually useful if we would add an instantiation of MetaDataObject<mat44>

Again, I think that's a bad idea. It would be exporting a private niftilib struct as public ITK API.

Sure, it'd be nice if itk::MetaDataDictionary could support additional types, like say itk::Matrix, or 2D C arrays, or whatever, but not third party structs that might possibly change.

N-Dekker · 2022-03-21T21:02:07Z

Sure, it'd be nice if itk::MetaDataDictionary could support additional types, like say itk::Matrix, or 2D C arrays, or whatever, but not third party structs that might possibly change.

I think it would be feasible to add support for N-dimensional C-style arrays to itk::MetaDataDictionary. This PR currently certainly would not be sufficient. The implementation of SetMetaDataObjectValue would also need to be adjusted, in order to support C-style arrays:

ITK/Modules/Core/Common/include/itkMetaDataObject.hxx

Lines 55 to 60 in f84720e

    
           template <typename MetaDataObjectType> 
        
           void 
        
           MetaDataObject<MetaDataObjectType>::SetMetaDataObjectValue(const MetaDataObjectType & newValue) 
        
           { 
        
             m_MetaDataObjectValue = newValue; 
        
           }

In pseudo code, it might be:

if MetaDataObjectType is an array-type
  CopyArray(m_MetaDataObjectValue, newValue);
else
  m_MetaDataObjectValue = newValue; // Simple assignment.

This would allow encapsulating "qto_xyz" as a 2-D C-style array, as follows (mind the .m, referring to the array data of mat44):

EncapsulateMetaData(thisDic, "qto_xyz", nim->qto_xyz.m);

seanm · 2022-03-21T21:12:31Z

EncapsulateMetaData(thisDic, "qto_xyz", nim->qto_xyz.m);

Right; storing nim->qto_xyz.m I don't object to, but storing nim->qto_xyz I do object to.

N-Dekker · 2022-03-21T22:53:36Z

Related PR: #3320 "ENH: Support C-style arrays as MetaDataObjectType for MetaDataDictionary"

N-Dekker · 2022-03-23T18:38:01Z

Honestly I feel that it's a bit of a hack, to use bitwise comparison (memcmp) whenever the type of the stored objects does not support operator==. (memcmp might return non-zero when two structs have equal values, because of padding. Moreover, bitwise comparison does not follow the specific logic of floating point comparison, e.g., NaN != NaN (for any NaN), and -0.0 == 0.0.)

So I would prefer to only move this PR forward if or when we really have an actual use case. Especially now that we already have added C-array support, with 7299e5c

dzenanz · 2022-03-23T18:42:54Z

Should we close it, or keep at as draft indefinitely?

N-Dekker · 2022-03-23T20:54:23Z

Should we close it, or keep at as draft indefinitely?

Close-without-merge would be OK to me now. Note that the current implementation conflicts with 7299e5c (Support C-style arrays as MetaDataObjectType for MetaDataDictionary), but of course, this conflict could be resolved whenever necessary.

PranjalSahu · 2022-08-02T19:55:44Z

I am facing an issue related to this in Python wrappings.
While converting ITK image to xarray we need the metadata.

But for "qto_xyz" key the dict method fails for the Python object.
Refer: #3520

I wanted to know what would be the best way to handle this in ITK Python.
From my understanding, we probably need Matrix44 wrapping to handle this similar
to the issue #2270

@N-Dekker @seanm @Leengit

ENH: Let MetaDataDictionary support non-equality-comparable data

d84e49d

Follow-up to: pull request InsightSoftwareConsortium#2246 commit 694bbc0 "ENH: Add operator==, operator!= to MetaDataObject and MetaDataDictionary"

github-actions Bot added area:Core Issues affecting the Core module type:Enhancement Improvement of existing methods or implementation type:Testing Ensure that the purpose of a class is met/the results on a wide set of test cases are correct labels Mar 17, 2022

dzenanz approved these changes Mar 17, 2022

View reviewed changes

dzenanz requested a review from blowekamp March 17, 2022 16:31

Leengit reviewed Mar 17, 2022

View reviewed changes

dzenanz closed this Mar 23, 2022

Uh oh!

Conversation

N-Dekker commented Mar 17, 2022

Uh oh!

N-Dekker commented Mar 17, 2022

Uh oh!

N-Dekker commented Mar 17, 2022

Uh oh!

Leengit Mar 17, 2022

Choose a reason for hiding this comment

Uh oh!

N-Dekker Mar 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Leengit Mar 17, 2022

Choose a reason for hiding this comment

Uh oh!

N-Dekker Mar 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Leengit Mar 17, 2022

Choose a reason for hiding this comment

Uh oh!

Leengit Mar 17, 2022

Choose a reason for hiding this comment

Uh oh!

N-Dekker commented Mar 21, 2022

Uh oh!

dzenanz commented Mar 21, 2022

Uh oh!

N-Dekker commented Mar 21, 2022

Uh oh!

seanm commented Mar 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

N-Dekker commented Mar 21, 2022

Uh oh!

seanm commented Mar 21, 2022

Uh oh!

N-Dekker commented Mar 21, 2022

Uh oh!

N-Dekker commented Mar 23, 2022

Uh oh!

dzenanz commented Mar 23, 2022

Uh oh!

N-Dekker commented Mar 23, 2022

Uh oh!

PranjalSahu commented Aug 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

N-Dekker Mar 17, 2022 •

edited

Loading

N-Dekker Mar 17, 2022 •

edited

Loading

seanm commented Mar 21, 2022 •

edited

Loading

PranjalSahu commented Aug 2, 2022 •

edited

Loading