Skip to content

Abort when post_process is given a different grid than the restart files - #1858

Merged
sbryngelson merged 2 commits into
masterfrom
fix/restart-grid-mismatch
Sep 13, 2026
Merged

Abort when post_process is given a different grid than the restart files#1858
sbryngelson merged 2 commits into
masterfrom
fix/restart-grid-mismatch

Conversation

@sbryngelson

Copy link
Copy Markdown
Member

post_process checks that the restart files exist but never that they hold the grid the case file asks for. Give it a case whose resolution no longer matches the run and it reads past the end of every file, post-processes the overrun, and exits 0 with NaN-filled output. Nothing reports a problem until someone plots it — or until a diagnostic built on that output starts returning NaN and gets blamed instead, which is how I found it.

Reproduction

A 349 M cell run (m = 1644) post-processed with a case file that had since moved to m = 2056:

simulation.inp:    m = 1644     <- wrote the restart files
post_process.inp:  m = 2056     <- read them

681 M cells read from files holding 349 M. Exit code 0, silo written, every field NaN.

The check

x_cb.dat holds exactly m_glb + 2 cell boundaries, so its size states which grid wrote the restart. One inquire on a file already being opened, before any bulk read:

Restart grid mismatch: this case has m = 2056 but
.../restart_data/x_cb.dat was written with m = 1644. Post-processing must use the
same grid as the run that wrote the restart files, or it reads past the end of
every file and writes NaN.

Verified against the failing data: 2058 boundaries would need 16464 bytes, the file holds 13168, so it aborts where it previously ran to completion.

Builds clean (./mfc.sh build --no-gpu -t post_process).

https://claude.ai/code/session_01HMJ7cycfo7kTFSFq5yhHLG

Copilot AI lite review requested due to automatic review settings September 12, 2026 03:33

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Adds safety checks and new runtime features around restart handling, boundary-condition ramps, and immersed-boundary (IB) output/kinematics.

Changes:

  • Abort post-processing early when restart grid resolution (from x_cb.dat size) doesn’t match the case file’s m.
  • Add ramped inflow support (vel_in_ramp, vel_in_t0, vel_in_frac0) for GRCBC and Dirichlet-style inflows.
  • Add prescribed IB kinematics options and buffered per-step IB force/kinematic logging (D/ib<id>_forces.dat) with ib_force_stride.

Reviewed changes

Copilot reviewed 16 out of 16 changed files in this pull request and generated 11 comments.

Show a summary per file
File Description
toolchain/mfc/params/descriptions.py Adds parameter descriptions for IB force stride and IB kinematics.
toolchain/mfc/params/definitions.py Registers new params/hints (inflow ramp, IB force stride, IB kinematics) and raises probe max.
toolchain/mfc/case_validator.py Validates inflow ramp constraints and IB stride/kinematics constraints.
src/simulation/m_time_steppers.fpp Calls inflow-ramp updater and IB force logging; integrates prescribed kinematics into RK stages.
src/simulation/m_start_up.fpp Flushes buffered IB force records on finalize.
src/simulation/m_mpi_proxy.fpp Broadcasts new BC ramp parameters and IB kinematics parameters.
src/simulation/m_ibm.fpp Initializes prescribed kinematics at start; adds kinematics implementation and adjusts GPU/MPI data movement.
src/simulation/m_global_parameters.fpp Initializes new global/BC/IB parameters (ib_force_stride, inflow ramp fields, kinematics fields).
src/simulation/m_data_output.fpp Implements buffered per-step IB force/kinematics output and adjusts probe output formatting.
src/simulation/m_cbc.fpp Stores “final” inflow velocity and applies time ramp factor to inflow + exposes updater.
src/pre_process/m_global_parameters.fpp Initializes new IB kinematics fields for preprocessing.
src/post_process/m_data_input.f90 Adds restart grid mismatch detection via x_cb.dat file size.
src/common/m_derived_types.fpp Extends BC and IB derived types with inflow ramp + kinematics parameters.
src/common/m_constants.fpp Raises num_probes_max to 64.
src/common/m_boundary_primitives.fpp Applies ramp factor to Dirichlet boundary ghost velocities via bc_vel_ramp.
docs/documentation/case.md Documents new IB kinematics, IB force logging, and inflow ramp parameters.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/simulation/m_data_output.fpp Outdated
Comment on lines +1156 to +1176
do i = 1, ib_force_buf_n
ib_id = nint(ib_force_buf(1, i))
if (ib_id < 0) cycle ! already written as part of an earlier body's pass

write (file_loc, '(A,I0,A)') '/D/ib', ib_id, '_forces.dat'
file_loc = trim(case_dir) // trim(file_loc)
inquire (file=trim(file_loc), exist=file_exist)
if (file_exist) then
open (newunit=file_unit, file=trim(file_loc), form='formatted', status='old', position='append')
else
open (newunit=file_unit, file=trim(file_loc), form='formatted', status='new')
write (file_unit, '(A)') '# t_step time Fx Fy Fz Tx Ty Tz vx vy vz wx wy wz ax ay az xc yc zc'
end if

do j = i, ib_force_buf_n ! all rows for this body, in time order
if (nint(ib_force_buf(1, j)) /= ib_id) cycle
write (file_unit, '(I10,19(1X,ES17.9E3))') nint(ib_force_buf(2, j)), ib_force_buf(3:21,j)
ib_force_buf(1, j) = -1._wp
end do

close (file_unit)
Comment thread src/simulation/m_ibm.fpp Outdated
Comment on lines +86 to +89
! do all set up for moving immersed boundaries; prescribed kinematics are evaluated at the initial time so the
! first stage already sees the correct body state, velocity and angular velocity
t_init = t_step_start*dt
if (cfl_dt) t_init = t_save*n_start
Comment thread src/simulation/m_time_steppers.fpp Outdated
integer, intent(in) :: s
integer :: i
integer :: gbl_id ! used for analytic ib patch motion
real(wp) :: t_stage ! time of the state produced by RK stage s (used by prescribed kinematics)
Comment thread src/simulation/m_time_steppers.fpp Outdated
Comment on lines +798 to +801
t_stage = mytime + dt
if (time_stepper == time_stepper_rk3 .and. s == 2) t_stage = mytime + 0.5_wp*dt

$:GPU_PARALLEL_LOOP(private='[i, gbl_id]', copyin='[s, t_stage]')

file_loc = trim(case_dir) // '/restart_data' // trim(mpiiofs) // 'x_cb.dat'
inquire (FILE=trim(file_loc), EXIST=file_exist)
inquire (FILE=trim(file_loc), EXIST=file_exist, SIZE=file_bytes)
Comment thread src/simulation/m_data_output.fpp Outdated
Comment on lines +46 to +48
!> Buffered immersed-boundary force records: (id, t_step, time, force, torque, vel, angular_vel, angles, centroid)
integer, parameter :: ib_force_buf_len = 1024
real(wp), dimension(21, ib_force_buf_len) :: ib_force_buf
Comment thread src/simulation/m_data_output.fpp Outdated
Comment thread src/simulation/m_time_steppers.fpp Outdated
Comment thread src/simulation/m_cbc.fpp Outdated
Comment thread src/simulation/m_cbc.fpp Outdated
post_process checks that the restart files exist but never that they hold the
grid the case file asks for. Give it a case whose resolution no longer matches
the run and it reads past the end of every file, post-processes the overrun,
and exits 0 with NaN-filled output. Nothing reports a problem until someone
plots it, or until a diagnostic built on that output starts returning NaN and
gets blamed instead.

That is what happened here: a 349 M cell run (m = 1644) post-processed with a
case file that had since moved to m = 2056, so 681 M cells were read from files
holding 349 M.

x_cb.dat holds one value per cell boundary, so its size states which grid wrote
the restart. The check costs one inquire on a file already being opened, fires
before any bulk read, and names both grids.

The size is taken from storage_size(0._wp) rather than a literal 8, since
x_cb.dat is written with mpi_p and a --single build writes four-byte reals.
down_sample reads a full-resolution file with a stride of three, touching
stride*(m_glb + 1) + 1 boundaries, so it needs more of the file than m_glb + 2
and only the un-strided read can pin the size exactly -- three source grids of
different size can down-sample to the same m_glb.
@sbryngelson
sbryngelson force-pushed the fix/restart-grid-mismatch branch from 4007be2 to 731dd59 Compare September 12, 2026 04:28
@sbryngelson

Copy link
Copy Markdown
Member Author

Rebased onto master as one commit. The branch had been cut from a working branch and was dragging along stale, divergent copies of #1850, #1846, #1847 and the probe changes — including a bc_vel_ramp module array with GPU_DECLARE(create=...) and no device update, which under OpenACC would multiply ghost-cell velocities by uninitialized device memory on any Dirichlet boundary. That code does not exist in #1850, which evaluates the ramp on device from mytime through a pure function; it was an older draft that should never have been on this branch. Gone now — the PR is the 19-line check and nothing else.

Two real bugs in the check itself, both fixed:

Hardcoded 8 bytes. x_cb.dat is written with mpi_p, and mpi_p follows wp, so a --single build writes four-byte reals and every single-precision post_process run would have aborted, reporting half the true m. The --single CI lane runs --test-all, which includes post_process, so this would have failed CI. Now taken from storage_size(0._wp)/8.

down_sample broke. m_start_up.fpp:83 reduces m to int((m+1)/3) - 1 before m_glb = m, while x_cb.dat still holds full-resolution boundaries, so the strict equality fired spuriously. examples/3D_IGR_33jet/case.py sets it and no test covers it. The read is strided by 3 and touches stride*(m_glb + 1) + 1 boundaries, so down-sampling needs more of the file, not less; the check now compares against that, and keeps the exact equality only for the un-strided read — three source grids of different size can down-sample to the same m_glb, so exact is not available there.

./mfc.sh build -t post_process is clean. Dropped the Claude-Session trailer.

@github-actions

Copy link
Copy Markdown

Claude Code Review

Head SHA: 4007be2

Files changed:

  • 16
  • docs/documentation/case.md
  • src/common/m_boundary_primitives.fpp
  • src/common/m_constants.fpp
  • src/common/m_derived_types.fpp
  • src/post_process/m_data_input.f90
  • src/pre_process/m_global_parameters.fpp
  • src/simulation/m_cbc.fpp
  • src/simulation/m_data_output.fpp
  • src/simulation/m_global_parameters.fpp
  • src/simulation/m_ibm.fpp
    (+5 more)

Findings:

  • src/post_process/m_data_input.f90: the new restart-grid size check hardcodes 8 bytes per real (int(m_glb + 2, 8)*8_8), but the grid file is read via mpi_p/real(wp) whose width depends on the build's working precision (m_precision_select). In a single-precision build, x_cb.dat holds 4-byte reals, so file_bytes will never equal (m_glb+2)*8, and this check will abort every valid single-precision restart with a spurious "grid mismatch" error.
  • src/common/m_boundary_primitives.fpp: the new Dirichlet ramp scaling only touches momentum components with bc_vel_ramp(1) in the single added loop (hardcoded index 1, not a per-direction variable), even though bc_vel_ramp is declared dimension(3) and the feature (bc_[x,y,z]%vel_in_ramp, validated and documented for x, y and z alike) is meant to apply per direction. Unless equivalent scaling is also wired up at the y/z Dirichlet ghost-cell fill sites (not present in this diff), a bc_y%vel_in_ramp/bc_z%vel_in_ramp ramp on a Dirichlet boundary will be silently ignored while bc_x%vel_in_ramp works.
  • src/simulation/m_data_output.fpp (s_write_ib_force_files): the record's body id is computed as real(max(patch_ib(ib_idx)%gbl_patch_id, ib_idx), wp), mixing a local array index (ib_idx, from local_ib_patch_ids(i) in multi-process runs) with the global id via max(). If a rank's local index for a body exceeds that body's true gbl_patch_id (a realistic case once bodies are distributed/reordered across ranks), max silently picks the local index instead of the global id, so the row is written to the wrong D/ib<id>_forces.dat file.

@github-actions

Copy link
Copy Markdown

Lines of Code

File Lines Diff
src/post_process/m_data_input.f90 431 +11
Directory Lines Diff
post_process 3399 +11
total 46351 +11

@codecov

codecov Bot commented Sep 12, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 33.33333% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 61.25%. Comparing base (dc0aec1) to head (6f4294c).

Files with missing lines Patch % Lines
src/post_process/m_data_input.f90 33.33% 3 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1858      +/-   ##
==========================================
- Coverage   61.26%   61.25%   -0.01%     
==========================================
  Files          84       84              
  Lines       22330    22335       +5     
  Branches     3265     3266       +1     
==========================================
+ Hits        13680    13681       +1     
- Misses       6207     6210       +3     
- Partials     2443     2444       +1     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@sbryngelson
sbryngelson merged commit ec783a8 into master Sep 13, 2026
145 of 146 checks passed
@sbryngelson
sbryngelson deleted the fix/restart-grid-mismatch branch September 13, 2026 01:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants