Fix restaring from files not containing the derefinement counter (from #1073) #1089

pgrete · 2024-05-23T16:32:28Z

PR Summary

In #1073 the per-block derefinement counter was stored in restart files (and restored on restarts).
This changes the number of entries in the flattened loc.level-gid-lid-cnghost-gflag vector (from 5 per block to 6 per block).
If one currently restarts with from an older file this is not taken into account and logical locations (and potentially more are off).
I came across this because of a

### PARTHENON ERROR
  Message:     Somehow didn't find a tree.
  File:        /project/project_465000552/env/src/athenapk/external/parthenon/src/mesh/forest/forest.hpp
  Line number: 128
Somehow didn't find a tree.

on restart.

Effectively, this renders restart any older file impossible/erroneous right now.
The proposed fix is not necessarily super-clean but it circumvents bumping the output format version (and introducing similar logic anyway).
But if there's a different/cleaner solution, I'm also open to suggestions.

Note, that this still doesn't fully fix a restart problem I'm tracking down (also in context of reviewing #1055) but it fixes the error above.
It might also be that the restart I'm trying to fix are broken because I used some Parthenon version in between that didn't contain all the mesh/tree related bugfixes that went in over the past weeks.
I'm still investigating, but I'm fairly sure that this PR and the proposed fix are necessary no matter what.

PR Checklist

Yurlungur

This is gross but we gotta do what we gotta do

lroberts36

This looks like a reasonable fix to me, sorry for not thinking of that before. A possibly cleaner choice, but that would require more work, would be to include the derefinement count in a separate dataset in the hdf5 file.

pgrete · 2024-05-23T17:56:27Z

This looks like a reasonable fix to me, sorry for not thinking of that before. A possibly cleaner choice, but that would require more work, would be to include the derefinement count in a separate dataset in the hdf5 file.

Given that the other PR is just 8 days in, I'm kind of in favor of the clean fix rather than carrying around a work-around (that might inspire similar extension in the future).
I should be able to get to that early next week.

pgrete · 2024-05-23T17:56:59Z

(We could still merge this in now for an immediate resolution)

lroberts36 · 2024-05-23T18:05:40Z

(We could still merge this in now for an immediate resolution)

Let me see if I can get this to work this morning.

pgrete · 2024-05-23T18:11:22Z

My plan would be to extend the mesh_info object with a new vector containing the counters (which would by default set to 0 if not present).

lroberts36 · 2024-05-23T18:47:08Z

@pgrete and @Yurlungur: I just switched to making a new dataset and leaving level-gid-lid... as it was previous to #1073. I think this should be a clean fix. If the new dataset is not present in the file, HDF5 writes some complaints to screen but the exception is caught and it runs through.

tst/regression/test_suites/restart/parthinput.restart

Yurlungur

Thanks for the quick fix, @lroberts36 . This cleaner solution seems good to me. Some nitpicks/hdf5 foo below.

Yurlungur · 2024-05-23T19:47:21Z

src/outputs/restart_hdf5.cpp

+  try {
+    mesh_info.derefinement_count = ReadDataset<int>("/Blocks/derefinement_count");
+  } catch (const std::runtime_error &e) {


knowing HDF5 I bet there's voluminous output here... I'm fine with this solution, but if we wanted to reduce the amount that HDF5 complains, you could instead check for the existence of "/Blocks/derefinement_count" with, e.g., H5Lexists (https://docs.hdfgroup.org/hdf5/v1_12/group___h5_l.html#title11). Despite the function being documented as looking at links, it works for any "name" in an HDF5 file: https://forum.hdfgroup.org/t/check-if-dataset-exists/1454/5

Honestly, I am a little confused by the output. The beginning of ReadDataset does

// make sure dataset exists auto status = PARTHENON_HDF5_CHECK(H5Oexists_by_name(fh_, name.c_str(), H5P_DEFAULT)); PARTHENON_REQUIRE_THROWS( status > 0, "Dataset '" + name + "' does not exist in HDF5 file " + filename_);

and that throw is what I am catching here I think.

Ah. That makes sense. In which case yeah I wouldn't expect HDF5 to complain at all. Bizarre.

yeah in that case disregard my suggestion.

I am going to leave it as is for now.

Given that the (quite extensive) error message is printed by every rank, I now went for the H5Lexists option as @Yurlungur suggested (and we already use for the SparseInfo field).

src/outputs/restart_hdf5.cpp

tst/regression/test_suites/restart/parthinput.restart

lroberts36 · 2024-05-23T20:09:05Z

I will leave it to @pgrete to click automerge when he is happy with this.

pgrete · 2024-05-27T13:41:54Z

src/outputs/restart_hdf5.cpp

+    mesh_info.derefinement_count = ReadDataset<int>("/Blocks/derefinement_count");
+  } catch (const std::runtime_error &e) {
+    // File does not contain this dataset, so must be older. Set to default value of zero
+    if (Globals::my_rank == 0)


Extend to the check to only print with mesh refinement enabled?

I think because of the HDF5 spew that comes out when the dataset doesn't exist, we should probably always leave the warning in to provide some explanation to the user of what is going on.

Given that I removed the hdf5 error message, I now added the additional check.

pgrete · 2024-05-27T13:43:18Z

src/outputs/parthenon_hdf5.cpp

+    hsize_t loc_cnt[2] = {num_blocks_local, 1};
+    hsize_t glob_cnt[2] = {max_blocks_global, 1};
+    std::vector<int> tmpID = OutputUtils::ComputeDerefinementCount(pm);
+    HDF5Write2D(gBlocks, "derefinement_count", tmpID.data(), &loc_offset[0], &loc_cnt[0],
+                &glob_cnt[0], pl);


Shall we always write (i..e, a bunch of zeros for static runs) or move the if (pm->pmr) from ComputeDerefinementCount here to only write the deref counter for multilevel sims?

I kind of lean towards always writing to file, with or without refinement, just to keep the structure of the files the same. I think this should happen with things written as they are now.

tst/regression/test_suites/restart/parthinput.restart

pgrete added 2 commits May 23, 2024 15:53

Fix deref couter reading from previous restarts

e560ea7

Fix logic.

5e25fc0

pgrete added the bug Something isn't working label May 23, 2024

pgrete requested review from Yurlungur and lroberts36 May 23, 2024 16:32

Yurlungur approved these changes May 23, 2024

View reviewed changes

lroberts36 approved these changes May 23, 2024

View reviewed changes

make a separate dataset for derefinement count

32c6141

lroberts36 added 2 commits May 23, 2024 12:50

Merge branch 'develop' into pgrete/fix-deref-counter-for-restarts

54f6daa

don't set derefinement count to large number in restart test

5bfdfbe

lroberts36 requested a review from Yurlungur May 23, 2024 18:54

lroberts36 reviewed May 23, 2024

View reviewed changes

tst/regression/test_suites/restart/parthinput.restart Show resolved Hide resolved

lroberts36 added 2 commits May 23, 2024 13:27

Add warning

320ef1e

ignore derefinement_count in phdf_diff

bceceeb

Yurlungur approved these changes May 23, 2024

View reviewed changes

only warn on rank 0

52035a6

pgrete commented May 27, 2024

View reviewed changes

Merge branch 'develop' into pgrete/fix-deref-counter-for-restarts

28775f7

pgrete enabled auto-merge (squash) May 27, 2024 13:46

pgrete disabled auto-merge May 27, 2024 13:46

remove hdf debug output

f79ab08

pgrete enabled auto-merge (squash) May 29, 2024 10:31

pgrete merged commit bd615b7 into develop May 29, 2024
50 checks passed

pgrete deleted the pgrete/fix-deref-counter-for-restarts branch May 29, 2024 11:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix restaring from files not containing the derefinement counter (from #1073) #1089

Fix restaring from files not containing the derefinement counter (from #1073) #1089

pgrete commented May 23, 2024 •

edited by lroberts36

Loading

Yurlungur left a comment

lroberts36 left a comment

pgrete commented May 23, 2024

pgrete commented May 23, 2024

lroberts36 commented May 23, 2024

pgrete commented May 23, 2024

lroberts36 commented May 23, 2024 •

edited

Loading

Yurlungur left a comment

Yurlungur May 23, 2024

lroberts36 May 23, 2024

Yurlungur May 23, 2024

Yurlungur May 23, 2024

lroberts36 May 23, 2024

pgrete May 29, 2024

lroberts36 commented May 23, 2024

pgrete May 27, 2024

lroberts36 May 28, 2024

pgrete May 29, 2024

pgrete May 27, 2024

lroberts36 May 28, 2024

Fix restaring from files not containing the derefinement counter (from #1073) #1089

Fix restaring from files not containing the derefinement counter (from #1073) #1089

Conversation

pgrete commented May 23, 2024 • edited by lroberts36 Loading

PR Summary

PR Checklist

Yurlungur left a comment

Choose a reason for hiding this comment

lroberts36 left a comment

Choose a reason for hiding this comment

pgrete commented May 23, 2024

pgrete commented May 23, 2024

lroberts36 commented May 23, 2024

pgrete commented May 23, 2024

lroberts36 commented May 23, 2024 • edited Loading

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

lroberts36 commented May 23, 2024

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

pgrete commented May 23, 2024 •

edited by lroberts36

Loading

lroberts36 commented May 23, 2024 •

edited

Loading