Add new-format network_expansion templater#102
Merged
Conversation
Extends the new-format templater with two output tables: network_expansion_options and network_transmission_path_expansion_costs. A single expansion_id keyed by expansion_type (forward / reverse / constraint_relaxation) unifies physical paths and constraint-group relaxations under one schema, so downstream consumers don't have to join back to network_transmission_paths to classify rows. Option selection picks the lowest dollars-per-MW per expansion_id using the first year with complete costs. Cost is divided by max(forward, reverse) so an asymmetric option can be represented in the translator as a single extendable PyPSA Link. Known-table discovery for the local cache moves from hard-coded lists to a static manifest (known_tables.yaml), so augmentation tables can be enumerated by prefix — necessary because v7.5 has one table per flow path x scenario. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
At nem_regions and single_region the network paths table is aggregated before the expansion templater runs, so flow-path augmentation entries keyed by raw IASR sub-region path IDs no longer line up with the surviving paths. Drop intra-region entries and re-key cross-region ones (NNSW-SQ -> NSW-QLD, suffixes preserved); at single_region drop all flow-path augmentations entirely. REZ and constraint-group entries are unaffected: REZ entries remap automatically via the geo-to-path lookup built from the already-aggregated paths, and constraint groups stay valid at all granularities since they can still bite on REZ-to- region lines. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Audited the file against the CLAUDE.md rules that landed on
new-format-network-tables and applied the same treatment we did to the
transmission tests: full-DataFrame assertions instead of row-count / set
membership / iloc / pd.isna probes, full log lines instead of marker +
per-name any() checks, and csv_str_to_df for empty expected outputs.
Also dropped four of the five DataFrame-builder helpers (_fp_costs,
_rez_options, _rez_costs, _paths_table) — they hid only short, non-private
column lists, and inlining via csv_str_to_df reads more consistently with
the rest of the file. _fp_options is kept because its column list pulls in
two private constants from the source module that csv_str_to_df can't
reference; a file-level docstring records the rationale.
While reviewing test_first_year_with_complete_costs_warns_..., the source
warning was too terse ("No year with complete costs for 'X'; skipping.")
to be useful in a real run. Replaced with a message that names the failure
mode and the likely cause.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Same audit pass as the test_network_expansion.py cleanup, applied to the three
new-format integration tests:
- Drop redundant `assert "..." in result` lines from _new_format; the
immediately-following `result["..."]` access raises KeyError with the same
diagnostic and is consistent with the other two integration tests.
- Drop three module-level column constants (_FP_AUG_COST_COLS,
_REZ_AUG_OPTION_COLS, _REZ_AUG_COST_COLS) and the
pd.DataFrame([(...)], columns=...) input pattern that depended on them;
inline as csv_str_to_df instead. Keep _FP_AUG_OPTION_COLS, which still
pulls two private constants from the source module.
Left the `set(expansion["expansion_id"]) == {...}` content checks in the
nem_regions and single_region integration tests in place: they pin the
intersection of granularity, REZ remapping, and augmentation filtering at
the orchestrator level — worth the slight maintenance cost over the strict
"presence + columns + row count" rule.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The unit test for _new_parallel_path_rows in test_network_expansion.py exercised the helper's content but no integration test triggered the un-suffixed-corridor branch via create_ispypsa_inputs_template. A refactor that dropped the _append_new_parallel_paths(...) call from create_template.py would have passed all tests. Extend test_create_ispypsa_inputs_template_new_format with two suffixed siblings (CNSW-SNW (NTH), (STH)) in flow_path_transfer_capability plus an un-suffixed CNSW-SNW augmentation. The new `assert "CNSW-SNW" in set(paths["path_id"])` is the load-bearing check — its comment names _append_new_parallel_paths so a regression failure points straight at the broken wiring. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
_log_fuzzy_match emitted one log line per row of the input series, so when callers passed e.g. one cost row per year for the same option name, each name-matching decision was logged N times. In a real run with several years × dozens of expansions, this produced hundreds of redundant lines that masked the actually-distinct decisions. Dedup with sorted(set(zip(...))) so each (original, match) decision appears exactly once. The CLAUDE.md exception that lets fuzzy matching log per-decision (rather than as a summary) is preserved — one line per distinct decision is the audit unit, not one line per row. Sorted output also gives stable ordering across runs. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The previous wording ("aggregate individual row contents into a sorted
list") was being read as a blanket rule. The actual concern was redundant
firings of one logical event (e.g. once per year per option), not
per-decision logs where each line is a distinct audit point. Recast the
rule around that, with the existing fuzzy-match log promoted from
"exception" to canonical example of the per-decision pattern.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Two issues from a code-review pass. The parallel-path append (for augmentation corridors with no matching existing path, e.g. CNSW-SNW alongside CNSW-SNW_NTH/_STH) used to live in create_template.py, where its position in the call sequence was an implicit contract: if reordered, those corridors would silently misclassify as constraint groups in the expansion output. Moved the append into _template_network_transmission so the contract is enforced where the paths are built. Pulled the design rationale (corridor-keyed augmentations, why a synthetic third Link, why explicit zero capacity) into the docstring of _append_new_parallel_paths in its new home. _build_geo_from_to_path_id_map collapsed duplicate geo_from values for subregions, relying on the implicit guarantee that REZ option tables never contain subregion IDs — a hidden precondition. Threaded rez_ids through _template_network_expansion so the map is built from REZ rows only. The collision becomes structurally impossible. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
A multi-agent docstring sweep flagged four examples that had drifted from the code or under-documented their outputs: - _aggregate_to_nem_regions: the example dropped the N1-NSW NaN row from the returned limits, even though _remap_limit_path_ids keeps any row whose path_id is in the rename map. - _append_new_parallel_paths: the rationale spends a paragraph on why limits are explicit zeros, not NaN — but the example only showed the paths half of the returned tuple. Added the limits side so the zero-capacity rows are visible alongside the rationale. - _template_network_expansion: the example didn't include the new required rez_ids input, so a reader couldn't reproduce it. - _aggregate_flow_path_augmentations_to_nem_regions: shown only as set notation over keys, hiding the dict-of-DataFrames structure and the "Flow path" column rewrite. Aligned with the wrapper's format. Also fixed a stray double-space in the def line of the same function. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds direct tests for utilities and branches that were only covered end-to-end (or not at all): the IASR-prefix typo absorption, the no-numeric-capacity INFO log, em-dash alignment, earliest-complete-year selection, the unknown-granularity ValueError, and the small parsing helpers. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The use_new_table_format=true path had no test driving it via the CLI, which is the surface most likely to regress when AEMO updates the 7.5 workbook or the templater logic shifts. Added a parameterised CLI test over all three regional granularities that asserts row counts derived from named structural quantities (flow paths, REZs, parallel-path injections, REZs without limits, etc.) plus referential integrity between paths/limits and options/costs. To keep CI off the workbook binary, the input fixture is committed as parsed CSVs at tests/test_workbook_table_cache/7.5/. The existing 6.0 truncated fixture was moved into a sibling 6.0/ subdir so the two versions sit alongside each other and serve different purposes (truncated unit-test inputs vs full e2e inputs) without aliasing. Flag flips for subprocess CLI tests need to cross the process boundary, so feature_flags.py now honours an ISPYPSA_USE_NEW_TABLE_FORMAT env override on top of the YAML default. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The two tests had identical inputs and exercised the same code path; collapsing them follows the combined output-plus-log pattern already used elsewhere in this file. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
list_templater_output_files returned the old-format file names regardless of feature flag, so for new-format runs doit's target list pointed at files that don't exist. The task therefore re-ran on every invocation, silently masking the cache-skipping behaviour. Added a feature-flag branch returning _NEW_FORMAT_TEMPLATE_OUTPUTS for new-format runs. The bug was discovered by extending the new-format CLI test to do a second invocation and assert up-to-date detection. That assertion lives in a new mechanism test sibling to the existing 6.0 mechanism test — test_create_ispypsa_inputs_task_new_format — covering the same fresh-run / up-to-date / config_changed / extensive-trigger flow against the new-format CLI path. The new-format coverage is split into a parallel test file rather than parameterising the existing tests. Trades some duplication during the transition for a cleaner handover when 6.0 is dropped: the legacy file is deleted and the new-format file is renamed in place, no diffs inside test bodies. Helpers are split the same way — format-agnostic infrastructure (run_cli_command, build_mock_config, etc.) stays shared in cli_test_helpers.py; the 7.5-specific fixtures live in cli_test_helpers_new_table_formats.py. Step-by-step handover plan is documented in the new test file's module docstring. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
🚀 New features to boost your workflow:
|
Adds FEATURE_FLAG_CLEANUP[use_new_table_format] markers at every site that will need attention when the feature flag is retired. A single grep across the repo will surface the full removal checklist instead of relying on recall of where the gating lives. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
EllieKallmier
approved these changes
May 18, 2026
Member
EllieKallmier
left a comment
EllieKallmier
left a comment
There was a problem hiding this comment.
Looking good! Left a few of the usual small questions/comments and have a few higher-level thoughts that I'll pop here to document (with no intent to block merging this in).
Thought notes:
- I think for me this PR is on the chunky side and possibly felt like could've been split up a tiny bit? Probably the only thing that feels like an easy split out is the table caching partitioning piece - and ofc I can see how interconnected it all is in the end so maybe there's not an obvious way to chunk this one. Either way not a big deal just noting for reflections' sake!
- Sometimes I find the kind of chained/nested helper-function flow breaks up the description of a function's purpose/process a little obscured or not quick to grasp. E.g., the
_select_least_cost_option_per_expansionfunction calls functions (that sometimes call other functions) - all good, makes sense - but from that function itself some of the detail of its process isn't obvious, like what "complete" costs means, or that the max. directional limit is used to determine the min cost/MW. I am certain that this is pure preference on my part here so I'm not suggesting we should repeat this kind of detail heaps through every docstring - I'm also thinking that this will prob be covered in the methodology docs rather than docstrings/API docs (?) just noticed myself sometimes needing to jump around a bit to see the full picture. - Last thought/question - while reviewing this PR I was thinking about the role of the templater (again, lol) and just thinking that there's a bit of decision-making that happens here in terms of setting up the inputs. Mostly w.r.t the option choice/filtering step, where there's a bit of I guess loss of information when the least-cost option gets chosen? Knowing that we are also building in the regional granularity stuff that will also be a bit opinionated by design, I don't think this is wrong/bad - I'm not really sure what this thought is at the end of it lol more stream-of-consciousness sorry!
Ok that's it from me, happy to go ahead with this bad boy :)
| new_paths, new_limits = _new_parallel_path_rows( | ||
| flow_path_options, set(paths["path_id"]) | ||
| ) | ||
| if new_paths.empty: |
Member
There was a problem hiding this comment.
Is this check needed? Being picky to check my understanding of the data validation/handling empties practices - this one to me looks still a bit defensive - very happy to be wrong!
Member
Author
There was a problem hiding this comment.
Good catch. This line was really just preventing a pandas warning when new_paths was empty. Which is better handled by fixing how _new_parallel_path_rows constructs the empty return case. I'll fix this.
| expansion_id option_name forward_mw reverse_mw | ||
| CQ-NQ CQ-NQ Option 1 1000.0 1200.0 | ||
| """ | ||
| if not flow_path_options: |
Member
There was a problem hiding this comment.
(comparing to my comment in transmission.py) - this one makes sense to me re: making sure the right columns/structure is returned (where it otherwise wouldn't be without the check)
Member
Author
There was a problem hiding this comment.
Yeah, I think some defensive checks still make sense, particularly in the templater which is pre-any validation. You might be fine without them too because the IASR inputs are static and the checks just provide a bit of robustness to IASR inputs drift. Whether this one, which would only fire with zero flow path options, makes sense is arguable.
|
|
||
| def convert(r): | ||
| if pd.isna(r): | ||
| return None |
Member
There was a problem hiding this comment.
what's the benefit of converting NaN to None here?
| return long.dropna(subset=["cost"]).reset_index(drop=True) | ||
|
|
||
|
|
||
| def _looks_like_financial_year(col: str) -> bool: |
Member
There was a problem hiding this comment.
I feel like this func could also go into helpers.py for reuse
| group constraints that can be relaxed through expansion. Physical paths | ||
| emit two rows (forward and reverse); constraint groups emit one | ||
| constraint_relaxation row. | ||
| columns: |
Member
There was a problem hiding this comment.
small deal - I've been switching up the allowed_values_from structure to use - table_name: column_name instead of splitting across table: name \ column: name, I think based on a suggestion from a previous review. I'm not particularly attached to one more than the other so don't mind what we go with but prob should pick one standard - do you have a pref?
Member
Author
There was a problem hiding this comment.
My bad, I'll change to the new format - table_name: column_name
| assert result == [ | ||
| result = _build_required_tables("7.5") | ||
| # Base topology tables come first | ||
| assert result[:4] == [ |
Member
There was a problem hiding this comment.
is the order of the tables important to preserve/test here?
Member
Author
There was a problem hiding this comment.
No, that's actualy not great as it makes the test more fragile than it needs to be. I'll fix.
Co-locates it with the other financial-year utilities so detect and convert live together. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The early-return guard in _append_new_parallel_paths only existed to dodge a pandas FutureWarning fired when an empty, object-dtype capacity column is concatenated onto a populated float64 one. Explicitly typing the capacity column in _new_parallel_path_rows fixes the root cause, so the guard can go. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The convert() closure only special-cased NaN to return None, but NaN and None are dropped identically downstream and the float64 dtype it avoided never survives past the concat in _load_all_options/_load_all_costs. Series.replace expresses the same map-or-pass-through behaviour in one line. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…xpansion_options schema
Replaces the verbose table:/column: pair entries with the compact
{table: column} mapping form for the expansion_id allowed-values source.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Table order is meaningless for a download/cache list, so the positional result[:4] slice tested nothing of value and would break on reorder or a new base table. Use membership checks to match the rest of the test. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Member
Author
|
Thank @EllieKallmier, I apprecaite the stream-of-consciousness and all the comments. Very helpful to be aware of your takes. (1) Noted. I'll keep looking to make the PRs more compact where possible. |
…chemas
Every allowed_values_from is now a list of {table: column} entries, so
single- and multi-source columns share one shape and a future parser can
always treat the field as a list.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Member
Sweet! Yeah cool, that seems like a nice straightforward op to handle this case - plenty of flexibility but a user could essentially ignore the network_transmission_path_expansion_costs 'extra' info without needing to interact/make decisions. Sounds good to me! |
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
2 participants
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.
Summary
network_expansiontemplater that turns IASR flow-path and REZaugmentation tables into two ISPyPSA inputs:
network_expansion_options(selectedleast-cost option per expandable element) and
network_transmission_path_expansion_costs(long-form $/MW cost trajectory).
create_ispypsa_inputs_templatebehind the existinguse_new_table_formatfeature flag, with a granularity-aware filter that drops orre-keys augmentation entries when paths are aggregated to NEM regions / single region.
_template_network_transmissionto inject zero-capacity parallel paths foraugmentation corridors (e.g.
CNSW-SNW) that exist alongside suffixed siblings(
CNSW-SNW_NTH/_STH) — without this, the orchestrator misclassifies them asconstraint relaxations.
6.0/,7.5/) anddrives required-table discovery from a checked-in
known_tables.yamlmanifest so theaugmentation prefixes can be enumerated per version.
network_expansion_optionsnow keys on(expansion_id, expansion_type)with allowed values
forward/reverse/constraint_relaxation; cost-per-MW divisordocumented as
max(forward, reverse).them,
_financial_year_string_to_end_year_inthelper, deduplicated fuzzy-match loglines, and CLAUDE.md additions covering I/O-example docstrings, integration-test
scope, and the "no hidden preconditions" rule.
Example output
network_expansion_options— physical paths emit forward+reverse rows; constraintgroups (ids not in
network_transmission_paths) emit oneconstraint_relaxationrow:network_transmission_path_expansion_costs— long-form, $/MW of the larger directionalcapacity:
Where the changes live