From abc87a7e9cf60857ba2403d789938d209dafd9f7 Mon Sep 17 00:00:00 2001 From: Omar Awile Date: Tue, 8 Nov 2022 13:14:38 +0100 Subject: [PATCH] Check also datatype sizes for floats to avoid losing bits (#636) * Make sure we only warn when there is actually a problem * BufferInfo needs to know for what operation it is being queried, so I added a little enum type for that. * Use clog instead of cerr * Extend DataTypeClass giving it bitwise operators so we can do type checks a little more elegantly. * note: the DataTypeClass enum values have now new fixed bit values differing from the default values. * Naive set of tests. Co-authored-by: Luc Grosheintz --- include/highfive/H5DataType.hpp | 35 ++++++++++++------- include/highfive/bits/H5Attribute_misc.hpp | 12 ++++--- include/highfive/bits/H5ReadWrite_misc.hpp | 20 +++++++++-- include/highfive/bits/H5Slice_traits_misc.hpp | 14 ++++---- tests/unit/tests_high_five_base.cpp | 18 ++++++++++ 5 files changed, 74 insertions(+), 25 deletions(-) diff --git a/include/highfive/H5DataType.hpp b/include/highfive/H5DataType.hpp index 1194e53c5..c1b42d631 100644 --- a/include/highfive/H5DataType.hpp +++ b/include/highfive/H5DataType.hpp @@ -9,6 +9,7 @@ #ifndef H5DATATYPE_HPP #define H5DATATYPE_HPP +#include #include #include "H5Object.hpp" @@ -23,20 +24,30 @@ namespace HighFive { /// \brief Enum of Fundamental data classes /// enum class DataTypeClass { - Time, - Integer, - Float, - String, - BitField, - Opaque, - Compound, - Reference, - Enum, - VarLen, - Array, - Invalid + Time = 1 << 1, + Integer = 1 << 2, + Float = 1 << 3, + String = 1 << 4, + BitField = 1 << 5, + Opaque = 1 << 6, + Compound = 1 << 7, + Reference = 1 << 8, + Enum = 1 << 9, + VarLen = 1 << 10, + Array = 1 << 11, + Invalid = 0 }; +inline DataTypeClass operator|(DataTypeClass lhs, DataTypeClass rhs) { + using T = std::underlying_type::type; + return static_cast(static_cast(lhs) | static_cast(rhs)); +} + +inline DataTypeClass operator&(DataTypeClass lhs, DataTypeClass rhs) { + using T = std::underlying_type::type; + return static_cast(static_cast(lhs) & static_cast(rhs)); +} + /// /// \brief HDF5 Data Type diff --git a/include/highfive/bits/H5Attribute_misc.hpp b/include/highfive/bits/H5Attribute_misc.hpp index b90ca67c2..777f0c824 100644 --- a/include/highfive/bits/H5Attribute_misc.hpp +++ b/include/highfive/bits/H5Attribute_misc.hpp @@ -61,8 +61,10 @@ inline T Attribute::read() const { template inline void Attribute::read(T& array) const { const DataSpace& mem_space = getMemSpace(); - const details::BufferInfo buffer_info(getDataType(), - [this]() -> std::string { return this->getName(); }); + const details::BufferInfo buffer_info( + getDataType(), + [this]() -> std::string { return this->getName(); }, + details::BufferInfo::read); if (!details::checkDimensions(mem_space, buffer_info.n_dimensions)) { std::ostringstream ss; @@ -99,8 +101,10 @@ inline void Attribute::read(T* array, const DataType& dtype) const { template inline void Attribute::write(const T& buffer) { const DataSpace& mem_space = getMemSpace(); - const details::BufferInfo buffer_info(getDataType(), - [this]() -> std::string { return this->getName(); }); + const details::BufferInfo buffer_info( + getDataType(), + [this]() -> std::string { return this->getName(); }, + details::BufferInfo::write); if (!details::checkDimensions(mem_space, buffer_info.n_dimensions)) { std::ostringstream ss; diff --git a/include/highfive/bits/H5ReadWrite_misc.hpp b/include/highfive/bits/H5ReadWrite_misc.hpp index 8be5e1807..bd31f2b53 100644 --- a/include/highfive/bits/H5ReadWrite_misc.hpp +++ b/include/highfive/bits/H5ReadWrite_misc.hpp @@ -45,8 +45,11 @@ struct BufferInfo { using char_array_t = typename details::type_char_array::type; static constexpr bool is_char_array = !std::is_same::value; + enum Operation { read, write }; + const Operation op; + template - BufferInfo(const DataType& dtype, F getName); + BufferInfo(const DataType& dtype, F getName, Operation _op); // member data for info depending on the destination dataset type const bool is_fixed_len_string; @@ -103,8 +106,9 @@ struct string_type_checker { template template -BufferInfo::BufferInfo(const DataType& dtype, F getName) - : is_fixed_len_string(dtype.isFixedLenStr()) +BufferInfo::BufferInfo(const DataType& dtype, F getName, Operation _op) + : op(_op) + , is_fixed_len_string(dtype.isFixedLenStr()) // In case we are using Fixed-len strings we need to subtract one dimension , n_dimensions(details::inspector::recursive_ndim - ((is_fixed_len_string && is_char_array) ? 1 : 0)) @@ -120,6 +124,16 @@ BufferInfo::BufferInfo(const DataType& dtype, F getName) std::cerr << "HighFive WARNING \"" << getName() << "\": data and hdf5 dataset have different types: " << data_type.string() << " -> " << dtype.string() << std::endl; + } else if ((dtype.getClass() & data_type.getClass()) == DataTypeClass::Float) { + if ((op == read) && (dtype.getSize() > data_type.getSize())) { + std::clog << "HighFive WARNING \"" << getName() + << "\": hdf5 dataset has higher floating point precision than data on read: " + << dtype.string() << " -> " << data_type.string() << std::endl; + } else if ((op == write) && (dtype.getSize() < data_type.getSize())) { + std::clog << "HighFive WARNING \"" << getName() + << "\": data has higher floating point precision than hdf5 dataset on write: " + << data_type.string() << " -> " << dtype.string() << std::endl; + } } } diff --git a/include/highfive/bits/H5Slice_traits_misc.hpp b/include/highfive/bits/H5Slice_traits_misc.hpp index 6b7dc3d40..3a1d0de20 100644 --- a/include/highfive/bits/H5Slice_traits_misc.hpp +++ b/include/highfive/bits/H5Slice_traits_misc.hpp @@ -172,9 +172,10 @@ template inline void SliceTraits::read(T& array, const DataTransferProps& xfer_props) const { const auto& slice = static_cast(*this); const DataSpace& mem_space = slice.getMemSpace(); - const details::BufferInfo buffer_info(slice.getDataType(), [slice]() -> std::string { - return details::get_dataset(slice).getPath(); - }); + const details::BufferInfo buffer_info( + slice.getDataType(), + [slice]() -> std::string { return details::get_dataset(slice).getPath(); }, + details::BufferInfo::Operation::read); if (!details::checkDimensions(mem_space, buffer_info.n_dimensions)) { std::ostringstream ss; @@ -225,9 +226,10 @@ template inline void SliceTraits::write(const T& buffer, const DataTransferProps& xfer_props) { const auto& slice = static_cast(*this); const DataSpace& mem_space = slice.getMemSpace(); - const details::BufferInfo buffer_info(slice.getDataType(), [slice]() -> std::string { - return details::get_dataset(slice).getPath(); - }); + const details::BufferInfo buffer_info( + slice.getDataType(), + [slice]() -> std::string { return details::get_dataset(slice).getPath(); }, + details::BufferInfo::Operation::write); if (!details::checkDimensions(mem_space, buffer_info.n_dimensions)) { std::ostringstream ss; diff --git a/tests/unit/tests_high_five_base.cpp b/tests/unit/tests_high_five_base.cpp index 04d39e6c7..101da13bf 100644 --- a/tests/unit/tests_high_five_base.cpp +++ b/tests/unit/tests_high_five_base.cpp @@ -2446,6 +2446,24 @@ TEST_CASE("HighFiveReadWriteConsts") { } } +TEST_CASE("HighFiveDataTypeClass") { + auto Float = DataTypeClass::Float; + auto String = DataTypeClass::String; + auto Invalid = DataTypeClass::Invalid; + + CHECK(Float != Invalid); + + CHECK((Float & Float) == Float); + CHECK((Float | Float) == Float); + + CHECK((Float & String) == Invalid); + CHECK((Float | String) != Invalid); + + CHECK(((Float & String) & Float) == Invalid); + CHECK(((Float | String) & Float) == Float); + CHECK(((Float | String) & String) == String); +} + #ifdef H5_USE_EIGEN template