From 68525e15fa54b99910d393f684bb1b3ec8a8a525 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Franz=20P=C3=B6schel?= Date: Fri, 6 Aug 2021 12:49:38 +0200 Subject: [PATCH 1/5] Reparse iteration encoding when changing it or the filename --- include/openPMD/Error.hpp | 12 ++++++++++++ include/openPMD/Series.hpp | 2 +- src/Error.cpp | 5 +++++ src/Series.cpp | 36 +++++++++++++++++++++++++++--------- test/SerialIOTest.cpp | 35 +++++++++++++++++++++++++++++++++++ 5 files changed, 80 insertions(+), 10 deletions(-) diff --git a/include/openPMD/Error.hpp b/include/openPMD/Error.hpp index 84db417275..0afb69998a 100644 --- a/include/openPMD/Error.hpp +++ b/include/openPMD/Error.hpp @@ -50,5 +50,17 @@ namespace error OperationUnsupportedInBackend( std::string backend_in, std::string what ); }; + + /** + * @brief The API was used in an illegal way. + * + * Example: File-based iteration encoding is selected without specifying an + * expansion pattern. + */ + class WrongAPIUsage : public Error + { + public: + WrongAPIUsage( std::string what ); + }; } } diff --git a/include/openPMD/Series.hpp b/include/openPMD/Series.hpp index 2469d5c29c..3c21df532c 100644 --- a/include/openPMD/Series.hpp +++ b/include/openPMD/Series.hpp @@ -84,7 +84,7 @@ class SeriesData : public AttributableData std::string m_name; std::string m_filenamePrefix; std::string m_filenamePostfix; - int m_filenamePadding; + int m_filenamePadding = 0; IterationEncoding m_iterationEncoding{}; Format m_format; /** diff --git a/src/Error.cpp b/src/Error.cpp index 9998784276..c91331f52c 100644 --- a/src/Error.cpp +++ b/src/Error.cpp @@ -15,5 +15,10 @@ namespace error , backend{ std::move( backend_in ) } { } + + WrongAPIUsage::WrongAPIUsage( std::string what ) + : Error( "Wrong API usage: " + what ) + { + } } } diff --git a/src/Series.cpp b/src/Series.cpp index 19c2de72b3..d28f703154 100644 --- a/src/Series.cpp +++ b/src/Series.cpp @@ -18,15 +18,16 @@ * and the GNU Lesser General Public License along with openPMD-api. * If not, see . */ -#include "openPMD/auxiliary/Date.hpp" -#include "openPMD/auxiliary/Filesystem.hpp" -#include "openPMD/auxiliary/JSON.hpp" -#include "openPMD/auxiliary/StringManip.hpp" +#include "openPMD/Series.hpp" +#include "openPMD/Error.hpp" #include "openPMD/IO/AbstractIOHandler.hpp" #include "openPMD/IO/AbstractIOHandlerHelper.hpp" #include "openPMD/IO/Format.hpp" #include "openPMD/ReadIterations.hpp" -#include "openPMD/Series.hpp" +#include "openPMD/auxiliary/Date.hpp" +#include "openPMD/auxiliary/Filesystem.hpp" +#include "openPMD/auxiliary/JSON.hpp" +#include "openPMD/auxiliary/StringManip.hpp" #include "openPMD/version.hpp" #include @@ -280,8 +281,11 @@ SeriesInterface::setIterationEncoding(IterationEncoding ie) switch( ie ) { case IterationEncoding::fileBased: - setIterationFormat(series.m_name); - setAttribute("iterationEncoding", std::string("fileBased")); + setIterationFormat( series.m_name ); + setAttribute( "iterationEncoding", std::string( "fileBased" ) ); + // This checks that the name contains the expansion pattern + // (e.g. %T) and parses it + setName( series.m_name ); break; case IterationEncoding::groupBased: setIterationFormat(BASEPATH); @@ -330,8 +334,22 @@ SeriesInterface::setName(std::string const& n) if( written() ) throw std::runtime_error("A files name can not (yet) be changed after it has been written."); - if( series.m_iterationEncoding == IterationEncoding::fileBased && !auxiliary::contains(series.m_name, "%T") ) - throw std::runtime_error("For fileBased formats the iteration regex %T must be included in the file name"); + if( series.m_iterationEncoding == IterationEncoding::fileBased ) + { + + // Just put any filename extension now and ignore it + // We only care for the prefix, postfix and padding + auto input = parseInput( n + ".json" ); + if( input->iterationEncoding != IterationEncoding::fileBased ) + { + throw error::WrongAPIUsage( + "For fileBased formats the iteration expansion pattern %T must " + "be included in the file name" ); + } + series.m_filenamePrefix = input->filenamePrefix; + series.m_filenamePostfix = input->filenamePostfix; + series.m_filenamePadding = input->filenamePadding; + } series.m_name = n; dirty() = true; diff --git a/test/SerialIOTest.cpp b/test/SerialIOTest.cpp index 5db2142eb7..ef22ee5c5b 100644 --- a/test/SerialIOTest.cpp +++ b/test/SerialIOTest.cpp @@ -4566,3 +4566,38 @@ TEST_CASE( "no_explicit_flush", "[serial]" ) no_explicit_flush( "../samples/no_explicit_flush." + t ); } } + +TEST_CASE( "late_setting_of_iterationencoding", "[serial]" ) +{ + { + ::openPMD::Series series = ::openPMD::Series( + "../samples/error.json", ::openPMD::Access::CREATE ); + series.iterations[ 10 ]; + REQUIRE_THROWS_WITH( + series.setIterationEncoding( + ::openPMD::IterationEncoding::fileBased ), + Catch::Equals( "Wrong API usage: For fileBased formats the " + "iteration expansion pattern %T must " + "be included in the file name" ) ); + series.flush(); + } + { + ::openPMD::Series series = ::openPMD::Series( + "../samples/asdf_%T.json", ::openPMD::Access::CREATE ); + series.iterations[ 10 ]; + series.setName( "change_name_%T" ); + series.flush(); + } + { + ::openPMD::Series series = ::openPMD::Series( + "../samples/asdf.json", ::openPMD::Access::CREATE ); + series.iterations[ 10 ]; + series.setName( "change_name_and_encoding_%T" ); + series.setIterationEncoding( IterationEncoding::fileBased ); + series.flush(); + } + + REQUIRE( auxiliary::file_exists( "../samples/change_name_10.json" ) ); + REQUIRE( auxiliary::file_exists( + "../samples/change_name_and_encoding_10.json" ) ); +} From 84722d47b6d03af5750117daf1c346af8d8552ae Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Franz=20P=C3=B6schel?= Date: Fri, 6 Aug 2021 17:07:10 +0200 Subject: [PATCH 2/5] Make it possible to set name independent from filenames --- include/openPMD/Series.hpp | 4 ++- src/Series.cpp | 56 +++++++++++++++++++++++++++++++------- test/CoreTest.cpp | 4 +-- test/SerialIOTest.cpp | 20 ++++++++++++-- 4 files changed, 68 insertions(+), 16 deletions(-) diff --git a/include/openPMD/Series.hpp b/include/openPMD/Series.hpp index 3c21df532c..cf34a3074d 100644 --- a/include/openPMD/Series.hpp +++ b/include/openPMD/Series.hpp @@ -84,7 +84,7 @@ class SeriesData : public AttributableData std::string m_name; std::string m_filenamePrefix; std::string m_filenamePostfix; - int m_filenamePadding = 0; + int m_filenamePadding = -1; IterationEncoding m_iterationEncoding{}; Format m_format; /** @@ -346,6 +346,8 @@ class SeriesInterface : public AttributableInterface } } std::unique_ptr< ParsedInput > parseInput(std::string); + bool hasExpansionPattern( std::string filenameWithExtension ); + bool reparseExpansionPattern( std::string filenameWithExtension ); void init(std::shared_ptr< AbstractIOHandler >, std::unique_ptr< ParsedInput >); void initDefaults( IterationEncoding ); /** diff --git a/src/Series.cpp b/src/Series.cpp index d28f703154..7eca51a4c8 100644 --- a/src/Series.cpp +++ b/src/Series.cpp @@ -88,7 +88,7 @@ struct SeriesInterface::ParsedInput IterationEncoding iterationEncoding; std::string filenamePrefix; std::string filenamePostfix; - int filenamePadding; + int filenamePadding = -1; }; //ParsedInput SeriesInterface::SeriesInterface( @@ -285,7 +285,16 @@ SeriesInterface::setIterationEncoding(IterationEncoding ie) setAttribute( "iterationEncoding", std::string( "fileBased" ) ); // This checks that the name contains the expansion pattern // (e.g. %T) and parses it - setName( series.m_name ); + if( series.m_filenamePadding < 0 ) + { + if( not reparseExpansionPattern( series.m_name ) ) + { + throw error::WrongAPIUsage( + "For fileBased formats the iteration expansion pattern " + "%T must " + "be included in the file name" ); + } + } break; case IterationEncoding::groupBased: setIterationFormat(BASEPATH); @@ -336,19 +345,25 @@ SeriesInterface::setName(std::string const& n) if( series.m_iterationEncoding == IterationEncoding::fileBased ) { - - // Just put any filename extension now and ignore it - // We only care for the prefix, postfix and padding - auto input = parseInput( n + ".json" ); - if( input->iterationEncoding != IterationEncoding::fileBased ) + // Setting a new name should not alter file names in filebased iteration + // encoding + // Just make sure that an expansion pattern is active + // Our filename parser expects an extension, so just add any and ignore + // the result for that + if( hasExpansionPattern( n + ".json" ) ) + { + reparseExpansionPattern( n + ".json" ); + } + else if( series.m_filenamePadding < 0 ) { throw error::WrongAPIUsage( "For fileBased formats the iteration expansion pattern %T must " "be included in the file name" ); } - series.m_filenamePrefix = input->filenamePrefix; - series.m_filenamePostfix = input->filenamePostfix; - series.m_filenamePadding = input->filenamePadding; + else + { + // no-op, keep the old pattern and set the new name + } } series.m_name = n; @@ -436,6 +451,27 @@ SeriesInterface::parseInput(std::string filepath) return input; } +bool SeriesInterface::hasExpansionPattern( std::string filenameWithExtension ) +{ + auto input = parseInput( std::move( filenameWithExtension ) ); + return input->iterationEncoding == IterationEncoding::fileBased; +} + +bool SeriesInterface::reparseExpansionPattern( + std::string filenameWithExtension ) +{ + auto input = parseInput( std::move( filenameWithExtension ) ); + if( input->iterationEncoding != IterationEncoding::fileBased ) + { + return false; + } + auto & series = get(); + series.m_filenamePrefix = input->filenamePrefix; + series.m_filenamePostfix = input->filenamePostfix; + series.m_filenamePadding = input->filenamePadding; + return true; +} + void SeriesInterface::init( std::shared_ptr< AbstractIOHandler > ioHandler, std::unique_ptr< SeriesInterface::ParsedInput > input ) diff --git a/test/CoreTest.cpp b/test/CoreTest.cpp index 72830b0df2..17095ca1b3 100644 --- a/test/CoreTest.cpp +++ b/test/CoreTest.cpp @@ -688,14 +688,14 @@ TEST_CASE( "structure_test", "[core]" ) TEST_CASE( "wrapper_test", "[core]" ) { - Series o = Series("./new_openpmd_output.json", Access::CREATE); + Series o = Series("./new_openpmd_output_%T.json", Access::CREATE); o.setOpenPMDextension(42); o.setIterationEncoding(IterationEncoding::fileBased); Series copy = o; REQUIRE(copy.openPMDextension() == 42); REQUIRE(copy.iterationEncoding() == IterationEncoding::fileBased); - REQUIRE(copy.name() == "new_openpmd_output"); + REQUIRE(copy.name() == "new_openpmd_output_%T"); copy.setOpenPMD("1.2.0"); copy.setIterationEncoding(IterationEncoding::groupBased); copy.setName("other_name"); diff --git a/test/SerialIOTest.cpp b/test/SerialIOTest.cpp index ef22ee5c5b..16cf4133cd 100644 --- a/test/SerialIOTest.cpp +++ b/test/SerialIOTest.cpp @@ -4583,9 +4583,20 @@ TEST_CASE( "late_setting_of_iterationencoding", "[serial]" ) } { ::openPMD::Series series = ::openPMD::Series( - "../samples/asdf_%T.json", ::openPMD::Access::CREATE ); + "../samples/asdf_%T.json", + ::openPMD::Access::CREATE ); series.iterations[ 10 ]; - series.setName( "change_name_%T" ); + series.setName( + "change_name_%T" ); + series.flush(); + } + { + ::openPMD::Series series = ::openPMD::Series( + "../samples/change_name_keep_filename_%T.json", + ::openPMD::Access::CREATE ); + series.iterations[ 10 ]; + series.setName( + "expansion_pattern_was_specified_previously_filename_will_stay" ); series.flush(); } { @@ -4597,7 +4608,10 @@ TEST_CASE( "late_setting_of_iterationencoding", "[serial]" ) series.flush(); } - REQUIRE( auxiliary::file_exists( "../samples/change_name_10.json" ) ); + REQUIRE( auxiliary::file_exists( + "../samples/change_name_10.json" ) ); + REQUIRE( auxiliary::file_exists( + "../samples/change_name_keep_filename_10.json" ) ); REQUIRE( auxiliary::file_exists( "../samples/change_name_and_encoding_10.json" ) ); } From 0d8c0031aeb48633d1ac271083bfd50b5629127c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Franz=20P=C3=B6schel?= Date: Mon, 9 Aug 2021 10:29:43 +0200 Subject: [PATCH 3/5] Don't use not keyword, MSVC doesnt like it --- src/Series.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Series.cpp b/src/Series.cpp index 7eca51a4c8..d583828bba 100644 --- a/src/Series.cpp +++ b/src/Series.cpp @@ -287,7 +287,7 @@ SeriesInterface::setIterationEncoding(IterationEncoding ie) // (e.g. %T) and parses it if( series.m_filenamePadding < 0 ) { - if( not reparseExpansionPattern( series.m_name ) ) + if( !reparseExpansionPattern( series.m_name ) ) { throw error::WrongAPIUsage( "For fileBased formats the iteration expansion pattern " From e159ea014f0d84d97f475b79a3bdf56e3c4a3b2a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Franz=20P=C3=B6schel?= Date: Mon, 9 Aug 2021 10:41:42 +0200 Subject: [PATCH 4/5] Python bindings for WrongAPIUsage --- src/binding/python/Error.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/binding/python/Error.cpp b/src/binding/python/Error.cpp index f0c218e1a0..6eb5c18aa3 100644 --- a/src/binding/python/Error.cpp +++ b/src/binding/python/Error.cpp @@ -10,6 +10,8 @@ void init_Error( py::module & m ) auto & baseError = py::register_exception< Error >( m, "Error" ); py::register_exception< error::OperationUnsupportedInBackend >( m, "ErrorOperationUnsupportedInBackend", baseError ); + py::register_exception< error::WrongAPIUsage >( + m, "ErrorWrongAPIUsage", baseError ); #ifndef NDEBUG m.def( "test_throw", []( std::string description ) { From 3d5fa8a1c5dfb01d5bcc5fa063db5fad23fc8781 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Franz=20P=C3=B6schel?= Date: Thu, 12 Aug 2021 16:42:51 +0200 Subject: [PATCH 5/5] Cleanup 1) Revert unnecessary changes (include ordering) 2) Update comments --- src/Series.cpp | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/src/Series.cpp b/src/Series.cpp index d583828bba..b658e1460b 100644 --- a/src/Series.cpp +++ b/src/Series.cpp @@ -18,16 +18,16 @@ * and the GNU Lesser General Public License along with openPMD-api. * If not, see . */ -#include "openPMD/Series.hpp" -#include "openPMD/Error.hpp" -#include "openPMD/IO/AbstractIOHandler.hpp" -#include "openPMD/IO/AbstractIOHandlerHelper.hpp" -#include "openPMD/IO/Format.hpp" -#include "openPMD/ReadIterations.hpp" #include "openPMD/auxiliary/Date.hpp" #include "openPMD/auxiliary/Filesystem.hpp" #include "openPMD/auxiliary/JSON.hpp" #include "openPMD/auxiliary/StringManip.hpp" +#include "openPMD/IO/AbstractIOHandler.hpp" +#include "openPMD/IO/AbstractIOHandlerHelper.hpp" +#include "openPMD/IO/Format.hpp" +#include "openPMD/Error.hpp" +#include "openPMD/ReadIterations.hpp" +#include "openPMD/Series.hpp" #include "openPMD/version.hpp" #include @@ -345,9 +345,8 @@ SeriesInterface::setName(std::string const& n) if( series.m_iterationEncoding == IterationEncoding::fileBased ) { - // Setting a new name should not alter file names in filebased iteration - // encoding - // Just make sure that an expansion pattern is active + // If the filename specifies an expansion pattern, set it. + // If not, check if one is already active. // Our filename parser expects an extension, so just add any and ignore // the result for that if( hasExpansionPattern( n + ".json" ) )