Release: 0.14.2 (#1090)

* Bug fix: Don't forget closing files (#1083) * Failing test * Bug fix: Don't forget closing files * HDF5: Fix String Vlen Attribute Reads (#1084) We inofficially try to also support HDF5 variable lengths strings in reading, just to increase compatibility. This was never working it seems. * Fix reading of vector attributes with only one contained value (#1085) * Failing test * Conversions in Attribute.hpp 1) single values to 1-value vectors 2) vectors to arrays 3) arrays to vectors * Some cleanup in Attribute.hpp 1) Simplify types in DoConvert, remove unnecessary template parameter 2) Replace a long if-then-else chain by variantSrc::visit * CoreTest: Fix std::array constructors Make more widely compile-able. * Explicit casting in some places This avoids some warnings * Intel compilers: Don't use variantSrc::visit They don't like it * Remove scattered checks for vector attributes * Generalize icpc guard As defined in CMake for compiler identification. * Doc ICC version (2021.3.0) * setAttribute: Reject Empty Strings (#1087) * setAttribute: Reject Empty Strings Some backends, especially HDF5, do not allow us to define zero-sized strings. We thus need to catch this in the frontend and forward the restriction to the user. * Test: setAttribute("key", "") throws * setAttribute Check: C++14 compatible * Don't read iterations if they have already been parsed (#1089) * Release: 0.14.2 Co-authored-by: Franz Pöschel <[email protected]>
openPMD · Aug 18, 2021 · e00bede · e00bede
1 parent 7f499d4
commit e00bede
Show file tree

Hide file tree

Showing 18 changed files with 342 additions and 114 deletions.
diff --git a/CHANGELOG.rst b/CHANGELOG.rst
@@ -3,6 +3,32 @@
 Changelog
 =========
 
+0.14.2
+------
+**Date:** 2021-08-17
+
+Various Reader Fixes
+
+This releases fixes regressions in reads, closing files properly, avoiding inefficient parsing and allowing more permissive casts in attribute reads.
+(Inofficial) support for HDF5 vlen string reads has been fixed.
+
+Changes to "0.14.1"
+^^^^^^^^^^^^^^^^^^^
+
+Bug Fixes
+"""""""""
+
+- do not forget to close files #1083
+- reading of vector attributes with only one contained value #1085
+- do not read iterations if they have already been parsed #1089
+- HDF5: fix string vlen attribute reads #1084
+
+Other
+"""""
+
+- ``setAttribute``: reject empty strings #1087
+
+
 0.14.1
 ------
 **Date:** 2021-08-04

diff --git a/CITATION.cff b/CITATION.cff
@@ -25,7 +25,7 @@ contact:
   orcid: https://orcid.org/0000-0003-1943-7141
   email: [email protected]
 title: "openPMD-api: C++ & Python API for Scientific I/O with openPMD"
-version: 0.14.1
+version: 0.14.2
 repository-code: https://github.com/openPMD/openPMD-api
 doi: 10.14278/rodare.27
 license: LGPL-3.0-or-later

diff --git a/CMakeLists.txt b/CMakeLists.txt
@@ -2,7 +2,7 @@
 #
 cmake_minimum_required(VERSION 3.15.0)
 
-project(openPMD VERSION 0.14.1) # LANGUAGES CXX
+project(openPMD VERSION 0.14.2) # LANGUAGES CXX
 
 # the openPMD "markup"/"schema" standard version
 set(openPMD_STANDARD_VERSION 1.1.0)

diff --git a/docs/source/conf.py b/docs/source/conf.py
@@ -83,9 +83,9 @@
 # built documents.
 #
 # The short X.Y version.
-version = u'0.14.1'
+version = u'0.14.2'
 # The full version, including alpha/beta/rc tags.
-release = u'0.14.1'
+release = u'0.14.2'
 
 # The language for content autogenerated by Sphinx. Refer to documentation
 # for a list of supported languages.

diff --git a/docs/source/index.rst b/docs/source/index.rst
@@ -42,7 +42,7 @@ openPMD-api version      supported openPMD standard versions
 ======================== ===================================
 ``2.0.0+``               ``2.0.0+``      (not released yet)
 ``1.0.0+``               ``1.0.1-1.1.0`` (not released yet)
-``0.13.1-0.14.1`` (beta) ``1.0.0-1.1.0``
+``0.13.1-0.14.2`` (beta) ``1.0.0-1.1.0``
 ``0.1.0-0.12.0`` (alpha) ``1.0.0-1.1.0``
 ======================== ===================================
 

diff --git a/include/openPMD/backend/Attributable.hpp b/include/openPMD/backend/Attributable.hpp
@@ -31,6 +31,7 @@
 #include <vector>
 #include <string>
 #include <cstddef>
+#include <type_traits>
 
 // expose private and protected members for invasive testing
 #ifndef OPENPMD_protected
@@ -83,8 +84,29 @@ class AttributableData
 private:
     A_MAP m_attributes;
 };
+
+/** Verify values of attributes in the frontend
+ *
+ * verify string attributes are not empty (backend restriction, e.g., HDF5)
+ */
+template< typename T >
+inline void
+attr_value_check( std::string const /* key */, T /* value */ )
+{
 }
 
+template< >
+inline void
+attr_value_check( std::string const key, std::string const value )
+{
+    if( value.empty() )
+        throw std::runtime_error(
+                "[setAttribute] Value for string attribute '" + key +
+                "' must not be empty!" );
+}
+
+} // namespace internal
+
 /** @brief Layer to manage storage of attributes associated with file objects.
  *
  * Mandatory and user-defined Attributes and their data for every object in the
@@ -394,6 +416,8 @@ template< typename T >
 inline bool
 AttributableInterface::setAttribute( std::string const & key, T value )
 {
+    internal::attr_value_check( key, value );
+
     auto & attri = get();
     if(IOHandler() && Access::READ_ONLY == IOHandler()->m_frontendAccess )
     {
@@ -420,6 +444,7 @@ AttributableInterface::setAttribute( std::string const & key, T value )
         return false;
     }
 }
+
 inline bool
 AttributableInterface::setAttribute( std::string const & key, char const value[] )
 {

diff --git a/include/openPMD/backend/Attribute.hpp b/include/openPMD/backend/Attribute.hpp
@@ -98,8 +98,7 @@ struct DoConvert;
 template< typename T, typename U >
 struct DoConvert<T, U, false>
 {
-    template< typename PV >
-    U operator()( PV )
+    U operator()( T * )
     {
         throw std::runtime_error("getCast: no cast possible.");
     }
@@ -108,8 +107,7 @@ struct DoConvert<T, U, false>
 template< typename T, typename U >
 struct DoConvert<T, U, true>
 {
-    template< typename PV >
-    U operator()( PV pv )
+    U operator()( T * pv )
     {
         return static_cast< U >( *pv );
     }
@@ -120,8 +118,8 @@ struct DoConvert<std::vector< T >, std::vector< U >, false>
 {
     static constexpr bool convertible = std::is_convertible<T, U>::value;
 
-    template< typename PV, typename UU = U >
-    auto operator()( PV pv )
+    template< typename UU = U >
+    auto operator()( std::vector< T > const * pv )
     -> typename std::enable_if< convertible, std::vector< UU > >::type
     {
         std::vector< U > u;
@@ -130,14 +128,101 @@ struct DoConvert<std::vector< T >, std::vector< U >, false>
         return u;
     }
 
-    template< typename PV, typename UU = U >
-    auto operator()( PV )
+    template< typename UU = U >
+    auto operator()( std::vector< T > const * )
     -> typename std::enable_if< !convertible, std::vector< UU > >::type
     {
         throw std::runtime_error("getCast: no vector cast possible.");
     }
 };
 
+// conversion cast: turn a single value into a 1-element vector
+template< typename T, typename U >
+struct DoConvert<T, std::vector< U >, false>
+{
+    static constexpr bool convertible = std::is_convertible<T, U>::value;
+
+    template< typename UU = U >
+    auto operator()( T const * pv )
+    -> typename std::enable_if< convertible, std::vector< UU > >::type
+    {
+        std::vector< U > u;
+        u.reserve( 1 );
+        u.push_back( static_cast< U >( *pv ) );
+        return u;
+    }
+
+    template< typename UU = U >
+    auto operator()( T const * )
+    -> typename std::enable_if< !convertible, std::vector< UU > >::type
+    {
+        throw std::runtime_error(
+            "getCast: no scalar to vector conversion possible.");
+    }
+};
+
+// conversion cast: array to vector
+// if a backend reports a std::array<> for something where the frontend expects
+// a vector
+template< typename T, typename U, size_t n >
+struct DoConvert<std::array< T, n >, std::vector< U >, false>
+{
+    static constexpr bool convertible = std::is_convertible<T, U>::value;
+
+    template< typename UU = U >
+    auto operator()( std::array< T, n > const * pv )
+    -> typename std::enable_if< convertible, std::vector< UU > >::type
+    {
+        std::vector< U > u;
+        u.reserve( n );
+        std::copy( pv->begin(), pv->end(), std::back_inserter(u) );
+        return u;
+    }
+
+    template< typename UU = U >
+    auto operator()( std::array< T, n > const * )
+    -> typename std::enable_if< !convertible, std::vector< UU > >::type
+    {
+        throw std::runtime_error(
+            "getCast: no array to vector conversion possible.");
+    }
+};
+
+// conversion cast: vector to array
+// if a backend reports a std::vector<> for something where the frontend expects
+// an array
+template< typename T, typename U, size_t n >
+struct DoConvert<std::vector< T >, std::array< U, n >, false>
+{
+    static constexpr bool convertible = std::is_convertible<T, U>::value;
+
+    template< typename UU = U >
+    auto operator()( std::vector< T > const * pv )
+    -> typename std::enable_if< convertible, std::array< UU, n > >::type
+    {
+        std::array< U, n > u;
+        if( n != pv->size() )
+        {
+            throw std::runtime_error(
+                "getCast: no vector to array conversion possible "
+                "(wrong requested array size).");
+        }
+        for( size_t i = 0; i < n; ++i )
+        {
+            u[ i ] = static_cast< U >( ( *pv )[ i ] );
+        }
+        return u;
+    }
+
+    template< typename UU = U >
+    auto operator()( std::vector< T > const * )
+    -> typename std::enable_if< !convertible, std::array< UU, n > >::type
+    {
+        throw std::runtime_error(
+            "getCast: no vector to array conversion possible.");
+    }
+};
+
 /** Retrieve a stored specific Attribute and cast if convertible.
  *
  * @throw   std::runtime_error if stored object is not static castable to U.
@@ -150,6 +235,12 @@ getCast( Attribute const & a )
 {
     auto v = a.getResource();
 
+    // icpc 2021.3.0 does not like variantSrc::visit (with mpark-variant)
+    // we use variantSrc::visit for the other compilers to avoid having an
+    // endless list of if-then-else
+    // also, once we switch to C++17, we might throw this out in
+    // favor of a hopefully working std::visit
+#if defined(__ICC) || defined(__INTEL_COMPILER)
     if(auto pvalue_c = variantSrc::get_if< char >( &v ) )
         return DoConvert<char, U>{}(pvalue_c);
     else if(auto pvalue_uc = variantSrc::get_if< unsigned char >( &v ) )
@@ -226,6 +317,15 @@ getCast( Attribute const & a )
         return DoConvert<bool, U>{}(pvalue_b);
     else
         throw std::runtime_error("getCast: unknown Datatype.");
+
+#else
+    return variantSrc::visit(
+        []( auto && containedValue ) -> U {
+            using containedType = std::decay_t< decltype( containedValue ) >;
+            return DoConvert< containedType, U >{}( &containedValue );
+        },
+        v );
+#endif
 }
 
 template< typename U >

diff --git a/include/openPMD/version.hpp b/include/openPMD/version.hpp
@@ -29,7 +29,7 @@
  */
 #define OPENPMDAPI_VERSION_MAJOR 0
 #define OPENPMDAPI_VERSION_MINOR 14
-#define OPENPMDAPI_VERSION_PATCH 1
+#define OPENPMDAPI_VERSION_PATCH 2
 #define OPENPMDAPI_VERSION_LABEL ""
 /** @} */
 

diff --git a/setup.py b/setup.py
@@ -156,7 +156,7 @@ def build_extension(self, ext):
 setup(
     name='openPMD-api',
     # note PEP-440 syntax: x.y.zaN but x.y.z.devN
-    version='0.14.1',
+    version='0.14.2',
     author='Axel Huebl, Franz Poeschel, Fabian Koller, Junmin Gu',
     author_email='[email protected], [email protected]',
     maintainer='Axel Huebl',

diff --git a/src/IO/HDF5/HDF5IOHandler.cpp b/src/IO/HDF5/HDF5IOHandler.cpp
@@ -1479,18 +1479,19 @@ HDF5IOHandlerImpl::readAttribute(Writable* writable,
         {
             if( H5Tis_variable_str(attr_type) )
             {
-                char* c = nullptr;
+                // refs.:
+                // https://github.com/HDFGroup/hdf5/blob/hdf5-1_12_0/tools/src/h5dump/h5dump_xml.c
+                hsize_t size = H5Tget_size(attr_type); // not yet the actual string length
+                std::vector< char > vc(size); // byte buffer to vlen strings
                 status = H5Aread(attr_id,
                                  attr_type,
-                                 c);
-                VERIFY(status == 0,
-                    "[HDF5] Internal error: Failed to read attribute " + attr_name +
-                    " at " + concrete_h5_file_position(writable));
-                a = Attribute(auxiliary::strip(std::string(c), {'\0'}));
-                status = H5Dvlen_reclaim(attr_type,
-                                         attr_space,
-                                         H5P_DEFAULT,
-                                         c);
+                                 vc.data());
+                auto c_str = *((char**)vc.data()); // get actual string out
+                a = Attribute(std::string(c_str));
+                // free dynamically allocated vlen memory from H5Aread
+                H5Dvlen_reclaim(attr_type, attr_space, H5P_DEFAULT, vc.data());
+                // 1.12+:
+                //H5Treclaim(attr_type, attr_space, H5P_DEFAULT, vc.data());
             } else
             {
                 hsize_t size = H5Tget_size(attr_type);

diff --git a/src/Mesh.cpp b/src/Mesh.cpp
@@ -318,35 +318,27 @@ Mesh::read()
     aRead.name = "axisLabels";
     IOHandler()->enqueue(IOTask(this, aRead));
     IOHandler()->flush();
-    if( *aRead.dtype == DT::VEC_STRING )
+    if( *aRead.dtype == DT::VEC_STRING || *aRead.dtype == DT::STRING)
         setAxisLabels(Attribute(*aRead.resource).get< std::vector< std::string > >());
-    else if( *aRead.dtype == DT::STRING )
-        setAxisLabels({Attribute(*aRead.resource).get< std::string >()});
     else
         throw std::runtime_error("Unexpected Attribute datatype for 'axisLabels'");
 
     aRead.name = "gridSpacing";
     IOHandler()->enqueue(IOTask(this, aRead));
     IOHandler()->flush();
     Attribute a = Attribute(*aRead.resource);
-    if( *aRead.dtype == DT::VEC_FLOAT )
+    if( *aRead.dtype == DT::VEC_FLOAT || *aRead.dtype == DT::FLOAT )
         setGridSpacing(a.get< std::vector< float > >());
-    else if( *aRead.dtype == DT::FLOAT )
-        setGridSpacing(std::vector< float >({a.get< float >()}));
-    else if( *aRead.dtype == DT::VEC_DOUBLE )
+    else if( *aRead.dtype == DT::VEC_DOUBLE || *aRead.dtype == DT::DOUBLE )
         setGridSpacing(a.get< std::vector< double > >());
-    else if( *aRead.dtype == DT::DOUBLE )
-        setGridSpacing(std::vector< double >({a.get< double >()}));
     else
         throw std::runtime_error("Unexpected Attribute datatype for 'gridSpacing'");
 
     aRead.name = "gridGlobalOffset";
     IOHandler()->enqueue(IOTask(this, aRead));
     IOHandler()->flush();
-    if( *aRead.dtype == DT::VEC_DOUBLE )
+    if( *aRead.dtype == DT::VEC_DOUBLE || *aRead.dtype == DT::DOUBLE )
         setGridGlobalOffset(Attribute(*aRead.resource).get< std::vector< double > >());
-    else if( *aRead.dtype == DT::DOUBLE )
-        setGridGlobalOffset({Attribute(*aRead.resource).get< double >()});
     else
         throw std::runtime_error("Unexpected Attribute datatype for 'gridGlobalOffset'");
 

diff --git a/src/RecordComponent.cpp b/src/RecordComponent.cpp
@@ -343,9 +343,8 @@ RecordComponent::readBase()
 
         // uint64_t check
         Datatype const attrDtype = *aRead.dtype;
-        if( isSame( attrDtype, determineDatatype< uint64_t >() ) )
-            e.push_back( a.get< uint64_t >() );
-        else if( isSame( attrDtype, determineDatatype< std::vector< uint64_t > >() ) )
+        if( isSame( attrDtype, determineDatatype< std::vector< uint64_t > >() )
+            || isSame( attrDtype, determineDatatype< uint64_t >() ) )
             for( auto const& val : a.get< std::vector< uint64_t > >() )
                 e.push_back( val );
         else