Extend SurfaceLayer for all boundary sides - #3860
Conversation
…particles correctly across AMR levels without breaking AMReX cic_/mac_interpolate_mapped_z which read idata(k) Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…_MANAGED works under CUDA Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…nce_microphysics so next sub-cycle's FillPatchFineLevel sees consistent boundary values and doesn't drain the bubble interior Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…qs/qg) plus qt; abort at setup if requested field is unsupported by the active moisture model
…ain metric, and pos_phys/pos_comp helpers for the upcoming computational-coordinate particle position refactor
…e floor formula (uses AMReX DefaultAssignor), drop AoS k slot, drop terrain-correction dance (FixKIndexAMR now just Redistribute), drop ExtractAndRouteOORParticles, swap mapped-z interp with plain interp; uniform-z bubble cases pass; ParticleWoA terrain and ParticleAdvect_AMR1_box mass-density gold need follow-up
…particles from crossing levels mid-coarse-step (which caused double-advection on the fine level); reintroduce a simplified ExtractAndRouteOORParticles that routes fine-level OOR particles back to L0 using DefaultAssignor [run-ci]
This reverts commit 1de10e6.
…rticle species (e.g., super_droplets_moisture_mass_density). field_name = <species>_<mesh_var> dispatches to ERFPC::computeMeshVar; per-level deposit + average down matches the existing _count pattern
…te prob-name branches
|
@asalmgren I think the review comments should be addressed now |
asalmgren
left a comment
There was a problem hiding this comment.
Review: index handling on the new high faces
Thanks for this — the overall structure (per-orientation SurfaceLayer instances, face-prefixed inputs, face-indexed checkpoint entries) reads well, and the zlo path is mostly preserved. I focused on the new non-zlo code paths and found a consistent class of index bug there, plus one regression that reaches the existing zlo path.
The most important structural point: the new ABL_MOST_Cloudchamber test sets most.average_policy = 1 on all six faces and sets most.zref explicitly on all six, so the plane-average path and the default-zref path on high faces get no CI coverage — and both are broken (comments 1-4 below).
High severity
ERF_MOSTAverage.cpp:1290, 1381, 1483—sm_index = bigEnd(dir) + 1compared against cell-centered boxes means the plane averages for U/V/Theta/Qv, theta_v and Umag are never accumulated on any high face and end up zero.ERF_MOSTAverage.cpp:645— high-facezrefis not snapped to the cell centre (so opposite walls disagree for a symmetric domain); thelkformula treatszrefas a wall distance while the asserts and the default treat it as an absolute coordinate, so the no-most.zrefdefault selectslk = 0; andm_geom[0]is mixed withm_geom[lev].ERF_SurfaceLayer.cpp:976—bxyis not nodal in x, so the xhi wall stress on w-momentum is written to an interior node and never reaches the boundary node.ERF_SurfaceLayer.cpp:956, 1016, 1020— theis_low_face ? n : n+1shift is applied to tangential indices fordir == 0/dir == 1, offsetting opposite walls by one cell.ERF_DiffusionSrcForState_N.cpp:76(and_S.cpp:78,_T.cpp:94) —use_SurfLayeris now ignored, so the native-SHOC "don't re-apply the surface scalar flux" decision made inERF_SlowRhsPre.cpp:267is discarded and the flux is double-counted.
Medium
6. ERF_MOSTStress.H:1417 — q_star becomes exactly 0 for existing moist zlo runs that don't set most.surf_moist. Looks intentional, but it changes previously-working zlo behavior.
7. ERF_SurfaceLayer.cpp:395 — compute_fluxes expands the box to the full domain height while the FABs only own their own box's k-range; out-of-bounds if the grid is decomposed in z.
Low: ERF.cpp:1336 (O(N^2) checkpoint reads on restart), ERF_Plotfile.cpp:1820 (unguarded Tau11 deref with diffusion off), ERF_MOSTAverage.cpp:797 (2*prob_lo offset in the low-face terrain zref), ERF_MakeNewArrays.cpp:717 (SFS_q2fx3_lev not nulled).
Suggested gate before merge: switch the test to most.average_policy = 0 (the block already commented out at lines 74-79 of ABL_MOST_Cloudchamber.i) and drop the explicit most.zref lines. That alone exercises items 1 and 2.
Review produced with Claude Code.
|
@asalmgren The new review issues should be addressed now. |
SummaryThings I'd want addressed before merge 1. Plane averaging double-counts on lateral faces under tiling.compute_plane_averages, first loop (ERF_MOSTAverage.cpp:1213-1228): the face test is on vbx (the grid), then every tile of that grid gets pbx.setSmall(dir,sm_index); pbx.setBig(dir,sm_index). TileNoZ() returns (mfiter_tile_size[0], mfiter_tile_size[1], 1024000), and AMReX's default mfiter_tile_size is (1024000,8,8) — so on CPU there is tiling in y. For a ylo/yhi face, every y-tile of a grid collapses onto the same y-plane and gets summed, while ncell_plane counts the plane once. A grid 32 cells deep in y over-counts by 4×. x-faces are safe only because the default x tile size is huge; set fabarray.mfiter_tile_size and they break too. The Tv block and the Umag block right below both test pbx instead and are correct — that internal inconsistency is the tell. Switching the first loop to the pbx test matches them and fixes it. The new gtest wouldn't catch this: the domain is (3,5,7)–(5,8,11), one box, 4 cells in y, so no tile split. Bumping that domain past 8 cells in y (or adding a multi-box BoxArray) would cover it, and would also give the first multi-grid coverage of these paths. 2. t31_arr / t32_arr null-deref guard is inverted. ERF_SurfaceLayer.cpp:~690:When dir == 0 the short circuit skips the pointer check entirely, so a null tau31 dereferences. In practice update_diffusive_arrays allocates tau21/31/32 whenever l_Surf_X || l_Surf_Y, so this only fires with l_use_diff == false — but then it's a segfault where an assert would be much kinder. (Tau_lev[...] != nullptr) alone is the right condition; the dir == 0 clause buys nothing. 3. Ghost values on lateral faces are never communicated.update_fluxes now skips FillBoundary on u_star/t_star/q_star/olen unless coordDir() == 2. But compute_u_flux for dir == 0 reads u_star_arr(ic,j-1,kc) and umm_arr(ic,j-1,kc) at the low y-node of every grid. With more than one grid in y along an x-wall, that's a ghost cell filled only by whatever compute_fluxes happened to compute over its own growntilebox, using average-MF ghosts that (see 4) are themselves not filled tangentially. The comment says the faces are "filled individually on corresponding ranks," which is true of the valid region but not the halo. Worth either restoring FillBoundary for all faces or documenting why the grown-tilebox computation is sufficient. 4. Periodicity(IntVect::TheDimensionVector(dir)) on the average FillBoundary calls (lines 1677, 1800, 1944-1947).Periodicity takes period lengths, so this asks for period 1 in the collapsed direction and non-periodic in the other two. It does propagate the single valid plane into the z-ghosts, which I assume is the intent — but it also drops the tangential periodic images that geom.periodicity() used to supply. For a zlo run with periodic x/y and average_policy = 1/2, the ghost values feeding um_arr(i-1,...) at the periodic boundary now come from nowhere. No gold file changed, so either the affected configs aren't in CI or the ghosts don't reach the answer — but the reasoning should be in a comment rather than left implicit. 6. Cross-BoxArray MFIter indexing.compute_fluxes and fill_qsurf_with_qsat now iterate *m_lmask_lev[lev][0] and call ->array(mfi) on MultiFabs built on a different BoxArray (collapsed in dir, not in z). This works only because both derive from the same 3D grids with the same DM and because of the new full-z-column assert — AMReX will not catch a violation, even in debug. The rationale in the comment is good; I'd add an AMREX_ASSERT on boxArray().size() and DM equality so a future decomposition change fails loudly. Relatedly, gtbx.grow(2,3) in fill_qsurf_with_qsat hardcodes a ghost width that must not exceed ng[0] of the 2D MF — worth deriving from nGrowVect(). |
This allows the MOST/Surface Layer boundary condition to be specified on all 6 faces instead of only zlo.
Main changes to
SurfaceLayer/MOSTAverageclasses:SurfaceLayerandMOSTAverageclasses to determine behavior on different faces.erf.most.surf_temp, can now beerf.xlo.most.surf_temp).SurfaceLayerwrites to Tau and heat/moisture flux arrays for each face. Directional velocity averages and orientation is passed to MOSTStress.Main changes to
ERF:SurfaceLayerinstance all 6 faces where the BC type is set tosurface_layer(otherwise nullptr)A follow-up PR will be made to adapt this for the new cloudchamber interface and problem configuration.