Conversation
Stands up the whole declaration -> runner -> farm-uplift -> reporting pipeline on a placebo (zero injected uplift) whole-farm campaign, so a campaign is declared rather than hand-wired. - src/wind_up/farm.py: farm_uplift(), the pure headline function, with the capacity-factor cap and non-negativity floor, per-turbine guard flags and spread. - benchmarking/campaigns/: SyntheticCampaign (private, holds the injected upgrades) derives CampaignSpec (public facts only); CampaignRunner; the report; and the two placebo campaigns with their driver. - true_farm_uplift(): the N-turbine generalisation of true_net_uplift. - conditional_truth_vs_estimate moved from inspect_prepost_hard_case into harness/plots.py, so campaigns/report.py does not import the v0 pipeline for a frame-shaping helper. Re-exported, so existing callers are unchanged. Consumers read per-turbine facts through spec.timing_for()/usable_mask() and the mode through spec.mode, so C8's per-turbine change histories change accessor bodies rather than every call site. Results on real Hill of Towie SCADA are logged in docs/v1/findings_campaigns.md (CF1-CF5): truth is exactly 0 in both modes; toggle beats prepost by an order of magnitude; the farm result reaches +0.148% with six test turbines; and T06 is identified as the best failure-mode fixture turbine. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ECPVDa4P3dbiWYCQKbz18D
aclerc
commented
Sep 1, 2026
aclerc
left a comment
aclerc
left a comment
Contributor
Author
There was a problem hiding this comment.
some terminology comments
PR #136 review. Two terminology changes in the product surface: - TurbineUplift.treated_energy -> actual_energy. The codebase already pairs "actual" with "counterfactual" -- power_model documents its headline as ``sum_actual / sum_counterfactual - 1`` -- and farm.py already had counterfactual_energy, so "treated" was the odd one out. The docstring formula now reads the same way in both places. "actual" is also neutral for the analyses C8 covers that are not upgrades: confirming stable performance, or quantifying a loss event. - "headline" -> "result" throughout, since a farm is not necessarily the top level of aggregation. src/wind_up/ no longer contains "treated". The benchmarking layer keeps it (including the shared treated_mask helpers) for C8 to sweep once the neutral umbrella term is settled, rather than leaving a third vocabulary in play. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ECPVDa4P3dbiWYCQKbz18D
Contributor
There was a problem hiding this comment.
Pull request overview
Implements C1’s campaign framework and placebo analysis pipeline.
Changes:
- Adds guarded farm-level uplift aggregation and pooled synthetic truth.
- Adds campaign declarations, method selection, runner, reporting, and placebo drivers.
- Adds comprehensive campaign tests and supporting design/findings documentation.
Reviewed changes
Copilot reviewed 29 out of 31 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
src/wind_up/farm.py |
Adds farm uplift aggregation. |
src/wind_up/__init__.py |
Exports farm uplift API. |
tests/wind_up/__init__.py |
Initializes test package. |
tests/wind_up/test_farm.py |
Tests aggregation and guards. |
benchmarking/synthetic/ground_truth.py |
Adds pooled farm truth. |
benchmarking/synthetic/generator.py |
Exposes farm truth on datasets. |
benchmarking/synthetic/__init__.py |
Exports farm truth helper. |
tests/benchmarking/synthetic/test_ground_truth.py |
Tests pooled truth behavior. |
benchmarking/campaigns/__init__.py |
Exports campaign APIs. |
benchmarking/campaigns/declaration.py |
Defines campaign declarations and specs. |
benchmarking/campaigns/methods.py |
Selects applicable methods. |
benchmarking/campaigns/runner.py |
Runs and aggregates campaigns. |
benchmarking/campaigns/report.py |
Writes campaign reports and plots. |
benchmarking/campaigns/placebo.py |
Declares and runs placebo campaigns. |
tests/benchmarking/campaigns/__init__.py |
Initializes campaign tests. |
tests/benchmarking/campaigns/test_declaration.py |
Tests campaign declarations. |
tests/benchmarking/campaigns/test_methods.py |
Tests method selection. |
tests/benchmarking/campaigns/test_runner.py |
Tests campaign execution. |
tests/benchmarking/campaigns/test_report.py |
Tests report generation. |
tests/benchmarking/campaigns/test_placebo.py |
Tests placebo declarations. |
tests/benchmarking/campaigns/test_placebo_end_to_end.py |
Exercises real baseline methods. |
benchmarking/harness/plots.py |
Centralizes conditional plot shaping. |
benchmarking/harness/__init__.py |
Exports plotting helper. |
benchmarking/baselines/inspect_prepost_hard_case.py |
Uses shared plotting helper. |
docs/v1/README.md |
Links campaign findings. |
docs/v1/issues_campaigns.md |
Updates campaign terminology and scope. |
docs/v1/findings_campaigns.md |
Records placebo findings. |
docs/superpowers/specs/2026-08-28-w0-src-layout-rename-design.md |
Updates layout design text. |
docs/superpowers/specs/2026-08-28-c1-campaign-runner-placebo-design.md |
Documents C1 design. |
docs/superpowers/specs/2026-08-27-realistic-campaigns-design.md |
Aligns campaign terminology. |
docs/superpowers/plans/2026-09-01-c1-campaign-runner-placebo.md |
Provides implementation plan. |
💡 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>
Copilot review on #136. The guards accepted non-finite actual energy and non-finite or non-positive rated power, and each failed in its own way: - NaN energy passed with used=True and no guard, then vanished from the result entirely, because the summation skips NaN. The farm number looked clean and n_guarded reported 0. - Infinite energy produced an infinite farm result. - A negative rating made the capacity cap clip the counterfactual to a *negative* value, violating the documented non-negativity floor, and gave a result below -100%. - A NaN rating silently disabled the cap. All four are now dropped with explicit guard reasons (non_finite_energy, invalid_rating), so they are visible in n_guarded and the per-turbine detail rather than silent. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ECPVDa4P3dbiWYCQKbz18D
Copilot review on #136. farm_uplift can drop a turbine (a non-finite estimate, say), so a method's farm estimate may cover a subset of the campaign while its truth was pooled over every upgraded turbine. signed_error then subtracted one estimand from another -- and it did so precisely when a method misbehaved, which is when the number matters most. Each farm row's truth is now pooled over that method's used turbines. n_guarded flags the rows where turbines were dropped, since a method that dropped some is not directly comparable with one that used them all. CampaignResult.truth_farm_uplift is unchanged: it remains the all-turbine campaign truth. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ECPVDa4P3dbiWYCQKbz18D
…n needed Copilot review on #136, two independent placebo fixes: - candidate_references included excluded turbines, so the spec simultaneously offered a turbine as a reference and forbade its use. Behaviour was correct because the runner drops those rows, but C3 is "automatic reference selection from the CampaignSpec" and would have read the contradictory field. - build_hot_v0_context fetches ERA5, and only the power model reads it, so include_power_model=False still paid the network dependency. It is now built only when the power model is enabled, which makes the fast path genuinely offline. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ECPVDa4P3dbiWYCQKbz18D
Copilot review on #136. The report built truth for every condition whenever a method reported any conditional output, so a method reporting one axis still got plots for the others. toggle_specialist reports only "power", so every toggle run wrote ws and ti charts whose method series was entirely NaN while being labelled method-vs-truth. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ECPVDa4P3dbiWYCQKbz18D
The placebo is a whole-farm campaign and v0 enumerates test/reference combinations per turbine, so a whole-farm v0 run is not tractable. Recording the decision rather than leaving an unmet acceptance criterion: Copilot correctly flagged on #136 that the scope asked for v0 as an optional slow method and the driver did not provide one. The seam still accepts V0BinnedMethod unchanged, so a later campaign over a small turbine subset can include it. C4 still expects v0. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ECPVDa4P3dbiWYCQKbz18D
C8 scope gains two items that belong with its vocabulary sweep:
- Disambiguate "window" in benchmarking/harness/campaign.py, which uses it for two
different spans in one docstring: CampaignWindow is the whole baseline-plus-activity
span, while its prose says "post window" / "activity window" for the treated part
alone. That is the ambiguity C1 renamed `window` to `analysis_period` to escape, so a
reader who knows the harness will misread the spec field.
- Retire "treated" from the benchmarking layer in the same pass (443 uses, 64 of them
the shared treated_mask helpers). src/ is already clear of it.
Also drops "treated period" as the example neutral fallback, since the same issue
retires that word.
The robustness design note was untracked and had picked up the C1 naming pass
("brief" -> "campaign spec"), so those edits existed only in the working tree.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ECPVDa4P3dbiWYCQKbz18D
aclerc
added a commit
that referenced
this pull request
Sep 2, 2026
* Mark C0, W0 and C1 done in the campaigns issues list C1 merged as PR #136. C0 and W0 were already complete but unmarked, so the list did not show where the tranche stood. Each now carries a Status line in the same shape C7 uses, and the suggested order marks the finished issues and names C2 as next. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ECPVDa4P3dbiWYCQKbz18D * add spec * C2: campaign context on the method seam Methods learn their candidate references and row validity from a CampaignContext the runner derives from the CampaignSpec, instead of each deriving "every turbine except the test one" -- nine such derivations across four methods, and a declaration nothing read. CampaignContext (harness/context.py) is a narrow per-test-turbine view, so methods are not coupled to the analyst-facing declaration that C8 generalizes and W2 promotes to public API. context_for (campaigns/context.py) is the one translation point, and the one place to audit that no truth reaches a method. Declared validity (exclusions, C8 histories, C5 wake gating) rides the context; screened validity (R3) stays method-internal. valid_for_uplift is named for its purpose: data can be invalid for uplift and still valid for a northing analysis. MethodInput gains a lazily-built default context that is today's implicit contract, so existing call sites are unchanged. Adds ColumnSchema.northed(role) for R1's derived column. Behaviour change: the campaign path now honours the declared candidate_references, so the placebo's six upgraded turbines stop acting as each other's references. Placebo CF1-CF5 in findings_campaigns.md are stale as a result. The study path is untouched. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ECPVDa4P3dbiWYCQKbz18D * C2 review: enforce context invariants, cover declared turbines Addresses the PR 137 review. MethodInput now rejects two silent-failure modes: an input with neither a campaign_context nor an upgrade_timing (which built a context with timing=None and failed later in resolve_toggle), and a context whose test_wtg differs from the input's (which would estimate one turbine while reading another's references and row validity). context_for now covers every declared turbine present in the frame, not just the test turbine and its references, and select() raises rather than silently keeping the rows of a turbine it has no validity for. A method co-analysing several turbines keeps them via select(also=...); those rows were previously exempt from declared validity. Also hoists a set() rebuilt per column in the two wide-format methods. Not changed: build_toggle_df already de-duplicates its index internally and documents itself as indexed by unique timestamps, so passing a long-format index is correct and unchanged from before this branch. Docs: each C-issue with a real Hill of Towie counterpart now runs it alongside the synthetic campaign to test the shape of the declaration (no ground truth, not scored); C3/C4/C5 name their counterpart. W2 gains a YAML-declared CampaignSpec, with those three real campaigns as the acceptance test for "easy to use". C2's done-when records the placebo re-record as its closing step. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ECPVDa4P3dbiWYCQKbz18D * CF6: re-record the placebo under the C2 reference rule C2 made the campaign path honour the declared candidate_references, so the six upgraded placebo turbines no longer serve as each other's references. Re-runs both placebo campaigns and records the result as CF6, controlled against the same driver run from the pre-C2 commit a1f96af. The control reads +0.1485%, matching the +0.148% CF3 recorded, so the comparison is a like-for-like A/B of the reference rule. power_model's prepost farm error falls from +0.148% to +0.039%, while its mean per-turbine absolute error rises slightly (0.481 -> 0.515 pp): the headline gain is cancellation across turbines, not better individual estimates. The mechanism is explicitly not established. Truth is still exactly 0.0 in both modes. CF1 is marked superseded; CF2-CF5 carry a note that they predate the reference rule and are not reproducible as written, their sweeps not having been re-run. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ECPVDa4P3dbiWYCQKbz18D * Re-record the toggle compare baseline (stale since #126/#128) The portable baseline dated from de85f84 (2026-07-15). #126 (V1 toggle specialist) and #128 (custom filter to toggle specialist) then added ~270 lines to toggle_specialist.py without re-recording it, so toggle_specialist had been reading MOVED on every run since: 67 of 84 cells, worst 1.52 pp, against a band of 1e-5 pp. The drift is confined to the conditional per-bin cells (condition == "power"); no overall row moved, so the headline P50 was never affected. That is consistent with those PRs reworking the conditional path deliberately and forgetting the re-record. Not caused by the C2 work on this branch: an A/B with the context calls neutralised is bit-identical, and a control sweep on the pre-C2 commit a1f96af produces the same 19 moved cp_0pct cells to the digit. Recorded from a clean tree at 2e13ac3 on linux, then verified with a fresh compare: power_model UNCHANGED (max delta 0.0531 pp, band 0.1) and toggle_specialist UNCHANGED (max delta 5e-07 pp, band 1e-5) — its effectively bit-exact band is usable again as the sharp instrument for small regressions. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ECPVDa4P3dbiWYCQKbz18D --------- Co-authored-by: Claude Opus 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.
Complete issue C1 (see
issues_campaigns.md)