Require the relaxation targets that grcbc_out and grcbc_vel_out read - #1877
Require the relaxation targets that grcbc_out and grcbc_vel_out read#1877sbryngelson wants to merge 2 commits into
Conversation
m_cbc.fpp relaxes a GRCBC outflow toward a prescribed pressure:
L(adv%end) = c*(1 - Ma)*(pres - pres_out(dir))/Del_out(dir)
and grcbc_vel_out reads vel_out through dir_idx, so which component is the
normal one depends on the direction. Neither target was required. Setting
grcbc_out without pres_out validated cleanly, then relaxed the boundary
toward an undefined value.
The failure mode is silent and expensive to chase. On a uniform quiescent
2D field with no body and no disturbance, density in the last cell of the
outflow direction walked away linearly at 0.032 per step from step 1,
crossed zero near step 31, and the run aborted on ICFL at step 57 with
nothing in the output naming the boundary condition. ICFL sat at exactly
0.4000 for 56 steps and then jumped to 1.3e15 in one step, so it did not
read as a boundary problem either; only dumping the field every step and
finding every deviation pinned to cell i = m identified it.
check_grcbc now requires bc_<dir>%pres_out when grcbc_out is set, and
bc_<dir>%vel_out(1..num_dims) when grcbc_vel_out is set. Requiring every
component rather than guessing the normal one follows
check_synthetic_turbulence, and avoids the direction-blindness that MFlowCode#1854
corrects on the inflow side, where vel_in(1) is not the normal component
for a y or z boundary.
Verified it rejects nothing in tree: all 181 example cases generate and
validate, 9 of which use grcbc_out or grcbc_vel_out, and the full
validator suite passes (72 tests). Three regression tests cover the
rejection, the acceptance once targets are given, and that the third
velocity component is required only in 3D.
Made with Claude Code.
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Pull request overview
Tightens case validation for GRCBC outflow options by requiring the pressure/velocity targets that the CBC kernel actually reads, and adds regression tests to prevent silent misconfiguration.
Changes:
- Enforce
bc_<dir>%pres_outwhenbc_<dir>%grcbc_outis enabled. - Enforce
bc_<dir>%vel_out(1..num_dims)whenbc_<dir>%grcbc_vel_outis enabled. - Add validator regression tests covering rejection/acceptance and 2D vs 3D component requirements.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| toolchain/mfc/case_validator.py | Adds required-field validation for GRCBC outflow target parameters. |
| toolchain/mfc/test_case_validator.py | Adds regression tests for the new GRCBC outflow target validation rules. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| # direction. Require all num_dims components rather than guess, matching check_synthetic_turbulence. | ||
| for d in range(1, num_dims + 1): | ||
| self.prohibit( | ||
| not self.is_set(f"bc_{dir}%vel_out({d})"), | ||
| f"bc_{dir}%vel_out({d}) must be specified for all num_dims when bc_{dir}%grcbc_vel_out is enabled", |
| # The kernel reads vel_out through dir_idx, so which component is the normal one depends on the | ||
| # direction. Require all num_dims components rather than guess, matching check_synthetic_turbulence. | ||
| for d in range(1, num_dims + 1): | ||
| self.prohibit( | ||
| not self.is_set(f"bc_{dir}%vel_out({d})"), | ||
| f"bc_{dir}%vel_out({d}) must be specified for all num_dims when bc_{dir}%grcbc_vel_out is enabled", | ||
| ) |
| # m_cbc.fpp relaxes the outflow toward this pressure: | ||
| # L(adv%end) = c*(1 - Ma)*(pres - pres_out(dir))/Del_out(dir) | ||
| # Left unset it relaxes toward an undefined target, which is silent: the boundary cell simply | ||
| # walks away, and the run aborts on ICFL tens of steps later with nothing pointing at the BC. |
|
Thanks - taking the second point, which is correct and was the real bug in this PR. vel_out components (accepted). ! Add GRCBC for Subsonic Outflow (Normal Velocity)
L(adv%end) = L(adv%end) + rho*c**2*(1 - Ma)*(vel(dir_idx(1)) + vel_out(CBC_DIR, dir_idx(1))*sign(1, cbc_loc))/Del_out(CBC_DIR)and Added a regression test for the direction-awareness ( Message wording (accepted). It now names the specific component instead of the phrase "for all num_dims". L(adv%end) comment (checked, no change needed to the logic). Both sides really do write else if ((cbc_loc == -1 .and. bc%beg == BC_CHAR_NR_SUB_OUTFLOW) .or. (cbc_loc == 1 .and. bc%end == BC_CHAR_NR_SUB_OUTFLOW)) thenwith |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1877 +/- ##
==========================================
- Coverage 61.26% 61.25% -0.02%
==========================================
Files 84 84
Lines 22330 22336 +6
Branches 3265 3266 +1
==========================================
+ Hits 13680 13681 +1
- Misses 6207 6211 +4
- Partials 2443 2444 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Contribution Policy
Made with Claude Code.
What
check_grcbcnow requires the targets the CBC kernel actually reads:bc_<dir>%pres_outwhenbc_<dir>%grcbc_outis setbc_<dir>%vel_out(1..num_dims)whenbc_<dir>%grcbc_vel_outis setNeither was required before. Setting
grcbc_outwithoutpres_outvalidated cleanly.Why
m_cbc.fpprelaxes a GRCBC outflow toward a prescribed pressure:With
pres_outnever given, that relaxes the boundary toward an undefined value.The failure is silent and lands nowhere near its cause. I hit it on a 2D case and lost hours to it. On a uniform quiescent field -- no gust, no immersed boundary, nothing to disturb it -- density in the last cell of the outflow direction walked away linearly at 0.032 per step from step 1, crossed zero near step 31, and the run aborted on ICFL at step 57:
m = 1912, so every deviation sat on the outflow boundary. Nothing in the output named the BC.It did not read as a boundary problem either.
run_time.infshows ICFL at exactly 0.4000 for 56 steps with no drift, then 1.3e15 in one step -- so it looks like a single catastrophic event rather than a boundary slowly poisoning the solution, and the obvious suspects (grid, initial condition, immersed boundary, resolution) all get eliminated first. It was identified only by dumping the field every step and asking where it first departed from uniform.The same configuration is fine the moment
pres_outis supplied, which is whatexamples/2D_shockdropletand friends already do.The
vel_outhalfgrcbc_vel_outreadsvel_out(${CBC_DIR}$, dir_idx(1))-- the normal component, selected throughdir_idx, which is(1,2,3)/(2,1,3)/(3,1,2)for x / y / z. Sovel_out(1)is not the normal component for a y or z boundary. Rather than guess which component to require, this requires allnum_dimsof them, followingcheck_synthetic_turbulence. That is the same direction-blindness that #1854 corrects on the inflow side; the outflow side was never covered.Verification
grcbc_outorgrcbc_vel_out../mfc.sh formatclean, and the full precheck passes including "All example cases are valid".TestGrcbcOutflowTargets): the rejection, the acceptance once targets are given, and that the third velocity component is required only in 3D.No Fortran changes, no GPU code, no effect on any case that was already valid.
https://claude.ai/code/session_01HMJ7cycfo7kTFSFq5yhHLG