From b6d8b2801cd24381040c587360f3d9ae3648bb2b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Franz=20P=C3=B6schel?= Date: Mon, 8 Feb 2021 13:16:56 +0100 Subject: [PATCH 1/6] Test vector of strings with new attribute layout --- test/SerialIOTest.cpp | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/test/SerialIOTest.cpp b/test/SerialIOTest.cpp index f0c999f376..25ab245001 100644 --- a/test/SerialIOTest.cpp +++ b/test/SerialIOTest.cpp @@ -3146,7 +3146,11 @@ bp4_steps( std::string const & file, std::string const & options_write, std::str for( size_t i = 0; i < 10; ++i ) { auto iteration = iterations[ i ]; - auto E_x = iteration.meshes[ "E" ][ "x" ]; + auto E = iteration.meshes[ "E" ]; + auto E_x = E[ "x" ]; + E.setAttribute( + "vector_of_string", + std::vector< std::string >{ "vector", "of", "string" } ); E_x.resetDataset( openPMD::Dataset( openPMD::Datatype::INT, { 10 } ) ); std::vector< int > data( 10, i ); @@ -3165,7 +3169,12 @@ bp4_steps( std::string const & file, std::string const & options_write, std::str size_t last_iteration_index = 0; for( auto iteration : readSeries.readIterations() ) { - auto E_x = iteration.meshes[ "E" ][ "x" ]; + auto E = iteration.meshes[ "E" ]; + auto E_x = E[ "x" ]; + REQUIRE( + E.getAttribute( "vector_of_string" ) + .get< std::vector< std::string > >() == + std::vector< std::string >{ "vector", "of", "string" } ); REQUIRE( E_x.getDimensionality() == 1 ); REQUIRE( E_x.getExtent()[ 0 ] == 10 ); auto chunk = E_x.loadChunk< int >( { 0 }, { 10 } ); From 8ea9f0596dee1c52154c89f1d134dea2d1ccabba Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Franz=20P=C3=B6schel?= Date: Fri, 5 Feb 2021 18:17:48 +0100 Subject: [PATCH 2/6] ADIOS new layout: Fix vector of string chars may become uint8_t on disk which our reading procedures can't handle --- .../IO/ADIOS/ADIOS2PreloadAttributes.hpp | 5 +- src/IO/ADIOS/ADIOS2Auxiliary.cpp | 38 ++++------ src/IO/ADIOS/ADIOS2IOHandler.cpp | 73 +++++++++++++------ src/IO/ADIOS/ADIOS2PreloadAttributes.cpp | 11 +++ 4 files changed, 81 insertions(+), 46 deletions(-) diff --git a/include/openPMD/IO/ADIOS/ADIOS2PreloadAttributes.hpp b/include/openPMD/IO/ADIOS/ADIOS2PreloadAttributes.hpp index cd1ca89dc2..d6f0760a5c 100644 --- a/include/openPMD/IO/ADIOS/ADIOS2PreloadAttributes.hpp +++ b/include/openPMD/IO/ADIOS/ADIOS2PreloadAttributes.hpp @@ -132,8 +132,9 @@ namespace detail * of PreloadAdiosAttributes is called. */ template< typename T > - AttributeWithShape< T > - getAttribute( std::string const & name ) const; + AttributeWithShape< T > getAttribute( std::string const & name ) const; + + Datatype attributeType( std::string const & name ) const; }; template< typename T > diff --git a/src/IO/ADIOS/ADIOS2Auxiliary.cpp b/src/IO/ADIOS/ADIOS2Auxiliary.cpp index e12622493c..42da0141e2 100644 --- a/src/IO/ADIOS/ADIOS2Auxiliary.cpp +++ b/src/IO/ADIOS/ADIOS2Auxiliary.cpp @@ -230,37 +230,29 @@ namespace detail : toVectorType( basicType ); return openPmdType; } - else if( shape.size() == 2 && basicType == Datatype::CHAR ) + else if( + shape.size() == 2 && + ( basicType == Datatype::CHAR || + basicType == Datatype::UCHAR ) ) { return Datatype::VEC_STRING; } else { - throw std::runtime_error( - "[ADIOS2] Unexpected shape for " + attributeName ); + std::stringstream errorMsg; + errorMsg << "[ADIOS2] Unexpected shape for " + << attributeName << ": ["; + for( auto const ext : shape ) + { + errorMsg << std::to_string( ext ) << ", "; + } + errorMsg << "] of type " + << datatypeToString( basicType ); + throw std::runtime_error( errorMsg.str() ); } } } - if( shape.size() <= 1 ) - { - // size == 0 <=> global single value variable - auto size = shape.size() == 0 ? 1 : shape[ 0 ]; - Datatype openPmdType = size == 1 - ? basicType - : size == 7 && basicType == Datatype::DOUBLE - ? Datatype::ARR_DBL_7 - : toVectorType( basicType ); - return openPmdType; - } - else if( shape.size() == 2 && basicType == Datatype::CHAR ) - { - return Datatype::VEC_STRING; - } - else - { - throw std::runtime_error( - "[ADIOS2] Unexpected shape for " + attributeName ); - } + throw std::runtime_error( "Unreachable!" ); } } } // namespace detail diff --git a/src/IO/ADIOS/ADIOS2IOHandler.cpp b/src/IO/ADIOS/ADIOS2IOHandler.cpp index 4f1d3828ed..ed1bb7dd84 100644 --- a/src/IO/ADIOS/ADIOS2IOHandler.cpp +++ b/src/IO/ADIOS/ADIOS2IOHandler.cpp @@ -1659,30 +1659,61 @@ namespace detail std::string name, std::shared_ptr< Attribute::resource > resource ) { - detail::AttributeWithShape< char > attr = - preloadedAttributes.getAttribute< char >( name ); - if( attr.shape.size() != 2 ) - { - throw std::runtime_error( "[ADIOS2] Expecting 2D ADIOS variable" ); - } - size_t height = attr.shape[ 0 ]; - size_t width = attr.shape[ 1 ]; + /* + * char_type parameter only for specifying the "template" type. + */ + auto loadFromDatatype = + [ &preloadedAttributes, &name, &resource ]( auto char_type ) { + using char_t = decltype( char_type ); + detail::AttributeWithShape< char_t > attr = + preloadedAttributes.getAttribute< char_t >( name ); + if( attr.shape.size() != 2 ) + { + throw std::runtime_error( + "[ADIOS2] Expecting 2D ADIOS variable" ); + } + char_t const * loadedData = attr.data; + size_t height = attr.shape[ 0 ]; + size_t width = attr.shape[ 1 ]; - std::vector< std::string > res( height ); - for( size_t i = 0; i < height; ++i ) + std::vector< std::string > res( height ); + for( size_t i = 0; i < height; ++i ) + { + size_t start = i * width; + char const * start_ptr = + reinterpret_cast< char const * >( loadedData + start ); + size_t j = 0; + while( j < width && start_ptr[ j ] != 0 ) + { + ++j; + } + std::string & str = res[ i ]; + str.append( start_ptr, start_ptr + j ); + } + + *resource = res; + }; + /* + * If writing char variables in ADIOS2, they might become uint8_t on + * disk. So allow reading from both types. + */ + switch( preloadedAttributes.attributeType( name ) ) { - size_t start = i * width; - char const * start_ptr = attr.data + start; - size_t j = 0; - while( j < width && start_ptr[ j ] != 0 ) - { - ++j; - } - std::string & str = res[ i ]; - str.append( start_ptr, start_ptr + j ); + case Datatype::CHAR: { + loadFromDatatype( char{} ); + break; + } + case Datatype::UCHAR: { + using uchar_t = unsigned char; + loadFromDatatype( uchar_t{} ); + break; + } + default: { + throw std::runtime_error( + "[ADIOS2] Expecting 2D ADIOS variable of " + "type signed or unsigned char." ); + } } - - *resource = res; } template< typename T, size_t n > diff --git a/src/IO/ADIOS/ADIOS2PreloadAttributes.cpp b/src/IO/ADIOS/ADIOS2PreloadAttributes.cpp index ca32095f68..b55427a3b1 100644 --- a/src/IO/ADIOS/ADIOS2PreloadAttributes.cpp +++ b/src/IO/ADIOS/ADIOS2PreloadAttributes.cpp @@ -288,6 +288,17 @@ namespace detail pair.second ); } } + + Datatype + PreloadAdiosAttributes::attributeType( std::string const & name ) const + { + auto it = m_offsets.find( name ); + if( it == m_offsets.end() ) + { + return Datatype::UNDEFINED; + } + return it->second.dt; + } } // namespace detail } // namespace openPMD From d939233f42769e9cef2414445fd69763242e18a3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Franz=20P=C3=B6schel?= Date: Thu, 11 Feb 2021 13:49:55 +0100 Subject: [PATCH 3/6] Use Datatype::CHAR to represent ADIOS2 signed char --- .../IO/ADIOS/ADIOS2PreloadAttributes.hpp | 13 ++++- src/IO/ADIOS/ADIOS2IOHandler.cpp | 49 ++++++++++++++----- 2 files changed, 49 insertions(+), 13 deletions(-) diff --git a/include/openPMD/IO/ADIOS/ADIOS2PreloadAttributes.hpp b/include/openPMD/IO/ADIOS/ADIOS2PreloadAttributes.hpp index d6f0760a5c..ad8e96b0ee 100644 --- a/include/openPMD/IO/ADIOS/ADIOS2PreloadAttributes.hpp +++ b/include/openPMD/IO/ADIOS/ADIOS2PreloadAttributes.hpp @@ -27,6 +27,7 @@ #include #include #include +#include #include "openPMD/Datatype.hpp" @@ -148,8 +149,18 @@ namespace detail "[ADIOS2] Requested attribute not found: " + name ); } AttributeLocation const & location = it->second; - if( location.dt != determineDatatype< T >() ) + Datatype determinedDatatype = determineDatatype< T >(); + if( std::is_same< T, signed char >::value ) { + // workaround: we use Datatype::CHAR to represent ADIOS2 signed char + // (ADIOS2 does not have chars with unspecified signed-ness + // anyway) + determinedDatatype = Datatype::CHAR; + } + if( location.dt != determinedDatatype ) + { + std::cout << "location.dt=" << location.dt << + "\tdetermineDatatype=" << determineDatatype< T > () << std::endl; throw std::runtime_error( "[ADIOS2] Wrong datatype for attribute: " + name ); } diff --git a/src/IO/ADIOS/ADIOS2IOHandler.cpp b/src/IO/ADIOS/ADIOS2IOHandler.cpp index ed1bb7dd84..97ee97f839 100644 --- a/src/IO/ADIOS/ADIOS2IOHandler.cpp +++ b/src/IO/ADIOS/ADIOS2IOHandler.cpp @@ -1677,30 +1677,55 @@ namespace detail size_t width = attr.shape[ 1 ]; std::vector< std::string > res( height ); - for( size_t i = 0; i < height; ++i ) + if( std::is_signed< char >::value == + std::is_signed< char_t >::value ) { - size_t start = i * width; - char const * start_ptr = - reinterpret_cast< char const * >( loadedData + start ); - size_t j = 0; - while( j < width && start_ptr[ j ] != 0 ) + // reinterpret_cast-ing will do + for( size_t i = 0; i < height; ++i ) { - ++j; + size_t start = i * width; + char const * start_ptr = + reinterpret_cast< char const * >( + loadedData + start ); + size_t j = 0; + while( j < width && start_ptr[ j ] != 0 ) + { + ++j; + } + std::string & str = res[ i ]; + str.append( start_ptr, start_ptr + j ); + } + } + else + { + // let's play safe and convert explicitly + std::vector< char > converted( width ); + for( size_t i = 0; i < height; ++i ) + { + size_t start = i * width; + auto const * start_ptr = loadedData + start; + size_t j = 0; + while( j < width && start_ptr[ j ] != 0 ) + { + converted[ j ] = start_ptr[ j ]; + ++j; + } + std::string & str = res[ i ]; + str.append( converted.data(), converted.data() + j ); } - std::string & str = res[ i ]; - str.append( start_ptr, start_ptr + j ); } *resource = res; }; /* - * If writing char variables in ADIOS2, they might become uint8_t on - * disk. So allow reading from both types. + * If writing char variables in ADIOS2, they might become either int8_t + * or uint8_t on disk. So allow reading from both types. */ switch( preloadedAttributes.attributeType( name ) ) { case Datatype::CHAR: { - loadFromDatatype( char{} ); + using schar_t = signed char; + loadFromDatatype( schar_t{} ); break; } case Datatype::UCHAR: { From 97053aaa04b291e9c59927c861cc96e2ac9eedeb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Franz=20P=C3=B6schel?= Date: Mon, 22 Feb 2021 12:07:32 +0100 Subject: [PATCH 4/6] Inline code commentary and better error messages --- .../IO/ADIOS/ADIOS2PreloadAttributes.hpp | 10 ++++--- src/IO/ADIOS/ADIOS2IOHandler.cpp | 27 ++++++++++++++++--- 2 files changed, 30 insertions(+), 7 deletions(-) diff --git a/include/openPMD/IO/ADIOS/ADIOS2PreloadAttributes.hpp b/include/openPMD/IO/ADIOS/ADIOS2PreloadAttributes.hpp index ad8e96b0ee..e50fe18d1e 100644 --- a/include/openPMD/IO/ADIOS/ADIOS2PreloadAttributes.hpp +++ b/include/openPMD/IO/ADIOS/ADIOS2PreloadAttributes.hpp @@ -26,6 +26,7 @@ #include #include #include +#include #include #include @@ -159,10 +160,11 @@ namespace detail } if( location.dt != determinedDatatype ) { - std::cout << "location.dt=" << location.dt << - "\tdetermineDatatype=" << determineDatatype< T > () << std::endl; - throw std::runtime_error( - "[ADIOS2] Wrong datatype for attribute: " + name ); + std::stringstream errorMsg; + errorMsg << "[ADIOS2] Wrong datatype for attribute: " << name + << "(location.dt=" << location.dt + << ", T=" << determineDatatype< T >() << ")"; + throw std::runtime_error( errorMsg.str() ); } AttributeWithShape< T > res; res.shape = location.shape; diff --git a/src/IO/ADIOS/ADIOS2IOHandler.cpp b/src/IO/ADIOS/ADIOS2IOHandler.cpp index 97ee97f839..fc6785b5e7 100644 --- a/src/IO/ADIOS/ADIOS2IOHandler.cpp +++ b/src/IO/ADIOS/ADIOS2IOHandler.cpp @@ -1680,7 +1680,15 @@ namespace detail if( std::is_signed< char >::value == std::is_signed< char_t >::value ) { - // reinterpret_cast-ing will do + /* + * This branch is chosen if the signedness of the + * ADIOS variable corresponds with the signedness of the + * char type on the current platform. + * In this case, the C++ standard guarantees that the + * representations for char and (un)signed char are + * identical, reinterpret_cast-ing the loadedData to + * char in order to construct our strings will be fine. + */ for( size_t i = 0; i < height; ++i ) { size_t start = i * width; @@ -1698,7 +1706,13 @@ namespace detail } else { - // let's play safe and convert explicitly + /* + * This branch is chosen if the signedness of the + * ADIOS variable is different from the signedness of the + * char type on the current platform. + * In this case, we play it safe, and explicitly convert + * the loadedData to char pointwise. + */ std::vector< char > converted( width ); for( size_t i = 0; i < height; ++i ) { @@ -1719,10 +1733,17 @@ namespace detail }; /* * If writing char variables in ADIOS2, they might become either int8_t - * or uint8_t on disk. So allow reading from both types. + * or uint8_t on disk depending on the platform. + * So allow reading from both types. */ switch( preloadedAttributes.attributeType( name ) ) { + /* + * Workaround for two bugs at once: + * ADIOS2 does not have an explicit char type, + * we don't have an explicit schar type. + * Until this is fixed, we use CHAR to represent ADIOS signed char. + */ case Datatype::CHAR: { using schar_t = signed char; loadFromDatatype( schar_t{} ); From 87dbbb61deaed0bf6c274985e92af09fc3bf17c1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Franz=20P=C3=B6schel?= Date: Mon, 22 Feb 2021 12:21:07 +0100 Subject: [PATCH 5/6] test cross platform chars in ADIOS2 --- test/SerialIOTest.cpp | 123 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 123 insertions(+) diff --git a/test/SerialIOTest.cpp b/test/SerialIOTest.cpp index 25ab245001..6c2a95858a 100644 --- a/test/SerialIOTest.cpp +++ b/test/SerialIOTest.cpp @@ -8,6 +8,9 @@ #include "openPMD/auxiliary/Filesystem.hpp" #include "openPMD/openPMD.hpp" +#if openPMD_HAVE_ADIOS2 +#include +#endif #include #include @@ -38,6 +41,126 @@ std::vector< std::string > testedFileExtensions() return { allExtensions.begin(), newEnd }; } +#if openPMD_HAVE_ADIOS2 +TEST_CASE( "adios2_char_portability", "[serial][adios2]" ) +{ + /* + * This tests portability of char attributes in ADIOS2 in schema 20210209. + */ + + // @todo remove new_attribute_layout key as soon as schema-based versioning + // is merged + std::string const config = R"END( +{ + "adios2": + { + "new_attribute_layout": true, + "schema": 20210209 + } +})END"; + { + adios2::ADIOS adios; + auto IO = adios.DeclareIO( "IO" ); + auto engine = IO.Open( + "../samples/adios2_char_portability.bp", adios2::Mode::Write ); + engine.BeginStep(); + + // write default openPMD attributes + auto writeAttribute = + [ &engine, &IO ]( std::string const & name, auto value ) + { + using variable_type = decltype( value ); + engine.Put( IO.DefineVariable< variable_type >( name ), value ); + }; + writeAttribute( "/basePath", std::string( "/data/%T/" ) ); + writeAttribute( "/date", std::string( "2021-02-22 11:14:00 +0000" ) ); + writeAttribute( "/iterationEncoding", std::string( "groupBased" ) ); + writeAttribute( "/iterationFormat", std::string( "/data/%T/" ) ); + writeAttribute( "/openPMD", std::string( "1.1.0" ) ); + writeAttribute( "/openPMDextension", uint32_t( 0 ) ); + writeAttribute( "/software", std::string( "openPMD-api" ) ); + writeAttribute( "/softwareVersion", std::string( "0.14.0-dev" ) ); + + IO.DefineAttribute< uint64_t >( + "__openPMD_internal/openPMD2_adios2_schema", 20210209 ); + IO.DefineAttribute< unsigned char >( "__openPMD_internal/useSteps", 1 ); + + // write char things that should be read back properly + + std::string baseString = "abcdefghi"; + // null termination not necessary, ADIOS knows the size of its variables + std::vector< signed char > signedVector( 9 ); + std::vector< unsigned char > unsignedVector( 9 ); + for( unsigned i = 0; i < 9; ++i ) + { + signedVector[ i ] = baseString[ i ]; + unsignedVector[ i ] = baseString[ i ]; + } + engine.Put( + IO.DefineVariable< signed char >( + "/signedVector", { 3, 3 }, { 0, 0 }, { 3, 3 } ), + signedVector.data() ); + engine.Put( + IO.DefineVariable< unsigned char >( + "/unsignedVector", { 3, 3 }, { 0, 0 }, { 3, 3 } ), + unsignedVector.data() ); + engine.Put( + IO.DefineVariable< char >( + "/unspecifiedVector", { 3, 3 }, { 0, 0 }, { 3, 3 } ), + baseString.c_str() ); + + writeAttribute( "/signedChar", ( signed char )'a' ); + writeAttribute( "/unsignedChar", ( unsigned char )'a' ); + writeAttribute( "/char", ( char )'a' ); + + engine.EndStep(); + engine.Close(); + } + + { + if( auxiliary::getEnvString( "OPENPMD_BP_BACKEND", "ADIOS2" ) != + "ADIOS2" ) + { + return; + } + Series read( + "../samples/adios2_char_portability.bp", + Access::READ_ONLY, + config ); + auto signedVectorAttribute = read.getAttribute( "signedVector" ); + REQUIRE( signedVectorAttribute.dtype == Datatype::VEC_STRING ); + auto unsignedVectorAttribute = read.getAttribute( "unsignedVector" ); + REQUIRE( unsignedVectorAttribute.dtype == Datatype::VEC_STRING ); + auto unspecifiedVectorAttribute = + read.getAttribute( "unspecifiedVector" ); + REQUIRE( unspecifiedVectorAttribute.dtype == Datatype::VEC_STRING ); + std::vector< std::string > desiredVector{ "abc", "def", "ghi" }; + REQUIRE( + signedVectorAttribute.get< std::vector< std::string > >() == + desiredVector ); + REQUIRE( + unsignedVectorAttribute.get< std::vector< std::string > >() == + desiredVector ); + REQUIRE( + unspecifiedVectorAttribute.get< std::vector< std::string > >() == + desiredVector ); + + auto signedCharAttribute = read.getAttribute( "signedChar" ); + // we don't have that datatype yet + // REQUIRE(unsignedCharAttribute.dtype == Datatype::SCHAR); + auto unsignedCharAttribute = read.getAttribute( "unsignedChar" ); + REQUIRE( unsignedCharAttribute.dtype == Datatype::UCHAR ); + auto charAttribute = read.getAttribute( "char" ); + // might currently report Datatype::UCHAR on some platforms + // REQUIRE(unsignedCharAttribute.dtype == Datatype::CHAR); + + REQUIRE( signedCharAttribute.get< char >() == char( 'a' ) ); + REQUIRE( unsignedCharAttribute.get< char >() == char( 'a' ) ); + REQUIRE( charAttribute.get< char >() == char( 'a' ) ); + } +} +#endif + void write_and_read_many_iterations( std::string const & ext ) { constexpr unsigned int nIterations = 1000; From d424b02861230d9e249c3e7d5a554f31c16478e7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Franz=20P=C3=B6schel?= Date: Fri, 5 Mar 2021 15:06:44 +0100 Subject: [PATCH 6/6] Skip test if new layout env var is set --- test/SerialIOTest.cpp | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/test/SerialIOTest.cpp b/test/SerialIOTest.cpp index 6c2a95858a..f97a98d4b2 100644 --- a/test/SerialIOTest.cpp +++ b/test/SerialIOTest.cpp @@ -48,6 +48,15 @@ TEST_CASE( "adios2_char_portability", "[serial][adios2]" ) * This tests portability of char attributes in ADIOS2 in schema 20210209. */ + if( auxiliary::getEnvString("OPENPMD_NEW_ATTRIBUTE_LAYOUT", "NOT_SET") == "NOT_SET") + { + /* + * @todo As soon as we have added automatic detection for the new + * layout, this environment variable should be ignore read-side. + * Then we can delete this if condition again. + */ + return; + } // @todo remove new_attribute_layout key as soon as schema-based versioning // is merged std::string const config = R"END(