Restart bugfix #311

nmsriram · 2020-09-30T14:08:34Z

PR Summary

This merge fixes a parallel restart bug in the restart path. Parallel restarts with same (or different) number of processors now work for the advection example.

Also fixes a restart bug with reflective boundary conditions with an ugly hack in mesh/mesh_refinement.cpp to apply reflective boundary conditions. It is expected that @Yurlungur or @jonahm-LANL will fix this when doing the boundary condition refactor. As a result this should get merged in before the boundary condition fix.

We can now restart with a number of processors different from the original run, and even restart serial from parallel (or vice versa) and still get identical answers.

I believe #61 is fixed with this PR.

Known issue:

PR Checklist

Code passes cpplint
New features are documented.
Adds a test for any bugs fixed. Adds tests for new features.
Code is formatted
Changes are summarized in CHANGELOG.md

Yurlungur

Thanks for the fix. I don't love the function name GetNbs as nb* is a cryptic name. But since that's what they're called internally, I think it's fine.

JoshuaSBrown

I'm still getting errors when running the tests, which tests is this supposed to fix?

27/36 Test #27: regression_test:advection_performance .............................***Failed  336.32 sec
      Start 28: regression_mpi_test:advection_performance
28/36 Test #28: regression_mpi_test:advection_performance .........................***Failed  412.27 sec
      Start 29: regression_test:restart
29/36 Test #29: regression_test:restart ...........................................   Passed   69.50 sec
      Start 30: regression_mpi_test:restart
30/36 Test #30: regression_mpi_test:restart .......................................***Failed    1.38 sec

src/mesh/mesh.cpp

src/mesh/mesh.hpp

src/mesh/mesh_refinement.hpp

JoshuaSBrown · 2020-09-30T14:32:24Z

src/mesh/mesh_refinement.hpp

+    return std::vector<int>(
+        {refine_flag_, neighbor_rflag_, deref_count_, deref_threshold_});
+  }
+  void SetInternals(std::vector<int> a) {


a should be const reference.

JoshuaSBrown · 2020-09-30T14:34:12Z

src/outputs/restart.hpp

@@ -116,6 +116,7 @@ class RestartReader {
  std::vector<T> ReadDataset(const char *name, size_t *count = nullptr) {
    // Returns entire 1D array.
    // status, never checked.  We should...
+    std::vector<T> data;


Good catch! Oh nevermind, I guess this didn't actually really change anything...

Is this ifdef behavior actually what we want? (and/or is this a leftover from Athena++?)
I'm asking as having the code fail seems like a preferable solution rather than returning an empty object (due to lack of implementation or similar).

src/parthenon_manager.cpp

src/mesh/mesh.cpp

JoshuaSBrown · 2020-09-30T15:43:51Z

It looks like the test will run but something is still throwing an error:

/projects/opt/ppc64le/p9/openmpi/4.0.2-gcc_7.4.0/bin/mpiexec -n 2 '/home/joshuasbrown/Software/parthenon_x86_64/parthenon/build/example/advection/advection-example' -i '/home/joshuasbrown/Software/parthenon_x86_64/parthenon/tst/regression/test_suites/restart/parthinput.restart' 'parthenon/job/problem_id=gold'
--------------------------------------------------------------------------
WARNING: There was an error initializing an OpenFabrics device.

  Local host:   cn2020
  Local device: mlx5_1
--------------------------------------------------------------------------
Kokkos::OpenMP::initialize WARNING: OMP_PROC_BIND environment variable not set
  In general, for best performance with OpenMP 4.0 or better set OMP_PROC_BIND=spread and OMP_PLACES=threads
  For best performance with OpenMP 3.1 set OMP_PROC_BIND=true
  For unit testing set OMP_PROC_BIND=false
Kokkos::OpenMP::initialize WARNING: OMP_PROC_BIND environment variable not set
  In general, for best performance with OpenMP 4.0 or better set OMP_PROC_BIND=spread and OMP_PLACES=threads
  For best performance with OpenMP 3.1 set OMP_PROC_BIND=true
  For unit testing set OMP_PROC_BIND=false
### Warning in Mesh::Initialize
The number of MeshBlocks increased more than twice during initialization.
More computing power than you expected may be required.
### Warning in Mesh::Initialize
The number of MeshBlocks increased more than twice during initialization.
More computing power than you expected may be required.

Setup complete, executing driver...

cycle=0 time=0.0000000000000000e+00 dt=1.7578125000000000e-03 zone-cycles/wsec = 0.00e+00
cycle=1 time=1.7578125000000000e-03 dt=1.7578125000000000e-03 zone-cycles/wsec = 1.85e+04
cycle=2 time=3.5156250000000001e-03 dt=1.7578125000000000e-03 zone-cycles/wsec = 1.36e+05
cycle=3 time=5.2734375000000003e-03 dt=1.7578125000000000e-03 zone-cycles/wsec = 6.90e+04
[cn2020:30009] 1 more process has sent help message help-mpi-btl-openib.txt / error in device init
[cn2020:30009] Set MCA parameter "orte_base_help_aggregate" to 0 to see all help / error messages

JoshuaSBrown

Other than my optional improvements this is good. The error I was running into is located in the python regression testing framework so this has my green light.

Yurlungur · 2020-10-01T22:48:29Z

@nmsriram is this ready for merge?

src/mesh/mesh.cpp

src/mesh/mesh_refinement.hpp

pgrete · 2020-10-02T17:10:41Z

src/mesh/mesh_refinement.hpp

@@ -76,6 +76,16 @@ class MeshRefinement {
  // and/or in BoundaryValues::ProlongateBoundaries() (for SMR and AMR)
  int AddToRefinement(ParArrayND<Real> pvar_cc, ParArrayND<Real> pcoarse_cc);
  int AddToRefinement(FaceField *pvar_fc, FaceField *pcoarse_fc);
+  std::vector<int> GetInternals() {


Given that int32_t is used above to read, I suggest to also be more specific here.

pgrete · 2020-10-02T17:15:49Z

src/outputs/restart.hpp

@@ -116,6 +116,7 @@ class RestartReader {
  std::vector<T> ReadDataset(const char *name, size_t *count = nullptr) {
    // Returns entire 1D array.
    // status, never checked.  We should...
+    std::vector<T> data;


Is this ifdef behavior actually what we want? (and/or is this a leftover from Athena++?)
I'm asking as having the code fail seems like a preferable solution rather than returning an empty object (due to lack of implementation or similar).

codecov · 2020-10-02T18:07:21Z

Codecov Report

Merging #311 into develop will increase coverage by 0.00%.
The diff coverage is 76.81%.

@@           Coverage Diff            @@
##           develop     #311   +/-   ##
========================================
  Coverage    55.82%   55.82%           
========================================
  Files          107      107           
  Lines        11103    11128   +25     
========================================
+ Hits          6198     6212   +14     
- Misses        4905     4916   +11

Impacted Files	Coverage Δ
src/mesh/mesh_refinement.cpp	`27.21% <30.43%> (+0.10%)`	⬆️
src/interface/variable_pack.hpp	`82.70% <100.00%> (+0.26%)`	⬆️
src/mesh/mesh.cpp	`53.86% <100.00%> (+0.12%)`	⬆️
src/outputs/parthenon_hdf5.cpp	`96.61% <100.00%> (+2.34%)`	⬆️
src/outputs/restart.cpp	`92.73% <100.00%> (+0.06%)`	⬆️
src/outputs/restart.hpp	`85.18% <100.00%> (ø)`
src/parthenon_manager.cpp	`80.15% <100.00%> (+0.65%)`	⬆️
src/mesh/amr_loadbalance.cpp	`75.21% <0.00%> (-0.15%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 66afc3e...194dc5d. Read the comment docs.

nmsriram · 2020-10-13T18:36:33Z

@pgrete the reason to return an empty array is that we may change the physics on restart and I didn't want the request for those physics to kill the code. I could go either way on this, perhaps with a query function.

pgrete · 2020-10-23T09:37:46Z

@nmsriram there are a couple of comments left. It'd be great if they could be addressed (or replied to) before we go ahead and merge this.

Also (unrelated to this PR): I just noticed that quite a bit of the restart functionality has been implemented using macros.
What's the motivation behind those macros and could we potentially replace those (not in this PR) with standard functions?

…g / restrict

N blocks instead of reading at offset for that particular rank. Added capability to restart with or without ghost zones.

…GPU data is initialized on restart.

…gold to itself

…ix/restart2

nmsriram · 2020-10-23T19:29:44Z

@pgrete the reason they are macros is because I couldn't figure out how to do it different. About a week ago @AndrewGaspar commented essentially the same thing and showed a std::function way. I'm a bit hesitant to change this code at this point and will do it in a subsequent PR.

Yurlungur

nice work. this one was a doozy.

Yurlungur · 2020-10-23T20:08:24Z

src/interface/variable_pack.hpp

@@ -154,7 +154,7 @@ void FillVarView(const vpack_types::VarList<T> &vars, PackIndexMap *vmap,
  auto host_sp = Kokkos::create_mirror_view(Kokkos::HostSpace(), sparse_assoc);

  int vindex = 0;
-  int sparse_start;
+  int sparse_start = -22000; // Initialize to an obviously wrong number


We should perhaps formalize this throughout the code and make a habit of initializing to signal NaNs. That's for another time, though.

Should we create an issue?

yes good idea

Yurlungur · 2020-10-23T20:09:11Z

src/mesh/mesh_refinement.cpp

@@ -38,6 +38,52 @@

 namespace parthenon {

+// TODO(Yurlunger) Identify an idiomatic way of applying reflective


Suggested change

// TODO(Yurlunger) Identify an idiomatic way of applying reflective

// TODO(JMM) Identify an idiomatic way of applying reflective

src/mesh/mesh_refinement.cpp

tst/regression/test_suites/restart/restart.py

src/parthenon_manager.cpp

pgrete · 2020-10-26T11:13:27Z

src/parthenon_manager.cpp

@@ -192,7 +192,9 @@ ParthenonManager::ProcessPackagesDefault(std::unique_ptr<ParameterInput> &pin) {
 void ParthenonManager::RestartPackages(Mesh &rm, RestartReader &resfile) {
  // Restart packages with information for blocks in ids from the restart file
  // Assumption: blocks are contiguous in restart file, may have to revisit this.
-  const IndexDomain interior = IndexDomain::interior;
+  //  const IndexDomain interior = IndexDomain::interior;


Suggested change

// const IndexDomain interior = IndexDomain::interior;

JoshuaSBrown · 2020-10-26T15:13:07Z

src/mesh/mesh_refinement.cpp

@@ -129,6 +175,7 @@ void MeshRefinement::RestrictCellCenteredValues(const ParArrayND<Real> &fine,
               (fine(n, 0, j, i + 1) * vol01 + fine(n, 0, j + 1, i + 1) * vol11)) /
              tvol;
        });
+    applyBounds(pmb, coarse, cib, cjb);


Why do we only have a function like this for the 2d case?

good catch. I think this should be for all 3 cases, @nmsriram

Oh brother. yes. of course. @JoshuaSBrown / @Yurlungur Should I put in another PR to fix this, or should I just depend on @Yurlungur to fix it?

Whoever, gets to it first I suppose.

JoshuaSBrown · 2020-10-26T15:13:53Z

src/outputs/parthenon_hdf5.cpp

+  const IndexDomain theDomain =
+      (output_params.include_ghost_zones ? IndexDomain::entire : IndexDomain::interior);


JoshuaSBrown · 2020-10-26T15:20:30Z

src/outputs/restart.cpp

+  auto nx1 = out_ib.e - out_ib.s + 1; // SS mb.block_size.nx1;
+  auto nx2 = out_jb.e - out_jb.s + 1; // SS mb.block_size.nx2;
+  auto nx3 = out_kb.e - out_kb.s + 1; // SS mb.block_size.nx3;
+  int bsize[3] = {mb.block_size.nx1, mb.block_size.nx2, mb.block_size.nx3};


These comments are confusing to me, what does the SS mean, this kind of looks like you are saying nx1 is the same as mb.block_size.nx1. If so why are you using mb.block_size.nx1 to define bsize and not the more concise nx1 nx2 and nx3 variables?

Ok, I see, nx1 etc will change depending on theDomain.

Sorry, SS are my initials and I use // SS or //SS where I think I may have screwed up and may need to go back.

JoshuaSBrown · 2020-10-26T15:22:08Z

src/outputs/restart.hpp

@@ -180,6 +179,9 @@ class RestartReader {
  // perhaps belongs in a destructor?
  void Close();

+  // Does file have ghost cells?
+  int hasGhost;


Why int instead of bool?

Because it is read in from the HDF file and this requires fewer lines of code

JoshuaSBrown · 2020-10-26T17:07:23Z

    Start  1: unit_tests:Just check everything
 1/37 Test  #1: unit_tests:Just check everything ..................................   Passed    0.27 sec
      Start  2: unit_tests:Task Object Lifecycle
 2/37 Test  #2: unit_tests:Task Object Lifecycle ..................................   Passed    0.27 sec
      Start  3: unit_tests:Can create a vector-valued face-variable
 3/37 Test  #3: unit_tests:Can create a vector-valued face-variable ...............   Passed    0.25 sec
      Start  4: unit_tests:Add and Get is called
 4/37 Test  #4: unit_tests:Add and Get is called ..................................   Passed    0.26 sec
      Start  5: unit_tests:reset is called
 5/37 Test  #5: unit_tests:reset is called ........................................   Passed    0.26 sec
      Start  6: unit_tests:when hasKey is called
 6/37 Test  #6: unit_tests:when hasKey is called ..................................   Passed    0.29 sec
      Start  7: unit_tests:Physical constants
 7/37 Test  #7: unit_tests:Physical constants .....................................   Passed    0.29 sec
      Start  8: unit_tests:Checking IndexShape indices
 8/37 Test  #8: unit_tests:Checking IndexShape indices ............................   Passed    0.25 sec
      Start  9: unit_tests:Checking IndexShape cell counts
 9/37 Test  #9: unit_tests:Checking IndexShape cell counts ........................   Passed    0.24 sec
      Start 10: unit_tests:par_for loops
10/37 Test #10: unit_tests:par_for loops ..........................................   Passed    0.61 sec
      Start 11: unit_tests:nested par_for loops
11/37 Test #11: unit_tests:nested par_for loops ...................................   Passed    0.29 sec
      Start 12: unit_tests:Overlapping SpaceInstances
12/37 Test #12: unit_tests:Overlapping SpaceInstances .............................   Passed    0.34 sec
      Start 13: unit_tests:Built-in flags are registered
13/37 Test #13: unit_tests:Built-in flags are registered ..........................   Passed    0.34 sec
      Start 14: unit_tests:A Metadata flag is allocated
14/37 Test #14: unit_tests:A Metadata flag is allocated ...........................   Passed    0.26 sec
      Start 15: unit_tests:A Metadata struct is created
15/37 Test #15: unit_tests:A Metadata struct is created ...........................   Passed    0.25 sec
      Start 16: unit_tests:ParArrayND
16/37 Test #16: unit_tests:ParArrayND .............................................   Passed    0.31 sec
      Start 17: unit_tests:ParArrayND with LayoutLeft
17/37 Test #17: unit_tests:ParArrayND with LayoutLeft .............................   Passed    0.35 sec
      Start 18: unit_tests:Time simple stencil operations
18/37 Test #18: unit_tests:Time simple stencil operations .........................   Passed    0.27 sec
      Start 19: unit_tests:Check registry pressure
19/37 Test #19: unit_tests:Check registry pressure ................................   Passed    0.99 sec
      Start 20: unit_tests:Check many arrays
20/37 Test #20: unit_tests:Check many arrays ......................................   Passed    0.26 sec
      Start 21: unit_tests:Swarm memory management
21/37 Test #21: unit_tests:Swarm memory management ................................   Passed    0.31 sec
      Start 22: unit_tests:Can pull variables from containers based on Metadata
22/37 Test #22: unit_tests:Can pull variables from containers based on Metadata ...   Passed    0.67 sec
      Start 23: unit_tests:Test required/desired checking from inputs
23/37 Test #23: unit_tests:Test required/desired checking from inputs .............   Passed    0.26 sec
      Start 24: unit_tests:Parthenon Error Checking
24/37 Test #24: unit_tests:Parthenon Error Checking ...............................   Passed    0.27 sec
      Start 25: unit_tests:Check that integer division, rounded up, works
25/37 Test #25: unit_tests:Check that integer division, rounded up, works .........   Passed    0.27 sec
      Start 26: unit_tests:Check that partitioning a container works
26/37 Test #26: unit_tests:Check that partitioning a container works ..............   Passed    0.26 sec
      Start 27: performance_tests:Catch2 Container Iterator Performance
27/37 Test #27: performance_tests:Catch2 Container Iterator Performance ...........   Passed   11.80 sec
      Start 28: regression_test:advection_performance
28/37 Test #28: regression_test:advection_performance .............................   Passed  334.83 sec
      Start 29: regression_mpi_test:advection_performance

29/37 Test #29: regression_mpi_test:advection_performance .........................   Passed  1095.79 sec
      Start 30: regression_test:restart
30/37 Test #30: regression_test:restart ...........................................   Passed   69.44 sec
      Start 31: regression_mpi_test:restart
31/37 Test #31: regression_mpi_test:restart .......................................   Passed  112.76 sec
      Start 32: regression_test:calculate_pi
32/37 Test #32: regression_test:calculate_pi ......................................   Passed    2.77 sec
      Start 33: regression_mpi_test:calculate_pi
33/37 Test #33: regression_mpi_test:calculate_pi ..................................   Passed    2.92 sec
      Start 34: regression_test:advection_convergence
34/37 Test #34: regression_test:advection_convergence .............................   Passed  376.76 sec
      Start 35: regression_mpi_test:advection_convergence
35/37 Test #35: regression_mpi_test:advection_convergence .........................   Passed  682.34 sec
      Start 36: regression_test:output_hdf5
36/37 Test #36: regression_test:output_hdf5 .......................................   Passed  241.76 sec
      Start 37: regression_mpi_test:output_hdf5
37/37 Test #37: regression_mpi_test:output_hdf5 ...................................   Passed  368.85 sec

Congrats! It works.

Yurlungur

not sure why my approval didn't stick. I approve.

nmsriram mentioned this pull request Sep 30, 2020

Support restarts #61

Closed

nmsriram requested review from JoshuaSBrown, pgrete and Yurlungur September 30, 2020 14:10

JoshuaSBrown added the bug Something isn't working label Sep 30, 2020

Yurlungur approved these changes Sep 30, 2020

View reviewed changes

JoshuaSBrown reviewed Sep 30, 2020

View reviewed changes

JoshuaSBrown approved these changes Sep 30, 2020

View reviewed changes

JoshuaSBrown mentioned this pull request Sep 30, 2020

Bug in mpi restart regression test/infrastructure #312

Closed

pgrete reviewed Oct 2, 2020

View reviewed changes

sriram-LANL added 9 commits October 23, 2020 09:58

added a hack for applying reflective boundary conditions after prolon…

77e0cc7

…g / restrict

added initializationf or sparse variable start

e3c7aa8

Fixed mpi bug in restart.hpp where every rank was reading the first

05c0bf9

N blocks instead of reading at offset for that particular rank. Added capability to restart with or without ghost zones.

Merge branch 'develop' into sriram/tmp2

7a8968c

read in nbdel and nbnew from restart file. Added deep copy to ensure …

c6f9843

…GPU data is initialized on restart.

parallel regression test now checks silver file instead of comparing …

4561a85

…gold to itself

fixed formatting to satisfy cpplint and clang-format

98cf1ae

changed syntax for pointer from (*v). to v->

f1f9721

Merge branch 'bugfix/restart2' of github.com:lanl/parthenon into bugf…

0e20618

…ix/restart2

nmsriram force-pushed the bugfix/restart_parallel branch from 2f76ee2 to 0e20618 Compare October 23, 2020 19:13

sriram-LANL added 3 commits October 23, 2020 13:35

updated changelog

a1f5a48

fixed PR number in changelog

9d40867

fixed make format errors

194dc5d

Yurlungur approved these changes Oct 23, 2020

View reviewed changes

nmsriram requested review from Yurlungur and JoshuaSBrown October 23, 2020 20:33

pgrete approved these changes Oct 26, 2020

View reviewed changes

JoshuaSBrown reviewed Oct 26, 2020

View reviewed changes

Yurlungur mentioned this pull request Oct 26, 2020

Signal NaNs #338

Closed

Yurlungur approved these changes Oct 26, 2020

View reviewed changes

nmsriram merged commit 3920633 into develop Oct 26, 2020

Yurlungur deleted the bugfix/restart_parallel branch December 7, 2020 20:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Restart bugfix #311

Restart bugfix #311

nmsriram commented Sep 30, 2020 •

edited

Loading

Yurlungur left a comment

JoshuaSBrown left a comment

JoshuaSBrown Sep 30, 2020

JoshuaSBrown Sep 30, 2020

pgrete Oct 2, 2020

JoshuaSBrown commented Sep 30, 2020

JoshuaSBrown left a comment

Yurlungur commented Oct 1, 2020

pgrete Oct 2, 2020

nmsriram Oct 13, 2020

pgrete Oct 2, 2020

codecov bot commented Oct 2, 2020 •

edited

Loading

nmsriram commented Oct 13, 2020

pgrete commented Oct 23, 2020

nmsriram commented Oct 23, 2020

Yurlungur left a comment

Yurlungur Oct 23, 2020

JoshuaSBrown Oct 26, 2020

Yurlungur Oct 26, 2020

Yurlungur Oct 23, 2020

pgrete Oct 26, 2020

JoshuaSBrown Oct 26, 2020

Yurlungur Oct 26, 2020

nmsriram Oct 26, 2020

JoshuaSBrown Oct 26, 2020

JoshuaSBrown Oct 26, 2020

JoshuaSBrown Oct 26, 2020 •

edited

Loading

nmsriram Oct 26, 2020

JoshuaSBrown Oct 26, 2020

nmsriram Oct 26, 2020

JoshuaSBrown commented Oct 26, 2020

Yurlungur left a comment

		@@ -38,6 +38,52 @@

		namespace parthenon {

		// TODO(Yurlunger) Identify an idiomatic way of applying reflective

	// TODO(Yurlunger) Identify an idiomatic way of applying reflective
	// TODO(JMM) Identify an idiomatic way of applying reflective

		const IndexDomain theDomain =
		(output_params.include_ghost_zones ? IndexDomain::entire : IndexDomain::interior);

Restart bugfix #311

Restart bugfix #311

Conversation

nmsriram commented Sep 30, 2020 • edited Loading

PR Summary

PR Checklist

Yurlungur left a comment

Choose a reason for hiding this comment

JoshuaSBrown left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoshuaSBrown commented Sep 30, 2020

JoshuaSBrown left a comment

Choose a reason for hiding this comment

Yurlungur commented Oct 1, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Oct 2, 2020 • edited Loading

Codecov Report

nmsriram commented Oct 13, 2020

pgrete commented Oct 23, 2020

nmsriram commented Oct 23, 2020

Yurlungur left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoshuaSBrown Oct 26, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoshuaSBrown commented Oct 26, 2020

Yurlungur left a comment

Choose a reason for hiding this comment

nmsriram commented Sep 30, 2020 •

edited

Loading

codecov bot commented Oct 2, 2020 •

edited

Loading

JoshuaSBrown Oct 26, 2020 •

edited

Loading