Record immersed-boundary force history to one shared file - #1846
Record immersed-boundary force history to one shared file#1846sbryngelson wants to merge 14 commits into
Conversation
`ib_state_wrt` writes the force, torque and kinematic state of each immersed boundary only at snapshot intervals, so the force history is sampled at the field-output cadence. That is far too coarse to compare a transient load against an experiment or a reference computation, or to drive a reduced-order model from a force signal: typical cases here write a field every few hundred steps. Write one record per time step to `D/ib<id>_forces.dat` (time step, time, force, torque, velocity, angular velocity, angles, centroid), under the existing `ib_state_wrt` flag. Records are buffered per rank and flushed in batches rather than opened per body per step: opening a file per body per step is a metadata operation per step on a parallel filesystem and does not scale, and a particle bed of a thousand bodies would issue on the order of a hundred million of them over a long run. Each buffered row carries its own global body id, so a body changing owner mid-run needs no special handling, and `ib_force_stride` subsamples runs long enough for the record itself to become large. Claude-Session: https://claude.ai/code/session_01HMJ7cycfo7kTFSFq5yhHLG
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
Adds per-time-step immersed-boundary force/kinematics logging (buffered + flushed in batches) so IB force histories are recorded at high temporal resolution, with optional subsampling via ib_force_stride.
Changes:
- Introduces
ib_force_strideparameter (docs + toolchain registration + validation). - Writes buffered per-step IB force/torque/kinematics rows and flushes at shutdown.
- Hooks per-step recording into the time-step loop when
ib_state_wrtis enabled.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| toolchain/mfc/params/descriptions.py | Adds description string for ib_force_stride. |
| toolchain/mfc/params/definitions.py | Registers ib_force_stride in parameter definitions. |
| toolchain/mfc/case_validator.py | Validates ib_force_stride value in IBM checks. |
| src/simulation/m_time_steppers.fpp | Calls per-step IB force logging routine. |
| src/simulation/m_start_up.fpp | Flushes buffered IB force records at shutdown. |
| src/simulation/m_global_parameters.fpp | Sets default ib_force_stride = 1. |
| src/simulation/m_data_output.fpp | Implements buffered IB force record writing + flush to D/ib<id>_forces.dat. |
| docs/documentation/case.md | Documents ib_force_stride and new per-step IB force record behavior. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| 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' |
| !> 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 |
| $:GPU_UPDATE(host='[patch_ib(1:num_ibs)]') | ||
|
|
||
| do i = 1, n_write | ||
| ib_idx = i | ||
| if (num_procs > 1) ib_idx = local_ib_patch_ids(i) |
| ) | ||
| self.prohibit(not ib and num_ibs > 0, "num_ibs is set, but ib is not enabled") | ||
| self.prohibit(ib_state_wrt and not ib, "ib_state_wrt requires ib to be enabled") | ||
| ib_force_stride = self.get("ib_force_stride", 1) or 1 |
| integer :: i, j, ib_id, file_unit | ||
| logical :: file_exist | ||
|
|
||
| do i = 1, ib_force_buf_n |
| 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 |
ES18.10E3 is exactly wide enough for a negative value with a three-digit exponent (-1.2345678901E+003), so adjacent values were written with no space between them and the file could not be read as whitespace-separated columns. Use an explicit 1X separator. Claude-Session: https://claude.ai/code/session_01HMJ7cycfo7kTFSFq5yhHLG
|
Claude Code Review Head SHA: 0d79d0e Files changed:
Findings:
|
A rank that stops owning a body kept whatever it had buffered and wrote it at shutdown, after the new owner's newer records, so the file came out non-monotonic in time: on a 64-rank flapping case the time jumped from 3.24 back to 0 partway through. The claim in the original comment -- that a body changing owner needs no special handling because each row carries its own body id -- is true for which file a record lands in, but not for its order. Flush before the handoff so a rank cannot hold stale records, and say so correctly in the comment. Found on Frontier, 8 nodes, flapping plate whose centroid crosses a rank boundary during the stroke. Built with --gpu mp. Claude-Session: https://claude.ai/code/session_01HMJ7cycfo7kTFSFq5yhHLG
|
Two updates pushed after running this on a real case. A correctness fix. My original note claimed that because each buffered row carries its own body id, a body changing owner mid-run needed no special handling. That is true for which file a record lands in, but not for its order: a rank that stops owning a body kept its buffered records and wrote them at shutdown, after the new owner's newer ones. On a 64-rank flapping case the recorded time jumped from 3.24 back to 0 partway through the file. Fixed by flushing before the ownership handoff, and the comment now says the right thing. The column separator. Both were found by using the output rather than by review, which is the argument for landing this with a case that exercises it. The branch builds clean on Frontier with |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1846 +/- ##
==========================================
+ Coverage 61.35% 61.37% +0.01%
==========================================
Files 84 84
Lines 22225 22264 +39
Branches 3255 3265 +10
==========================================
+ Hits 13636 13664 +28
- Misses 6153 6158 +5
- Partials 2436 2442 +6 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
sbryngelson
changed the title
s_write_ib_force_history runs at RK stage 1, before the step's force has been computed, so the row for step N carries the force from the end of step N-1. The first step of a run has no N-1: patch_ib%force is still zero and the row records identically zero force. On a run chained across a queue's walltime limit that lands once per restart. A six-wingbeat flapping case split into four segments wrote zeros at steps 20649, 34649 and 48649, sitting among neighbours of -0.134, +0.474 and -0.475, which corrupts any per-beat trough or peak taken over the joined trace. Skip the write when t_step == t_step_start. Moving the call to the end of the step instead would drop the final step's row, since the write sits immediately before the t_step == t_step_stop return. Made with Claude Code.
|
It seems to me that |
|
yep i know. i'm working on this one still |
Skipping the t_step_start row left its bytes unwritten rather than shifting the file up, so row 0 was a hole. Nothing pre-fills ib_forces.dat -- both open paths create it empty and every write lands at a computed offset -- and an unwritten row is a sparse hole that reads back as NUL, not blanks. The packer's _extract_doubles collapses [\n\t\s]+, which does not match NUL, so the NULs survived into a token and float() raised on it. That is the "Failed to interpret the content of D/ib_forces.dat as a list of floating point numbers" on all eleven IBM tests, on every platform -- it is I/O arithmetic, so CPU, macOS, Intel and both Frontier compilers failed it alike. Counting rows from the first recorded step instead makes the file dense from row 0 wherever the run starts. On a restart the old numbering was worse than one missing row: with t_step_start=20649 every row beneath it was a hole, a multi-megabyte NUL prefix, and the offset formula in case.md then resolved to the wrong record for anyone using it as documented. That formula is updated to match. Verified on the failing cases: ib_forces.dat now contains no NUL bytes, its size is an exact multiple of the 353-byte record, and the first record is the first real measurement rather than the zero row the skip was added to remove. All eleven tests pass. Goldens regenerated. They lose exactly the leading zero-force row -- 1000 values, fifty bodies by twenty columns, for E085CC5A -- which is the row the skip was meant to drop. 158 of the remaining 49000 values shift by at most 5.5e-18 absolute, on quantities of order 1e-10, from regenerating on a different machine; that is well inside the 1e-12 absolute tolerance. Claude-Session: https://claude.ai/code/session_017zrZooJPhZtZYgg9fJiYhg
|
Both fair. The second point landed right after I changed the design, so let me lay out where it actually stands. Opt-out — you're right, and it's still missing. Writes at scale — this isn't file-per-body any more. At 1000 ranks holding 100 bodies each that's one open for the whole run plus one write per body per recorded step, against the inquire + open + close per body per rank per step the old shape cost — roughly 3e5 metadata ops per step. That cost is exactly what you're describing, and it's what prompted the rewrite, so I think this half is already answered. Shout if you see a failure mode I've missed in the offset scheme. Separately, I just pushed Rows now count from the first step actually recorded. Restart was the worse case under the old numbering: with |
The history had no off switch: it wrote whenever ib_state_wrt was set, and ib_force_stride could not express "never" because the validator requires it to be >= 1. At the rank counts this is meant for, a per-step write every step is a cost a run should opt into rather than inherit. It also means asking for the SILO point mesh silently enrolled a run in force-history I/O. Those are now independent: ib_force_wrt gates the history, ib_state_wrt keeps gating the point mesh, and neither implies the other. Default off follows the rest of MFC's output flags -- probe_wrt, prim_vars_wrt, fft_wrt and ib_state_wrt itself are all logicals that default to false -- so a sentinel stride value would have been the odd one out. The flag is on in the generated IBM test cases and in the three IBM examples that were relying on the old coupling, so the writer, the byte offsets and the row numbering stay under CI rather than only being exercised by hand. Verified both directions on CPU: with the flag set, all eleven tests carrying ib_forces.dat in their goldens pass; with it absent, a case that ran 49 steps wrote no force file at all. Note for review: three IBM examples set ib_state_wrt and so used to write a history. Two of them kept passing after the file stopped being written, even though their goldens still list D/ib_forces.dat -- only ibm_viscous_drag_over_cylinder failed, on a line count. An expected output file can therefore vanish without the suite noticing, which looks worth a separate issue.
|
Pushed the opt-out (
It also decouples two things that should not have been joined. Previously, asking for the SILO point mesh via The flag is on in the generated IBM test cases and in the three IBM examples that were relying on the old coupling, so the writer, the byte offsets and the row numbering stay under CI instead of only being exercised by hand. Verified both directions on CPU: with the flag set, all eleven tests carrying One thing worth splitting off. While checking that default-off really suppresses the file, I broke three IBM examples on purpose by removing the flag. Only So an expected output file can disappear entirely and the suite can still report success. That is independent of this PR and I have not filed it yet; happy to open an issue if it is not already known. |
|
Correction to my previous comment: the test-harness concern I raised at the end of it is wrong, and I should not have reported it without testing it directly. I went to file it as an issue and checked the mechanism first. What I originally saw was two of those examples passing in a run where their case directories still held output from an earlier The commit message on |
s_open_ib_force_history passed mpi_info_int to MPI_FILE_DELETE and
MPI_FILE_OPEN, but that handle is only created when parallel_io is on:
s_initialize_global_parameters_module returns before MPI_INFO_CREATE
otherwise. The IBM test cases set parallel_io = T only for
post_process and run the solver with it off, so the handle reaching
these calls was never initialised.
Every IBM case on an MPI build therefore aborted at the first recorded
step:
Fatal error in internal_Info_dup: Invalid argument
MPI_Info_dup(info=0x0, newinfo=...) failed
which is 12 jobs across Frontier, Ubuntu/Intel and macOS -- anywhere
MPI is on. It did not reproduce without MPI, which is why building
the serial configuration alone missed it.
MPI_INFO_NULL is valid whatever parallel_io is set to, and the hint
mpi_info_int carries only disables ROMIO write data sieving, which
this writer does not depend on: it computes its own offsets and writes
each record with MPI_FILE_WRITE_AT.
Verified against the build configuration CI used for the failing jobs,
cpu-503a859d02: the two cases that aborted there now pass, and a pass
requires the file to exist, since compare() rejects a candidate missing
any entry its golden lists.
# Conflicts: # src/simulation/m_data_output.fpp
Lines of Code
|
Records each immersed body's force, torque and kinematics as the run proceeds, rather than only at save intervals, so a body's history can be read without post-processing whole restart dumps.
D/ib_forces.dat, one shared fixed-width text file:Column names and the record geometry go in the sibling
D/ib_forces.hdr; a header line inside the file would shift every byte offset the writer computes.Why it is laid out this way
The obvious shape - a file per body, appended each step - does not survive contact with a real run. Each append costs an
inquire, anopenand aclose, per body, per rank, per step:A Lustre MDS sustains order 1e4-1e5 operations per second, so at a thousand ranks this is seconds per step of pure metadata, and it degrades the filesystem for every other job on the machine. Buffering does not rescue it: with moving bodies the buffer has to be flushed before ownership can change hands, which is every step, so it never accumulates more than one row per body.
Instead every record goes straight to a computed offset in one file that is opened once for the whole run:
with
row = t_step/ib_force_stride. That is one file open for the run rather thanranks * bodiesper step, and it drops the buffer, the sentinel bookkeeping that tracked which rows had been written, the O(n^2) scan over the buffer per distinct body, and the flush-before-handoff entirely - a record lands at its own offset, so a rank giving up a body has nothing left to reorder.This works on a text file because Fortran's
ESdescriptors are fixed width. Checked before relying on it:'(I10,19(1X,ES17.9E3))'emits exactly 352 characters for ordinary values, for negatives with three-digit exponents, and for NaN and Inf, which right-justify in their fields rather than widening them.IB_REC_FMT,IB_REC_BODYandIB_REC_LENsit together with a note that they have to be edited together, because widening the format alone would shear the file.The one hazard is that unwritten regions of a sparsely written file read back as NUL bytes, which would corrupt the text. It does not arise because ownership is a half-open interval test, so every body is owned by exactly one rank at every step and no slot is skipped - verified below rather than assumed.
What this gives you
numpy.loadtxt('D/ib_forces.dat')with no decode step; one body's history isd[d[:,0] == id].ib_force_stridesubsamples long runs.Robustness
The offsets assume
IB_REC_FMTemits exactlyIB_REC_BODYcharacters, which is checked at open time rather than left as a comment - a format one character wider would otherwise shear every record after the first with no other symptom:Confirmed by deliberately widening
I10toI11: the run aborts on that check instead of writing a sheared file.MPI_MODE_CREATEdoes not truncate, so the file is deleted before it is opened, as theib_statewriter already does. Without that, a shorter run following a longer one in the same directory keeps the earlier tail past its last record.Testing
11 IB goldens regenerated and passing (
5A22B45F E085CC5A D6794F4C 4BED9896 C8AD6271 49893269 135F548B B317404C F200F862 DA8FCD2D E5B66084), CPU, gfortran.Record width measured across ordinary, negative-with-three-digit-exponent, NaN and Inf values: 352 characters in every case.
File structure: 200 records of 353 bytes, every byte printable or newline, every record newline-terminated, no NUL holes.
The same case at 1 rank and at 2 ranks produces byte-identical files (70,600 bytes each).
numpy.loadtxtreturns(200, 20); body 1's 50 rows have strictly increasing time.Serial build (
--no-mpi, direct-access branch) produces a file of the same size and structure as the MPI build.Not covered, and worth weighing:
ranks * bodiesper step - rather than measurement..hdrfile is a new convention here. The alternative, a header line, would shift every offset after it.Fyaround 1e-18 against a column scaled at 1e-3) compare on absolute error first against the 1e-12 default tolerance, so they do not carry the cross-compiler fragility that bit the prescribed-kinematics goldens. Checked rather than assumed.Note on the earlier revision
This replaces the per-body writer this PR originally carried. The review comments on that code about the concurrent-writer race, the
nint/sentinel scheme and the O(n^2) flush are resolved by the code no longer existing; theib_force_stridevalidator fix from that round is kept.pack.pygainsib_forcesalongsideprobeandintegralas a multi-column time series, which is what it is.