twocylinders ternary complements both branches; internal_flow=false yields internal flow

[WeiqunZhang]

Severity: high · Category: correctness · Fix order: 7 of 29 — fix this 7th.

_{Filenames are numbered in reverse fix order: 001 = fix last, 029 = fix first. This file is 023.}

Location: src/embedded_boundaries/eb_twocylinders.cpp:64

Based on commit 7307d872 (line numbers refer to that tree).

The defect

What is wrong — The ternary's third operand (the plain UnionIF for external flow) is implicitly converted to ComplementIF via ComplementIF's non-explicit converting constructor, so BOTH branches build the complemented geometry; internal_flow=false (the default) yields internal-flow geometry.

Why it matters

3D EB build, incflo.geometry=twocylinders with twocylinders.internal_flow unset or false: instead of flow around two solid cylinders, the fluid region becomes the interior of the two cylinders and the rest of the domain is covered. Verified by compiling the expression against AMReX headers: deduced type ComplementIF<UnionIF<...>>, IF value at cylinder center is -0.25 (fluid) for both branches.

How to reach it

• test_3d/inputs.couette_twocylinders as shipped: 3D EB build, incflo.geometry="twocylinders", twocylinders.internal_flow=false, center1/center2 given. Dispatch at embedded_boundaries.cpp:45 has no guarding assert.

Suggested fix

A C++ ternary must deduce a single common type. EB2::ComplementIF has a non-explicit one-argument constructor (AMReX_EB2_IF_Complement.H) and UnionIF has no conversion the other way, so the false branch is silently wrapped in a complement too. Replace the ternary with an explicit if/else where each branch keeps its own concrete IF type and calls EB2::makeShop and EB2::Build locally; the Build arguments (geom.back(), max_level_here, max_coarsening_level) are identical in both branches, so only the geometry construction is duplicated. This keeps each implicit function at a single concrete type, the way every other file in src/embedded_boundaries uses them. An equivalent alternative, closer to eb_cylinder.cpp's idiom, is to build the CylinderIFs with has_fluid_inside = inside and use makeIntersection for the internal case (De Morgan), if the maintainer prefers no code duplication. Worth also reporting ComplementIF's implicit converting constructor upstream to AMReX, since any similar ternary will miscompile-by-conversion the same way.

--- a/src/embedded_boundaries/eb_twocylinders.cpp
+++ b/src/embedded_boundaries/eb_twocylinders.cpp
@@ -61,14 +61,17 @@
     // Build the implicit function as a union of two cylinders
     EB2::CylinderIF cyl1(radius1, direction1, center1, false);
     EB2::CylinderIF cyl2(radius2, direction2, center2, false);
-    auto twocylinders = inside ? EB2::makeComplement(EB2::makeUnion(cyl1, cyl2))
-                               : EB2::makeUnion(cyl1, cyl2);
-
-    // Generate GeometryShop
-    auto gshop = EB2::makeShop(twocylinders);
 
     // Build index space
     int max_level_here = 0;
     int max_coarsening_level = 100;
-    EB2::Build(gshop, geom.back(), max_level_here, max_level_here + max_coarsening_level);
+    if (inside) {
+        // Generate GeometryShop
+        auto gshop = EB2::makeShop(EB2::makeComplement(EB2::makeUnion(cyl1, cyl2)));
+        EB2::Build(gshop, geom.back(), max_level_here, max_level_here + max_coarsening_level);
+    } else {
+        // Generate GeometryShop
+        auto gshop = EB2::makeShop(EB2::makeUnion(cyl1, cyl2));
+        EB2::Build(gshop, geom.back(), max_level_here, max_level_here + max_coarsening_level);
+    }
 }

_{Splits the mixed-type ternary into separate if/else branches so the false branch keeps type UnionIF instead of being implicitly converted through ComplementIF's non-explicit converting constructor (AMReX_EB2_IF_Complement.H:27). Each branch makes its own shop and calls EB2::Build; internal_flow=false now yields solid cylinders (external flow) as intended. Uses only existing EB2::makeUnion/makeComplement/makeShop APIs; behavior for internal_flow=true is unchanged.}

_{Diff(s) are against 7307d872, written from the current source and verified only with git apply --check — never compiled, never run, never applied to the tree. Treat them as precise intent, not tested patches.}

Verification evidence

F008 — confirmed (two independent verifier lenses)

Lens 1 (refutation attempt): Lines 64-65: auto twocylinders = inside ? EB2::makeComplement(EB2::makeUnion(cyl1, cyl2)) : EB2::makeUnion(cyl1, cyl2);. AMReX_EB2_IF_Complement.H:27 has non-explicit ComplementIF (F a_f), and UnionIF has no reverse conversion, so the ternary converts the false branch to ComplementIF. Compiled test against AMReX headers: deduced type is ComplementIF<UnionIF<...>>; IF at cylinder center = -0.25 (fluid) for BOTH inside=0 and inside=1, while the intended plain union gives +0.25 (body). Path reachable via geom_type=="twocylinders" in embedded_boundaries.cpp:45.

Lens 2 (reachability/intent): eb_twocylinders.cpp:64-65 ternary; ComplementIF's non-explicit ctor ComplementIF (F a_f) (AMReX_EB2_IF_Complement.H:27) makes [expr.cond] convert the false-branch UnionIF to ComplementIF. Compile test vs real headers: static_assert deduced type ComplementIF<UnionIF<...>> passed; inside=false yields -0.25 (fluid) at cylinder center vs +0.25 for plain union. Ternary introduced in e78cd29 (2024); prior code was unconditional makeUnion, so inversion is unintended. IF feeds EB2::Build directly; nothing masks it.

---

_{Based on commit 7307d872, which is also the tree the audit verified against. From an automated audit of src/. Audit finding id: F008. Reviewer unit(s): EmbeddedBoundaries. Nothing here was compiled or run — the failure scenarios are code reasoning, so the reaching configuration above is the cheapest way to confirm or refute it.}