Skip to content

feat: add fp-stability command for Verrou-based FP instability testing#1403

Draft
sbryngelson wants to merge 4 commits intoMFlowCode:masterfrom
sbryngelson:fp-stability
Draft

feat: add fp-stability command for Verrou-based FP instability testing#1403
sbryngelson wants to merge 4 commits intoMFlowCode:masterfrom
sbryngelson:fp-stability

Conversation

@sbryngelson
Copy link
Copy Markdown
Member

@sbryngelson sbryngelson commented May 6, 2026

Summary

Adds a Verrou-based floating-point stability CI suite (closes #650).

  • ./mfc.sh fp-stability runs three 1-D shock cases under randomized IEEE-754 rounding and reports the max L∞ deviation from a nearest-rounding reference
  • sod_standard (standard Sod, threshold 1e-13) — well-conditioned canary that should always pass
  • sod_strong (p_L/p_R=100,000, 50 cells, 10 steps) — probes HLLC xi-factor cancellation (s_L - vel_L)/(s_L - s_S) near sonic contact
  • water_stiffened (stiffened EOS pi_inf=4046, 50 cells, 10 steps) — probes pressure-recovery cancellation p = (E - pi_inf)/gamma (loses ~4 decimal digits when pi_inf/p_right ~ 40,000)
  • verrou_dd_sym on failure: when a case exceeds its threshold, delta-debug bisection automatically runs to identify the minimal set of responsible function symbols; logs saved to fp-stability-logs/ and uploaded as a CI artifact
  • GitHub Actions workflow (.github/workflows/fp-stability.yml): builds Verrou once (~20 min), caches it, builds MFC in serial debug mode, runs the suite with N=5 samples

Test plan

  • CI passes on push (check Actions tab on this branch)
  • sod_standard reports PASS at threshold 1e-13
  • sod_strong and water_stiffened report their instability magnitudes
  • On failure, fp-stability-logs/ artifact contains dd_sym.log and rddmin_summary
  • ./mfc.sh fp-stability --help shows available options

Adds ./mfc.sh fp-stability — a persistent floating-point stability test
suite using Verrou's random IEEE-754 rounding mode.

For each registered test case the runner:
  1. Generates initial conditions via pre_process
  2. Runs simulation once with --rounding-mode=nearest (reference)
  3. Runs simulation N times with --rounding-mode=random
  4. Reports max L-inf deviation vs threshold (PASS/FAIL)

Two cases probe known ill-conditioning in MFC:
  - sod_strong: 1-D Sod p_L/p_R=100,000 — HLLC xi-factor cancellation
    (s_L - vel_L)/(s_L - s_S) near sonic contact
  - water_stiffened: 1-D water shock pi_inf=4046 — pressure recovery
    p=(E-pi_inf)/gamma loses ~4 decimal digits on low-pressure side

Requires a Verrou-enabled Valgrind at $VERROU_HOME/bin/valgrind
(default: $HOME/.local/verrou). Silently skips if not found.
Binaries are auto-discovered from build/install/ or passed explicitly.
@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 6, 2026

Claude Code Review

Head SHA: 5c8eaab

Files changed:

  • 5
  • .github/workflows/fp-stability.yml
  • .gitignore
  • toolchain/main.py
  • toolchain/mfc/cli/commands.py
  • toolchain/mfc/fp_stability.py

Findings

1. cons.unindent() skipped on exception — console permanently indented after any case error

File: toolchain/mfc/fp_stability.py

In _run_case, cons.indent() is called before the try block, but cons.unindent() is placed after the try/finally block — outside of any exception-protection:

cons.indent()
...
work_dir = tempfile.mkdtemp(...)
try:
    _run_preprocess(...)            # can raise MFCException
    _run_simulation_verrou(...)     # can raise MFCException
    ...
finally:
    shutil.rmtree(work_dir, ignore_errors=True)

cons.unindent()   # ← never reached when MFCException propagates out
cons.print()
return passed

When _run_preprocess or _run_simulation_verrou raises MFCException, the exception bypasses cons.unindent(). The caller (fp_stability()) catches the exception and continues to the next case, but every subsequent cons.print() call will be at the wrong indentation level for all remaining cases.

cons.unindent() and cons.print() should be moved inside the finally block alongside shutil.rmtree.


2. _exclude_args is defined but never called — dead code

File: toolchain/mfc/fp_stability.py

def _exclude_args(exclude_file: str) -> list:
    """Return --exclude flag if a verrou exclusion file is provided."""
    if exclude_file and os.path.isfile(exclude_file):
        return [f"--exclude={exclude_file}"]
    return []

This function is never called anywhere in the module. All invocations of _run_simulation_verrou hard-code [] for exclude_args, and there is no corresponding CLI argument for an exclusion file in FP_STABILITY_COMMAND. The function should either be wired up (add a --exclude CLI argument and thread it through _run_case) or removed.

@sbryngelson
Copy link
Copy Markdown
Member Author

Automated Code Review

Summary: 1 critical issue, 3 important issues.

Critical Issue

Console indent level leaks permanently on exception (confidence: 95%)

File: toolchain/mfc/fp_stability.py

cons.unindent() is called after the try/finally block, not inside it. When _run_simulation_verrou or _run_preprocess raises MFCException, the finally correctly cleans up work_dir, but the exception propagates past cons.unindent(). Subsequent case names are printed with one extra level of indentation for the rest of the run. This appears to already be noted in the PR description.

Fix: Move cons.unindent() and cons.print() into a finally clause:

try:
    ...
finally:
    shutil.rmtree(work_dir, ignore_errors=True)
    cons.unindent()
    cons.print()

Important Issues

1. _exclude_args() is defined but never called — delta-debug workflow not wired up (confidence: 100%)

File: toolchain/mfc/fp_stability.py

_exclude_args(exclude_file: str) is defined but referenced nowhere. _run_simulation_verrou is always called with a hardcoded empty list [] for exclude_args. There is no --exclude-file CLI argument in FP_STABILITY_COMMAND. The verrou_dd_sym invocation advertised in the PR title/description ("delta-debug bisection automatically runs to isolate responsible function symbols") is entirely absent from the implementation.

2. Workflow triggers on every push to every branch (confidence: 90%)

File: .github/workflows/fp-stability.yml

on:
  push:
  workflow_dispatch:

No branch filter (branches: [master]) and no path filter. Every push to every branch triggers the 20-minute Verrou build + test suite. Additionally, the schedule: trigger mentioned in the PR description ("runs weekly/on-push") is missing entirely.

Fix: Add appropriate filters:

on:
  push:
    branches: [master]
  schedule:
    - cron: '0 3 * * 1'   # weekly on Monday
  workflow_dispatch:

3. Build uses --debug but not --single (confidence: 80%)

File: .github/workflows/fp-stability.yml

The workflow builds with --no-mpi --debug but double precision. For the stiffened EOS pressure-recovery case (p = (E - pi_inf)/gamma) where pi_inf/p_right ~ 40,000, double-precision arithmetic leaves ~12 correct digits after ~4 are lost, so Verrou's random-rounding perturbations may be too small relative to threshold 1e-10 to reliably flag instability (false negatives). Single-precision (--single) makes the ill-conditioning far more apparent (4 digits of loss out of 7 total). Consider using --single for FP stability builds.

Suggestions

  • ARG("samples") or 5 — the or 5 fallback is unreachable; samples has default=5 in FP_STABILITY_COMMAND so ARG() always returns a value
  • _find_binary uses os.getcwd() instead of MFC_ROOT_DIR — will fail if ./mfc.sh fp-stability is run from a subdirectory. Use MFC_ROOT_DIR (already imported from .common)

Strengths

  • All subprocess.run calls use list arguments (never shell=True with interpolated user input) — no injection surface
  • Temp directories cleaned up in finally blocks
  • Cache key verrou-{hash}-valgrind-{version} correctly busts on version changes
  • .gitignore exception uses recursive glob correctly

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.

Valgrind verrou to check for sketchy float operations

1 participant