Skip to content

implement multi-farm support in pywake_api - #62

Open
kilojoules wants to merge 5 commits into
mainfrom
support-multiple-farms
Open

implement multi-farm support in pywake_api#62
kilojoules wants to merge 5 commits into
mainfrom
support-multiple-farms

Conversation

@kilojoules

Copy link
Copy Markdown
Member

No description provided.

@bjarketol

Copy link
Copy Markdown
Contributor

Hi @kilojoules,

I did a quick agent-based review. It found a few things.

High

  • Per-farm AEP normalization mismatchwifa/pywake_api.py:1040-1043, 1252-1254. For time-series cases, aep_per_turbine is computed with normalize_probabilities=True while total AEP uses normalize_probabilities=False. Multi-farm per-farm AEP is derived from aep_per_turbine, so it is on a different basis than total AEP. Fix: use the same normalization (False) for per-turbine AEP in time-series, then slice/sum per farm.

  • Turbine deduplication by name onlywifa/pywake_api.py:285-293 (_build_multifarm_turbines). Two farms whose turbine YAMLs share a name (e.g. IEA-15MW) but differ in power curve/geometry are silently merged into the first farm's spec — wrong physics, no warning. Fix: dedupe by structural fingerprint, or raise when distinct specs share a name. At minimum warn on reuse.

Medium

  • hub_heights collision across farmswifa/pywake_api.py:97-100, 294, 1234. Single-type farms always key hub_heights with local "0"; hub_heights.update(hh) overwrites prior farms. Distinct hub heights are lost, breaking height-dependent site interpolation / FUGA setup. Fix: key by global turbine-type index (or farm-scoped key) and derive first_hh from merged global turbine objects.

  • generate_outputs still receives a list in multi-farm pathwifa/pywake_api.py:~1249. generate_outputs(sim_results, system_dat, ...) was written for a single-farm dict; in the multi-farm branch system_dat["wind_farm"] is a list. Either crashes iterating farm_dat["layouts"] or silently writes only farm 0's artifacts. The new test only checks the return value. Fix: teach generate_outputs to handle a farm list (per-farm outputs), or branch/skip and document the gap. Extend the test to assert output-dir contents.

  • farm_dat = farms[0] as a single-farm stand-inwifa/pywake_api.py:~1218. Inline comment admits "assumes shared turbine spec across farms." Any downstream code reading farm_dat (e.g. rd lookup, layout-derived state) silently uses farm 0 only — defeating the point of multi-farm. Fix: refactor downstream usages to operate per-farm, or assert single-turbine-type in the multi-farm path. Don't paper over with a comment.

  • Flaky wall-clock assertiontests/test_pywake.py:~516 (test_pywake_multifarm_runs_fast). assert elapsed < 10.0 is flaky on shared CI and doesn't validate correctness. Fix: drop the timing assertion (or move to an opt-in benchmark) and rename the test to reflect what it verifies (per-farm AEP + wake interaction).

Low

  • Unused _type_nameswifa/pywake_api.py:~284. Unpacked from _build_farm_turbine_list but never used; the type-name → global-index map is rebuilt from wt.name(), inconsistent with the per-farm "0"-style keys. Fix: remove the unused return, or use _type_names consistently.

  • layouts shape assumptionwifa/pywake_api.py:~136 (_build_farm_turbine_list). The multi-type branch indexes farm_dat["layouts"][0]["turbine_types"] while the single-type branch handles both list and dict forms; a multi-type config with dict-form layouts crashes with an opaque error. Fix: normalize layouts to a list once at the top of the helper.

@SchmJo

SchmJo commented May 11, 2026

Copy link
Copy Markdown
Contributor

is this only for pywake? can we setup a new example case, such that I can also adopt foxes to read multiple farms from windio? Thanks

Address PR #62 review:
- dedupe turbines across farms by full spec, not name; raise on
  same-name/different-spec conflicts
- key hub_heights by global type index so specs with distinct hub
  heights no longer collide on per-farm "0" keys
- take FUGA rotor diameter from merged specs instead of farms[0]
- per-turbine AEP on same normalization basis as total, so per-farm
  AEP sums match the returned total
- drop wall-clock assertion from multifarm test; assert outputs exist
- fold duplicated create_turbines/_build_farm_turbine_list into
  create_turbine + _build_multifarm_turbines

Add examples/cases/multiple_wind_farms from the IEA Wind 2200-22-MW
Reference Offshore Wind Plant (Apache-2.0): three farms sharing one
site and resource, for other tool APIs (foxes) to adopt.

Multi-farm wind_farm arrays validate only against upstream
IEAWindSystems/windIO schemas, so point the pin there; move AWAKEN
cutin/cutout under performance to conform.

Refs #61
@kilojoules

Copy link
Copy Markdown
Member Author

Pushed 6ec490f addressing the review findings and adding the requested example case:

  • New examples/cases/multiple_wind_farms: the IEA Wind 2200-22-MW Reference Offshore Wind Plant (three farms sharing one site and energy resource, from IEAWindSystems/IEA-Wind-2200-22-ROWP, Apache-2.0). run_pywake returns one AEP per farm, and tests/test_pywake.py::test_pywake_multifarm_rowp_example runs it end to end. The example is tool-agnostic windIO, so foxes can adopt the same input — the runner support here is pywake-only so far; foxes just needs its reader to accept a list of farms.
  • Review fixes: turbines are deduplicated across farms by full spec (same name + different spec now raises), hub heights are keyed by global type index instead of colliding per-farm "0" keys, the FUGA rotor diameter comes from the merged specs instead of farms[0], per-turbine AEP uses the same normalization basis as the total (so per-farm AEPs sum to it), and the wall-clock assertion is dropped. generate_outputs never touches wind_farm, so the multi-farm path writes the combined outputs fine; splitting artifacts per farm would be a follow-up.
  • Multi-farm wind_farm arrays only validate against the upstream IEAWindSystems/windIO schema (the EUFlow fork predates it), so the dependency pin moved to upstream, and AWAKEN's cutin_wind_speed/cutout_wind_speed moved under performance to conform to that schema.

@kilojoules

Copy link
Copy Markdown
Member Author

@bjarketol I think my coding agent adressed these things, detailed in the above message. Does this look OK to commit?

@kilojoules

Copy link
Copy Markdown
Member Author

@SchmJo I added a multi wind farm example

@SchmJo

SchmJo commented Jul 3, 2026

Copy link
Copy Markdown
Contributor

@kilojoules Supported by foxes now. Added to foxes tests, and they are green.

@bjarketol

Copy link
Copy Markdown
Contributor

Second-round agent review (4 agents: fix verification, fresh bug hunt, data/schema/cross-tool, test adequacy), at 952e8cd.

Round 1: all 8 findings verified fixed in 6ec490f — including numerically confirming per-farm AEPs sum to the total on both Weibull and time-series paths, and that generate_outputs never touches wind_farm.

New critical findings

  1. Single-farm regression (crash): a farm with a turbine_types dict but no per-position turbine_types in the layout (schema-valid, ran fine on main) now raises KeyError at wifa/pywake_api.py:107. Needs a fallback to type 0 when there's one spec and the layout key is absent.
  2. Silent per-farm AEP misattribution: farm_slices are sized by len(layout["turbine_types"]) but x/y concatenation by the coordinate arrays (pywake_api.py:207-209 vs 1131-1134). Probe with 3 coordinates / 2 type entries ran without error — one farm's AEP absorbed a neighbor's turbine, another turbine vanished from all per-farm totals. Needs an explicit length-mismatch ValueError.
  3. Type indices resolved by dict position, not key: layout integers are windIO keys into the turbine_types mapping, but the code does list(dict.values()) + positional lookup (pywake_api.py:106-107, 207). 1-based keys (see examples/cases/windio_4turbines_multipleTurbines) → IndexError; unordered keys → silently wrong turbine model. Pre-existing flaw, but this PR rewrote the mapping layer with no coverage.
  4. test_foxes_multiple_farms fails in the default env: plain uv run pytest resolves foxes 1.7.2 from PyPI (via wayve), which can't read a wind_farm list — the PR's own test fails with TypeError. Passes only with --extra foxes (1.8.4 from eu_flow). Needs a capability skip or explicit CI extra.
  5. wayve and floris crash with a raw TypeError on the multi-farm example (verified). Both should detect the list and raise a clear NotImplementedError; same audit for cs_api.

Medium

  • Both new git deps are floating (windIO head-of-main, foxes@eu_flow branch) — only uv.lock protects CI. Pin a commit/tag.
  • Time-series multi-hub-height paths crash (now reachable via multi-farm): missing TI leaves site = None (pywake_api.py:467-498); two types at the same hub height hit a dead dedup guard → xarray dim conflict (:471-473).
  • docs/source/getting_started/installation.rst:60-80 still instructs installing the EUFLOW windIO fork, whose schema may reject the new example.
  • foxes merges all farms into one 100-turbine result with no farm coordinate — per-farm AEP unrecoverable, inconsistent with pywake's return. Document at minimum.
  • Weak tests: ROWP test only asserts len(aep) == 3 and positivity; nothing asserts sum(per_farm) == total; multi-farm output artifacts unchecked; test_foxes_multiple_farms asserts nothing. Untested: multi-farm + time-series, dict-form layouts, multi-type single farm.

Low

  • run_pywake docstring still says returns float; multi-farm returns list[float].
  • Spec-conflict check uses dict !=; numpy arrays from dict-input callers → "ambiguous truth value" instead of the intended ValueError.
  • ROWP example dir needs a short README with Apache-2.0 attribution (WIFA is MIT); €/m vs USD/m inconsistency in wind_farm.yaml.

Verified good

Type indexing matches from_WindTurbine_lst order, farm slices align with concatenation order (incl. mixed-type farms), zero-turbine farms work; ROWP example internally consistent; AWAKEN cutin/cutout move is schema-correct and fixes a silent bug (values were previously ignored — AWAKEN AEP numbers will shift).

Recommendation: hold merge until criticals 1–4 are addressed. Highest-value tests: multi-type single farm with 1-based keys, sum(per_farm) == total for both resource types, length-mismatch ValueError.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants