Skip to content

Append to probe and CoM files only when continuing a run - #1867

Open
sbryngelson wants to merge 2 commits into
masterfrom
fix/probe-files-append-across-runs
Open

Append to probe and CoM files only when continuing a run#1867
sbryngelson wants to merge 2 commits into
masterfrom
fix/probe-files-append-across-runs

Conversation

@sbryngelson

Copy link
Copy Markdown
Member

Written with Claude Code.

The bug

s_open_probe_files appends whenever D/probe*_prim.dat already exists:

if (file_exist) then
    open (i + 30, FILE=trim(file_path), form='formatted', STATUS='old', POSITION='append')

Correct when a run is being continued. Wrong when one is being started over — which is what happens every time a case is re-run in place after a parameter change. The second run's rows land on top of the first's, nothing in the file marks the join, and the time column simply resets partway down. A reader sees one monotonic series and is silently wrong.

The two runs need not even share a grid: a case whose resolution changed between runs produces a file whose first half was recorded at different probe locations.

s_open_com_files has the same pattern.

The fix

Append only when continuing: t_step_start > 0, or n_start > 0 under cfl_dt — the same test m_start_up already uses to decide whether a run is fresh. A fresh start replaces the file, which is what every other output MFC writes already does.

How the validation is obtained

examples/2D_probe_rerun/ — a 64×64 case with two probes, 20 steps. Run it twice in the same directory:

./mfc.sh run examples/2D_probe_rerun/case.py -n 1
./mfc.sh run examples/2D_probe_rerun/case.py -n 1
wc -l D/probe1_prim.dat
rows in D/probe1_prim.dat
before 40 — two runs of 20, spliced
after 20

The time column read straight through, before:

0.028977
0.030682
0.032386      <- end of run 1
0.000000      <- run 2 starts, time goes backwards
0.001705
0.003409

Measured with the same binary either side of the change; both runs complete, no failures. The README carries the table and the reasoning.

Why it is worth fixing rather than documenting

This produced three separate wrong numbers in one project before it was noticed. The worst: a jet whose probe files held a t = 15 run of 4,962 rows followed by a t = 40 run of 13,233 — on different grids. Read together they manufactured a velocity drop of 0.99 U_j in a single sample, which was time running backwards at the seam, and which was diagnosed as a physical instability first.

A related symptom is louder: if the probe output format changes between runs, the column count changes partway down and numpy.loadtxt refuses the file outright ("the number of columns changed from 11 to 18"). That one announces itself. The time reset does not.

Scope

Only affects re-running a case in a directory that already holds probe or CoM output. Restarts are unchanged — they still append, which is the behaviour they need. No golden file moves; the test suite runs each case in a fresh directory.

s_open_probe_files appends whenever D/probe*_prim.dat already exists. That is
right when a run is being continued and wrong when one is being started over,
which is what happens every time a case is re-run in place after a parameter
change. The second run's rows land on top of the first's, nothing in the file
marks the join, and the time column resets partway down. A reader sees one
monotonic series and is silently wrong. The two runs need not even share a
grid.

Append only when t_step_start > 0, or n_start > 0 under cfl_dt. A fresh start
replaces the file, which is what every other output MFC writes already does.
s_open_com_files had the same pattern and gets the same treatment.

examples/2D_probe_rerun runs 20 steps twice in the same directory. Before, the
probe file holds 40 rows and its time column runs 0.032386 then 0.000000; after,
20 rows.

This produced three separate wrong numbers in one project before it was caught.
The worst was a jet whose probe files held a t = 15 run of 4,962 rows followed
by a t = 40 run of 13,233, on different grids; read together they manufactured a
velocity drop of 0.99 U_j in a single sample, which was time running backwards
at the seam and was diagnosed as a physical instability first.
Copilot AI lite review requested due to automatic review settings September 12, 2026 18:34

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

Fixes probe and center-of-mass (CoM) output files being unintentionally appended when re-running a case “from scratch” in the same directory, which can silently splice multiple runs into one misleading time series.

Changes:

  • Determine whether a run is a “fresh start” vs “continuation” and only append for continuations.
  • Replace (truncate) existing probe/CoM files on fresh starts to avoid splicing data across separate runs.
  • Add a reproducible example (examples/2D_probe_rerun/) documenting the bug and validation.

Reviewed changes

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

File Description
src/simulation/m_data_output.fpp Add fresh_start gating so probe/CoM files are replaced on fresh runs and appended only on continuations.
examples/2D_probe_rerun/case.py New example case to reproduce the probe append/splice issue by running twice in-place.
examples/2D_probe_rerun/README.md Documentation demonstrating the incorrect behavior before and correct behavior after the fix.

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

integer :: i !< Generic loop iterator
logical :: fresh_start

fresh_start = (t_step_start == 0) .or. (cfl_dt .and. n_start == 0)
Comment on lines +149 to +151
logical :: fresh_start

fresh_start = (t_step_start == 0) .or. (cfl_dt .and. n_start == 0)

MFC appends to D/probe*_prim.dat when the file already exists. That is right when a run is being continued
and wrong when one is being started over: the second run's rows land on top of the first's, nothing in the
file marks the join, and the time column simply resets partway down. A reader sees one monotonic series.
@github-actions

Copy link
Copy Markdown

Lines of Code

File Lines Diff
src/simulation/m_data_output.fpp 1375 +8
Directory Lines Diff
simulation 28094 +8
total 46348 +8

@github-actions

Copy link
Copy Markdown

Claude Code Review

Head SHA: de78f37

Files changed:

  • 3
  • examples/2D_probe_rerun/README.md
  • examples/2D_probe_rerun/case.py
  • src/simulation/m_data_output.fpp

Findings:

  • src/simulation/m_data_output.fpp: in both s_open_com_files and s_open_probe_files, fresh_start = (t_step_start == 0) .or. (cfl_dt .and. n_start == 0) OR's the two restart checks instead of selecting between them by mode. When cfl_dt is enabled, restarts are indicated by n_start > 0 while t_step_start stays at its default 0; the t_step_start == 0 term is true regardless of n_start, so fresh_start always evaluates .true. for any cfl_dt run. That routes every cfl_dt restart into the new STATUS='replace' branch, silently truncating the existing probe/CoM data files that the restart should have appended to — the opposite of this patch's intent. The condition should pick the check based on cfl_dt (e.g. merge(n_start == 0, t_step_start == 0, cfl_dt)) rather than OR-ing them.

@codecov

codecov Bot commented Sep 13, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 28.57143% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 61.23%. Comparing base (dc0aec1) to head (4d6ac9b).
⚠️ Report is 4 commits behind head on master.

Files with missing lines Patch % Lines
src/simulation/m_data_output.fpp 28.57% 4 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1867      +/-   ##
==========================================
- Coverage   61.26%   61.23%   -0.03%     
==========================================
  Files          84       84              
  Lines       22330    22340      +10     
  Branches     3265     3267       +2     
==========================================
+ Hits        13680    13681       +1     
- Misses       6207     6215       +8     
- 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.

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