From b3a2b8da99b036fa75fa6c6f4aaa0047f438399c Mon Sep 17 00:00:00 2001 From: Brandon Barker Date: Wed, 13 Nov 2024 10:53:16 -0500 Subject: [PATCH 1/7] Fix: Bounds in output for Metadata::None variables (#1188) * fix: metadata::none output bounds * tst: add option in advection example to test metadata::none output. Add to hdf5 regression tests * changelog * gold file SHA * update metadata_none_Var in advection example to fit block sizes. update golds. * updates to metadata none var test * update SHA for golds --- CHANGELOG.md | 1 + CMakeLists.txt | 4 +-- example/advection/advection_package.cpp | 33 +++++++++++++++++++ src/outputs/output_utils.hpp | 6 ++-- .../test_suites/output_hdf5/output_hdf5.py | 12 +++++++ .../output_hdf5/parthinput.advection | 4 ++- 6 files changed, 54 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 554debf3c568..1d361fc0567f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -21,6 +21,7 @@ - [[PR 1172]](https://github.com/parthenon-hpc-lab/parthenon/pull/1172) Make parthenon manager robust against external MPI init and finalize calls ### Fixed (not changing behavior/API/variables/...) +- [[PR 1188]](https://github.com/parthenon-hpc-lab/parthenon/pull/1188) Fix hdf5 output issue for metadata none variables, update test. - [[PR 1170]](https://github.com/parthenon-hpc-lab/parthenon/pull/1170) Fixed incorrect initialization of array by a const not constexpr - [[PR 1189]](https://github.com/parthenon-hpc-lab/parthenon/pull/1189) Address CUDA MPI/ICP issue with Kokkos <=4.4.1 - [[PR 1178]](https://github.com/parthenon-hpc-lab/parthenon/pull/1178) Fix issue with mesh pointer when using relative residual tolerance in BiCGSTAB solver. diff --git a/CMakeLists.txt b/CMakeLists.txt index 61fd4fd3cfff..590f277ee8d5 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -75,9 +75,9 @@ include(cmake/Format.cmake) include(cmake/Lint.cmake) # regression test reference data -set(REGRESSION_GOLD_STANDARD_VER 24 CACHE STRING "Version of gold standard to download and use") +set(REGRESSION_GOLD_STANDARD_VER 25 CACHE STRING "Version of gold standard to download and use") set(REGRESSION_GOLD_STANDARD_HASH - "SHA512=e220df92a335131131e42ddb52dc221a6dbd6bb56361483b4af0292620eeb82ffb21ef3b95fd9a7c5cc158fb754da0bf1a1015bec98b5bbad05f4bceb1ee99bc" + "SHA512=314dc8312366d81ba33d1fde25812e9a7697b2f529de29e22662df0d458f1c4bc5b5bb4e649888170f66ffec0df1be20a9cf401944531c1c1ad835e26eaad28f" CACHE STRING "Hash of default gold standard file to download") option(REGRESSION_GOLD_STANDARD_SYNC "Automatically sync gold standard files." ON) diff --git a/example/advection/advection_package.cpp b/example/advection/advection_package.cpp index f799a2909262..64e4ea30712d 100644 --- a/example/advection/advection_package.cpp +++ b/example/advection/advection_package.cpp @@ -196,6 +196,20 @@ std::shared_ptr Initialize(ParameterInput *pin) { m = Metadata({Metadata::Cell, Metadata::OneCopy}, std::vector({1})); pkg->AddField("my_derived_var", m); + // Create a Metadata::None variable for IO testing purposes. + // Only load if test_metadata_none is specified in the Advection block + auto test_metadata_none = + pin->GetOrAddBoolean("Advection", "test_metadata_none", false); + pkg->AddParam("test_metadata_none", test_metadata_none); + if (test_metadata_none) { + const int nx1 = pin->GetOrAddInteger("parthenon/meshblock", "nx1", 1); + const int nx2 = pin->GetOrAddInteger("parthenon/meshblock", "nx2", 1); + const int nx3 = pin->GetOrAddInteger("parthenon/meshblock", "nx3", 1); + std::vector test_shape = {nx1 + 1, nx2 + 1, nx3 + 1, 3}; + m = Metadata({Metadata::OneCopy, Metadata::None}, test_shape); + pkg->AddField("metadata_none_var", m); + } + // List (vector) of HistoryOutputVar that will all be enrolled as output variables parthenon::HstVar_list hst_vars = {}; // Now we add a couple of callback functions @@ -281,6 +295,7 @@ AmrTag CheckRefinement(MeshBlockData *rc) { void PreFill(MeshBlockData *rc) { auto pmb = rc->GetBlockPointer(); auto pkg = pmb->packages.Get("advection_package"); + const bool test_metadata_none = pkg->Param("test_metadata_none"); bool fill_derived = pkg->Param("fill_derived"); if (fill_derived) { @@ -302,6 +317,24 @@ void PreFill(MeshBlockData *rc) { v(out + n, k, j, i) = 1.0 - v(in + n, k, j, i); }); } + + // Fill the metadata::None var with index gymnastics. + if (test_metadata_none) { + const int nx1 = pmb->cellbounds.ncellsi(IndexDomain::interior); + const int nx2 = pmb->cellbounds.ncellsj(IndexDomain::interior); + const int nx3 = pmb->cellbounds.ncellsk(IndexDomain::interior); + + // packing in principle unnecessary/convoluted here and just done for demonstration + std::vector vars({"metadata_none_var"}); + PackIndexMap imap; + const auto &v = rc->PackVariables(vars, imap); + + pmb->par_for( + PARTHENON_AUTO_LABEL, 0, 2, 0, nx3, 0, nx2, 0, nx1, + KOKKOS_LAMBDA(const int n, const int k, const int j, const int i) { + v(n, k, j, i) = n + k + j + i; + }); + } } // this is the package registered function to fill derived diff --git a/src/outputs/output_utils.hpp b/src/outputs/output_utils.hpp index 5c094ef688d7..e9fcee04c986 100644 --- a/src/outputs/output_utils.hpp +++ b/src/outputs/output_utils.hpp @@ -312,11 +312,11 @@ void PackOrUnpackVar(const VarInfo &info, bool do_ghosts, idx_t &idx, Function_t auto [kb, jb, ib] = info.GetPaddedBoundsKJI(domain); if (info.where == MetadataFlag({Metadata::None})) { kb.s = 0; - kb.e = shape[4]; + kb.e = std::max(0, shape[4] - 1); jb.s = 0; - jb.e = shape[5]; + jb.e = std::max(0, shape[5] - 1); ib.s = 0; - ib.e = shape[6]; + ib.e = std::max(0, shape[6] - 1); } for (int topo = 0; topo < shape[0]; ++topo) { for (int t = 0; t < shape[1]; ++t) { diff --git a/tst/regression/test_suites/output_hdf5/output_hdf5.py b/tst/regression/test_suites/output_hdf5/output_hdf5.py index cbef0aba2aa7..fd68c8e17851 100644 --- a/tst/regression/test_suites/output_hdf5/output_hdf5.py +++ b/tst/regression/test_suites/output_hdf5/output_hdf5.py @@ -190,6 +190,18 @@ def Analyse(self, parameters): ) analyze_status = False + # check contents of matadata_none_var + for dim in [2, 3]: + data = phdf.phdf(f"advection_{dim}d.out0.final.phdf") + profile = data.Get("metadata_none_var", flatten=False) + for index in np.ndindex(profile.shape): + ib, j, k, l, m = index + expected_value = l + k + j + m + actual_value = profile[ib, j, k, l, m] + if expected_value != actual_value: + print("metadata_none_var is incorrect") + analyze_status = False + # Checking Parthenon histograms versus numpy ones for dim in [2, 3]: # 1D histogram with binning of a variable with bins defined by a var diff --git a/tst/regression/test_suites/output_hdf5/parthinput.advection b/tst/regression/test_suites/output_hdf5/parthinput.advection index 3061d2bff961..118a82b58e56 100644 --- a/tst/regression/test_suites/output_hdf5/parthinput.advection +++ b/tst/regression/test_suites/output_hdf5/parthinput.advection @@ -54,6 +54,7 @@ vx = 1.0 vy = 1.0 vz = 1.0 profile = hard_sphere +test_metadata_none = true refine_tol = 0.3 # control the package specific refinement tagging function derefine_tol = 0.03 @@ -64,7 +65,8 @@ file_type = hdf5 dt = 1.0 variables = advected, one_minus_advected, & # comments are ok one_minus_advected_sq, & # on every (& characters are ok in comments) - one_minus_sqrt_one_minus_advected_sq # line + one_minus_sqrt_one_minus_advected_sq, & # line + metadata_none_var file_type = hst From cbc36bdfcfb6e664c93515cfc9536cae33f1b13c Mon Sep 17 00:00:00 2001 From: Ben Ryan Date: Wed, 13 Nov 2024 09:42:19 -0700 Subject: [PATCH 2/7] Modify how we loop through packages to enroll history outputs --- src/outputs/history.cpp | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/outputs/history.cpp b/src/outputs/history.cpp index 2e1310062d48..41ab26a516f8 100644 --- a/src/outputs/history.cpp +++ b/src/outputs/history.cpp @@ -93,8 +93,14 @@ void HistoryOutput::WriteOutputFile(Mesh *pm, ParameterInput *pin, SimTime *tm, md_base->Initialize(pm->block_list, pm); } - // Loop over all packages of the application - for (const auto &pkg : packages) { + // Loop over all packages of the application in alphabetical order + std::vector keys; + for (const auto& pair : packages) { + keys.push_back(pair.first); + } + std::sort(keys.begin(), keys.end()); + for (const auto &key : keys) { + const auto &pkg = packages[key]; const auto ¶ms = pkg.second->AllParams(); // Check if the package has enrolled scalar history functions which are stored in the From 37929e9cfa8edce8d03f4957619196b76bbaa6fb Mon Sep 17 00:00:00 2001 From: Ben Ryan Date: Wed, 13 Nov 2024 09:45:56 -0700 Subject: [PATCH 3/7] CHANGELOG --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1d361fc0567f..a4e0a5ca9cd2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,7 @@ - [[PR 1161]](https://github.com/parthenon-hpc-lab/parthenon/pull/1161) Make flux field Metadata accessible, add Metadata::CellMemAligned flag, small perfomance upgrades ### Changed (changing behavior/API/variables/...) +- [[PR 1209]](https://github.com/parthenon-hpc-lab/parthenon/pull/1209) Ordered history output - [[PR 1206]](https://github.com/parthenon-hpc-lab/parthenon/pull/1206) Leapfrog fix - [[PR1203]](https://github.com/parthenon-hpc-lab/parthenon/pull/1203) Pin Ubuntu CI image - [[PR1177]](https://github.com/parthenon-hpc-lab/parthenon/pull/1177) Make mesh-level boundary conditions usable without the "user" flag From cf158716c75ea74aec5b325906c23bf912c0c594 Mon Sep 17 00:00:00 2001 From: Ben Ryan Date: Wed, 13 Nov 2024 09:47:36 -0700 Subject: [PATCH 4/7] formatting --- src/outputs/history.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/outputs/history.cpp b/src/outputs/history.cpp index 41ab26a516f8..51b4e523e7f0 100644 --- a/src/outputs/history.cpp +++ b/src/outputs/history.cpp @@ -95,8 +95,8 @@ void HistoryOutput::WriteOutputFile(Mesh *pm, ParameterInput *pin, SimTime *tm, // Loop over all packages of the application in alphabetical order std::vector keys; - for (const auto& pair : packages) { - keys.push_back(pair.first); + for (const auto &pair : packages) { + keys.push_back(pair.first); } std::sort(keys.begin(), keys.end()); for (const auto &key : keys) { From 6939da2a07c34ae2366c45fae5d1d0fa429d38c6 Mon Sep 17 00:00:00 2001 From: Ben Ryan Date: Wed, 13 Nov 2024 09:58:34 -0700 Subject: [PATCH 5/7] Oops key vs pair --- src/outputs/history.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/outputs/history.cpp b/src/outputs/history.cpp index 51b4e523e7f0..2aade658fe00 100644 --- a/src/outputs/history.cpp +++ b/src/outputs/history.cpp @@ -101,7 +101,7 @@ void HistoryOutput::WriteOutputFile(Mesh *pm, ParameterInput *pin, SimTime *tm, std::sort(keys.begin(), keys.end()); for (const auto &key : keys) { const auto &pkg = packages[key]; - const auto ¶ms = pkg.second->AllParams(); + const auto ¶ms = pkg->AllParams(); // Check if the package has enrolled scalar history functions which are stored in the // Params under the `hist_param_key` name. From 0968c3ea8c4e0877041b2c5ee154fa8c15c5ec92 Mon Sep 17 00:00:00 2001 From: Ben Ryan Date: Wed, 13 Nov 2024 10:01:15 -0700 Subject: [PATCH 6/7] Add note to docs about ordering of history outputs h/t AMD --- doc/sphinx/src/outputs.rst | 4 +++- src/outputs/history.cpp | 3 ++- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/doc/sphinx/src/outputs.rst b/doc/sphinx/src/outputs.rst index 7196e4777bb3..fa44e27da298 100644 --- a/doc/sphinx/src/outputs.rst +++ b/doc/sphinx/src/outputs.rst @@ -194,7 +194,9 @@ block might look like This will produce a text file (``.hst``) output file every 1 units of simulation time. The content of the file is determined by the functions -enrolled by specific packages, see :ref:`state history output`. +enrolled by specific packages, see :ref:`state history output`. Per-package history +outputs will always be in alphabetical order by package name, which may not match +the order in which packages were added to a simulation. Histograms ---------- diff --git a/src/outputs/history.cpp b/src/outputs/history.cpp index 2aade658fe00..913a722496a7 100644 --- a/src/outputs/history.cpp +++ b/src/outputs/history.cpp @@ -93,7 +93,8 @@ void HistoryOutput::WriteOutputFile(Mesh *pm, ParameterInput *pin, SimTime *tm, md_base->Initialize(pm->block_list, pm); } - // Loop over all packages of the application in alphabetical order + // Loop over all packages of the application in alphabetical order to ensure consistency + // of ordering of data in columns. std::vector keys; for (const auto &pair : packages) { keys.push_back(pair.first); From 817ea62fefd9bb31dd1a62d9e8ccf4111ad3ce16 Mon Sep 17 00:00:00 2001 From: Ben Ryan Date: Wed, 13 Nov 2024 11:16:33 -0700 Subject: [PATCH 7/7] Oops include_what_you_use --- src/outputs/history.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/src/outputs/history.cpp b/src/outputs/history.cpp index 913a722496a7..074bea4feb0c 100644 --- a/src/outputs/history.cpp +++ b/src/outputs/history.cpp @@ -18,6 +18,7 @@ // \brief writes history output data, volume-averaged quantities that are output // frequently in time to trace their history. +#include #include #include #include