Keep every column of probe output in golden packs

[sbryngelson]

Fixes #1711.

The defect

toolchain/mfc/packer/pack.py treats every .dat file under D/ as a spatial field of the form <x> [<y> <z>] <value>, infers dimensionality from the first line, and keeps only the last column of each row:

ndims   = len(_extract_doubles(content.split("\n", 1)[0])) - 1
doubles = _extract_doubles(content)[ndims :: ndims + 1]

Probe output is not a spatial field. It is a multi-column time series whose columns are distinct physical quantities, and the set varies by configuration:

Configuration	Columns written
1D, general (m_data_output.fpp:1519)	nondim_time, rho, vel(1), pres
bubbles (line 1490)	nondim_time, rho, vel(1), pres, alf, R, Rdot, nR, nRdot
hypoelastic (line 1515)	nondim_time, rho, vel(1), vel(2), pres, tau_e(1..3)
3D (line 1524)	nondim_time, rho, vel(1..3), pres, gamma, pi_inf, qv, c, accel

Under the field interpretation only the final column survived. In 3D that means the golden validated the acceleration magnitude and discarded density, velocity, pressure, gamma, pi_inf, qv, and the sound speed.

lag_bubble files were already special-cased for exactly this reason. Probe files are the remaining case.

Change

Retain every column for probe files. No header line to skip — s_write_probe_files writes data rows only.

Effect on existing goldens

Golden	Before	After
tests/5CAA4E68 (1D exp_bubscreen)	50 values	450
tests/FBB296DA (1D bubblescreen)	50 values	450
tests/AE9A7D73 (1D poly_bubscreen)	1 value	9

Nine columns per row instead of one. The added values are what was being dropped: density, velocity, pressure, void fraction, and the bubble radius/velocity moments. That widening is the point of the change — it is coverage these cases should always have had.

The regenerated metadata records (dirty). That is inherent to regenerating tracked goldens: writing them dirties the tree before the metadata is stamped. Newly added goldens do not have this problem.

Verification

The three probe cases pass against their regenerated goldens. The change is gated on "probe" in short_filepath, so non-probe files take the original code path unchanged and no other golden should move. A full local suite run confirms it: 627 passed, 0 failed.

An earlier run of the same suite reported the three probe cases failing with "Variable count didn't match". That did not reproduce — the three pass in isolation, pass on consecutive repeat runs, and pass in the clean full-suite run above. It matches non-reproducible flakiness seen on an unrelated branch this session (two chemistry cases, likewise green individually), and appears to be local contention at high -j rather than anything in this change. Worth knowing it has been seen, in case CI shows it.

Why now

#1707 is a defect in the probe sound speed. It could not be given a regression test through the normal golden path, because c is never the last column in any configuration — a case could exercise the defective code, emit visibly wrong output, and still pass. This PR is a prerequisite for testing that fix, and stands on its own regardless.

Note that even with this change, observing c specifically requires a 3D probe case; the 1D writes do not emit it at all.

---

Handover notes

Branch and commits

repo    MFlowCode/MFC
branch  fix/packer-probe-columns    (base: master)
  0367641  Keep every column of probe output in golden packs
  c185dc8  Regenerate the three probe goldens with full column coverage

Split into two commits so the goldens are regenerated from a clean checkout of the packer change. Note the regenerated metadata still records (dirty) — that is unavoidable when regenerating tracked goldens, because writing them dirties the tree before the metadata is stamped. Newly added goldens do not have this problem. Do not chase it.

Environment

./mfc.sh build -j 16
./mfc.sh test  -j 12
./mfc.sh test --only 5CAA4E68 AE9A7D73 FBB296DA          # the three probe cases
./mfc.sh test --generate --only 5CAA4E68 AE9A7D73 FBB296DA

GNU 15.2.0, MPI, no GPU, macOS arm64.

The mechanism, precisely

toolchain/mfc/packer/pack.py, in compile():

ndims   = len(_extract_doubles(content.split("\n", 1)[0])) - 1
doubles = _extract_doubles(content)[ndims :: ndims + 1]

Every .dat under D/ is assumed to be <x> [<y> <z>] <value>, so only the last column of each row survives. lag_bubble files were already special-cased above this for the same reason; probe files were the remaining case. The fix adds an elif "probe" in short_filepath branch keeping all columns. s_write_probe_files writes data rows only — there is no header to skip.

Why this was needed

#1707 is a defect in the probe sound speed. It could not be given a regression test through the normal golden path, because c is never the last column in any configuration:

configuration	columns written	kept before this PR
1D general (m_data_output.fpp)	nondim_time, rho, vel(1), pres	pres
bubbles	nondim_time, rho, vel(1), pres, alf, R, Rdot, nR, nRdot	nRdot
hypoelastic	nondim_time, rho, vel(1), vel(2), pres, tau_e(1..3)	tau_e(3)
3D	nondim_time, rho, vel(1..3), pres, gamma, pi_inf, qv, c, accel	accel

A case could exercise the defective code, emit visibly wrong output, and still pass its golden. I confirmed this empirically: a purpose-built qv /= 0 probe case passed with the bug present.

Even after this PR, observing c requires a 3D probe case — the 1D writes do not emit it at all. Anyone writing the #1707 regression test needs 3D plus qv /= 0.

Golden impact

golden	before	after
tests/5CAA4E68 (1D exp_bubscreen)	50 values	450
tests/FBB296DA (1D bubblescreen)	50 values	450
tests/AE9A7D73 (1D poly_bubscreen)	1 value	9

Nine columns per row instead of one. The widening is the point.

Verification, and a flake to expect

three probe cases:  3 passed
full suite:         627 passed, 0 failed

An earlier full-suite run reported these same three cases failing with Variable count didn't match. It did not reproduce: they pass in isolation, pass on two consecutive repeat runs, and pass in a clean full-suite run. It matches non-reproducible flakiness seen on unrelated branches in the same session (two chemistry cases, likewise green individually) and appears to be local contention at high -j. If CI shows it, re-run before investigating.

Related work

• Golden packer keeps only the last column of probe output, hiding probe regressions #1711 — the issue this closes.
• bug: simulation probe sound speed omits qv from enthalpy, disagreeing with post-process #1707 — blocked on this; needs a 3D probe case with qv /= 0.
• Pass an eos_state to s_compute_speed_of_sound #1714 — eos_state refactor, which subsumes the bug: simulation probe sound speed omits qv from enthalpy, disagreeing with post-process #1707 code fix via the interface.

---

Purpose in the series

Test-infrastructure prerequisite, not a feature.

#1707 is a defect in the probe sound speed that could not be given a regression test — c is never the last column of probe output, and the packer kept only the last column. A case could exercise the defective code, emit visibly wrong output, and pass its golden. I confirmed that empirically before writing this.

So this exists to make a class of regression testable at all. The widened goldens are the point, not a side effect: three cases went from validating one column to validating nine.

Note #1714 fixes the #1707 code path via the interface, so this is no longer strictly blocking that fix — but it remains the only way a probe regression becomes visible to the suite.

---

Working conventions and hazards (shared across this series)

Collected from the work that produced #1705, #1709, #1712, #1713, #1714, #1716. Every one of these cost real time or produced a wrong result before being caught.

Testing

A regression test that cannot fail is worse than no test. Always verify the negative: revert the fix, rebuild, confirm the case fails, restore. Two ways this silently broke here:

• git stash push -- <file> has nothing to stash once the fix is committed, so the "reverted" run tests the fixed binary and reports a pass identical to a real one. Use git checkout master -- <file>, rebuild, test, then git checkout HEAD -- <file>.
• Picking the wrong case configuration. A probe case at model_eqns = 3 cannot detect a sound-speed defect, because the six-equation branch of s_compute_speed_of_sound touches neither H nor qv.

The golden packer discards data. toolchain/mfc/packer/pack.py treated every .dat under D/ as <x> [<y> <z>] <value> and kept only the last column of each row. Probe and integral output are multi-column time series, so most columns were never compared (#1711, fixed in #1712). Before asserting that a golden covers something, check it is actually in golden.txt.

Case labels are load-bearing. The golden UUID is crc32(sha1(str(trace))) — the label chain determines the directory name. Renaming a label renames the golden. Avoid ! in labels (history expansion in interactive bash).

Local suite runs are flaky at high -j. Non-reproducible failures appeared on several unrelated branches at -j 12–16 (chemistry cases, probe cases) that passed individually and in clean reruns. Re-run before investigating.

Removing parameters or features

Deregistering a parameter breaks things that are not the source tree. Removing pref/rhoref from the registry broke the entire suite because BASE_CFG in toolchain/mfc/test/case.py set them for every case. Also check fp_stability.py, params_tests/mutation_tests.py, and lint fixtures that use real parameter names as examples.

Grep the generated artifacts, not just the sources. A stale TYPED_DECLS entry naming a deleted type survived removal and did not break the build only because the parameter had also left every target's namelist vars, so it was never emitted. Check generated_decls.fpp, generated_constants.fpp, SIM_GPU_DECL_VARS, and the MPI broadcast generators.

Dead-local tell: after removing a block, a local with exactly one remaining occurrence in its file is almost certainly its own declaration. Two occurrences often means declaration plus a private() entry.

Fortran is case-insensitive. A local pRef shadowed the module global pref in the hardcoded-IC files; the read site was spelled pref and looked like a reference to the global. It is not. Confirm scope before concluding a global is live.

GPU

A CPU test run cannot catch a missing private(). It is a silent device race. Audit by hand or by script when adding per-cell state.

Do not match GPU_PARALLEL_LOOP naively — END_GPU_PARALLEL_LOOP contains the same substring and will register as a loop start, producing false positives. Exclude it explicitly.

Derived-type components cannot have runtime extents. dimension(num_fluids) in a type fails to compile outside case-optimized builds, where num_fluids is a parameter.

Benchmarking

Run ./mfc.sh bench in the foreground on an idle machine. Running it in the background while a pre-commit precheck ran at -j 12 produced a bogus +50% regression on a case that executes none of the changed code. Baseline noise here is ±3.5%; run the baseline twice before trusting any delta, and sanity-check that the regressing cases actually execute the modified code.

GitHub mechanics

• --force-with-lease needs an explicit SHA (--force-with-lease=<branch>:<sha>) when the ref has not been fetched in the current clone; the bare form fails with "stale info".
• gh run view --log-failed can miss the real output entirely. On the Frontier jobs the failing step carried only a non-zero exit while the actual test output lived in a separate Print Logs step that succeeded. Fetch the full log.
• gh run rerun <id> --failed refuses while the workflow is still running; retry later.
• Frontier jobs fail spuriously with some regularity — uniform ~15-minute deaths across heterogeneous jobs, and shared-Lustre install races (file INSTALL cannot set modification time ... No such file or directory, exit 143). Check for a real error before assuming a code fault.

[Copilot]

Pull request overview

This PR updates the golden packer so probe .dat outputs under D/ are treated as multi-column time series (retaining all columns) instead of being misinterpreted as spatial fields (which previously retained only the last column). This improves regression coverage for probe diagnostics that are not in the final column.

Changes:

• Special-case probe outputs in toolchain/mfc/packer/pack.py to retain all numeric columns.
• Regenerate affected probe-based golden packs to include the full probe column set.
• Update golden metadata files produced during regeneration.

Reviewed changes

Copilot reviewed 6 out of 7 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
toolchain/mfc/packer/pack.py	Adds probe-specific parsing path to retain all columns for probe outputs.
tests/FBB296DA/golden.txt	Updated golden pack content reflecting full probe columns.
tests/FBB296DA/golden-metadata.txt	Updated provenance metadata for regenerated golden.
tests/AE9A7D73/golden.txt	Updated golden pack content reflecting full probe columns.
tests/AE9A7D73/golden-metadata.txt	Updated provenance metadata for regenerated golden.
tests/5CAA4E68/golden-metadata.txt	Updated provenance metadata for regenerated golden.

---

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

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 61.24%. Comparing base (8dfe8c7) to head (47f63c7).
⚠️ Report is 3 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1712      +/-   ##
==========================================
+ Coverage   60.77%   61.24%   +0.46%     
==========================================
  Files          83       83              
  Lines       20872    20700     -172     
  Branches     3101     3072      -29     
==========================================
- Hits        12685    12677       -8     
+ Misses       6121     5969     -152     
+ Partials     2066     2054      -12     

☔ 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]

Ordering note: #1716 removes integral output entirely (unused since it was added; confirmed with its author).

Once both land, the or "integral" in short_filepath clause added here becomes dead and should be dropped, along with the mention of int_pres/max_pres in the surrounding comment. Harmless if left — the match simply never fires — but it will read as referring to a feature that no longer exists.

The two PRs do not conflict and can merge in either order. If #1716 goes first I will drop the clause from this branch before merge; if this goes first I will drop it in a follow-up.