Skip to content

IB force history: record each run's last step; don't overwrite on restart - #1950

Open
sbryngelson wants to merge 5 commits into
MFlowCode:masterfrom
sbryngelson:fix-ib-force-history-restart
Open

sbryngelson wants to merge 5 commits into
MFlowCode:masterfrom
sbryngelson:fix-ib-force-history-restart

Conversation

@sbryngelson

@sbryngelson sbryngelson commented Oct 7, 2026 •

Copy link
Copy Markdown
Member

Description

Chaining a run with ib_force_wrt across restarts loses IB force history in two ways.

  1. One step is missing at every restart. s_write_ib_force_history(t_step) runs at RK stage 1 of step N and records the force left by step N-1. The time loop in p_main exits as soon as t_step == t_step_stop, so the record for t_step_stop (the force after the run's last step) is never written. The next run deliberately skips its t_step_start, because at that point the force is still the one from before the run (the existing comment explains this). So no run ever writes that step. Our exports showed exactly one missing record at each restart step, e.g. steps 12735, 25470 and 38205 in a three-chunk run.
  2. A restart wipes the history. s_open_ib_force_history deletes and recreates D/ib_forces.dat on every run. A restarted run therefore keeps only its own rows, and every earlier chunk's history is gone unless the user renamed the file in between.

Fix.

  • p_main calls s_write_ib_force_history(t_step) once more after the time loop, which writes the run's last step. I did not write the resumed run's first step instead, because that would record the stale (zero) force the existing skip avoids.
  • A resumed run writes D/ib_forces_<t_step_start>.dat (D/ib_forces_n<n_start>.dat with cfl_dt) instead of overwriting D/ib_forces.dat. A fresh run still writes D/ib_forces.dat. Within each file the documented layout is unchanged: row 0 is that run's first recorded step, there is no header, and offsets are fixed.
  • docs/documentation/case.md describes the per-run files and the new last row.

Why not append into one file. That needs either absolute step-based rows, or a row base inferred from the existing file. Absolute rows leave NUL-filled holes whenever a run starts from a restart without the earlier history (the existing docs and comments avoid holes on purpose). An inferred base is silently misaligned after a chunk killed past its last restart dump. Separate files avoid both, and they still concatenate into the full history.

Impact on results and goldens

The physics is unchanged. Every run with ib_force_wrt now has one more row per body in D/ib_forces.dat. Ten goldens contain that file (135F548B, 49893269, 4BED9896, 5A22B45F, B317404C, C8AD6271, D6794F4C, E085CC5A, E5B66084, F200F862), and they are updated only on the D/ib_forces.dat line:

  • I regenerated them on a Frontier CPU compute node (GNU 12.3 + Cray MPICH, Release).
  • Using the harness's own packtol.compare at each test's tolerance (1e-10, Examples 1e-3), the regenerated values for every field, including the existing ib_forces rows, match the committed NVHPC 25.11 goldens.
  • I then appended only the new final row(s) to each old ib_forces line. The old line is a string prefix of the new one, every other line and every golden-metadata.txt is untouched, and the change is one line per golden.
  • The appended values come from the GNU run. If you would rather have them from your NVHPC lane, ./mfc.sh test --generate --only <those 10> reproduces them.

Verification

All runs were on a Frontier CPU compute node (GNU 12.3 + Cray MPICH, Release, 2 ranks). The case is a 2D circle moving at a prescribed velocity, with ib_force_wrt.

run rows written steps covered
straight 0→40, this branch 40 1..40
0→20 then restart 20→40, this branch 20 + 20 (ib_forces.dat, ib_forces_20.dat) 1..20, 21..40
0→20 then restart 20→40, master 19, then overwritten by 19 1..19, then only 21..39

On this branch the two files concatenated match the straight run's ib_forces.dat: the same 40 times, and a maximum absolute difference of 2e-17 over all columns.

With the updated goldens, on the same node:

code the 10 ib_forces tests
this branch 10/10 pass
master 10/10 fail: Variable count didn't match for D/ib_forces.dat (the missing last row)

simulation compiles with CCE 19 (CPU) and GNU 12.3. Precheck passes, apart from two test_thermochem cases that fail on the Frontier login node because they compile with the system /usr/bin/gfortran (addressed by #1943).

Contribution Policy

We do not accept pull requests generated primarily by AI without genuine understanding or real-world usage context.

All contributions are expected to demonstrate:

  • A clear understanding of the codebase
  • Alignment with product direction
  • Thoughtful reasoning behind changes
  • Evidence of real-world usage or hands-on experience with the problem

If these expectations are not met, we would prefer to implement the changes ourselves rather than spend time reviewing low-effort submissions.


Acknowledgement

  • I confirm this PR meets the above expectations and reflects my own understanding and real-world context.

This PR was prepared with the assistance of an AI tool (Claude Code). We hit this in chunked production runs on Frontier; the fix was exercised on the small cases above.

PR template credit: junegunn

sbryngelson and others added 2 commits October 6, 2026 23:22
…tart

The record for step N is written at the start of step N, and the time loop
exits before starting its last step, so no run ever recorded the force at
t_step_stop. The next run skips its first step on purpose (that force is
still the one from before the run), so every restart left exactly one step
missing from the history. Write the last step's record after the loop.

A resumed run also deleted and recreated D/ib_forces.dat, losing the earlier
history. It now writes D/ib_forces_<t_step_start>.dat (D/ib_forces_n<n_start>
.dat with cfl_dt), so the per-run files concatenate into a complete history.

Co-Authored-By: Claude <noreply@anthropic.com>
Only the D/ib_forces.dat line changes: the old line is kept verbatim and the
new last-step row(s) are appended. Regenerated with GNU 12.3 + MPI, Release
on Frontier; every pre-existing value matches the committed NVHPC goldens
under each test's tolerance. All other lines and the metadata are untouched.

Co-Authored-By: Claude <noreply@anthropic.com>
@sbryngelson
sbryngelson marked this pull request as ready for review October 8, 2026 16:52
Copilot AI balanced review requested due to automatic review settings October 8, 2026 16:52

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.

🟢 Approval recommended

The changes preserve existing sampling semantics, with reported restart and golden-test verification supporting correctness.

2 open findings
What changed in this PR

Updates MFC’s immersed-boundary force output to preserve history across restarts without changing the physics.

Changes:

  • Writes the final force sample when allowed by the configured stride.
  • Uses restart-specific filenames to preserve earlier history.
  • Documents per-run output and extends force-history goldens.
File Description
tests/​F200F862/​golden.txt Adds the final force record.
tests/​E5B66084/​golden.txt Adds the final force record.
tests/​B317404C/​golden.txt Adds the final force record.
src/​simulation/​p_main.fpp Calls the history writer after time stepping.
src/​simulation/​m_data_output.fpp Selects restart-specific history filenames.
docs/​documentation/​case.md Describes endpoints and per-run history files.

🧠 Review effort: Balanced


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

Comment thread docs/documentation/case.md Outdated
Comment thread src/simulation/m_data_output.fpp
…ntees

The final record still obeys ib_force_stride, and with cfl_dt the step
counter restarts each run, so a gap-free join needs stride 1 there.
Enable ib_force_wrt in the particle-cloud restart cases. run_restart now
appends the resumed run's ib_forces_<mid>.dat to the first run's
ib_forces.dat, so the roundtrip checks that the first file survives and
the two join into the straight run's history. Goldens gain the
ib_forces.dat entry only.
@github-actions

github-actions Bot commented Oct 8, 2026

Copy link
Copy Markdown

Lines of Code

File Lines Diff
src/simulation/m_data_output.fpp 1584 +6
src/simulation/p_main.fpp 75 +2
Directory Lines Diff
simulation 28433 +8
total 47414 +8

@codecov

codecov Bot commented Oct 9, 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.79%. Comparing base (27cae2c) to head (2e08a89).
⚠️ Report is 2 commits behind head on master.

Files with missing lines Patch % Lines
src/simulation/m_data_output.fpp 40.00% 3 Missing ⚠️
src/simulation/p_main.fpp 0.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1950      +/-   ##
==========================================
- Coverage   61.79%   61.79%   -0.01%     
==========================================
  Files          86       86              
  Lines       22773    22778       +5     
  Branches     3353     3355       +2     
==========================================
+ Hits        14073    14075       +2     
- Misses       6211     6213       +2     
- Partials     2489     2490       +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.

This branch has not been deployed

No deployments
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