Add review suggestions

Author: franzpoeschel

examples/10_streaming_read.cpp

@@ -67,8 +67,8 @@ int main()
                 for (auto const &chunk : rc.second.availableChunks())
                 {
                     std::cout << "\n\tRank " << chunk.sourceID << "\t"
-                              << auxiliary::format_vec(chunk.offset) << "\t– "
-                              << auxiliary::format_vec(chunk.extent);
+                              << auxiliary::vec_as_string(chunk.offset) << "\t– "
+                              << auxiliary::vec_as_string(chunk.extent);
                 }
                 std::cout << std::endl;
             }

include/openPMD/auxiliary/StringManip.hpp

@@ -243,6 +243,14 @@ namespace auxiliary
         return std::forward<S>(s);
     }
 
+    /** Write a string representation of a vector or another iterable
+     *  container to a stream.
+     *
+     * @param s The stream to write to.
+     * @param vec The vector or other iterable container.
+     * @return The modified stream. Each item is
+     *         formatted using the default definition for operator<<().
+     */
     template <typename Stream, typename Vec>
     auto write_vec_to_stream(Stream &&s, Vec const &vec) -> Stream &&
     {
@@ -265,6 +273,13 @@ namespace auxiliary
         return std::forward<Stream>(s);
     }
 
+    /** Create a string representation of a vector or another iterable
+     *  container.
+     *
+     * @param vec The vector or other iterable container.
+     * @return A string that shows the items of the container. Each item is
+     *         formatted using the default definition for operator<<().
+     */
     template <typename Vec>
     auto vec_as_string(Vec const &vec) -> std::string
     {

src/IO/ADIOS/ADIOS2File.cpp

@@ -119,7 +119,8 @@ void WriteDataset::call(ADIOS2File &ba, detail::BufferedPut &bp)
                             "Can only write a single element to LocalValue "
                             "variables (extent == Extent{1}, but extent of '" +
                                 bp.name + " was " +
-                                auxiliary::format_vec(bp.param.extent) + "').");
+                                auxiliary::vec_as_string(bp.param.extent) +
+                                "').");
                     }
                     engine.Put(var, *ptr);
                 }
@@ -207,7 +208,7 @@ struct RunUniquePtrPut
                     "Can only write a single element to LocalValue "
                     "variables (extent == Extent{1}, but extent of '" +
                         bufferedPut.name + " was " +
-                        auxiliary::format_vec(bufferedPut.extent) + "').");
+                        auxiliary::vec_as_string(bufferedPut.extent) + "').");
             }
             engine.Put(var, *ptr);
         }

src/IO/ADIOS/ADIOS2IOHandler.cpp

@@ -944,7 +944,7 @@ void ADIOS2IOHandlerImpl::createDataset(
                         "Shape for local value array must be a 1D array "
                         "equivalent to the MPI size ('" +
                             varName + "' has shape " +
-                            auxiliary::format_vec(parameters.extent) +
+                            auxiliary::vec_as_string(parameters.extent) +
                             ", but should have shape [" +
                             std::to_string(required_size) + "]).");
                 }

test/ParallelIOTest.cpp

@@ -7,6 +7,7 @@
 #include "openPMD/IO/Access.hpp"
 #include "openPMD/auxiliary/Environment.hpp"
 #include "openPMD/auxiliary/Filesystem.hpp"
+#include "openPMD/backend/PatchRecordComponent.hpp"
 #include "openPMD/openPMD.hpp"
 #include <catch2/catch.hpp>
 
@@ -387,7 +388,7 @@ void available_chunks_test(std::string const &file_ending)
     MPI_Comm_size(MPI_COMM_WORLD, &r_mpi_size);
     unsigned mpi_rank{static_cast<unsigned>(r_mpi_rank)},
         mpi_size{static_cast<unsigned>(r_mpi_size)};
-    std::string name = "../samples/available_chunks." + file_ending;
+    std::string name = "../samples/parallel_available_chunks." + file_ending;
 
     /*
      * ADIOS2 assigns writerIDs to blocks in a BP file by id of the substream
@@ -400,7 +401,6 @@ void available_chunks_test(std::string const &file_ending)
     {
         "engine":
         {
-            "type": "bp4",
             "parameters":
             {
                 "NumAggregators":)END"
@@ -421,6 +421,14 @@ void available_chunks_test(std::string const &file_ending)
         E_x.resetDataset({Datatype::INT, {mpi_size, 4}});
         E_x.storeChunk(data, {mpi_rank, 0}, {1, 4});
 
+        /*
+         * Verify that block decomposition also works in "local value" variable
+         * shape. That shape instructs the data to participate in ADIOS2
+         * metadata aggregation, hence there is only one "real" written block,
+         * the aggregated one. We still need the original logical blocks to be
+         * present in reading.
+         */
+
         auto electrons = it0.particles["e"].particlePatches;
         auto numParticles = electrons["numParticles"];
         auto numParticlesOffset = electrons["numParticlesOffset"];
@@ -493,12 +501,40 @@ void available_chunks_test(std::string const &file_ending)
         {
             REQUIRE(ranks[i] == i);
         }
+
+        auto electrons = it0.particles["e"].particlePatches;
+        for (PatchRecordComponent *prc :
+             {static_cast<PatchRecordComponent *>(&electrons["numParticles"]),
+              static_cast<PatchRecordComponent *>(
+                  &electrons["numParticlesOffset"]),
+              &electrons["offset"]["x"],
+              &electrons["offset"]["y"],
+              &electrons["extent"]["z"],
+              &electrons["offset"]["x"],
+              &electrons["extent"]["y"],
+              &electrons["extent"]["z"]})
+        {
+            auto available_chunks = prc->availableChunks();
+            REQUIRE(size_t(r_mpi_size) == available_chunks.size());
+            for (size_t i = 0; i < available_chunks.size(); ++i)
+            {
+                auto const &chunk = available_chunks[i];
+                REQUIRE(chunk.extent == Extent{1});
+                REQUIRE(chunk.offset == Offset{i});
+                REQUIRE(chunk.sourceID == i);
+            }
+        }
     }
 }
 
 TEST_CASE("available_chunks_test", "[parallel][adios]")
 {
+#if HAS_ADIOS_2_9
+    available_chunks_test("bp4");
+    available_chunks_test("bp5");
+#else
     available_chunks_test("bp");
+#endif
 }
 #endif