Remove integral output

[sbryngelson]

Removes integral output — integral_wrt, num_integrals, and the 30 integral(i)%{x,y,z}{min,max} parameters.

Why

It writes region-integrated pressure to D/integral<i>_prim.dat for Euler-Euler bubble runs. Nothing exercises it: no example, test, or benchmark sets integral_wrt, and it has had no coverage since it was added roughly nine years ago. Confirmed with the maintainer (its author) that it has no users.

Unlike the four-equation removal, this touches no hot path — the output was cold-guarded behind if (integral_wrt). The value here is removed surface area, not simplified code. 130 lines of 182 deleted are the output block itself.

What was removed

Fortran

• the 118-line output block in s_write_probe_files and the file-open block in s_open_probe_files (simulation/m_data_output.fpp)
• type integral_parameters (common/m_derived_types.fpp)
• the fypp MPI broadcast loop over integral(j)%{xmin..zmax} (simulation/m_mpi_proxy.fpp)
• the default assignments (simulation/m_global_parameters.fpp)

Toolchain

• registry entries, constraints and descriptions for integral_wrt, num_integrals, and the 30 integral(i)%* parameters
• check_probe_integral_output becomes check_probe_output, keeping only the probe fd_order rule; lint_docs.py's skip-set entry renamed to match
• case.md entries

Locals that die with it: int_pres and max_pres (used only in the removed block), plus rad, thickness and trigger, whose declarations were annotated "For integral quantities".

Verification

build (GNU 15.2, MPI):   clean
full suite:              627 passed, 0 failed
goldens regenerated:     none
toolchain unit tests:    170 passed
precheck (7/7):          pass

No golden moved, which is expected — no test enabled the feature. That is also the reason it could be removed with this little risk.

Ordering note for #1712

#1712 extends the golden packer to keep every column of probe and integral output. Once both PRs land, the or "integral" in short_filepath clause in toolchain/mfc/packer/pack.py becomes dead and should be dropped. It is harmless if left — the match simply never fires — but it will read as referring to a feature that no longer exists.

Merge order does not otherwise matter; the two do not conflict.

---

Handover notes

Branch

repo    MFlowCode/MFC
branch  remove/integral-output   (base: master, at b40e08f)
  ca7840d  Remove integral output

Environment

./mfc.sh build -j 16
./mfc.sh test  -j 12
cd toolchain && python -m pytest mfc/params_tests -q

GNU 15.2.0, MPI on, no GPU, macOS arm64. Not built for GPU or with case optimization — worth confirming in CI, though the removal touches no GPU-declared state beyond deleting the integral broadcast.

How the removal was scoped

Start from grep -rn "integral_wrt\|num_integrals\|integral(" src/ toolchain/ docs/. Two false positives to keep:

• m_thinc.fpp's f_mthinc_volume_integral — unrelated volume integral in the THINC reconstruction.
• any integral hit inside params_tests/ fixtures (there were none, but check before deleting).

The output block was located by matching if (integral_wrt .and. bubbles_euler) then and walking to its balanced end if — 118 lines, ending at the subroutine's last statement. If you redo this, count if (...) then / end if at matching depth rather than relying on indentation.

After removing the block, check for locals that were only used inside it. int_pres and max_pres dropped to zero occurrences; rad, thickness and trigger were left with exactly one (their declarations), which is the signature of an orphan.

What was deliberately not done

No deprecation period. The feature is undocumented beyond two table rows, has no test coverage, and its author confirms no users. A case setting integral_wrt will now fail validation with the standard unknown-parameter error, which names the offending key.

---

Purpose

Surface-area reduction, same argument as #1713 but weaker and worth stating honestly.

model_eqns = 4 was removed because it was an unverifiable physics model that forced branches through the pressure inversion, energy assembly, primitive recovery, HLLC, IBM, acoustic source and both init paths — removing it deleted 500 lines and simplified hot code.

Integral output is not that. It is a cold-guarded diagnostic writer costing ~20 references across two files, with no hot-path branching. I argued against removing it on those grounds: it is a documented, registered, opt-in feature, and removal breaks any external user silently.

What changed the decision is a fact only the maintainer could supply — he wrote it about nine years ago and confirms it has no users. With that settled the case is clear, but the precedent should not be read as "untested implies removable". The reasoning was: untested and no hot-path cost and authorial confirmation of zero users.

---

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

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.

Removes the unused “integral output” feature and its associated parameters, validation, docs, and MPI broadcast, reducing surface area without affecting hot paths.

Changes:

• Deleted integral_wrt, num_integrals, and per-integral region parameters from the toolchain parameter registry/constraints/descriptions.
• Removed simulation-side integral output file opening/writing logic and MPI broadcast for integral region bounds.
• Updated validation and documentation/lint tooling to reflect probe-only output checks.

Reviewed changes

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

Show a summary per file

File	Description
toolchain/mfc/params/descriptions.py	Removes integral-related parameter descriptions from the docs text source.
toolchain/mfc/params/definitions.py	Removes integral parameters from the registry, constraints, dependencies, and derived-type registry.
toolchain/mfc/lint_docs.py	Renames the skipped validator check to match the new probe-only check name.
toolchain/mfc/case_validator.py	Renames and simplifies the probe/integral output validation to probe-only.
src/simulation/m_mpi_proxy.fpp	Removes MPI broadcast of integral region bounds.
src/simulation/m_global_parameters.fpp	Removes default assignments/initialization for integral output parameters/regions.
src/simulation/m_data_output.fpp	Removes integral output file opening and writing block.
src/common/m_derived_types.fpp	Deletes the integral_parameters derived type.
docs/documentation/case.md	Removes integral output parameters from the case parameter table.

---

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

[github-actions]

Claude Code Review

Head SHA: 7e30e9a

Files changed:

• 9
• docs/documentation/case.md
• src/common/m_derived_types.fpp
• src/simulation/m_data_output.fpp
• src/simulation/m_global_parameters.fpp
• src/simulation/m_mpi_proxy.fpp
• toolchain/mfc/case_validator.py
• toolchain/mfc/lint_docs.py
• toolchain/mfc/params/definitions.py
• toolchain/mfc/params/descriptions.py

Findings:

• src/simulation/m_global_parameters.fpp: removing the integral(i)%... default-initialization body left a dead, empty loop behind (do i = 1, num_probes_max / end do with nothing in between, right after the probe(i) defaults loop). It should be deleted along with the rest of the integral cleanup rather than left as a no-op.
• src/simulation/m_data_output.fpp: the npts local variable declaration (integer :: npts !< Number of included integral points) is kept in s_write_probe_files, but every assignment and read of npts lived only inside the now-deleted integral_wrt block. npts is now declared but never used/set, and should be removed along with rad, thickness, and trigger, which were correctly deleted in the same hunk.
• toolchain/mfc/params/descriptions.py: the diff removes the integral_wrt/num_integrals description entries, but the per-region regex descriptions (integral\((\d+)\)%xmin, %xmax, %ymin, %ymax, %zmin, %zmax) further down the same file still describe the now-removed integral(i)%... parameters. Since definitions.py no longer registers integral(i)%..., these entries are stale and should be removed for a complete cleanup.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 61.45%. Comparing base (b40e08f) to head (7e30e9a).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1716      +/-   ##
==========================================
+ Coverage   61.24%   61.45%   +0.21%     
==========================================
  Files          83       83              
  Lines       20700    20614      -86     
  Branches     3072     3055      -17     
==========================================
- Hits        12677    12668       -9     
+ Misses       5969     5892      -77     
  Partials     2054     2054              

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