Add SWOPP3 analysis script and violations module

[daniprec]

Summary

Adds two files for SWOPP3 post-processing and competition analysis.

routetools/violations.py

New core library module that counts Codabench-style route violations for SWOPP3 output folders:

• Wind violations: waypoints with max_wind_mps > 20
• Wave violations: waypoints with max_hs_m > 7
• Land violations: sampled track waypoints on land (same subsampling rule as Codabench scorer: step = max(1, len(waypoints) // 50))

Optionally accumulates smooth ERA5 wind/wave penalties alongside threshold counts. Great-circle cases are excluded by default (include_gc=False).

Key public API: count_folder_violations, format_grouped_violation_table, write_grouped_violation_csv.

scripts/swopp3_analysis.py

Publication-quality analysis script producing 13 matplotlib figures and summary CSVs for the four SWOPP3 experiments (no_penalty, no_penalty_fms, penalty, penalty_fms).

Usage:

python scripts/swopp3_analysis.py --data-dir output/ --output-dir output/analysis/ [--dpi 150] [--figures 1,2,3]

Validation

• All pre-commit hooks pass (ruff, codespell, type annotations)
• Violation counts validated against expected totals from output/cmaes_weather

[fjsuarez]

Review: PR #67 — Add SWOPP3 analysis script and violations module

Scope: 2 new files, +2974 lines, 1 commit, targeting feat/swopp3-clean

What's good

• routetools/violations.py is well-structured: typed dataclasses, Protocol for land-checker, proper separation of concerns, imports from routetools.weather constants (not hardcoded thresholds).
• scripts/swopp3_analysis.py is thorough — 13 publication-quality figures with consistent IE branding, good docstrings, graceful degradation for missing data.
• Clean merge against current feat/swopp3-clean.
• Good module docstring in violations.py with usage documentation.
• Threshold constants reused from routetools.weather.DEFAULT_TWS_LIMIT / DEFAULT_HS_LIMIT — consistent with the CodaBench scorer.

---

Issues to address

1. No tests for violations.py (blocking)

violations.py is being added to the core library (routetools/), not scripts. Per repo conventions, library modules need test coverage. There should be at least:

• count_summary_weather_violations — unit test with a temp CSV
• count_land_violations — unit test with mock land checker
• count_folder_violations — integration test with a small temp folder structure
• find_team_prefix — test with known file patterns + FileNotFoundError case
• is_gc_case — quick parametric test
• ScenarioViolationCounts.total_violations / total_penalty — property tests
• format_grouped_violation_table / write_grouped_violation_csv — roundtrip test

2. Very stale base — needs rebase (blocking)

The branch diverges from a point before all test files were added — the diff shows 5213 deleted test lines across 18 files. This means the branch is forked from before:

• USNYS port fix
• MWA wrapping fix
• FMS branch merge (PR #64)
• Circular interpolation tests
• All current test infrastructure

Please rebase onto current feat/swopp3-clean so the branch includes these and tests pass in isolation.

3. Global mutable state in swopp3_analysis.py

OUTPUT_DIR: Path = _REPO_ROOT / "output"
FIGS_DIR: Path = _REPO_ROOT / "output" / "analysis"
# ...
def main():
    global OUTPUT_DIR, FIGS_DIR

Every figure function reads OUTPUT_DIR / FIGS_DIR as globals. This makes the functions untestable in isolation and fragile. Consider passing these as parameters (or a simple config dataclass) through main() → figure functions.

4. swopp3_analysis.py is 2353 lines — consider splitting

The script handles 13 figures + summary table + data loading + style. Consider:

• Extract shared data loading into a small helper module (or reuse parts of violations.py)
• Group related figures into smaller modules

Not blocking, but the current size will make maintenance painful.

5. Hardcoded experiment folder names

EXPERIMENTS = {
    "no_penalty": {"folder": "swopp3_no_penalty", ...},
    "no_penalty_fms": {"folder": "swopp3_no_penalty_fms", ...},
    ...
}

The 4 experiment folder names and the team prefix IEUniversity-1 are hardcoded in both files. If PR #65 (experiment config profiles) lands first, consider reading folder names from config.toml instead of duplicating them.

6. violations.py duplicates ERA5 loading pattern

_loadable_era5_paths() and load_default_weather_resources() duplicate logic already in scripts/swopp3_run.py and routetools/era5/loader.py. Consider refactoring the shared ERA5 path resolution into a single utility.

7. load_tracks() uses random_state=42 without documenting why

sample = summary.sample(min(n_sample, len(summary)), replace=False, random_state=42)

This is fine for reproducibility but the magic number should have a comment noting it's for deterministic track sampling in figures.

8. Cartopy + shapefile as implicit dependencies

violations.py imports cartopy and shapefile/shapely at function level, which is good for lazy loading. But neither appears in pyproject.toml dependencies. They should be listed as optional extras or documented as required for the violations module.

---

Minor nits

• _red_line_rule() is defined but never called — dead code.
• _outlier_cap name suggests it returns capped data, but it returns a boolean mask. Consider _outlier_mask.
• The PR description says "Violation counts validated against expected totals from output/cmaes_weather" — this validation should be captured as an automated test, not a one-time manual check.

Suggested actions

1. Rebase onto current feat/swopp3-clean (blocking)
2. Add tests/test_violations.py with unit tests for the core library module (blocking)
3. Pass output paths as parameters instead of globals
4. Consider extracting shared ERA5 loading to avoid duplication

[daniprec]

Addressed the review comments on this PR.

What changed

1. Added tests/test_violations.py.
   This covers:

   • count_summary_weather_violations
   • count_land_violations
   • count_folder_violations
   • find_team_prefix, including the FileNotFoundError case
   • is_gc_case
   • ScenarioViolationCounts.total_violations
   • ScenarioViolationCounts.total_penalty
   • format_grouped_violation_table
   • write_grouped_violation_csv

2. Removed the mutable OUTPUT_DIR and FIGS_DIR pattern from scripts/swopp3_analysis.py.
   I introduced an immutable AnalysisPaths dataclass and passed it through the data-loading and figure-generation functions instead of mutating globals in main().

3. Removed the hardcoded IEUniversity-1 lookup from the analysis script.
   The script now reuses find_team_prefix() to detect the summary-file prefix per experiment folder.

4. Addressed the small script nits.

   • Removed the unused _red_line_rule() helper.
   • Renamed _outlier_cap() to _outlier_mask().
   • Added comments for deterministic random_state=42 and random_state=7 sampling.

5. Reduced duplication in routetools/violations.py.
   I moved the ERA5 continuation-file path logic into routetools.era5.loader.loadable_era5_paths() and reused it from violations.py.

6. Documented the geospatial dependency expectation in load_default_land_checker().
   I did not change pyproject.toml because touching packaging metadata on this branch forced an unrelated uv re-resolution issue. The repository checks pass without that packaging change, so I kept this PR scoped to code and tests.

Rebase status

I fetched origin/feat/swopp3-clean before replying.

Current state is:

• 5 ahead
• 0 behind

So this branch is not stale relative to the target branch in its current state.

Validation

Both commands pass on the updated branch:

• make hooks
• make test

Follow-up

I did not split scripts/swopp3_analysis.py into smaller modules in this PR.
I agree with the maintainability concern, but I kept that as follow-up work so the review fixes stay focused.

I also did not wire experiment folder names to config.toml here. That depends on the config-profile work landing first, and it felt better as a separate change than mixing it into this review pass.

[daniprec]

Follow-up after swopp3-config merged.

I revisited the experiment-folder comment and updated scripts/swopp3_analysis.py to read SWOPP3 output folder names from config.toml when the merged config defines a matching experiment output.

A few details:

1. The script now reads configured output folders from config.toml.
2. It keeps the current legacy folder names as a fallback when the configured folder does not exist locally.
3. That fallback matters here because the merged config defines split_penalty, while the current analysis data in this branch still lives under swopp3_penalty and swopp3_penalty_fms.
4. I added tests/test_swopp3_analysis.py to cover the config-backed folder resolution and the legacy fallback path.

Validation still passes:

• make hooks
• make test

[fjsuarez]

Review of #67: Add SWOPP3 analysis script and violations module

Good work overall — the violations.py module is well-structured with clean separation between threshold counting, land checking, and smooth penalty accumulation. The test coverage for violations.py is solid.

Summary

Approve with minor suggestions. Nothing is a blocker; the code is functional and correct.

What works well

• violations.py has a clean public API (count_folder_violations, format_grouped_violation_table, write_grouped_violation_csv)
• The Codabench subsampling rule (step = max(1, len(waypoints) // 50)) is faithfully reproduced
• ScenarioViolationCounts as a frozen dataclass is the right pattern
• loadable_era5_paths() in era5/loader.py is a useful generalized helper
• test_violations.py covers the key paths well (235 lines of solid tests)
• The analysis script produces publication-quality figures with consistent IE branding

Issues (all minor)

1. Unused case_id parameter in count_summary_weather_violations() (see inline comment)
2. travel_time unit ambiguity: The code correctly passes passage_hours, matching how the rest of the codebase works — but wind_penalty_smooth's docstring says "seconds". Worth a clarifying comment (see inline)
3. pd.concat in a loop in fig_penalty_tradeoff() — O(n²) pattern, easy to replace with list + single concat
4. Global warnings.filterwarnings("ignore") — could mask real warnings when importing individual figure functions; better scoped inside main()
5. No test for loadable_era5_paths() — new public function in era5/loader.py with branching logic (regex match, next-year file, glob fallback) that would benefit from a few unit tests
6. importlib test pattern — fragile for testing script internals; consider extracting shared helpers into an importable module

Questions

• The analysis script hardcodes 4 experiment configs (no_penalty, no_penalty_fms, penalty, penalty_fms). With the penalty sweep work we're doing now, we'll likely want more configs. Is the plan to parametrize these later via config.toml, or is this script specifically for the publication comparison?
• load_default_land_checker() creates a unary_union of all 10m Natural Earth shapes on every call (no caching). For a one-shot script this is fine but if count_folder_violations is called in a loop over many folders, the caller should pre-build the checker. Worth noting in the docstring?

[fjsuarez]

Unused parameter: case_id is accepted but never referenced in the function body. Either remove it or use it (e.g., for logging/validation).

[fjsuarez]

The wind_penalty_smooth docstring says travel_time is in seconds, but in practice the entire codebase (cmaes.py, swopp3_runner.py) passes passage_hours — i.e., hours. This code correctly follows the same convention (hours), but the mismatch with the docstring is confusing. Might be worth adding a comment here:

# travel_time is in hours, matching time_offset (hours since epoch).
# The wind_penalty_smooth/wave_penalty_smooth docstrings say "seconds"
# but the actual implementation just needs consistent units with time_offset.

[fjsuarez]

This suppresses ALL UserWarning globally for any code that imports this module as a library. This is fine for a CLI script but could mask real warnings if someone imports individual figure functions. Consider scoping it inside main() or using a context manager.

[fjsuarez]

pd.concat inside a loop is O(n²). Consider collecting dicts in a list and creating the DataFrame once at the end:

records = []
for case_id in OPT_CASES:
    for exp_key in ["no_penalty", "penalty"]:
        piece = sub[(sub["experiment"] == exp_key) & (sub["case_id"] == case_id)]
        if piece.empty:
            continue
        records.append({
            "case_id": case_id,
            "experiment": exp_key,
            "wind_violation_pct": piece["wind_viol"].mean() * 100,
            "wave_violation_pct": piece["wave_viol"].mean() * 100,
            "any_violation_pct": piece["any_viol"].mean() * 100,
            "mean_energy": piece["energy_cons_mwh"].mean(),
        })
metrics = pd.DataFrame(records)

[fjsuarez]

Missing test coverage: loadable_era5_paths() is a new public function called by violations.py, but there's no test for it. Would be good to add a simple test verifying:

1. Returns [path] when filename doesn't match the ERA5 pattern
2. Returns [path, next_year_path] when a continuation file exists
3. Handles the glob case (prefix_{next_year}_*.nc)

[fjsuarez]

Loading the script via importlib is fragile and pollutes sys.modules. If the helpers being tested (AnalysisPaths, _configured_output_dirs, _experiment_folder) are stable public-ish API, consider extracting them into a small importable module (e.g., routetools/analysis_config.py) that both the script and the tests can import normally. That would also make the 2500-line script slightly more manageable.