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 <[email protected]>
BlueBrain · Nov 8, 2022 · abc87a7 · abc87a7
1 parent 414702a
commit abc87a7
Show file tree

Hide file tree

Showing 5 changed files with 74 additions and 25 deletions.
diff --git a/include/highfive/H5DataType.hpp b/include/highfive/H5DataType.hpp
@@ -9,6 +9,7 @@
 #ifndef H5DATATYPE_HPP
 #define H5DATATYPE_HPP
 
+#include <type_traits>
 #include <vector>
 
 #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<DataTypeClass>::type;
+    return static_cast<DataTypeClass>(static_cast<T>(lhs) | static_cast<T>(rhs));
+}
+
+inline DataTypeClass operator&(DataTypeClass lhs, DataTypeClass rhs) {
+    using T = std::underlying_type<DataTypeClass>::type;
+    return static_cast<DataTypeClass>(static_cast<T>(lhs) & static_cast<T>(rhs));
+}
+
 
 ///
 /// \brief HDF5 Data Type

diff --git a/include/highfive/bits/H5Attribute_misc.hpp b/include/highfive/bits/H5Attribute_misc.hpp
@@ -61,8 +61,10 @@ inline T Attribute::read() const {
 template <typename T>
 inline void Attribute::read(T& array) const {
     const DataSpace& mem_space = getMemSpace();
-    const details::BufferInfo<T> buffer_info(getDataType(),
-                                             [this]() -> std::string { return this->getName(); });
+    const details::BufferInfo<T> buffer_info(
+        getDataType(),
+        [this]() -> std::string { return this->getName(); },
+        details::BufferInfo<T>::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 <typename T>
 inline void Attribute::write(const T& buffer) {
     const DataSpace& mem_space = getMemSpace();
-    const details::BufferInfo<T> buffer_info(getDataType(),
-                                             [this]() -> std::string { return this->getName(); });
+    const details::BufferInfo<T> buffer_info(
+        getDataType(),
+        [this]() -> std::string { return this->getName(); },
+        details::BufferInfo<T>::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
@@ -45,8 +45,11 @@ struct BufferInfo {
     using char_array_t = typename details::type_char_array<type_no_const>::type;
     static constexpr bool is_char_array = !std::is_same<char_array_t, void>::value;
 
+    enum Operation { read, write };
+    const Operation op;
+
     template <class F>
-    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<char*> {
 
 template <typename T>
 template <class F>
-BufferInfo<T>::BufferInfo(const DataType& dtype, F getName)
-    : is_fixed_len_string(dtype.isFixedLenStr())
+BufferInfo<T>::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<type_no_const>::recursive_ndim -
                    ((is_fixed_len_string && is_char_array) ? 1 : 0))
@@ -120,6 +124,16 @@ BufferInfo<T>::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
@@ -172,9 +172,10 @@ template <typename T>
 inline void SliceTraits<Derivate>::read(T& array, const DataTransferProps& xfer_props) const {
     const auto& slice = static_cast<const Derivate&>(*this);
     const DataSpace& mem_space = slice.getMemSpace();
-    const details::BufferInfo<T> buffer_info(slice.getDataType(), [slice]() -> std::string {
-        return details::get_dataset(slice).getPath();
-    });
+    const details::BufferInfo<T> buffer_info(
+        slice.getDataType(),
+        [slice]() -> std::string { return details::get_dataset(slice).getPath(); },
+        details::BufferInfo<T>::Operation::read);
 
     if (!details::checkDimensions(mem_space, buffer_info.n_dimensions)) {
         std::ostringstream ss;
@@ -225,9 +226,10 @@ template <typename T>
 inline void SliceTraits<Derivate>::write(const T& buffer, const DataTransferProps& xfer_props) {
     const auto& slice = static_cast<const Derivate&>(*this);
     const DataSpace& mem_space = slice.getMemSpace();
-    const details::BufferInfo<T> buffer_info(slice.getDataType(), [slice]() -> std::string {
-        return details::get_dataset(slice).getPath();
-    });
+    const details::BufferInfo<T> buffer_info(
+        slice.getDataType(),
+        [slice]() -> std::string { return details::get_dataset(slice).getPath(); },
+        details::BufferInfo<T>::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
@@ -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 <typename T>