search

IAS-Astrophysics / athenak

Performance-portable version of the Athena++ astrophysical AMR-MHD code using Kokkos.
BSD 3-Clause "New" or "Revised" License
35 stars 25 forks source link

Make restart file compatible with previous version #602

Closed mh-guo closed 1 month ago

mh-guo commented 2 months ago

The RegionSize struct has been changed since the merge of the DynGRMHD module. This leads to a change in the structure of the restart file so the the code cannot read the previous restart file correctly. This PR rollbacks the RegionSize struct so that the restart file is compatible with previous versions. In addition, the new variables ixd1, idx2, idx3 in RegionSize are used only once in the code so it seems not necessary. I'm also open to keeping these variables if they are necessary.

jfields7 commented 2 months ago

I don't remember exactly why I did this. It might have been to match GR-Athena++, or perhaps it was to eliminate the extra divisions. Nevertheless, it's a little surprising that this breaks the restart files; we've had issues with other quantities in the code needing to be added explicitly to the checkpointing procedure. Is the mesh saved via a straight memory dump into the file?

mh-guo commented 1 month ago

Yes the mesh is directly dumped into the restart file in restart.cpp, line 202, resfile.Write_any_type(&(pm->mesh_size), (sizeof(RegionSize)), "byte"); So a change of RegionSize leads to a change of the restart file that the code cannot handle. For other quantities in the code needing to be added explicitly to the checkpointing procedure, the code would check, say, whether pz4c is constructed, so it can handle them properly. Which solution do you think is better?

mh-guo commented 1 month ago

In addition, it's really the size of MeshBlock, mb_size, not the Mesh size mesh_size that uses the new variables like idx1. So the new variables in mesh_size are never used. Perhaps we could treat them separately instead of using the same struct RegionSize?

jfields7 commented 1 month ago

I'm fine with getting rid of it. I ran some quick tests comparing performance. It might be marginally faster to store the reciprocal spacings rather than doing the divisions on the fly, but it's not worth the headache of breaking everyone else's restart files. In principle we could also use these calculations inside the Z4c RHS, but I'd be shocked if it even made a dent in the performance cost.