Refactor/adforce eval - #3
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>
- 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>
plot_city_key: for each study city (New Orleans, Galveston, Miami) the
largest OBSERVED surges within city_radius_deg (default 2.0), one panel per
distinct storm, rendered with the standard example-panel plotter -- the
"did the model capture this city's storm history" complement of
plot_failures. The radius matters: nearest-city assignment alone handed the
whole Atlantic seaboard to Miami (Fernandina Beach posing as Miami).
Documented caveat: panels are gauge-record-limited -- Katrina's
extreme-surge gauges failed/predate the network and Ida's two nearest are
KNOWN_FAILED, so those storms cannot headline their own city.
CLI: python -m adforce.eval.validate validate.city_key=true
Outputs: img/comp/val_city_key_{city}.png + <thesis>/img/comp_val_city_key_{city}.pdf
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
plot_gauge_map: coastline (cartopy 50m, plain-axes fallback), every panel gauge (filled = contributes valid pairs, open = none), KNOWN_FAILED crosses, the Gulf/Florida selection boxes, and the three study cities with their city_radius_deg catchment circles -- the one-look explanation of per-city coverage (Miami's circle holds 4 gauges vs dozens in the Gulf catchments). CLI: python -m adforce.eval.validate validate.gauge_map=true Outputs: img/comp/val_gauge_map.png + <thesis>/img/comp_val_gauge_map.pdf Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- plot_city_gauge_maps: one zoomed map per study city, every gauge named
(cluster-indexed label ladder), valid/no-pair/failed markers at alpha 0.7
- _read_fort14 + _draw_mesh_bathymetry: fort.14.mid depth as the base layer
of BOTH map types (tricontourf, Blues, model land gray, faint element
edges on the zoomed maps) -- the maps now show model features, not just
geography: unmeshed water bodies render white (Lake Pontchartrain!),
which visually explains why its gauges contribute no valid pairs, and the
narrow Florida shelf shows why Miami barely surges
- marker alpha 0.7 across both maps (overlapping stations stay visible)
CLI: python -m adforce.eval.validate validate.city_maps=true
Outputs: img/comp/val_gauge_map_{city}.png + <thesis>/img/comp_val_gauge_map_{city}.pdf
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Both map types now show, per study city, the fort.14 node nearest the configured city point -- the location wrap.observe_max_point actually snaps observations to (purple triangle, alpha 0.7; stars also 0.7). For New Orleans the snap lands ~0.15 deg EAST of the city (the city point borders unmeshed Lake Pontchartrain), i.e. the BO potential heights are evaluated in the Lake Borgne corridor -- now visible. Per-city maps gain a compact marker legend. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
In-map name labels could not be made collision-free in dense clusters (Mobile Bay, downtown New Orleans); replaced with west->east numbered dots (white-halo digits) and a three-column full-name key in a dedicated axes row under the map (cartopy's aspect-locked GeoAxes ignores subplots_adjust, so the key needs real gridspec space). Bathymetry levels refined (17 steps, extra shallow-water resolution) so shelf features and the colorbar read properly. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
🟡 Changes recommended
There are a few correctness/provenance issues in the updated run-success detection and refactor-related generated headers/documentation that should be fixed to avoid wasted reruns and confusing outputs.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR completes the comp/ → adforce/eval refactor, updating imports/docs/tests and introducing a Hydra-driven evaluation/orchestration pipeline (launch/extract/harvest/pairs + tide diagnostics) while preserving the existing data/comp/ cache layout.
Changes:
- Rename/relocate validation code to
adforce.evaland update downstream users (worst/*, tests, docs). - Add Hydra configs + new eval utilities (matrix expansion, launch planning/execution, run-status/provenance checks, extraction/harvesting, tide-on diagnostics and model-vs-model comparisons).
- Make some paths more robust (e.g., tcpips directory creation under dangling symlinks) and add minimal ADCIRC deck editors (
fort.13/fort.15) used by sweeps.
File summaries
| File | Description |
|---|---|
| worst/gauge_fit.py | Update doc/import to new adforce.eval.annual_max location. |
| worst/gauge_fit_ns.py | Update import to new adforce.eval.annual_max location. |
| tests/test_gauge_evt.py | Switch test imports/constants from comp to adforce.eval. |
| tests/test_comp.py | Remove legacy comp test suite (superseded by tests/test_eval.py). |
| tcpips/constants.py | Replace eager os.makedirs with best-effort _ensure_dir for symlinked paths. |
| setup.py | Rename extras to [eval], add deprecated [comp] alias, package adforce.eval + YAML package_data. |
| rerun/adcirc/score_resolution.py | Mark script as superseded by adforce.eval.pairs. |
| rerun/adcirc/mannings_sweep.sh | Add documented GCP sweep script driven by adforce.eval.launch. |
| rerun/adcirc/extract_gauge_series.py | Mark script as superseded by adforce.eval.extract. |
| rerun/adcirc/Dockerfile | Add conda toolchain ar/ranlib/nm symlink workaround for builds. |
| REPRODUCE.md | Update validation commands/paths to adforce.eval.* and [eval] extra. |
| README.md | Rename comp section to adforce.eval and update install instructions. |
| docs/index.rst | Replace comp docs entry with eval. |
| docs/eval.rst | Add Sphinx API docs page for adforce.eval. |
| docs/conf.py | Update docs note about non-mocked deps to reference adforce.eval. |
| docs/comp.rst | Remove legacy Sphinx API docs page for comp. |
| data/comp/out/val_summary.csv | Update/pin validation summary rows (adds additional gauges/entries). |
| adforce/training/inputs.py | Add friction_cf support; explicitly refuse mannings_n as verified no-op. |
| adforce/training/driver.py | Add drive_storm seam + shared run-success detection helper. |
| adforce/fort61.py | Add read_fort61() helper and improve module docstring. |
| adforce/fort15.py | Add minimal fort.15 friction CF read/write helper. |
| adforce/fort13.py | Add minimal fort.13 default Manning’s-n read/write helper. |
| adforce/eval/init.py | Update package entry-point docs and exports for adforce.eval. |
| adforce/eval/_cli.py | Add helper to reject legacy argparse flags and print Hydra translations. |
| adforce/eval/constants.py | Adjust project-root resolution; make directory creation lazy via ensure_dirs(). |
| adforce/eval/coops.py | Ensure cache dirs are created lazily before CO-OPS metadata/data caching. |
| adforce/eval/annual_max.py | Port annual-max pipeline to Hydra and new package path; use lazy dir creation. |
| adforce/eval/validate.py | Hydra-ify validation CLI; add gauge-panel union logic + new plotting utilities and mesh base layer. |
| adforce/eval/nulltest.py | Hydra-ify null tests; align gauge selection with validate’s per-storm panel logic. |
| adforce/eval/sensitivity.py | Hydra-ify sensitivity entry point; ensure dirs lazily. |
| adforce/eval/detide.py | Add shared tide-on obs utilities (window fetches, alignment, skew surge). |
| adforce/eval/detide_sensitivity.py | Hydra-ify detide sensitivity; reuse shared skew-surge implementation. |
| adforce/eval/pairs.py | Add model-vs-model comparison utilities (resolution bias + interaction + generic comparisons). |
| adforce/eval/extract.py | Add remote-side reducer from fort.63.nc to gauge_ts.parquet. |
| adforce/eval/harvest.py | Add rsync-based minimal-artifact harvester from remote sweeps to laptop. |
| adforce/eval/status.py | Add provenance-aware run status classification (success/extracted/foreign/etc.). |
| adforce/eval/cells.py | Add deterministic cell identity + matrix expansion + provenance matching. |
| adforce/eval/sources.py | Add HF-archive and run-dir simulation sources (field + per-gauge artifacts). |
| adforce/eval/launch.py | Add Hydra sweep orchestrator (plan/launch/manifest/strip, controls, provenance safety). |
| adforce/eval/tidecheck.py | Add tide-only model-vs-NOAA prediction diagnostics. |
| adforce/eval/tideconst.py | Add per-constituent tidal diagnosis (model vs NOAA prediction). |
| adforce/eval/tidedb.py | Add three-way tidal amplitude check (model vs HAMTIDE vs NOAA harcon). |
| adforce/eval/twl.py | Add tide-on total-water-level scoring vs raw gauges + skew-surge metric. |
| adforce/eval/tidal_diagnosis.md | Add detailed experiment log and conclusions for tidal/friction diagnosis. |
| adforce/eval/config/eval_config.yaml | Add root Hydra config for validate/nulltest/sensitivity/detide_sensitivity. |
| adforce/eval/config/annual_max_config.yaml | Add root Hydra config for annual-max extraction. |
| adforce/eval/config/extract_config.yaml | Add root Hydra config for run extraction. |
| adforce/eval/config/harvest_config.yaml | Add root Hydra config for harvesting. |
| adforce/eval/config/launch_config.yaml | Add root Hydra config for sweep launch/orchestration. |
| adforce/eval/config/pairs_config.yaml | Add root Hydra config for model-vs-model comparisons. |
| adforce/eval/config/tidecheck_config.yaml | Add root Hydra config for tidecheck. |
| adforce/eval/config/tideconst_config.yaml | Add root Hydra config for tideconst. |
| adforce/eval/config/tidedb_config.yaml | Add root Hydra config for tidedb. |
| adforce/eval/config/twl_config.yaml | Add root Hydra config for TWL/scoring. |
| adforce/eval/config/scoring/default.yaml | Add overridable scoring config pinned to constants. |
| adforce/eval/config/source/hf_archive.yaml | Add config stub for HF archive simulation source. |
| adforce/eval/config/source/run_dirs.yaml | Add config stub for run-directory simulation source. |
| adforce/eval/config/matrix/archive_default.yaml | Add one-cell matrix for published archive reproduction. |
| adforce/eval/config/matrix/res_x_tide.yaml | Add res×tide matrix definition. |
| adforce/eval/config/matrix/mannings_tide.yaml | Add Manning’s-n sweep matrix (expected to be blocked as no-op). |
| adforce/eval/config/matrix/friction_tide.yaml | Add friction-CF tide-only sweep matrix. |
| adforce/eval/config/matrix/friction_storm.yaml | Add friction-CF storm-only sensitivity sweep matrix. |
Review details
Suppressed comments (1)
adforce/eval/detide_sensitivity.py:235
- This generated-file header still references the old
comp.detide_sensitivitymodule name; it should be updated toadforce.eval.detide_sensitivityso the emitted LaTeX carries the correct provenance.
- Files reviewed: 63/71 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.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.
Moved
comp/toadforce/eval, as its primary purpose is to test the quality ofadforce's ADCIRC setup. Also made it use hydra config for consistency etc.