Refactor/adforce eval - #2
Merged
Merged
Conversation
- adforce/eval/config/: eval_config.yaml (shared root: validate/nulltest/ sensitivity/detide_sensitivity), scoring/default.yaml (mirrors constants, drift-tested), annual_max_config.yaml; hydra output_subdir null + run.dir . - five argparse mains -> @hydra.main; _cli.reject_legacy_flags prints the exact override translation for stale --flag invocations - constants: import-time makedirs loop -> lazy ensure_dirs() (pytest --doctest-modules and future HPC imports stay side-effect-free); called by mains and low-level cache writers (coops, ts-cache, annual_max) - tests: test_scoring_config_matches_constants, test_cache_tag_matches_ legacy_literal (pins the v1 ts-cache tag byte-for-byte) - docs/REPRODUCE: hydra override syntax Gate: 47 passed 1 skipped; doctest collection clean; wrap tests 19 passed; full sweep via hydra -> val_summary.csv byte-identical (md5 f689004e); Katrina parquet deterministic (63650a83). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- cells.expand_matrix(): axes cartesian product - exclude + include/cells,
name_keys aliases (res-low_tide-off style), duplicate/baseline validation
- config/matrix/{res_x_tide,archive_default}.yaml, config/source/
{hf_archive,run_dirs}.yaml, config/pairs_config.yaml
- pairs.py: resolution_bias_table (port of rerun/adcirc/score_resolution.py),
tide_surge_interaction (canonical home for the forcing-triple analysis
whose producer script was never committed; series + HF-archive-val_summary
fallback with storm_src provenance), compare_cells (general pairwise),
archive_peak_table; hydra main (action=resolution_bias|interaction)
- SUPERSEDED headers on rerun/adcirc/{extract_gauge_series,score_resolution}.py
Acceptance (pinned as skipif-gated tests): resolution_bias.csv reproduced
exactly (shape+columns+allclose); tide_surge_interaction.csv low rows
683/683 exact incl. provenance labels; mid rows verified on the 182
locally-supportable rows (all 78 committed-archive rows allclose) -- the
remaining mid rows need remote extracts whose local copies are truncated
(mid_storm_runs_gs_*.parquet fail parquet magic-byte check).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- training/driver.py: extract drive_storm(storm, storm_ds, run_directory, cfg, resolution, mode, spinup_days, ...) + is_run_successful() from drive_all_adcirc (whose CLI behaviour is unchanged); the seam is the ONLY sanctioned path for tide-on historical runs (per-storm fort.15 windows vs wrap.stage_input_files' Katrina-pinned static decks) - eval/launch.py + config/launch_config.yaml: matrix -> per-(cell,storm) plan table (run/skip/blocked + reason); dry_run=true default; guards: swan cells blocked (driver has no SWAN staging), missing resolution decks blocked (fort.14.high is ARCHER2-only), FOREIGN dirs blocked unless overwrite; controls=true auto-adds tide-only cells for interaction triples; sequential subprocess execution with extract-after-run + strip_after_extract (hist_sweep disk lesson); eval_manifest.json with full-resolved-config sha256 (provenance-drift detection); slurm backend deliberately deferred with loud documentation - eval/harvest.py + config/harvest_config.yaml: rsync of the minimal artifact set (config.yaml, gauge_ts.parquet, fort.61.nc, maxele.63.nc, slurm.out, manifest) remote -> laptop - tcpips/constants.py: import-time makedirs -> _ensure_dir tolerant of the dangling data/era5 symlink (unmounted volume broke `import tcpips`, and transitively the training driver, on any machine without the drive) Gate: 52 passed 1 skipped (driver-seam test skips where adcircpy/stormevents absent -- same reason pytest.ini ignores training modules); dry-run smoke: 2x2 matrix x 2 storms -> 8-row plan, correct run dirs, nothing created. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR migrates the historical tide-gauge validation code from the legacy comp/ namespace into adforce.eval, switches the CLI entry points to Hydra configuration, and adds a broader “run/harvest/compare” evaluation framework for comparing ADCIRC configurations.
Changes:
- Moved/renamed validation + annual-maximum pipeline references from
comp.*toadforce.eval.*across analysis code, docs, and tests. - Added Hydra config trees and Hydra-based entry points for validation, null tests, sensitivity, and annual maxima.
- Introduced new eval orchestration utilities (run status, run-dir sources, extract/harvest, matrix launch planning, and model-vs-model comparisons).
Reviewed changes
Copilot reviewed 44 out of 44 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| worst/gauge_fit.py | Update annual-max import to new adforce.eval location. |
| worst/gauge_fit_ns.py | Update annual-max import to new adforce.eval location. |
| tests/test_gauge_evt.py | Repoint EVT pipeline tests from comp to adforce.eval. |
| tests/test_eval.py | Repoint validation tests to adforce.eval and add Hydra/config drift + orchestration tests. |
| tcpips/constants.py | Make directory creation best-effort to avoid import failure with dangling symlink. |
| setup.py | Package adforce.eval, add [eval] extra, and ship Hydra YAML via package_data. |
| rerun/adcirc/score_resolution.py | Mark legacy script as superseded by adforce.eval.pairs. |
| rerun/adcirc/extract_gauge_series.py | Mark legacy script as superseded by adforce.eval.extract. |
| REPRODUCE.md | Update reproduce instructions from comp.* to adforce.eval.* + Hydra overrides. |
| README.md | Rename module documentation from comp to adforce.eval and update extras install docs. |
| docs/index.rst | Swap docs toctree entry from comp to eval. |
| docs/eval.rst | Add Sphinx API page for the new adforce.eval package. |
| docs/conf.py | Update docs commentary to reference adforce.eval instead of comp. |
| docs/comp.rst | Remove old Sphinx API page for legacy comp package. |
| adforce/training/driver.py | Extract per-storm drive_storm seam + shared success-marker check. |
| adforce/fort61.py | Add fort.61.nc reader returning tidy long-format DataFrame. |
| adforce/eval/validate.py | Convert validation entry point to Hydra; refactor storm sourcing via FieldSource. |
| adforce/eval/status.py | Add run-directory status classification with provenance checking. |
| adforce/eval/sources.py | Add HF-archive and run-dir simulation source abstractions. |
| adforce/eval/sensitivity.py | Convert sensitivity CLI to Hydra and update docs/flags mapping. |
| adforce/eval/README.md | Update module README for new namespace + document orchestration framework. |
| adforce/eval/pairs.py | Add model-vs-model comparisons (resolution bias, tide-surge interaction, generic comparisons). |
| adforce/eval/nulltest.py | Convert nulltest CLI to Hydra and update docs/flags mapping. |
| adforce/eval/launch.py | Add Hydra-driven matrix expansion, planning, manifesting, and sequential launch runner. |
| adforce/eval/harvest.py | Add rsync-based artifact harvester for remote sweeps. |
| adforce/eval/extract.py | Add remote reducer from fort.63.nc to per-gauge gauge_ts.parquet. |
| adforce/eval/detide_sensitivity.py | Convert detide sensitivity CLI to Hydra and update docs/flags mapping. |
| adforce/eval/coops.py | Ensure cache/output dirs are created lazily (not at import). |
| adforce/eval/constants.py | Adjust project-root detection for new package path + add ensure_dirs() for lazy setup. |
| adforce/eval/config/source/run_dirs.yaml | Add Hydra config for run-dir simulation source. |
| adforce/eval/config/source/hf_archive.yaml | Add Hydra config for HF-archive simulation source. |
| adforce/eval/config/scoring/default.yaml | Add Hydra scoring defaults mirrored to constants (anti-drift). |
| adforce/eval/config/pairs_config.yaml | Add Hydra config for pairs entry point. |
| adforce/eval/config/matrix/res_x_tide.yaml | Add matrix definition for resolution × tide comparison. |
| adforce/eval/config/matrix/archive_default.yaml | Add matrix definition for archive-only baseline. |
| adforce/eval/config/launch_config.yaml | Add Hydra config for orchestrated launch planning/execution. |
| adforce/eval/config/harvest_config.yaml | Add Hydra config for artifact harvesting. |
| adforce/eval/config/extract_config.yaml | Add Hydra config for remote extraction/reduction. |
| adforce/eval/config/eval_config.yaml | Add shared Hydra root config for analysis entry points. |
| adforce/eval/config/annual_max_config.yaml | Add Hydra root config for annual-max pipeline. |
| adforce/eval/cells.py | Add cell identity, matrix expansion grammar, and provenance matching. |
| adforce/eval/annual_max.py | Convert annual-max pipeline to Hydra and ensure lazy directory creation. |
| adforce/eval/_cli.py | Add helper to reject legacy argparse flags with explicit Hydra translations. |
| adforce/eval/init.py | Update package docstring and entry-point reference to adforce.eval.validate. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+42
to
+45
| #: Success line written to <run>/slurm.out (same marker check_training_runs | ||
| #: and adforce.eval.status key on). | ||
| SUCCESS_MARKER = "Job completed successfully.\n" | ||
|
|
Comment on lines
+39
to
+42
| try: | ||
| os.makedirs(path, exist_ok=True) | ||
| except OSError: | ||
| pass |
Comment on lines
+141
to
+147
| def config_hash(resolved_cfg) -> str: | ||
| """sha256 of the full resolved run config: the provenance fingerprint | ||
| stored in the manifest so model-vs-model can detect drift in | ||
| un-overridden defaults between launches (see plan gap G5).""" | ||
| return hashlib.sha256( | ||
| OmegaConf.to_yaml(resolved_cfg, resolve=False).encode() | ||
| ).hexdigest() |
Comment on lines
+65
to
+69
| ds = xr.open_dataset(download_storm(fname)) | ||
| x, y, DEM, WD = ds.x.values, ds.y.values, ds.DEM.values, ds.WD.values | ||
| ssh = WD + DEM[None, :] | ||
| t = pd.to_datetime(ds.time.values) | ||
| return x, y, WD, ssh, t |
- eval/detide.py: windowed CO-OPS fetches (polite on cache miss), align_pair
(pre-forcing datum alignment, offset reported not hidden), skew_surge_peak
promoted from detide_sensitivity (re-exported there; ragged-index guard)
- eval/tidecheck.py + config: tide-only runs vs CO-OPS predictions --
datum_offset_m, amp_ratio, sub-hourly cross-correlation lag, r, rmse
- eval/twl.py + config: tide-on scoring -- storm+tide TWL vs raw gauge WL
(aligned peaks, hydrograph skill) with SKEW SURGE as headline (model skew
vs its own tide-only run, obs skew vs the prediction, inner-joined)
- tests: synthetic recovery of amp/lag/offset, alignment, phase-insensitive
skew, cached-predictions parse, gappy-obs TWL end-to-end (57 passed)
First results (data/comp/out/tidecheck_{low,mid}.csv, twl_low.csv):
* low tides: amp_ratio median 1.45, lag -66 min, r 0.82 (1019 pairs)
* mid tides: 1.41, -42 min, r 0.855 (969 pairs) -> the ~40% coastal tide
over-amplification is RESOLUTION-INDEPENDENT (forcing/friction, not mesh);
TX semi-enclosed bays are structurally broken at both (amp ~5.8x, r~0.1)
* low TWL: aligned peak bias -0.02 m but skew sim 0.07 vs obs 0.36 m --
tide error compensates surge deficit; skew exposes it
=> gate verdict: fix EC95d tidal forcing before any tide-on skill claims.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- eval/tideconst.py + config: fixed-list utide fits (model tide-only series
vs CO-OPS prediction, identical windows) -> per-constituent amplitude
ratio + phase difference, pooled across storms (Rayleigh caveat noted)
- eval/tidal_diagnosis.md: consolidated verdict with the tidecheck/twl/
tideconst tables. Headline: the Gulf panel is diurnal-dominated and the
DIURNAL band is inflated (O1 +34%, K1 +18%; M2 ~1.05) with a uniform
20-35 deg phase lead -> under-damped basin (near-resonant GoM diurnals),
pointing at the uniform fort.13 mannings_n=0.022 rather than the HAMTIDE
boundary input; TX bay gauges structurally broken at both resolutions
(unresolved inlets -> exclude from tide-on panels). Recommended: a
Manning's-n {0.022, 0.028, 0.035} tide-only mini-sweep scored by
tidecheck/tideconst.
- test: synthetic M2-only 1.4x plant recovered (58 passed)
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- adforce/fort13.py: minimal nodal-attribute editor -- rewrite the uniform
mannings_n_at_sea_floor DEFAULT, per-node overrides byte-preserved
(verified on the shipped decks: n=0.028 variant differs by exactly 1 line;
both fort.13.{low,mid} confirm the 0.022 control)
- training/inputs.py + driver.drive_storm: mannings_n pass-through; the
run-dir fort.13 is written with the override instead of copied
- eval/cells.py: ConfigCell.mannings_n; cell_id suffix (n0.028);
provenance_match checks the run's eval_axes block for friction cells and
tide-only controls (legacy dirs unaffected: checks fire only for
non-default cells)
- eval/launch.py: forcing/mannings_n pseudo-axes in matrix overrides (never
touch the wrap tree); eval_axes saved into <run>/config.yaml
- config/matrix/mannings_tide.yaml: {0.022 control, 0.028, 0.035} x
tide-only x mid -- the experiment from tidal_diagnosis.md, now with
ready-to-run GCP + scoring commands in the doc
- GCP-only: ARCHER2 references swept from eval docs/guards (allocation
retired; the subprocess backend IS the GCP path)
Gate: 60 passed 1 skipped (fort13 edit roundtrip incl. real-deck check,
mannings plan + eval_axes provenance lifecycle); dry-run smoke: 3 cells x
3 storms = 9-run plan with correct cell dirs; all touched modules compile.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- rerun/adcirc/mannings_sweep.sh: VM-side runner for the friction sweep, inheriting hist_sweep.sh conventions (adcirc-ws:lowres prep image with adcircpy/IBTrACS/aswip; read-only subtree mounts of a fresh branch clone over the baked /opt/worstsurge); dry-run plan first, then one in-container eval.launch invocation (per-run extract+strip, spot-safe resume) - eval/launch.py: strip_after_extract handles PE* partition DIRECTORIES (shutil.rmtree; os.remove threw inside the try and would have marked a successful run FAILED) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
conda-forge's 2026 compiler packages dropped the plain `ar` alias; cmake's static-archive steps (metis, datetime_fortran) call unprefixed ar/ranlib and failed with "Error running link command: no such file or directory". Symlink the x86_64-conda-linux-gnu-* binutils into place before configuring. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
get_default_config() re-initializes hydra for the wrap tree; inside the @hydra.main eval.launch app that threw "GlobalHydra is already initialized" -- only on dry_run=false, so every local gate missed it. Clear the app's hydra state first (cfg is already composed); regression test reproduces the app context and pins the fix. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… no-op
The 9-run GCP mannings sweep completed 9/9 but produced byte-identical
cells: the generated fort.15 (and the static decks) carry NWP=0, so fort.13
has NEVER been read by any run in this project. Friction is the NOLIBF=2
hybrid line CF=0.0025 HBREAK=1 -- uniform deep-water Cd over the whole
shelf, strengthening the under-damped-basin diagnosis.
- adforce/fort15.py: CF rewrite (HBREAK/FTHETA/FGAMMA kept; verified
single-line diff on generated + static decks)
- inputs/driver/cells/launch: friction_cf pseudo-axis end-to-end;
mannings_n now REFUSED at plan time and in generate_adcirc_inputs
(verified no-op; a silent burn of compute otherwise)
- config/matrix/friction_tide.yaml: CF {0.0025 control, 0.005, 0.0075} x
tide-only x mid; mannings_sweep.sh parameterized (MATRIX/STUDY env,
fort15.py added to the container mounts -- baked clone predates it)
- tidal_diagnosis.md: no-op post-mortem (byte-identical cells = incidental
end-to-end determinism proof) + take-2 experiment
- tests: fort15 editor roundtrip incl. static-deck pin, friction matrix
plan/cell_id/eval_axes provenance, mannings-blocked guard (63 passed)
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…al saturation 9/9 GCP runs, CF verified per deck, control byte-identical to the mannings runs (4th determinism check). CF 0.0025->0.005->0.0075: M2 1.15->1.05->0.98 (fully corrected, then over-damping), S2 1.44->1.24, phase leads halved, r unharmed; O1 saturates at ~1.34 and K1 at ~1.11 -- the residual diurnal bias is friction-INSENSITIVE (straits admittance / HAMTIDE diurnal boundary amplitudes). Verdict in tidal_diagnosis.md: CF~0.005 tide-side sweet spot; diurnal bias needs a boundary-side check; NO CF change for surge runs without re-running the surge validation (published skill was at 0.0025). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- eval/tidedb.py + config: three-way amplitude check (model / HAMTIDE via OPeNDAP Gulf subset / NOAA harcon). HAMTIDE ~+8% vs NOAA everywhere; model amplifies O1 +25% relative to its own forcing, friction-insensitive => EC95d basin response (straits admittance), mesh-level fix only. harcon shows K1 ~0.95 (tideconst K1 was partly P1 leakage in 13-d fits). - friction_storm sweep scored (Laura+Ida, 22 paired gauges): CF 0.005 damps surge peaks median -24% (-0.19 m), bias -0.09 -> -0.20 m, r 0.50 -> 0.43 => keep CF=0.0025 for surge; tides-vs-surge reconciliation requires NWP=1 spatial friction. Katrina storm-mode failed on GAHM ">4 isotachs" from prep-image env drift (known issue, tide-mode unaffected). - closing verdict in tidal_diagnosis.md: the tide-excluded surge methodology is justified, not merely convenient. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
moving comp to adforce/eval, and making it use hydra for config