Skip to content

BUG: Fix printing of values in MetaDataObject #4368

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
phcerdan wants to merge 1 commit into from
Closed

BUG: Fix printing of values in MetaDataObject #4368

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
70 changes: 12 additions & 58 deletions Modules/Core/Common/include/itkMetaDataObject.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,11 +63,6 @@ namespace itk
* string "[UNKNOWN PRINT CHARACTERISTICS]" that works for all possible
* MetaDataObject types.
*
* The application developer may overload the default implementation to provide
* a specialized Print() characteristics to produce results desirable for their application.
* A set of very crude Macros {NATIVE_TYPE_METADATAPRINT, ITK_OBJECT_TYPE_METADATAPRINT_1COMMA,
* ITK_IMAGE_TYPE_METADATAPRINT } are provided to facilitate a very simple implementation, and as an example.
*
* \ingroup ITKCommon
*
*/
Expand Down Expand Up @@ -123,13 +118,6 @@ class ITK_TEMPLATE_EXPORT MetaDataObject : public MetaDataObjectBase
void
SetMetaDataObjectValue(const MetaDataObjectType & newValue);

/**
* Defines the default behavior for printing out this element
* \param os An output stream
*/
void
Print(std::ostream & os) const override;

/** Returns (metaDataObject1 == metaDataObject2). */
friend bool
operator==(const Self & lhs, const Self & rhs)
Expand All @@ -144,10 +132,21 @@ class ITK_TEMPLATE_EXPORT MetaDataObject : public MetaDataObjectBase
return !(lhs == rhs);
}

/** Helper to print contents of a MetaDataObject. */
void
PrintValue(std::ostream & os) const;

protected:
MetaDataObject() = default;
~MetaDataObject() override = default;

/**
* Defines the default behavior for printing out this element
* \param os An output stream
*/
void
PrintSelf(std::ostream & os, Indent indent) const override;

private:
/** Assigns the value of `source` to `target`.
* \note The trailing return type is there, just to enable SFINAE.*/
Expand Down Expand Up @@ -262,53 +261,8 @@ ExposeMetaData(const MetaDataDictionary & Dictionary, const std::string key, T &
return true;
}

} // end namespace itk

/**
* \def ITK_NATIVE_TYPE_METADATAPRINT( TYPE_NAME )
* \brief An ugly macro to facilitate creating a simple implementation of
* the MetaDataObject<Type>::Print() function for types that
* have operator<< defined.
* \param TYPE_NAME the native type parameter type
*/
#define ITK_NATIVE_TYPE_METADATAPRINT(TYPE_NAME) \
template <> \
void itk::MetaDataObject<TYPE_NAME>::Print(std::ostream & os) const \
{ \
os << this->m_MetaDataObjectValue << std::endl; \
}

/**
* \def ITK_OBJECT_TYPE_METADATAPRINT_1COMMA( TYPE_NAME_PART1, TYPE_NAME_PART2 )
* \brief An ugly macro to facilitate creating a simple implementation of
* the MetaDataObject< Type >::Print() function for
* itk::Objects that have 1 comma in their type definition
* \param TYPE_NAME_PART1
* \param TYPE_NAME_PART2
*/
#define ITK_OBJECT_TYPE_METADATAPRINT_1COMMA(TYPE_NAME_PART1, TYPE_NAME_PART2) \
template <> \
void itk::MetaDataObject<TYPE_NAME_PART1, TYPE_NAME_PART2>::Print(std::ostream & os) const \
{ \
this->m_MetaDataObjectValue->Print(os); \
}

/**
* \def ITK_IMAGE_TYPE_METADATAPRINT( STORAGE_TYPE )
* An ugly macro to facilitate creating a simple implementation of
* the MetaDataObject<Type>::Print() function for
* itk::Image\<STORAGE_TYPE,[1-8]\>\::Pointer
* \param STORAGE_TYPE The storage type of the image type to print.
*/
#define ITK_IMAGE_TYPE_METADATAPRINT(STORAGE_TYPE) \
ITK_OBJECT_TYPE_METADATAPRINT_1COMMA(itk::Image<STORAGE_TYPE, 1>::Pointer) \
ITK_OBJECT_TYPE_METADATAPRINT_1COMMA(itk::Image<STORAGE_TYPE, 2>::Pointer) \
ITK_OBJECT_TYPE_METADATAPRINT_1COMMA(itk::Image<STORAGE_TYPE, 3>::Pointer) \
ITK_OBJECT_TYPE_METADATAPRINT_1COMMA(itk::Image<STORAGE_TYPE, 4>::Pointer) \
ITK_OBJECT_TYPE_METADATAPRINT_1COMMA(itk::Image<STORAGE_TYPE, 5>::Pointer) \
ITK_OBJECT_TYPE_METADATAPRINT_1COMMA(itk::Image<STORAGE_TYPE, 6>::Pointer) \
ITK_OBJECT_TYPE_METADATAPRINT_1COMMA(itk::Image<STORAGE_TYPE, 7>::Pointer) \
ITK_OBJECT_TYPE_METADATAPRINT_1COMMA(itk::Image<STORAGE_TYPE, 8>::Pointer)
} // end namespace itk

#ifndef ITK_MANUAL_INSTANTIATION
# include "itkMetaDataObject.hxx"
Expand Down
76 changes: 70 additions & 6 deletions Modules/Core/Common/include/itkMetaDataObject.hxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,9 +28,79 @@
#ifndef itkMetaDataObject_hxx
#define itkMetaDataObject_hxx

#include <type_traits>
#include <iterator>

template <typename T, typename = void>
inline constexpr bool is_iterable_print_v = false;
// Specialize for std::vector<T> and std::vector<std::vector<T>>
template <typename T>
inline constexpr bool is_iterable_print_v<std::vector<T>, std::void_t<>> = true;
template <typename T>
inline constexpr bool is_iterable_print_v<std::vector<std::vector<T>>, std::void_t<>> = true;
Comment on lines +34 to +40
Copy link
Contributor

@N-Dekker N-Dekker Dec 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice to see a use case for variable templates, thanks @phcerdan. Unfortunately "itkMetaDataObject.hxx" is usually #include'd (indirectly) by end users, and I guess it's not the intention to add is_iterable_print_v to the global namespace of the user, right? If you agree, you may possibly make is_iterable_print_v a private static member of MetaDataObject. Or otherwise, maybe it's simpler to just add overloaded private static MetaDataObject::IsIterablePrint(const T&) member functions. What do you think?

Copy link
Contributor Author

@phcerdan phcerdan Dec 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good points @N-Dekker, I will make static private members

Copy link
Contributor Author

@phcerdan phcerdan Dec 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GCC doesn't like it... (moving is_iterable_print inside the class).
https://stackoverflow.com/questions/72190700/explicit-template-argument-list-not-allowed-with-g-but-compiles-with-clang

I will explore further.

Copy link
Contributor Author

@phcerdan phcerdan Dec 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems fixed in gcc14, but we cannot afford that.

Copy link
Contributor

@N-Dekker N-Dekker Dec 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another option I guess would be to have constexpr static member functions IsIterablePrint(const T&) overloaded (rather than "specialized") for std::vector<T> and std::vector<std::vector<T>>, right?

You could then use it like:

if constexpr (IsIterablePrint(iterable))

I guess 😃

Copy link
Contributor

@N-Dekker N-Dekker Dec 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regarding the name of the boolean, maybe something like ShouldBePrintedElementWise ?


namespace itk
{
template <typename TIterable>
void
printIterable(std::ostream & os, const TIterable & iterable)
Copy link
Contributor

@N-Dekker N-Dekker Dec 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting pull request, thanks @phcerdan

Can you possibly hide printIterable (or whatever its name will be) from the public API? Because it isn't intended to be exposed directly to the end user, right? I guess it should be possible to make it a private (preferably static) member function of MetaDataObject. Otherwise, it might be possible to define it as a lambda, hidden locally inside MetaDataObject::PrintValue, but that seems more challenging to do.

Copy link
Contributor

@N-Dekker N-Dekker Dec 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you can make it private, you might as well just name the function something like PrintHelper, rather than printIterable. My 2 cent!

{
if constexpr (is_iterable_print_v<TIterable>)
{
os << "[";
auto begin = std::begin(iterable);
auto end = std::end(iterable);
for (auto it = begin; it != end; ++it)
{
if (it != begin)
{
os << ", ";
}
printIterable(os, *it);
}
os << "]";
}
else
{
// Handle non-iterable types
os << iterable;
}
}

template <typename MetaDataObjectType>
void
MetaDataObject<MetaDataObjectType>::PrintValue(std::ostream & os) const
{
if constexpr (is_iterable_print_v<MetaDataObjectType>)
{
os << "[";
auto begin = std::begin(m_MetaDataObjectValue);
auto end = std::end(m_MetaDataObjectValue);
for (auto it = begin; it != end; ++it)
{
if (it != begin)
{
os << ", ";
}
printIterable(os, *it);
}
os << "]";
}
else
{
os << m_MetaDataObjectValue;
}
}

template <typename MetaDataObjectType>
void
MetaDataObject<MetaDataObjectType>::PrintSelf(std::ostream & os, Indent indent) const
{
os << indent;
this->PrintValue(os);
os << std::endl;
}

template <typename MetaDataObjectType>
const char *
MetaDataObject<MetaDataObjectType>::GetMetaDataObjectTypeName() const
Expand Down Expand Up @@ -59,12 +129,6 @@ MetaDataObject<MetaDataObjectType>::SetMetaDataObjectValue(const MetaDataObjectT
Self::Assign(m_MetaDataObjectValue, newValue);
}

template <typename MetaDataObjectType>
void
MetaDataObject<MetaDataObjectType>::Print(std::ostream & os) const
{
Superclass::Print(os);
}

} // end namespace itk

Expand Down
11 changes: 5 additions & 6 deletions Modules/Core/Common/include/itkMetaDataObjectBase.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,17 +86,16 @@ class ITKCommon_EXPORT MetaDataObjectBase : public LightObject
return !(lhs == rhs);
}

protected:
MetaDataObjectBase();
~MetaDataObjectBase() override;

/**
* Defines the default behavior for printing out this element
* \param os An output stream
*/
virtual void
Print(std::ostream & os) const;

protected:
MetaDataObjectBase();
~MetaDataObjectBase() override;
void
PrintSelf(std::ostream & os, Indent indent) const override;

private:
virtual bool
Expand Down
4 changes: 2 additions & 2 deletions Modules/Core/Common/src/itkMetaDataObjectBase.cxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,9 +41,9 @@ MetaDataObjectBase::GetMetaDataObjectTypeInfo() const


void
MetaDataObjectBase::Print(std::ostream & os) const
MetaDataObjectBase::PrintSelf(std::ostream & os, Indent indent) const
{
os << "[UNKNOWN_PRINT_CHARACTERISTICS]" << std::endl;
os << indent << "[UNKNOWN_PRINT_CHARACTERISTICS]" << std::endl;
}

} // end namespace itk
13 changes: 13 additions & 0 deletions Modules/Core/Common/test/itkMetaDataObjectTest.cxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,8 @@ testMetaData(const TMetaData & value)
std::cout << "The metadata's type name is: " << metaDataObject->GetMetaDataObjectTypeName() << std::endl;
std::cout << "The metadata object: " << std::endl;
metaDataObject->Print(std::cout);
std::cout << "The metadata value only: " << std::endl;
metaDataObject->PrintValue(std::cout);

std::cout << std::endl << std::endl;

Expand All @@ -68,6 +70,17 @@ itkMetaDataObjectTest(int, char *[])
result += testMetaData<float>(-24);
result += testMetaData<double>(-24);
result += testMetaData<std::string>("I T K");
// Exercise printing of std::vector<T> and std::vector<std::vector<T>>
// These two types are special cased in itk::MetaDataObject::PrintValue()
auto v3 = std::vector<double>{ 1.0, 2.0, 3.0 };
result += testMetaData<std::vector<double>>(v3);
result += testMetaData<std::vector<std::vector<double>>>(std::vector<std::vector<double>>{ v3, v3 });
// Exercise itk::Array
auto a3 = itk::Array<double>(3, 5.0);
result += testMetaData<itk::Array<double>>(a3);
// Exercise itk::Matrix
auto m3 = itk::Matrix<double, 3, 3>();
result += testMetaData<itk::Matrix<double, 3, 3>>(m3);

using ImageType = itk::Image<unsigned short, 3>;
ImageType::Pointer image = nullptr;
Expand Down