fix openPMD plotfile output

[BenWibking]

Description

Fixes a segmentation fault during openPMD plotfile output when face-centered variables are enabled.

There was an implicit assumption in AMRSimulation<problem_t>::AverageFCToCC() that the number of face-centered ghost zones was the same as the number of cell-centered ghost zones. This PR allows the number of ghosts to differ.

I manually tested that this works inside the CUDA dev container by compiling Quokka with -DQUOKKA_OPENPMD=ON, running the HydroBlast3D problem, and then examining the output files with openPMD-viewer.

Related issues

Fixes #742.

Checklist

Before this pull request can be reviewed, all of these tasks should be completed. Denote completed tasks with an x inside the square brackets [ ] in the Markdown source below:

• I have added a description (see above).
• I have added a link to any related issues (if applicable; see above).
• I have read the Contributing Guide.
• I have added tests for any new physics that this PR adds to the code.
• (For quokka-astro org members) I have manually triggered the GPU tests with the magic comment /azp run.

[BenWibking]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 2 pipeline(s).

[BenWibking]

Out-of-bounds array access: https://app.circleci.com/pipelines/circleci/TJttRfCZuo9qQMru3cFqzy/SoCatADYnqm7LfdmA3zswt/1080/workflows/79cefde6-56f9-4d61-907d-d5d4c15e7315/jobs/2338

To be investigated.

[BenWibking]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 2 pipeline(s).

[BenWibking]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 2 pipeline(s).

[BenWibking]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 2 pipeline(s).

[sonarcloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[BenWibking]

@markkrumholz Not urgent, but this is now ready for review.

[markkrumholz]

There seems to be a slight inconsistency in the design: as written now, the code will always fail if run for a case with more cell-centered than face-centered ghost zones, due to the AMREX_ALWAYS_ASSERT added to the routine that does the averaging to centers. However, both doDiagnostics and renderAscent seem to support having more cell-centered than face-centered ghost zones, and gracefully use a std::min to handle that. This is confusing. I think we should put similar assertions in every routine if we don't want to support having nghost_cc > nghost_fc, or we should edit the averaging to center routine to also support this case. One possible way to do this would be to have the routine take an optional argument that provides a fill value to add to ghost zones that cannot be filled because the corresponding face-centered cells do not exist. I don't have strong feelings about which of these options we pick, but I think the design should be consistent as to whether we do or don't support the case nghost_cc > nghost_fc.

[BenWibking]

There seems to be a slight inconsistency in the design: as written now, the code will always fail if run for a case with more cell-centered than face-centered ghost zones, due to the AMREX_ALWAYS_ASSERT added to the routine that does the averaging to centers. However, both doDiagnostics and renderAscent seem to support having more cell-centered than face-centered ghost zones, and gracefully use a std::min to handle that. This is confusing. I think we should put similar assertions in every routine if we don't want to support having nghost_cc > nghost_fc, or we should edit the averaging to center routine to also support this case. One possible way to do this would be to have the routine take an optional argument that provides a fill value to add to ghost zones that cannot be filled because the corresponding face-centered cells do not exist. I don't have strong feelings about which of these options we pick, but I think the design should be consistent as to whether we do or don't support the case nghost_cc > nghost_fc.

In the current code, if I am remembering correctly, averaging to centers is only done for outputs. Everywhere an output needs to do that, it provides an appropriate number of ghost zones to use (that may be less than nghost_cc). If we are not doing MHD, then we don't need to carry around extra ghost faces we don't need, and we only need to average them to centers for outputs. I don't see an issue with this design in principle.

I don't think it's a good idea to allow averaging to centers with a fill value. That is almost certain to produce bugs in the future. The assertion just forces anyone who uses the averaging function to specify something sensible upfront. In any case, I don't know where we would need to do this.

[BenWibking]

Actually, ghost cells in outputs are only really needed for Ascent, due to the way it uses them to compute surfaces and streamlines in parallel without doing extra communication itself. For all other outputs, we could just get rid of the ghost cells. They are not supported in OpenPMD outputs at all, and in AMReX plotfiles I don't think they are used for anything.

Edit: I need to double check whether they are used by any of the diagnostics.

[BenWibking]

Just to clarify- the case where nghost_cc > nghost_fc is tested as part of the SphericalCollapse test suite problem, since tracer particles without MHD enabled set nghost_fc = 2, while nghost_cc is always 4.

[markkrumholz]

So my confusion is these lines (2092 - 2096) of Simulation.hpp:

auto const &state_cc = mf_cc.arrays(); auto const &state_fc = mf_fc.const_arrays(); amrex::ParallelFor(mf_cc, amrex::IntVect(AMREX_D_DECL(nGrow, nGrow, nGrow)), [=] AMREX_GPU_DEVICE(int boxidx, int i, int j, int k) { int const ng_cc = mf_cc.nGrow(); int const ng_fc = mf_fc.nGrow(); AMREX_ALWAYS_ASSERT(ng_cc <= ng_fc); // if this is false, we can't fill the ghost cells!

It seems to me like you are grabbing the state vectors, extracting their number of ghost zones, and then throwing an assertion that will fail anytime the cell-centered state variables have more ghost zones than the face-centered ones. So it seems like this should fail any time AverageFCToCC is called, which it always is any time you're writing a plot file, and it seems like it should fail independent of how many cells you're trying to write to that plot file. Am I misunderstanding what this code does?