search

AMReX-Fluids / AMReX-Hydro

AMReX-based hydro routines
https://amrex-fluids.github.io/amrex-hydro/docs_html
Other
11 stars 24 forks source link

Fix operations based on divergence for RZ in Godunov 2D edge state. #92

Closed esclapez closed 1 year ago

esclapez commented 2 years ago

@cgilet I've kept things simple, not differentiating Cartesian and RZ. If you thing performance is a big issue, I can keep the area scaling part in IsRZ tests. This fixes the weird behavior of scalar advection as in the case of a rising hot bubble shown below (left: after, right: before the fix)

Screen Shot 2022-07-09 at 12 06 02 AM
cgilet commented 2 years ago

@esclapez Can you please commit your inputs file as regtest.2d.hotspot_rz? I'm thinking this case would be a better regression test than the simpler bubble problem. And thanks for catching this! This bug has been around for too long (even in fortran Godunov).

esclapez commented 2 years ago

It took me a while to find where this stuff was coming from. Especially because I used the Fortran as my reference :)

cgilet commented 2 years ago

So, I did the math of computing the Taylor series in cylindrical coordinates, and it suggests the correction implemented here isn't quite right. 16577339705998614008001936763421

drummerdoc commented 2 years ago

Glad that u->0 as r->0, or that could get nasty. But also wondering if we could redo in terms of area-weighted fluxes in order to make rz and xy look more similar (like we used to do in the linear operators). Not saying I can do this in my head here, but ...

cgilet commented 2 years ago

I thought the deal with the linear operators was basically that the divergence theorem was invoked so that we could think about the divergence in terms of the surface integral, which would require that we know those surface values (ie edge states) already, right? ... I think the upwinding that happens in Godunov would prevent us from being able to add the rz correction after the fact like happened with the hoop stress.

drummerdoc commented 2 years ago

possibly. But there is some ambiguity about the U that appears in the Godunov extrapolation. The only quantity that really should be from CC data at time n is the S. The source terms and the U could probably be just as accurately (and maybe more stably) evaluated on the face at n+1/2 (or, formally averaged between the CC data at n and the face data at n+1/2. As I said, I'm just not sure, but there may be a cheat through this....

esclapez commented 1 year ago

@cgilet I agree ! This is ready to merge whenever you have time.

cgilet commented 1 year ago

Closed via commit f2cfecb875