Write a Sienna investments portfolio from an expansion plan by thomaschristopherking · Pull Request #11 · transition-zero/tz-oss-interop

thomaschristopherking · 2026-09-08T23:56:51Z

TL;DR

The two legs below this one carry a PLEXOS expansion plan as far as PyPSA and no further:
there was no Sienna investments vocabulary, no translation, no sink and no pipeline, so an
extendable network had nowhere to go. plexos-to-sienna-investments now writes a
SiennaSchemas PortfolioDocument beside the base system it expands, and
pypsa-to-sienna-investments does the same for a PyPSA network that states its own
expansion.

Third of four for transition-zero/tz-infra-interop#116,
stacked on #10. The fourth adds the AEMO case study.

Files to review (35, +4350 / -53):

File	Why
`docs/translation_mappings/translation-from-plexos-to-sienna-investments.md` (start here)	The whole mapping in PLEXOS words, and where the hub loses something.
`interop/plugins/shared/sienna_investments_constants.py` (new)	The vocabulary and one destination schema per type.
`interop/plugins/shared/pypsa_sienna_investments_translations/` (new)	One module per destination type, on the existing `Translation` runner.
`interop/plugins/sinks/emit_sienna_portfolio.py` (new)	Assembles the document and resolves references. It decides nothing.
`interop/pipelines/pypsa-to-sienna-investments.yaml` (new)	One run, two documents.
`tests/features/plexos_to_sienna_investments.feature` (new)	The chain end to end over the synthetic PLEXOS fixture.

Important

The Investments/ namespace is not what the ticket was scoped against. It has 21
types, not 26, and no Node or Zone; ExistingCapacity does not exist and
ExistingDevices is a supplemental attribute, not a component; TechnologyFinancialData
has no interest_rate; and CarbonCaps names no members and no region. Each of
those decides part of the mapping, and each is stated in
docs/specs/2026-09-08-plexos-sienna-investments-design.md.

Why

plexos-to-sienna writes a PowerSystems.jl System, in which every capacity is a number
taken from the source. There is no investment cost field on a ThermalStandard and no build
variable in a dispatch template, so an expansion plan translated that way loses the plan. The
destination for one is a separate schema with separate types, so this is a new destination
rather than a flag on the dispatch path.

How

One run writes both documents, because the portfolio depends on the base system:
ExistingDevices.existing_devices, RetirementPotential.eligible_generators and
TopologyMapping.buses are all lists of names in it. The pipeline therefore runs the
operations steps and the new investments steps over one network, and the operations steps use
the rule the branch below already has — an extendable component with no capacity a build
cannot take away is not a base-system component.

Two mappings have no field to land in, and the document states both:

WACC is written as all-equity financing. TechnologyFinancialData requires all six of
its fields and has none for a weighted average cost of capital, so the portfolio states
debt_fraction: 0, debt_rate: 0, tax_rate: 0 and return_on_equity equal to the
discount rate. With no debt the weighted average cost of capital is the return on equity,
so the number the model states is the number the solver uses.
A scoped constraint is left out. CarbonCaps names no members and no region, so a
constraint over a named subset cannot be written as one without silently widening it to the
whole system. Only a constraint covering every component becomes a cap; any other is
recorded and warned about.

Reviewer notes

A candidate the network prices nothing for is left out. An unset overnight_cost or
discount_rate used to be filled with zero, which builds for free and discounts at nothing;
a max_hours of zero gave a technology no energy. Each is now a drop with its own reason in
decisions.md, because zero is a number nobody meant.
ExistingDevices and RetirementPotential group by carrier. A technology's existing
fleet is the base-system components sharing its PyPSA carrier. No source states that
grouping; it is the translator's rule and the document says so.
base_year is a step parameter, not a sink parameter. No source states the dollar year.
Putting it on the sink would make the sink decide a component's field, which every sink in
this repository refuses to do. It defaults to 2020. A composed run cannot reach it, because
a chain prompts for no interior step; the case study in the branch above says so.
Component ids come from one counter across every portfolio type, and
supplemental-attribute ids from another, because an association identifies by
(component_id, attribute_id) and each namespace must be unique on its own.
A candidate with a non-finite p_nom_max or lifetime is left out. PyPSA's defaults
for both are infinity, and JSON has no way to write it.
The sink omits a null field at every depth rather than writing null, which is what
lets one struct column carry a discriminated cost type and still emit exactly the fields
each variant names.
Which extension records a hop consumed now lives on State. The operations step used to
import the investments step to know whether to report a record as dropped; the run reports
it once per hop instead, after the last step.

Tests

Nineteen new scenarios. plexos_to_sienna_investments.feature drives the whole chain over the
synthetic PLEXOS fixture: a candidate generator, a candidate battery, an existing plant
reaching ExistingDevices, a whole-model constraint becoming a CarbonCaps, a scoped
constraint left out. pypsa_to_sienna_investments/portfolio.feature drives the second leg
alone from a hand-built network, and covers each drop and each cost type. Full suite (624),
ruff, mypy, the import contracts and all six lints are green.

CI does not run on this pull request: the workflows trigger on a pull request into main,
and this one targets the branch below it in the stack.

Follow-up

The AEMO case study, and the gap-analysis entries for what the portfolio leaves out:
transport technologies, a technology's requirements list, target_year and
max_tons_mwh, and a storage technology's charge and energy capital costs.

Open workspace in Conductor

A portfolio is a different document from a system, with types of its own, so the names it is made of get their own module rather than joining the operations vocabulary. Every field name, every required flag and every nested struct here comes from the Investments namespace of SiennaSchemas. Several of them differ from what the operations side would suggest and are spelled out in the module docstring: a technology's region is a list of integer ids, its capital cost is a value curve inside a wrapper, and the three attributes naming things in the base system are supplemental attributes in a flat array rather than components. Beside it sits one translation module per destination type. A candidate becomes a technology named after it, priced by the overnight cost PyPSA annuitises and bounded by the capacity a build may reach. TechnologyFinancialData requires all six of its fields and has none for a weighted average cost of capital, so the financing is written as all equity: no debt, no tax, and a return on equity equal to the discount rate the network states. With no debt the average equals the return on equity, so the number the source states is the number the solver uses, and the report carries it as a derivation rather than a default. A constraint the sidecar carries becomes a carbon cap only where its members cover every component of the network. CarbonCaps names no members and no region, so a cap written from a constraint over part of a model would hold all of it, which is a different problem from the one the model states. A constraint over a subset, one holding its sum to something other than a ceiling, and one stating no right-hand side over the whole run are each left out, recorded through the reporting port and warned about. Three small additions elsewhere serve this. A row-position id may now start from a number other than one, because the three supplemental attributes share the counter their flat array numbers them by. An extension reader may mark a kind as read by another step of the same pipeline, so a hop reports as dropped only what nothing beside it consumes. And a carrier mapping now answers which Sienna type it sends a carrier to, which is what a technology's power_systems_type is. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Thomas C. King <thomas.k@transitionzero.org>

A portfolio names the components of a base system in three places, so without that system it points at nothing. One pipeline therefore runs the operations steps and the new investments step over one network and writes both documents. The operations steps already leave out a component whose capacity a build has yet to decide, so a candidate reaches the portfolio and the base system holds only what already runs; a plant that both runs and may expand reaches both. The step reads the network a second time and writes what the operations steps had no home for. It takes the size of one unit, the technical life and the planned retirement year off the extensions sidecar, because PyPSA has no column for any of the three, and it takes the build year off the network itself. Where it runs alongside the operations step, that step now says so rather than reporting the same records as dropped. The base year every cost is quoted in is a year no source states. It is a parameter of the step, defaulting to 2020, which is the year SiennaSchemas itself defaults a construction year to. It reaches both the portfolio's own financial data and each technology's. The sink makes no decisions: it groups the technology tables by type name, flattens the three attribute tables into the one array the document carries them in, resolves each region name to the base system's area id and each association's component name to that component's id, and leaves out a field no table wrote so the schema's own default applies rather than a null a reader has to interpret. The tests drive both routes from the user surface: the second leg alone over a hand-built network, and the whole chain from a PLEXOS model through the hub. The vocabulary they assert a portfolio with lives in the testing harness, and it walks the association table the way a consumer does rather than reaching into the flat array by position. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Thomas C. King <thomas.k@transitionzero.org>

The mapping document a user reads before running the chain, written in PLEXOS words rather than PyPSA ones. It states what each class becomes, field by field, which values are direct and which are derived, and which the user's own mappings file decides. Two rules get sections of their own, because both change what a solver is given. A WACC becomes an all-equity cost of capital, since the schema requires six financial fields and has none for a weighted average. A constraint over part of a model becomes nothing, since a cap in a portfolio names no members and would therefore hold more than the model meant. It also states where the route through PyPSA loses something and what carries it instead: the size of one unit, the technical life and the year a dated Units falls to zero all cross the hub in the extensions sidecar, and a run without that sidecar takes the schema's own defaults for all three. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Thomas C. King <thomas.k@transitionzero.org>

coderabbitai · 2026-09-08T23:56:58Z

Important

Draft PR not reviewed

Draft PRs are not automatically reviewed by default.

Trigger a manual review

To automatically review draft PRs, update your CodeRabbit configuration:

reviews:
  auto_review:
    drafts: true

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

thomaschristopherking

Review by /review-fix-cycle.

Labels follow Conventional Comments. (blocking) means the reviewer would not merge without it settled. (non-blocking) means it can land after.

The operations step imported the investments step and marked every generator and storage record read whenever that step ran beside it. A step had to know another step's name to decide what it reported, and the wholesale marking hid a real loss: a candidate the investments step then dropped for an unnamed carrier lost its sidecar values with no dropped event anywhere. The consumption record now lives on State. Every step builds its reader through State.extension_reader, so one record covers a whole hop, and the pipeline runner reports what none of them asked for once the last step has run. A hop that builds no reader reports nothing, which leaves the results pipelines, whose one caller reaches for a known record through record_for, saying what they said before. ExtensionReader.mark_read went with the conditional that called it, since a shared consumption record is what a split hop needed it for. Addresses r1-01 and r1-28. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Thomas C. King <thomas.k@transitionzero.org>

…st each one holds PyPSA defaults overnight_cost and discount_rate to NaN and max_hours to one hour of the power rating. The translator filled all three with zero, so a candidate that stated none of them reached the portfolio building for free, discounting at nothing, and holding no energy, with nothing in the report to say so. Each is now a drop with its own reason: the supply and storage paths share one set of expansion skips, and storage adds the energy rule the operations leg already applies to the same row. GenericOperationCost is chosen by cost_type, and only ThermalGenerationCost states a start-up and a shut-down cost. A Polars struct column holds one set of fields for every row, so the two stay null off the thermal branch and the sink now drops a null at every depth of a nested struct rather than only at the top, which leaves a renewable or hydro cost with exactly the fields its variant defines. A carrier the mappings file sends to HydroDispatch takes the HYDRO_GEN cost type rather than RENEWABLE, and a demand requirement names whichever of the two load types the base system holds its load as. A supplemental attribute association names the component it describes by id alone, so the portfolio's components now take their ids from one counter rather than each type numbering from one. The two candidate paths were the same flow written twice, and the three supplemental attributes were described by five structures a reader had to join on an enum key. One record now says everything each candidate table and each attribute does differently, and the skip reports both candidate tables share live beside the scope skips. The prose the review named as narration, inventory or argument is gone, along with a private module's __all__, two package exports nothing outside the package read, a one-line helper with one caller, and a step vocabulary no scenario uses. Addresses r1-02 through r1-05, r1-11 through r1-17, and r1-20 through r1-41 other than the shared reference check. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Thomas C. King <thomas.k@transitionzero.org>

The portfolio sink held a character-for-character copy of the reference check the system sink already had. It now lives in the module that says what files one Sienna system consists of, which the portfolio sink already reads the base system filename from, and both sinks call it. The two docstrings under the name and id column constants said what the constants say. Addresses r1-26 and r1-35. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Thomas C. King <thomas.k@transitionzero.org>

thomaschristopherking

Review by /review-fix-cycle.

Labels follow Conventional Comments. (blocking) means the reviewer would not merge without it settled. (non-blocking) means it can land after.

The candidate scope kept a row whose carrier the user mappings file sends to a Sienna type that kind of candidate never becomes, so an extendable storage unit mapped to a generator type was written as a StorageTechnology carrying a power_systems_type no storage technology holds, while the base system left the same row out. The scope skips now take the carriers of the types the kind may become and drop the rest, naming each one in the report, and the two candidate kinds declare the base system types they stand for. The framework a hop staged its records under moves off the consumption record and onto the report the run writes, since every step passed the same value and a second step passing another changed nothing. The run reports the unread records inline rather than through a one-line helper. The investments step reads its constraints through the extension reader, which a new read_all gives it, so the consumption record sees them instead of a raw read of the staged records that another step had to relay. Four places repeated the same translate, finalise, store and count, so they all go through one method that cannot forget to take the ids the table used. A generator and a storage unit may share a name, so the build and retirement years now belong to the source table they were read from rather than to one pair of maps the storage pass overwrote. The energy capacity limits event names the p_nom_min it multiplies as well as the p_nom_max. Two feature scenarios cover the new drop and the shared name. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Thomas C. King <thomas.k@transitionzero.org>

thomaschristopherking

Review by /review-fix-cycle.

Labels follow Conventional Comments. (blocking) means the reviewer would not merge without it settled. (non-blocking) means it can land after.

… a cap can bound The fleet lookup for a storage candidate read the base names of EnergyReservoirStorage alone, while a storage unit is accepted as a candidate against that type and HydroDispatch together. A StorageUnit whose carrier the mappings file sends to HydroDispatch therefore found no devices of its own carrier, and the portfolio named neither existing devices nor a retirement potential for it, with nothing reported. The per-kind values the fleet path hard-coded now sit on _CandidateKind, so the fleet, the years and the technologies are one loop over the two kinds and each reads the same base types the candidate was accepted against. The build year comes off each kind's own column rather than the generator's for both, and the two tuples of base types are named alike. The whole-model test for a constraint counted every generator and storage unit of the staged network, including a load-shedding generator an earlier hop of this translator writes and the base system then drops. A constraint naming everything the two documents hold looked scoped, and its cap was left out. The model is now what the base system and the portfolio hold between them: a component neither document carries emits nothing a cap could bound. The portfolio sink reads whether a table carries a region name off the table rather than off a list of the types that do, so the list cannot disagree with the schemas, and a CarbonCaps row no longer passes through code naming SupplyTechnology's columns. The two shared column names sit beside SIENNA_NAME_COLUMN and the three column classes take them from there. Alongside these: the portfolio's financial data takes the schema the constants already declare, the storage technology's efficiency takes sienna_constants' own EFFICIENCY_DTYPE, an unfilled outage_factor is typed like its four never-mapped siblings, a one-line id counter helper is inlined, and a docstring drops a sentence about a reader the State now keeps. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Thomas C. King <thomas.k@transitionzero.org>

thomaschristopherking

Review by /review-fix-cycle.

Labels follow Conventional Comments. (blocking) means the reviewer would not merge without it settled. (non-blocking) means it can land after.

SupplyTechnology, StorageTechnology and DemandRequirement each carried a REGION attribute aliasing SIENNA_REGION_COLUMN, but nothing reads it: the sink resolves region_name to the integer list directly. Removing the three attributes leaves SIENNA_REGION_COLUMN with no remaining user in this file, so its import goes too. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Thomas C. King <thomas.k@transitionzero.org>

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Thomas C. King <thomas.k@transitionzero.org>

thomaschristopherking · 2026-09-10T09:44:18Z

review-fix-cycle heartbeat: 2026-09-10T15:58:30Z

… table both answers r5-05: the fleet lookup matched a base-system device by name alone against every type the candidate was accepted against, and _GENERATOR_BASE_TYPES holds HydroDispatch, which only a StorageUnit becomes. A Generator and a StorageUnit may share a name, so a solar SupplyTechnology took a hydro storage unit as its existing device and as a device a plan may retire. Each kind now names the base system types its own PyPSA class writes, and the scope skips keep the types the candidate is accepted against. A new scenario builds a hydro StorageUnit and an extendable solar Generator of one name. r5-02: _technologies joined the source table and the destination table by row position, and _Candidates carried both only for that. The destination name column is a copy of the source name column, so one table answers both questions and the pair goes. r5-04: _write_table advanced the id counter its caller had already read, so a caller that built its translations first got duplicate ids. It now takes the builder and both reads the counter and advances it. r5-03: the with_fuel flag could not change a result. Every thermal carrier mapping names ThermalStandard, and a storage candidate whose carrier does not name a storage type is dropped before this line, so the fuel map is always silent for a storage row. r5-01: the runner joined two fields of State to report the unread sidecar records, and None silently meant that no step built a reader. State answers the question itself now. r5-10 and r5-11: the portfolio document helpers and two of the portfolio assertion steps were copies of the system ones. A portfolio keys its components by type the way a system does, so the system pair reads both, and the two steps take the portfolio phrasing as a second parser. r5-13, r5-14 and r5-17: six class docstrings repeated the class name and the SiennaSchemas source the module docstring gives, one docstring listed the rules the code below shows, and one comment argued why the other spans are absent. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

thomaschristopherking

Automatic review-and-fix, round 5

Four reviewers ran over git diff origin/issue-116-plexos-dated-units-and-constraints...HEAD. This run acts on a blocking comment, and on a comment from the pruning or the pointless reviewer that takes lines away. Every other comment is deferred, not rejected.

Acted on (9), in d632c4a

id	reviewer	where	claim
r5-01	thermo	`interop/core/runner.py:187`	The runner joins two fields of `State` to report the unread sidecar records, and `None` silently means that no step built a reader.
r5-02	thermo	`pypsa_to_sienna_investments_map_technologies.py:611`	`_technologies` joins the source table and the destination table by row position, and `_Candidates` exists only to carry both.
r5-03	thermo	`pypsa_to_sienna_investments_map_technologies.py:385`	The `with_fuel` flag cannot change the result, because a storage candidate never reaches this line with a thermal carrier.
r5-04	thermo	`pypsa_to_sienna_investments_map_technologies.py:394`	`_write_table` advances the id counter that its caller already read, so one rule sits in two places.
r5-05	correctness	`pypsa_to_sienna_investments_map_technologies.py:606`	A `SupplyTechnology` can take a `StorageUnit`'s base system device as its existing fleet, because the fleet matches a device by name alone.
r5-10	pointless	`builders/sienna_documents.py:305`	`portfolio_components_of_type` and `find_portfolio_component` are copies of the two system helpers.
r5-11	pointless	`steps/sienna_portfolio.py:29`	Two portfolio assertion steps are copies of the two system steps.
r5-13	pruning	`sienna_investments_constants.py:133`	Six class docstrings say the class name again, plus the SiennaSchemas source the module docstring gives.
r5-14	pruning	`_shared.py:77`	The second paragraph lists the rules the code below shows, then argues why each gets its own report.
r5-17	pruning	`_carbon_caps.py:60`	The comment runs to three sentences, and the last two argue why the other spans are absent.

r5-05 is a defect. A Generator and a StorageUnit may share a name, and _GENERATOR_BASE_TYPES holds HydroDispatch, which only a StorageUnit becomes. A new scenario builds a hydro StorageUnit and an extendable solar Generator of one name; without the fix the solar technology's existing_devices is ["OldSolar", "Shared"].

Deferred (8) — each one is minor or a nit, and each one adds lines or leaves the line count as it is.

r5-06 …map_technologies.py:264 — the step falls back to DEFAULT_BASE_YEAR for a params object the runner always builds from params_schema.
r5-07 …map_technologies.py:572 — _base_load_types and _base_names are the same loop.
r5-08 _shared.py:108 — a candidate on an AC bus that names no area becomes a technology with no region, and no rule drops it.
r5-09 runner.py:187 — the report of an unread extension record carries no step name, so the Step column of decisions.md is empty.
r5-12 _carbon_caps.py:99 — only one of the three CARBON_CAP_SKIPS rules has a scenario.
r5-15 extensions.py:541 — "consumption record" names an ExtensionConsumption, but "record" means a staged sidecar record in this file.
r5-16 _existing.py:224 — "the same counter" names no counter.
r5-17 is acted on; r5-15 and r5-16 are the two pruning nits left.

Three further comments were dropped as repeats of comments the earlier rounds already answered: emit_sienna_portfolio.py:187 (r3-05), pipeline.py:78 (r3-12, r4-01) and emit_sienna_portfolio.py:208 (r3-14).

625 tests pass, mypy is clean, and the nine repository lints pass.

… order r6-03: the fleet lookup grouped the base-system devices by carrier alone, so a technology took the plant of its carrier from every region. A candidate in one region named a plant in another as an existing device and as one a build may retire, and two candidates of one carrier in different regions each counted every device. The lookup keys on the carrier and the area now, and CandidateTechnology carries the region the enrichment already read off the bus. A new scenario builds two regions, each with a solar plant, and one candidate. r6-01: the step kept its own order of the three supplemental attribute types, and the sink read a second order from SUPPLEMENTAL_ATTRIBUTE_ORDER. An attribute takes its id from its place in the flat array, so the two orders had to agree and no code held them together. The step reads the constant now, which is what the constant's own comment already claimed. r6-02: enrich_from_names repeated what Polars replace_strict does, and it rebuilt the table once per column. The four candidate columns and the two demand columns are one with_columns batch each. r6-11: _STORAGE_FLEET_TYPES was another name for _STORAGE_BASE_TYPES. r6-12: the portfolio "without field" assertion step repeated the system one. The system step takes the portfolio sentence as a second parser and the dotted field path, which every current caller already satisfies. r6-05 to r6-08 and r6-10: five docstrings and one module docstring line that say the name again, or repeat a fact the constants file and the code below both state. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

thomaschristopherking

Automatic review-and-fix, round 6

Acted on (10), in 543cc41

id	reviewer	where	claim
r6-01	thermo	`…map_technologies.py:197`	The step keeps its own order of the three supplemental attribute types, and the sink reads a second order from the constants file.
r6-02	thermo	`_shared.py:199`	`enrich_from_names` repeats what the Polars expression `replace_strict` already does.
r6-03	correctness	`…map_technologies.py:594`	`_carrier_groups` groups the base-system devices by carrier alone, so a technology takes the devices of its carrier from every region.
r6-05	pruning	`_shared.py:217`	The `capacity_limits_struct` docstring says again what the name says.
r6-06	pruning	`_shared.py:227`	The `financial_data_struct` docstring says again what the name says.
r6-07	pruning	`_shared.py:145`	The `build_expansion_skips` docstring copies its sibling and carries a count a new rule makes wrong.
r6-08	pruning	`…map_technologies.py:187`	The `_Attribute` docstring lists the fields the five lines below it declare.
r6-10	pruning	`_carbon_caps.py:3`	The second line of the module docstring repeats `SiennaCarbonCapsCol`.
r6-11	pointless	`…map_technologies.py:119`	`_STORAGE_FLEET_TYPES` is only another name for `_STORAGE_BASE_TYPES`.
r6-12	pointless	`steps/sienna_portfolio.py:29`	`assert_portfolio_component_field_absent` repeats the system step.

r6-03 is a defect. A portfolio groups its technologies by region, and the fleet ignored the region: a candidate in the North named a South plant of the same carrier as an existing device and as one a build may retire. A new scenario builds two regions, each with a solar plant, and one candidate; without the fix the candidate's existing_devices is ["OldSolar_North", "OldSolar_South"].

Deferred (2)

r6-04 sienna_investments_constants.py:321 — ramp_limits and time_limits are typed pl.Utf8 while sienna_constants.py states both as a struct. No output carries the wrong type today, because finalise writes a null.
r6-09 …map_technologies.py:616 — the _technologies docstring names a fleet the function does not return.

626 tests pass, mypy is clean, and the nine repository lints pass.

r7-01: a source states whether its expansion plan has to meet a constraint, and the carbon cap rules never read it. A whole-model constraint the plan need not meet became a portfolio cap, so the run bounded a problem the model leaves free and decisions.md named no drop. The source table carries the flag now and a fourth rule drops the constraint, reports it and warns. A constraint that states nothing about the plan stays, which is the behaviour every source without the flag already had. A new scenario states the flag as false. r7-02: the mapping document said a fleet matches on the carrier alone, which the region rule made wrong. It states the carrier and the region now, and it lists the fourth constraint drop. r7-07: the TopologyMapping event built a DestinationField by hand while the module already holds the partial that builds it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

thomaschristopherking

Automatic review-and-fix, round 7

Acted on (3), in 3f64a64

id	reviewer	where	claim
r7-01	correctness	`_carbon_caps.py:94`	A constraint the source keeps out of the expansion plan still becomes a `CarbonCaps`, so the portfolio holds a limit the model does not state.
r7-02	thermo	`translation-from-plexos-to-sienna-investments.md:176`	The document says a fleet matches on the carrier alone, but the code matches on the carrier and the region.
r7-07	pointless	`_topology.py:62`	The `destinations` block of `TOPOLOGY_BUSES` builds a `DestinationField` by hand while the module already holds `_dest`.

r7-01 is a defect. ConstraintExtension.applies_to_expansion_plan carries the PLEXOS Include in LT Plan flag, and no rule read it: a whole-model constraint with the flag false became a portfolio-wide cap, with no drop recorded. A fourth rule drops it, reports it and warns. A constraint that states nothing about the plan stays, which is what every source without the flag already got. A new scenario states the flag as false; without the rule the run writes one CarbonCaps instead of none.

r7-02 was made wrong by round 6's own region fix, so correcting it finishes that change rather than acting on a minor comment.

Deferred (4)

r7-03 sienna_investments_constants.py:300 — CAPACITY_LIMITS_DTYPE is a second name for MIN_MAX_DTYPE.
r7-04 _shared.py:229 — "from the two PyPSA states" has no noun after "two".
r7-05 _supply.py:300 and r7-06 _storage.py:352 — "the two the caller's numbers decide" names neither the two translations nor the two numbers.

627 tests pass, mypy is clean, and the nine repository lints pass.

r8-02: the whole-model test compared member names alone. Two classes may hold an object of one name, so a constraint over a node, a line or an emission that carries a generator's name counted against that generator, and a node flow limit became a portfolio-wide carbon budget the model never stated. A model component now travels with the PyPSA class that wrote it, and a member counts only where the class matches too. A new scenario names a bus and a generator alike. This trades one failure for a smaller one. A sidecar carries the source's own class name, so a whole-model constraint that names an object the network states under another class name is now short of the model and is reported as scoped rather than written. Writing a cap nobody stated is the worse outcome of the two: a drop reaches decisions.md and the log. r8-01: _build_fleet read each kind's source table twice, because _years and _fleet_groups each looked it up, checked it for None and collected it. One collect serves both, and each reads the rows it is given. r8-03: the _existing.py module docstring said a technology's carrier alone picks its fleet, which the region rule made wrong. The function's own docstring already states the rule. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

thomaschristopherking

Automatic review-and-fix, round 8

Acted on (3), in d657c71

id	reviewer	where	claim
r8-01	thermo	`…map_technologies.py:509`	`_build_fleet` reads the same PyPSA source table twice for each kind, because `_years` and `_fleet_groups` each look it up, check it for `None` and collect it.
r8-02	correctness	`_carbon_caps.py:128`	The whole-model test compares member names only, so a constraint over a node, a line or an emission that shares a name with a generator becomes a portfolio-wide `CarbonCaps`.
r8-03	pruning, pointless	`_existing.py:4`	The module docstring says the carrier alone decides a technology's fleet, but the code matches on the carrier and the region.

r8-02 is a defect. The reviewer ran it: a model whose only generator is Bayswater, plus a constraint over the node Bayswater, gave a CarbonCaps of 20 Mt over the whole portfolio, with no warning. A model component now travels with the PyPSA class that wrote it, and a member counts only where the class matches too.

This fix trades one failure for a smaller one, and you should know which. A sidecar carries the source's own class name — the PLEXOS leg writes Battery where the network holds a StorageUnit. So a whole-model constraint that names an object the network states under another class name is now short of the model, and is reported as scoped rather than written. Writing a cap nobody stated is the worse of the two outcomes, because a drop reaches decisions.md and the log while a wrong number reaches neither. The lasting fix is for the sidecar to state member_class in the hub's own vocabulary, which is the parent branch's contract, not this one's.

Deferred (1)

r8-04 _topology.py:43 — TOPOLOGY_BUSES holds an identity expression that changes no column.

628 tests pass, mypy is clean, and the nine repository lints pass.

r9-01: PyPSA lets a network state p_nom_min above p_nom_max, and no rule read the pair. The technology then stated a capacity_limits.min above its capacity_limits.max, which no capacity can meet, and decisions.md named no drop. A storage candidate inverted its discharge limits and its energy limits together, because both read the same struct. build_expansion_skips takes the floor column now and drops the candidate, reports it and warns. A new scenario states a floor of 600 MW and a ceiling of 500 MW. r9-05: the fleet lookup parameter still named the carrier alone after the region rule keyed it on the carrier and the region. r9-03 and r9-04: two docstrings that repeat their own module docstring. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

thomaschristopherking

Automatic review-and-fix, round 9

Acted on (4), in 3ebd758

id	reviewer	where	claim
r9-01	correctness	`_shared.py:151`	A candidate whose `p_nom_min` is above its `p_nom_max` becomes a technology whose `capacity_limits.min` is above its `capacity_limits.max`, with no drop and nothing in `decisions.md`.
r9-03	pruning	`_storage.py:133`	The `storage_capital_cost_struct` docstring repeats the module docstring of the same file.
r9-04	pruning	`steps/sienna_portfolio.py:5`	The last sentence of the module docstring repeats its first line.
r9-05	pointless	`_existing.py:67`	The parameter name `devices_by_carrier` states one key part, but the body reads a key of two, the carrier and the region.

r9-01 is a defect, and CLAUDE.md names this exact case: a component whose minimum sits above its ceiling must be left out and recorded. PyPSA lets a network state the pair either way round, and a storage candidate inverted its discharge limits and its energy limits together because both read the same struct. A new scenario states a floor of 600 MW and a ceiling of 500 MW. A PLEXOS-sourced run cannot reach this rule, because its floor is Max Capacity × Units and its ceiling adds the units a build may add, so the mapping document needs no change.

r9-05 is a name my round 6 region fix left describing half its key, so correcting it finishes that change.

Deferred (1)

r9-02 _shared.py:145 — build_scope_skips and build_expansion_skips build the same skip partial from the same three values.

629 tests pass, mypy is clean, and the nine repository lints pass.

r10-03: _source_rows held two lines behind one call site, while _candidate_rows wrote the same get and collect pair by hand. One form of the rule is enough, so the fleet reads the rows where it needs them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

thomaschristopherking

Automatic review-and-fix, round 10

The correctness reviewer found no defect it could make fail with a concrete case. It ran the investments scenarios, the PLEXOS chain scenarios and mypy, and checked the new document against ALWAYS-001.

Acted on (1), in 3a9c292

id	reviewer	where	claim
r10-03	pointless	`…map_technologies.py:594`	`_source_rows` is a two-line function with one call site, and `_candidate_rows` writes the same two lines by hand.

Deferred (2)

r10-01 _existing.py:137 — three call sites repeat one id-note string literal, while the component modules import the same kind of note from _shared.py.
r10-02 _financials.py:24 — "the report names it after the field it is" uses shorthand the reader cannot decode.

629 tests pass, mypy is clean, and the nine repository lints pass.

r11-02 and r11-03: one comment repeated the _Numbering docstring in the step that holds the counter, and one repeated the _named_years docstring beside the code that converts the shape. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

thomaschristopherking

Automatic review-and-fix, round 11

Acted on (2), in f3e8ced — two comments that repeat a fact another file states: _shared.py:42 repeats the _Numbering docstring, and emit_sienna_portfolio.py:38 repeats the _named_years docstring beside the code that converts the shape.

Deferred, and this one needs your decision — r11-01, _carbon_caps.py:133, major

A constraint that names every object of a PLEXOS model loses its CarbonCaps as soon as the model holds a battery or a pumped-storage plant, because the member class comes from the PLEXOS vocabulary and the model class comes from the PyPSA one.

This is the cost of round 8's fix, now measured. The reviewer ran it: a PLEXOS model with GasPlant, candidate REZ_Solar and OldBattery, and a constraint naming all three, writes one EnergyReservoirStorage into system.json and zero CarbonCaps into portfolio.json. The same model without the battery writes the cap. The drop is reported and the run completes, but the reason it gives ("weight a named subset of the model") is not the real one.

Round 8 traded a silent wrong value for a reported drop, which is the trade CLAUDE.md asks for. The lasting fix is not in this branch: member_class is defined as the source's own class name, and making the two vocabularies agree means the PLEXOS hop must state a member in the vocabulary of the network it writes. A static class map does not do it, because a pumped-storage turbine is a PLEXOS Generator and a PyPSA StorageUnit, so the hop has to carry the provenance of each object it wrote. That changes ConstraintMember, plexos_pypsa_translations/_constraints.py, the plexos_to_pypsa scenario that asserts member_class is "Generator", and translation-from-plexos-to-pypsa.md — all of which belong to the parent branch's sidecar contract. Rough size: one provenance map threaded from component mapping into constraint mapping, about 40 to 60 lines across three files, plus a scenario and the document.

I did not make that change unattended, because which vocabulary the hub's member_class speaks is a contract decision rather than a fix.

Also deferred (2)

r11-04 _shared.py:63 — three constants take a name the sibling module pypsa_sienna_translations/_shared.py gives to a different sentence, so an import of the wrong one passes mypy and changes what decisions.md says.
r11-05 portfolio.feature:64 — one comment calls system.json "the operations system" where everything else calls it "the base system".

The pointless reviewer found nothing: it swept every new definition for call sites, checked all 31 __all__ exports, and confirmed no two scenarios repeat.

629 tests pass, mypy is clean, and the nine repository lints pass.

r12-01: the comment named an internal struct, which a specification of what the user sees does not cover, and the two steps below it already assert that the document writes neither field. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

thomaschristopherking

Automatic review-and-fix, round 12

Three of the four reviewers found nothing.

correctness read the whole diff and the code it depends on, ran the investments and PLEXOS chain scenarios, and tested seven hypotheses to destruction: an empty carrier mapping, an empty load-type list, a NaN build year, an infinite lifetime reaching a cast, a step losing the shared consumption record, a destination column leaking through finalise untranslated, and _stated missing a nested value. Each one is closed by code that already handles it.
thermo confirmed no file crosses 1000 lines, that the new sink adds no near-duplicate of the reference check, and that the repeated pipeline YAML follows an established pattern rather than a regression.
pointless ran its own AST sweep for small single-call helpers and zero-use names, and found nothing new.

Acted on (1) — portfolio.feature:48: a comment that names an internal struct and states an absence the two steps under it already assert.

629 tests pass, mypy is clean, and the nine repository lints pass.

r13-01: the validate_refs docstring repeated the function name and the message the raise below it already carries. r13-02: four banner comments in the technologies step named the method that follows each one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

…d may add r13-03: STORAGE_SKIPS tested max_hours only for a value above zero, and an infinite value passes that test. The energy limits multiply the power limits by max_hours, so the minimum came out at 0.0 times infinity, which is NaN, and the maximum came out at Infinity. The sink then wrote the literals NaN and Infinity, which no strict JSON reader accepts, and the minimum was a number the network never stated. An infinite max_hours survives the netCDF round trip, so a real network can state one. A fifth rule drops the candidate, reports it and warns. A new scenario states max_hours as inf, and the storage unit step of the test harness accepts that word. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

thomaschristopherking

Automatic review-and-fix, round 13

The correctness reviewer stopped on an API rate limit on its first attempt; the retry completed and found a defect.

Acted on (3), in 6a55d3f and fa3444e

id	reviewer	where	claim
r13-03	correctness	`_storage.py:103`	A storage candidate whose `max_hours` is infinite passes the skip rules and writes NaN and Infinity into `portfolio.json`.
r13-01	pruning	`_sienna_files.py:51`	The `validate_refs` docstring repeats the function name and the error message below it.
r13-02	pruning	`…map_technologies.py:288`	Four banner comments repeat the name of the method below each one.

r13-03 is a defect. STORAGE_SKIPS tested max_hours only for a value above zero, and infinity passes that test. The energy limits multiply the power limits by max_hours, so the minimum came out at 0.0 * inf, which is NaN, and the maximum came out at Infinity. The sink wrote the bare literals NaN and Infinity, which no strict JSON reader accepts. I confirmed an infinite max_hours survives the netCDF round trip, so a real network can state one. A fifth rule drops the candidate, reports it and warns; without the rule the new scenario writes one StorageTechnology instead of none.

The thermo reviewer's one comment repeated validate_refs being called once per association row, which an earlier round already answered, so it was dropped. The pointless reviewer found nothing.

630 tests pass, mypy is clean, and the nine repository lints pass.

r14-01: build_expansion_skips tested the lifetime for a finite value alone, so 0.0, 0.6 and -5.0 all kept the candidate. The capital recovery period is a whole number of years, so a lifetime of 0.6 truncated to zero and a lifetime of -5.0 wrote -5. No annuity can recover an overnight cost across zero years or fewer, and the portfolio stated the number silently. A sixth rule drops the candidate, reports it and warns. A new scenario states a lifetime of 0.5. r14-02: the mapping document named one storage-hours drop, and round 13 added a second. r14-03: the new scenario's description stated the same rule as the scenario below it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

thomaschristopherking

Automatic review-and-fix, round 14

Acted on (3), in 48036c3

id	reviewer	where	claim
r14-01	correctness	`_shared.py:175`	A candidate whose lifetime is below one year passes every skip rule and writes a capital recovery period of zero.
r14-02	thermo	`translation-from-plexos-to-sienna-investments.md:114`	The mapping document names one storage-hours drop, but `STORAGE_SKIPS` now holds two.
r14-03	pruning	`portfolio.feature:288`	The scenario description states the rule the scenario below it already states.

r14-01 is a defect. build_expansion_skips tested the lifetime for a finite value alone, so 0.0, 0.6 and -5.0 all kept the candidate. I ran the two parts: SUPPLY_SKIPS keeps a generator with lifetime 0.0, and financial_data_struct then writes capital_recovery_period 0 for both 0.0 and 0.6, and -5 for -5.0. No annuity can recover an overnight cost across zero years or fewer, and the portfolio stated the number with nothing in decisions.md. The PLEXOS leg makes it reachable: _PRICES_A_BUILD tests only that Economic Life is present. A sixth rule drops the candidate, reports it and warns; without the rule the new scenario writes one SupplyTechnology instead of none.

r14-02 was made incomplete by round 13's own rule, so correcting it finishes that change.

The pointless reviewer found nothing: it re-ran its AST and zero-use sweeps and confirmed the new scenario drives the whole rule rather than repeating the "holds no energy" one.

631 tests pass, mypy is clean, and the nine repository lints pass.

r15-01: a sidecar is JSON, and json.load reads the NaN and Infinity tokens, so a technical_life_years or a unit_size_mw that reaches the step is not always finite. The lifetime translation casts to a whole number, which raises on either token, so one sidecar value stopped the whole translation. A unit size wrote the token straight into portfolio.json, which no strict JSON reader accepts. Both columns take a finite number or nothing now, and a field with nothing is left out of the document. r15-02: NO_LIMIT_SKIP tested the limit for a null alone, and a Polars null is not a NaN, so a constraint stating an infinite right-hand side wrote max_mtons as Infinity. A fourth rule drops it, after the rule for a null limit, because is_finite answers null for a null. r15-04: the investments conftest copied write_user_mappings, which now sits in the shared conftest beside invoke_translate. r15-05: one scenario comment stated the absence the step below it asserts. Two new scenarios state a sidecar holding Infinity and NaN, and a constraint whose limit is Infinity. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

thomaschristopherking

Automatic review-and-fix, round 15

Acted on (4), in baed7d8

id	reviewer	where	severity	claim
r15-01	correctness	`…map_technologies.py:381`	critical	A sidecar `technical_life_years` that is not a finite number stops the run, and a sidecar `unit_size_mw` that is not finite writes `NaN` or `Infinity` into `portfolio.json`.
r15-02	correctness	`_carbon_caps.py:105`	major	A sidecar constraint whose right-hand side is not a finite number writes `"max_mtons": Infinity`, which is not valid JSON.
r15-04	pointless	`pypsa_to_sienna_investments/conftest.py:26`	minor	The `carrier_mappings_file` fixture writes the mappings YAML with 20 lines that copy `write_user_mappings`.
r15-05	pointless	`plexos_to_sienna_investments.feature:40`	nit	The comment states the absence the step below it asserts.

r15-01 is the first critical of the run, and it breaks the rule CLAUDE.md states most plainly: a model's data never stops a translation. A sidecar is JSON, and json.load reads the NaN and Infinity tokens, so a value reaching the step is not always finite. SUPPLY_LIFETIME and STORAGE_LIFETIME cast the technical life to a whole number, and I confirmed that cast raises InvalidOperationError on both tokens, so one sidecar value stopped the whole run. A unit size wrote the token straight into the document. Both columns now take a finite number or nothing, and a field with nothing is left out.

r15-02 is the same failure on the constraint path: NO_LIMIT_SKIP tests the limit for a null alone, and a Polars null is not a NaN. A fourth rule drops it, placed after the null rule because is_finite answers null for a null.

Two new scenarios state a sidecar holding Infinity and NaN, and a constraint whose limit is Infinity. Without their fixes the first pair fails and the second writes one CarbonCaps instead of none.

Deferred (1)

r15-03 _shared.py:162 — the six SkipRule blocks in build_expansion_skips repeat the same four lines, and a table of one-line rows would delete about 35 of them. The pruning reviewer found nothing this round.

633 tests pass, mypy is clean, and the nine repository lints pass.

…ing it r16-01: round 15 guarded the two sidecar numbers in the step, before the translation built its event. The event then stated that the sidecar said nothing, when the sidecar said Infinity, and no report or warning named the drop. CLAUDE.md asks for the source's own value in the report, so the guard moves into the expression of each translation, where the event reads the raw value and writes null beside it. decisions.md now carries the inf the sidecar stated. r16-05: three test modules kept their own copy of one carrier map, and the round-trip test reached into another package for two private names. The maps sit beside write_user_mappings in the shared conftest, and the investments conftest takes the five carriers it names from there. r16-04: a scenario description repeated its own title. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

thomaschristopherking

Automatic review-and-fix, round 16

Acted on (3), in fbf98ca

id	reviewer	where	severity	claim
r16-01	thermo	`…map_technologies.py:540`	major	`_finite_only` deletes a sidecar number with no event and no warning, and it erases the value the audit event must state.
r16-05	pointless	`pypsa_to_sienna/conftest.py:10`	minor	Three test modules keep their own copy of the same carrier map, and one reaches into another package for a private name.
r16-04	pruning	`portfolio.feature:272`	minor	The last clause of the scenario description repeats the title and the steps below it.

r16-01 is against round 15's own fix, and it is right. The guard sat in the step, before the translation built its event, so decisions.md said the sidecar stated nothing when the sidecar stated Infinity, and nothing named the drop. CLAUDE.md asks for the source's own value in the report. The guard now sits in the expression of each translation, where direct_translation reads the raw value for the source field and writes null beside it. I checked the result directly: the unit-size event now carries source=inf destination=None, and the scenario asserts decisions.md holds `pypsa.Generator.REZ_Solar._unit_size_mw` = inf.

Deferred (3)

r16-02 _carbon_caps.py:117 — the non-finite rule is correct only in its position, because filter_component turns a null keep value into a drop. The comment above it states that; fill_null(value=True) would make it independent.
r16-03 tests/step_defs/conftest.py:303 — write_user_mappings keeps a path parameter no caller passes.
r16-06 …map_technologies.py:103 — a question worth your answer: _GENERATOR_BASE_TYPES keeps a Generator candidate whose carrier the mappings file sends to HydroDispatch, but the operations step of the same run drops such a generator as untranslatable. The reviewer ran it: one run logs "2 Generator(s) have a carrier the mappings file sends to a Sienna type no generator becomes", while the shipped scenario at portfolio.feature:202 shows an extendable hydro Generator still becoming a SupplyTechnology. Removing HydroDispatch from that tuple would make the two agree and would merge it with _GENERATOR_FLEET_TYPES, but it changes a shipped scenario, so it is your call.

The pointless reviewer's second comment asked to delete the three portfolio step phrasings; round 12 already answered that item, so it was dropped.

633 tests pass, mypy is clean, and the nine repository lints pass.

r17-01: three shared steps carried a second phrasing, so a scenario could say either "parses as JSON" or "parses as a portfolio" for one assertion, and every new document kind would add one more decorator to each. The scenario line already names the file it reads, so the feature files use the phrase the steps already answered. The steps that read a portfolio's own shapes, its supplemental attributes and its absent components, keep their own words, because a system holds no such thing. r17-07: two files held the same step for one phrase; it sits beside the maps it reads now. r17-08: the mapping document listed four constraint drops, and round 15 added a fifth. r17-03 to r17-06: four comments that say a name again, state a count that disagrees with the code below it, or repeat a scenario title. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

thomaschristopherking

Automatic review-and-fix, round 17

Acted on (7), in 5dbda48

id	reviewer	where	severity	claim
r17-01	thermo	`steps/sienna_system.py:415`	major	Three shared steps carry a second phrasing, so the harness holds two names for one assertion.
r17-08	correctness	`translation-from-plexos-to-sienna-investments.md:165`	minor	The document says four kinds of `Constraint` are left out, but `CARBON_CAP_SKIPS` holds five rules.
r17-07	pointless	`test_pypsa_to_sienna_to_pypsa.py:47`	minor	Two files hold the same step definition for one phrase.
r17-03	pruning	`_shared.py:226`	minor	The `finite_or_null` summary line says again what the name says.
r17-04	pruning	`pypsa_to_sienna_investments/conftest.py:17`	minor	The comment above `_CARRIERS` repeats the names and gives a count the tuple below contradicts.
r17-05	pruning	`conftest.py:316`	nit	The second half of the comment above `STANDARD_PRIME_MOVER_MAP` repeats the calls below.
r17-06	pruning	`portfolio.feature:271`	nit	The second sentence of a scenario description says the title again.

r17-01 reverses part of round 5's own change. Round 5 deleted two duplicate step functions by giving the system steps a second parser; round 12's reviewer then declared that item closed on its own initiative, and I dropped a round 16 repeat of it on that basis. It never had a verdict from me, so it gets one now. The feature line already names the file it reads, so parses as JSON serves both documents and the aliases go. The steps that read a portfolio's own shapes, its supplemental attributes and its absent components, keep their own words, because a system holds no such thing.

r17-08 was made wrong by round 15's own rule, so correcting it finishes that change.

The correctness reviewer spent this round verifying round 16 rather than sweeping anew, and confirmed each part: finite_or_null passes 25.0 and 25.7 through and turns NaN, Infinity and null into null; SUPPLY_LIFETIME's cast writes 25 and raises nothing; the storage side does the same for both columns; finalise emits no second NOT_MAPPED, because it null-initialises only a column absent from the table; and the investments scenarios read the shared carrier maps rather than a stale user_mappings.yaml, because isolated_cwd is autouse.

Deferred (1)

r17-02 tests/step_defs/conftest.py:301 — the carrier maps and write_user_mappings sit in the 958-line root conftest, and plugin_fixtures.py states the split this repository already keeps. A sibling module would be their home.

633 tests pass, mypy is clean, and the nine repository lints pass.

r18-01: the bullet round 17 added said no JSON reader accepts NaN or Infinity, and the same branch states twice that json.load reads both tokens. The reason a cap goes is that a cap states a number of million tonnes, and neither word is one. r18-03: the two supplemental attribute steps held one body twice; the step that names a component finds its id and calls the step that takes one. r18-02 and r18-04: one comment the reading function already states, and an import block that holds one name. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

thomaschristopherking

Automatic review-and-fix, round 18

The correctness reviewer spent this round verifying round 17's rewording rather than sweeping anew, and it holds: it matched all 95 "parses as" lines in the two feature files against every registered step pattern, and each binds to exactly one step, so no assertion silently changed meaning. It also confirmed the moved @given reaches both its consumers.

Acted on (4), in cbc227e

id	reviewer	where	severity	claim
r18-01	pruning	`translation-from-plexos-to-sienna-investments.md:173`	major	The new bullet says no JSON reader accepts NaN or Infinity, but this branch states the opposite twice.
r18-03	pointless	`steps/sienna_portfolio.py:49`	minor	The two supplemental attribute steps hold the same body.
r18-02	pruning	`conftest.py:316`	minor	The comment above `STANDARD_PRIME_MOVER_MAP` says again what the function that reads it says.
r18-04	pointless	`test_pypsa_to_sienna_to_pypsa.py:7`	nit	The import block spans three lines for one name.

r18-01 is against round 17's own bullet, and it is right: finite_or_null's docstring and a scenario description both say json.load reads the two tokens, so the document contradicted them. The bullet now gives the reason in the model's own words, as the other four do.

Deferred (1)

r18-05 steps/sienna_portfolio.py:21 — assert_portfolio_component_absent still says "portfolio" while reading only the component groups the system module owns, so one scenario names one file two ways on adjacent lines. Moving it would make the word "portfolio" in a feature file mean one thing: the step reads the supplemental attributes.

633 tests pass, mypy is clean, and the nine repository lints pass.

thomaschristopherking · 2026-09-10T21:02:21Z

Superseded by #14, which squashes this whole stack into one branch off main. Closing this in favour of it.

thomaschristopherking and others added 3 commits September 8, 2026 23:51

thomaschristopherking commented Sep 9, 2026

View reviewed changes

thomaschristopherking and others added 3 commits September 9, 2026 00:28

thomaschristopherking commented Sep 9, 2026

View reviewed changes

Comment thread interop/plugins/shared/sienna_investments_constants.py Outdated

thomaschristopherking and others added 2 commits September 9, 2026 01:54

Record what the review rounds found on the investments branch

4e7b931

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Thomas C. King <thomas.k@transitionzero.org>

thomaschristopherking mentioned this pull request Sep 9, 2026

Give the AEMO case study an expansion path, and the gap analysis the portfolio's gaps #12

Closed

thomaschristopherking added this pull request to stack #13 September 9, 2026 09:58

thomaschristopherking added review-fix-running An automated review-and-fix cycle is running on this branch and removed review-fix-running An automated review-and-fix cycle is running on this branch labels Sep 9, 2026

thomaschristopherking commented Sep 10, 2026

View reviewed changes

thomaschristopherking and others added 2 commits September 10, 2026 13:37

thomaschristopherking commented Sep 10, 2026

View reviewed changes

thomaschristopherking removed the review-fix-running An automated review-and-fix cycle is running on this branch label Sep 10, 2026

thomaschristopherking mentioned this pull request Sep 10, 2026

Read a PLEXOS expansion plan and write a Sienna investments portfolio #14

Closed

thomaschristopherking closed this Sep 10, 2026

Conversation

thomaschristopherking commented Sep 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

TL;DR

Why

How

Reviewer notes

Tests

Follow-up

Uh oh!

coderabbitai Bot commented Sep 8, 2026

Draft PR not reviewed

Uh oh!

thomaschristopherking left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

thomaschristopherking left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

thomaschristopherking left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

thomaschristopherking left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

thomaschristopherking commented Sep 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thomaschristopherking left a comment

Choose a reason for hiding this comment

Automatic review-and-fix, round 5

Uh oh!

thomaschristopherking left a comment

Choose a reason for hiding this comment

Automatic review-and-fix, round 6

Uh oh!

thomaschristopherking left a comment

Choose a reason for hiding this comment

Automatic review-and-fix, round 7

Uh oh!

thomaschristopherking left a comment

Choose a reason for hiding this comment

Automatic review-and-fix, round 8

Uh oh!

thomaschristopherking left a comment

Choose a reason for hiding this comment

Automatic review-and-fix, round 9

Uh oh!

thomaschristopherking left a comment

Choose a reason for hiding this comment

Automatic review-and-fix, round 10

thomaschristopherking commented Sep 8, 2026 •

edited

Loading

thomaschristopherking commented Sep 10, 2026 •

edited

Loading