search

parthenon-hpc-lab / athenapk

AthenaPK: a performance portable version of Athena++ built on Parthenon and Kokkos
BSD 3-Clause "New" or "Revised" License
52 stars 12 forks source link

enhanced first-order flux correction #29

Open BenWibking opened 1 year ago

BenWibking commented 1 year ago

Two things are not clear to me:

  1. Does first-order flux correction (as implemented here) drop to first order fluxes for each stage separately? (i.e., does it still do higher-order time integration if you attempt to use it with RK integrators?)

  2. What happens if there is a bad cell at the edge of a MeshBlock? Are the new fluxes communicated to neighboring MeshBlocks?

BenWibking commented 1 year ago

If the answer to 1 is yes, based on a discussion with @forrestglines last week, I think it may be more robust to drop to first-order in time as well. I think we need to keep every RK stage in memory in this case.

pgrete commented 1 year ago
  1. yes, and I agree that a drop to first order in time would also be more consistent/robust, but would have required additional complexity (regarding keeping all stages in memory as you say).
  2. Yes and no:
    • Yes, for multilevel bounds because Multilevel FluxCorrection is always called. Basically "calc flux -> first_oder_correct -> multi_level_correct -> hope for the best" (with hope for the best because in principle that multi level correct may introduce negative fluxes again)
    • No, for same level bounds, though that might be an easy fix by just extending the bounds for the kernel by +/- 1 in each direction.
BenWibking commented 1 year ago
  • No, for same level bounds, though that might be an easy fix by just extending the bounds for the kernel by +/- 1 in each direction.

Would this require increasing the number of ghost zones by 1? That's not too bad of a tradeoff, though, IMO.

BenWibking commented 1 year ago
  1. yes, and I agree that a drop to first order in time would also be more consistent/robust, but would have required additional complexity (regarding keeping all stages in memory as you say).

Would it be straightforward to add support for ButcherIntegrator from this Parthenon PR: https://github.com/parthenon-hpc-lab/parthenon/pull/840?

Or would you prefer to stick to the low-storage ones in AthenaPK?

pgrete commented 1 year ago
  • No, for same level bounds, though that might be an easy fix by just extending the bounds for the kernel by +/- 1 in each direction.

Would this require increasing the number of ghost zones by 1? That's not too bad of a tradeoff, though, IMO.

Probably not (assuming that you're running with at least second order/2 ghost zones otherwise). Given that first order flux correction uses piecewise constant/donor cell reconstruction it just needs a single ghost one. Extending the "interior" region by one should thus be fine if I didn't miss anything.

pgrete commented 1 year ago
  • No, for same level bounds, though that might be an easy fix by just extending the bounds for the kernel by +/- 1 in each direction.

Would this require increasing the number of ghost zones by 1? That's not too bad of a tradeoff, though, IMO.

Probably not (assuming that you're running with at least second order/2 ghost zones otherwise). Given that first order flux correction uses piecewise constant/donor cell reconstruction it just needs a single ghost one. Extending the "interior" region by one should thus be fine if I didn't miss anything.

I was just about to implement this, but then realized that it also won't be correct (without even more logic). The issue is that the first predicted ghost cell layer would need to have all higher order fluxes calculated for an identical prediction as the neighboring block actually owing the data. This mean would have to extend the the calculation of the standard fluxes fluxes, which, for example, for PPM mean to increase the number of ghost zones required by one. Do we want/need this at the moment?

BenWibking commented 1 year ago

At the resolutions I've run on CPU, it's not needed for my simulations. Maybe we should discuss whether it's needed for the AGN jet simulations.