From f3356e20f70ebdbbd363b74daee62ec68d18202f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Franz=20P=C3=B6schel?= Date: Wed, 11 Aug 2021 07:51:09 +0200 Subject: [PATCH 1/6] Bug fix: Don't forget closing files (#1083) * Failing test * Bug fix: Don't forget closing files --- src/Series.cpp | 94 ++++++++++++++++++++++++------------------- test/SerialIOTest.cpp | 30 ++++++++++---- 2 files changed, 76 insertions(+), 48 deletions(-) diff --git a/src/Series.cpp b/src/Series.cpp index 7e0dcbe895..19c2de72b3 100644 --- a/src/Series.cpp +++ b/src/Series.cpp @@ -547,18 +547,18 @@ SeriesInterface::flushFileBased( iterations_iterator begin, iterations_iterator if( IOHandler()->m_frontendAccess == Access::READ_ONLY ) for( auto it = begin; it != end; ++it ) { + // Phase 1 switch( openIterationIfDirty( it->first, it->second ) ) { using IO = IterationOpened; - case IO::RemainsClosed: - continue; case IO::HasBeenOpened: - // continue below + it->second.flush(); + break; + case IO::RemainsClosed: break; } - it->second.flush(); - + // Phase 2 if( *it->second.m_closed == Iteration::CloseStatus::ClosedInFrontend ) { @@ -567,6 +567,8 @@ SeriesInterface::flushFileBased( iterations_iterator begin, iterations_iterator IOTask( &it->second, std::move( fClose ) ) ); *it->second.m_closed = Iteration::CloseStatus::ClosedInBackend; } + + // Phase 3 IOHandler()->flush(); } else @@ -574,31 +576,33 @@ SeriesInterface::flushFileBased( iterations_iterator begin, iterations_iterator bool allDirty = dirty(); for( auto it = begin; it != end; ++it ) { + // Phase 1 switch( openIterationIfDirty( it->first, it->second ) ) { using IO = IterationOpened; - case IO::RemainsClosed: - continue; - case IO::HasBeenOpened: - // continue below - break; - } - - /* as there is only one series, - * emulate the file belonging to each iteration as not yet written - */ - written() = false; - series.iterations.written() = false; + case IO::HasBeenOpened: { + /* as there is only one series, + * emulate the file belonging to each iteration as not yet + * written + */ + written() = false; + series.iterations.written() = false; - dirty() |= it->second.dirty(); - std::string filename = iterationFilename( it->first ); - it->second.flushFileBased( filename, it->first ); + dirty() |= it->second.dirty(); + std::string filename = iterationFilename( it->first ); + it->second.flushFileBased( filename, it->first ); - series.iterations.flush( - auxiliary::replace_first( basePath(), "%T/", "" ) ); + series.iterations.flush( + auxiliary::replace_first( basePath(), "%T/", "" ) ); - flushAttributes(); + flushAttributes(); + break; + } + case IO::RemainsClosed: + break; + } + // Phase 2 if( *it->second.m_closed == Iteration::CloseStatus::ClosedInFrontend ) { @@ -608,6 +612,7 @@ SeriesInterface::flushFileBased( iterations_iterator begin, iterations_iterator *it->second.m_closed = Iteration::CloseStatus::ClosedInBackend; } + // Phase 3 IOHandler()->flush(); /* reset the dirty bit for every iteration (i.e. file) @@ -625,23 +630,26 @@ SeriesInterface::flushGorVBased( iterations_iterator begin, iterations_iterator if( IOHandler()->m_frontendAccess == Access::READ_ONLY ) for( auto it = begin; it != end; ++it ) { + // Phase 1 switch( openIterationIfDirty( it->first, it->second ) ) { using IO = IterationOpened; - case IO::RemainsClosed: - continue; case IO::HasBeenOpened: - // continue below + it->second.flush(); + break; + case IO::RemainsClosed: break; } - it->second.flush(); + // Phase 2 if( *it->second.m_closed == Iteration::CloseStatus::ClosedInFrontend ) { // the iteration has no dedicated file in group-based mode *it->second.m_closed = Iteration::CloseStatus::ClosedInBackend; } + + // Phase 3 IOHandler()->flush(); } else @@ -654,26 +662,23 @@ SeriesInterface::flushGorVBased( iterations_iterator begin, iterations_iterator IOHandler()->enqueue(IOTask(this, fCreate)); } - series.iterations.flush(auxiliary::replace_first(basePath(), "%T/", "")); + series.iterations.flush( + auxiliary::replace_first( basePath(), "%T/", "" ) ); for( auto it = begin; it != end; ++it ) { + // Phase 1 switch( openIterationIfDirty( it->first, it->second ) ) { using IO = IterationOpened; - case IO::RemainsClosed: - continue; case IO::HasBeenOpened: - // continue below - break; - } - if( !it->second.written() ) - { - it->second.parent() = getWritable( &series.iterations ); - } - switch( iterationEncoding() ) - { - using IE = IterationEncoding; + if( !it->second.written() ) + { + it->second.parent() = getWritable( &series.iterations ); + } + switch( iterationEncoding() ) + { + using IE = IterationEncoding; case IE::groupBased: it->second.flushGroupBased( it->first ); break; @@ -683,8 +688,15 @@ SeriesInterface::flushGorVBased( iterations_iterator begin, iterations_iterator default: throw std::runtime_error( "[Series] Internal control flow error" ); + } + break; + case IO::RemainsClosed: + break; } - if( *it->second.m_closed == Iteration::CloseStatus::ClosedInFrontend ) + + // Phase 2 + if( *it->second.m_closed == + Iteration::CloseStatus::ClosedInFrontend ) { // the iteration has no dedicated file in group-based mode *it->second.m_closed = Iteration::CloseStatus::ClosedInBackend; diff --git a/test/SerialIOTest.cpp b/test/SerialIOTest.cpp index ebd94457b7..203ae4b496 100644 --- a/test/SerialIOTest.cpp +++ b/test/SerialIOTest.cpp @@ -171,11 +171,17 @@ TEST_CASE( "adios2_char_portability", "[serial][adios2]" ) } #endif -void -write_and_read_many_iterations( std::string const & ext ) { +void write_and_read_many_iterations( + std::string const & ext, bool intermittentFlushes ) +{ // the idea here is to trigger the maximum allowed number of file handles, // e.g., the upper limit in "ulimit -n" (default: often 1024). Once this // is reached, files should be closed automatically for open iterations + + // By flushing the series before closing an iteration, we ensure that the + // iteration is not dirty before closing + // Our flushing logic must not forget to close even if the iteration is + // otherwise untouched and needs not be flushed. unsigned int nIterations = auxiliary::getEnvNum( "OPENPMD_TEST_NFILES_MAX", 1030 ); std::string filename = "../samples/many_iterations/many_iterations_%T." + ext; @@ -189,8 +195,12 @@ write_and_read_many_iterations( std::string const & ext ) { // std::cout << "Putting iteration " << i << std::endl; Iteration it = write.iterations[i]; auto E_x = it.meshes["E"]["x"]; - E_x.resetDataset(ds); - E_x.storeChunk(data, {0}, {10}); + E_x.resetDataset( ds ); + E_x.storeChunk( data, { 0 }, { 10 } ); + if( intermittentFlushes ) + { + write.flush(); + } it.close(); } // ~Series intentionally not yet called @@ -201,8 +211,12 @@ write_and_read_many_iterations( std::string const & ext ) { iteration.second.open(); // std::cout << "Reading iteration " << iteration.first << // std::endl; - auto E_x = iteration.second.meshes["E"]["x"]; - auto chunk = E_x.loadChunk({0}, {10}); + auto E_x = iteration.second.meshes[ "E" ][ "x" ]; + auto chunk = E_x.loadChunk< float >( { 0 }, { 10 } ); + if( intermittentFlushes ) + { + read.flush(); + } iteration.second.close(); auto array = chunk.get(); @@ -218,11 +232,13 @@ write_and_read_many_iterations( std::string const & ext ) { TEST_CASE( "write_and_read_many_iterations", "[serial]" ) { + bool intermittentFlushes = false; if( auxiliary::directory_exists( "../samples/many_iterations" ) ) auxiliary::remove_directory( "../samples/many_iterations" ); for( auto const & t : testedFileExtensions() ) { - write_and_read_many_iterations( t ); + write_and_read_many_iterations( t, intermittentFlushes ); + intermittentFlushes = !intermittentFlushes; } } From a968d6b7f21741830099474157d43402ee3b5785 Mon Sep 17 00:00:00 2001 From: Axel Huebl Date: Tue, 10 Aug 2021 23:47:27 -0700 Subject: [PATCH 2/6] 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. --- src/IO/HDF5/HDF5IOHandler.cpp | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/src/IO/HDF5/HDF5IOHandler.cpp b/src/IO/HDF5/HDF5IOHandler.cpp index f91d4fe51a..f024cff6a6 100644 --- 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); From 34f04c12b944ddf49b94cd4ffb015db5d36778a2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Franz=20P=C3=B6schel?= Date: Wed, 11 Aug 2021 23:24:01 +0200 Subject: [PATCH 3/6] 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) --- include/openPMD/backend/Attribute.hpp | 116 ++++++++++++++++++++++++-- src/Mesh.cpp | 16 +--- src/RecordComponent.cpp | 5 +- src/backend/MeshRecordComponent.cpp | 12 +-- src/backend/PatchRecord.cpp | 15 +--- test/CoreTest.cpp | 70 ++++++++++++++++ 6 files changed, 188 insertions(+), 46 deletions(-) diff --git a/include/openPMD/backend/Attribute.hpp b/include/openPMD/backend/Attribute.hpp index 900d807be0..5471283662 100644 --- 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 { - template< typename PV > - U operator()( PV ) + U operator()( T * ) { throw std::runtime_error("getCast: no cast possible."); } @@ -108,8 +107,7 @@ struct DoConvert template< typename T, typename U > struct DoConvert { - template< typename PV > - U operator()( PV pv ) + U operator()( T * pv ) { return static_cast< U >( *pv ); } @@ -120,8 +118,8 @@ struct DoConvert, std::vector< U >, false> { static constexpr bool convertible = std::is_convertible::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< 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, false> +{ + static constexpr bool convertible = std::is_convertible::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::vector< U >, false> +{ + static constexpr bool convertible = std::is_convertible::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::array< U, n >, false> +{ + static constexpr bool convertible = std::is_convertible::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{}(pvalue_c); else if(auto pvalue_uc = variantSrc::get_if< unsigned char >( &v ) ) @@ -226,6 +317,15 @@ getCast( Attribute const & a ) return DoConvert{}(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/src/Mesh.cpp b/src/Mesh.cpp index 5d8ebe439d..e9551c42e6 100644 --- a/src/Mesh.cpp +++ b/src/Mesh.cpp @@ -318,10 +318,8 @@ 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'"); @@ -329,24 +327,18 @@ Mesh::read() 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 index bff6286e0e..d40061c198 100644 --- 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 diff --git a/src/backend/MeshRecordComponent.cpp b/src/backend/MeshRecordComponent.cpp index 0a3a72d597..8c9634a32c 100644 --- a/src/backend/MeshRecordComponent.cpp +++ b/src/backend/MeshRecordComponent.cpp @@ -39,18 +39,12 @@ MeshRecordComponent::read() 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 ) setPosition(a.get< std::vector< float > >()); - else if( *aRead.dtype == DT::FLOAT ) - setPosition(std::vector< float >({a.get< float >()})); - else if( *aRead.dtype == DT::VEC_DOUBLE ) + else if( *aRead.dtype == DT::VEC_DOUBLE || *aRead.dtype == DT::DOUBLE ) setPosition(a.get< std::vector< double > >()); - else if( *aRead.dtype == DT::DOUBLE ) - setPosition(std::vector< double >({a.get< double >()})); - else if( *aRead.dtype == DT::VEC_LONG_DOUBLE ) + else if( *aRead.dtype == DT::VEC_LONG_DOUBLE || *aRead.dtype == DT::LONG_DOUBLE ) setPosition(a.get< std::vector< long double > >()); - else if( *aRead.dtype == DT::LONG_DOUBLE ) - setPosition(std::vector< long double >({a.get< long double >()})); else throw std::runtime_error( "Unexpected Attribute datatype for 'position'"); diff --git a/src/backend/PatchRecord.cpp b/src/backend/PatchRecord.cpp index 9179c03c78..3926677a30 100644 --- a/src/backend/PatchRecord.cpp +++ b/src/backend/PatchRecord.cpp @@ -62,21 +62,8 @@ PatchRecord::read() IOHandler()->enqueue(IOTask(this, aRead)); IOHandler()->flush(); - if( *aRead.dtype == Datatype::ARR_DBL_7 ) + if( *aRead.dtype == Datatype::ARR_DBL_7 || *aRead.dtype == Datatype::VEC_DOUBLE ) this->setAttribute("unitDimension", Attribute(*aRead.resource).template get< std::array< double, 7 > >()); - else if( *aRead.dtype == Datatype::VEC_DOUBLE ) - { - auto vec = Attribute(*aRead.resource).template get< std::vector< double > >(); - if( vec.size() == 7 ) - { - std::array< double, 7 > arr; - std::copy(vec.begin(), - vec.end(), - arr.begin()); - this->setAttribute("unitDimension", arr); - } else - throw std::runtime_error("Unexpected Attribute datatype for 'unitDimension'"); - } else throw std::runtime_error("Unexpected Attribute datatype for 'unitDimension'"); diff --git a/test/CoreTest.cpp b/test/CoreTest.cpp index 8e5b2c9937..72830b0df2 100644 --- a/test/CoreTest.cpp +++ b/test/CoreTest.cpp @@ -950,3 +950,73 @@ TEST_CASE( "load_chunk_wrong_datatype", "[core]" ) "Type conversion during chunk loading not yet implemented" ) ); } } + +TEST_CASE( "DoConvert_single_value_to_vector", "[core]" ) +{ +#if openPMD_HAVE_ADIOS2 + { + Series write( "../samples/writeSingleMesh.bp", Access::CREATE ); + auto E_x = write.iterations[ 0 ].meshes[ "E" ][ "x" ]; + E_x.resetDataset( { Datatype::INT, { 10 } } ); + E_x.makeConstant( 10 ); + } + { + Series read( "../samples/writeSingleMesh.bp", Access::READ_ONLY ); + auto E = read.iterations[ 0 ].meshes[ "E" ]; + REQUIRE( E.axisLabels() == std::vector< std::string >{ "x" } ); + } +#endif + { + char val = 'x'; + Attribute attr{ val }; + + // the following conversions should be possible + REQUIRE( attr.get< char >() == 'x' ); // no conversion + REQUIRE( attr.get< unsigned char >() == 'x' ); + REQUIRE( attr.get< signed char >() == 'x' ); + // all the previous ones, but make them single-element vectors now + REQUIRE( + attr.get< std::vector< char > >() == std::vector< char >{ 'x' } ); + REQUIRE( + attr.get< std::vector< unsigned char > >() == + std::vector< unsigned char >{ 'x' } ); + REQUIRE( + attr.get< std::vector< signed char > >() == + std::vector< signed char >{ 'x' } ); + } + { + std::array< double, 7 > array{{ 0, 1, 2, 3, 4, 5, 6 }}; + Attribute attr{ array }; + + // the following conversions should be possible + REQUIRE( attr.get< std::array< double, 7 > >() == array ); + // we don't need array-to-array conversions, + // so array< int, 7 > cannot be loaded here + REQUIRE( + attr.get< std::vector< double > >() == + std::vector< double >{ 0, 1, 2, 3, 4, 5, 6 } ); + REQUIRE( + attr.get< std::vector< int > >() == + std::vector< int >{ 0, 1, 2, 3, 4, 5, 6 } ); + } + { + std::vector< double > vector{ 0, 1, 2, 3, 4, 5, 6 }; + std::array< double, 7 > arraydouble{{ 0, 1, 2, 3, 4, 5, 6 }}; + std::array< int, 7 > arrayint{{ 0, 1, 2, 3, 4, 5, 6 }}; + Attribute attr{ vector }; + + // the following conversions should be possible + REQUIRE( attr.get< std::array< double, 7 > >() == arraydouble ); + REQUIRE( attr.get< std::array< int, 7 > >() == arrayint ); + REQUIRE_THROWS_WITH( + ( attr.get< std::array< int, 8 > >() ), + Catch::Equals( "getCast: no vector to array conversion possible " + "(wrong requested array size)." ) ); + REQUIRE( + attr.get< std::vector< double > >() == + std::vector< double >{ 0, 1, 2, 3, 4, 5, 6 } ); + REQUIRE( + attr.get< std::vector< int > >() == + std::vector< int >{ 0, 1, 2, 3, 4, 5, 6 } ); + } +} From 39d01a0d866cf0ffaf1f66e1826854a70033b72a Mon Sep 17 00:00:00 2001 From: Axel Huebl Date: Fri, 13 Aug 2021 09:48:13 -0700 Subject: [PATCH 4/6] 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 --- include/openPMD/backend/Attributable.hpp | 25 ++++++++++++++++++++++++ test/ParallelIOTest.cpp | 2 ++ test/SerialIOTest.cpp | 2 ++ 3 files changed, 29 insertions(+) diff --git a/include/openPMD/backend/Attributable.hpp b/include/openPMD/backend/Attributable.hpp index 1e3c82a8af..a1e0c05a95 100644 --- a/include/openPMD/backend/Attributable.hpp +++ b/include/openPMD/backend/Attributable.hpp @@ -31,6 +31,7 @@ #include #include #include +#include // 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/test/ParallelIOTest.cpp b/test/ParallelIOTest.cpp index d271888322..da93297358 100644 --- a/test/ParallelIOTest.cpp +++ b/test/ParallelIOTest.cpp @@ -13,6 +13,7 @@ # include # include # include +# include # include # include # include @@ -226,6 +227,7 @@ TEST_CASE( "hdf5_write_test", "[parallel][hdf5]" ) auto mpi_rank = static_cast(mpi_r); Series o = Series("../samples/parallel_write.h5", Access::CREATE, MPI_COMM_WORLD); + REQUIRE_THROWS_AS(o.setAuthor(""), std::runtime_error); o.setAuthor("Parallel HDF5"); ParticleSpecies& e = o.iterations[1].particles["e"]; diff --git a/test/SerialIOTest.cpp b/test/SerialIOTest.cpp index 203ae4b496..87a0eee4a8 100644 --- a/test/SerialIOTest.cpp +++ b/test/SerialIOTest.cpp @@ -24,6 +24,7 @@ #include #include #include +#include #include #include #include @@ -1785,6 +1786,7 @@ void bool_test(const std::string & backend) { Series o = Series("../samples/serial_bool." + backend, Access::CREATE); + REQUIRE_THROWS_AS(o.setAuthor(""), std::runtime_error); o.setAttribute("Bool attribute (true)", true); o.setAttribute("Bool attribute (false)", false); } From 74c094190dffd5da262b09276cc02706a019222f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Franz=20P=C3=B6schel?= Date: Tue, 17 Aug 2021 22:53:42 +0200 Subject: [PATCH 5/6] Don't read iterations if they have already been parsed (#1089) --- src/Series.cpp | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/Series.cpp b/src/Series.cpp index 19c2de72b3..073d55968d 100644 --- a/src/Series.cpp +++ b/src/Series.cpp @@ -969,13 +969,15 @@ SeriesInterface::readGorVBased( bool do_init ) auto readSingleIteration = [&series, &pOpen, this] - (uint64_t index, std::string path, bool guardClosed ) + (uint64_t index, std::string path, bool guardAgainstRereading ) { if( series.iterations.contains( index ) ) { // maybe re-read auto & i = series.iterations.at( index ); - if( guardClosed && i.closedByWriter() ) + // i.written(): the iteration has already been parsed + // reparsing is not needed + if( guardAgainstRereading && i.written() ) { return; } From e4dc077a58d70c517d3e9ae36827dd6e80cf1f3a Mon Sep 17 00:00:00 2001 From: Axel Huebl Date: Tue, 17 Aug 2021 19:18:04 -0700 Subject: [PATCH 6/6] Release: 0.14.2 --- CHANGELOG.rst | 26 ++++++++++++++++++++++++++ CITATION.cff | 2 +- CMakeLists.txt | 2 +- docs/source/conf.py | 4 ++-- docs/source/index.rst | 2 +- include/openPMD/version.hpp | 2 +- setup.py | 2 +- test/SerialIOTest.cpp | 2 +- 8 files changed, 34 insertions(+), 8 deletions(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index a71b677542..135f8033d5 100644 --- 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 index fd78e38845..190a3cfcf5 100644 --- a/CITATION.cff +++ b/CITATION.cff @@ -25,7 +25,7 @@ contact: orcid: https://orcid.org/0000-0003-1943-7141 email: axelhuebl@lbl.gov 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 index f6eb11f2b2..c7909e5a61 100644 --- 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 index ad14d6c731..317b8b9e97 100644 --- 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 index 87ce86d57f..2b3f951cf3 100644 --- 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/version.hpp b/include/openPMD/version.hpp index a97ed3c798..89e2c1498f 100644 --- 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 index b81f516a77..59497492f2 100644 --- 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='axelhuebl@lbl.gov, f.poeschel@hzdr.de', maintainer='Axel Huebl', diff --git a/test/SerialIOTest.cpp b/test/SerialIOTest.cpp index 87a0eee4a8..35ee6351ae 100644 --- a/test/SerialIOTest.cpp +++ b/test/SerialIOTest.cpp @@ -90,7 +90,7 @@ TEST_CASE( "adios2_char_portability", "[serial][adios2]" ) writeAttribute( "/openPMD", std::string( "1.1.0" ) ); writeAttribute( "/openPMDextension", uint32_t( 0 ) ); writeAttribute( "/software", std::string( "openPMD-api" ) ); - writeAttribute( "/softwareVersion", std::string( "0.14.1-dev" ) ); + writeAttribute( "/softwareVersion", std::string( "0.14.2" ) ); IO.DefineAttribute< uint64_t >( "__openPMD_internal/openPMD2_adios2_schema", 20210209 );