Template existing and planned (ECAA) generators - #143
Conversation
…move some functions to helpers, pull in required tables
…tor shared helpers Functions (and mappings) that were shared/very similar across new_entrants templater module are pulled out and gently refactored to become generic helpers.
Codecov Report✅ All modified and coverable lines are covered by tests.
🚀 New features to boost your workflow:
|
nick-gorman
left a comment
There was a problem hiding this comment.
@EllieKallmier, generally I think this looks great and was pretty easy to follow. I just found a few pretty small things, I'll leave it up to you what you reckon is worth actioning.
|
|
||
| # Case mismatch between maximum_capacity's IASR ID and the summary's: the 'safe' | ||
| # fuzzy-matching threshold (90) would miss (fuzz.ratio("KiataWF1", "KIATAWF1") == 50). | ||
| _MAXIMUM_CAPACITY_ID_TYPO_FIX = dict( |
There was a problem hiding this comment.
I think this constant would read better with a generic name like IASR_TYPO_FIXES, because then the fixes themselves are defined on a table by table basis. Does that make sense?
There was a problem hiding this comment.
Yep I agree! I'll leave this for now as I have an update coming up that this name change will fit very nicely into :)
| I/O Example (subset of columns; regional_granularity="sub_regions"): | ||
| existing_committed_anticipated_additional_generator_summary: | ||
| IASR ID / DLT names Power Station Technology Type REZ ID Sub-region | ||
| BW01 Bayswater Steam Sub Critical NA CNSW | ||
|
|
||
| maximum_capacity_existing_committed_anticipated_additional_generators: | ||
| IASR ID Installed capacity (MW) Commissioning date | ||
| BW01 660.0 NaN | ||
|
|
||
| returns: | ||
| name power_station geo_id capacity commissioning_date | ||
| BW01 Bayswater CNSW 660.0 NaN |
There was a problem hiding this comment.
Maybe slightly reformatted and expanded I/O example like below?
| I/O Example (subset of columns; regional_granularity="sub_regions"): | |
| existing_committed_anticipated_additional_generator_summary: | |
| IASR ID / DLT names Power Station Technology Type REZ ID Sub-region | |
| BW01 Bayswater Steam Sub Critical NA CNSW | |
| maximum_capacity_existing_committed_anticipated_additional_generators: | |
| IASR ID Installed capacity (MW) Commissioning date | |
| BW01 660.0 NaN | |
| returns: | |
| name power_station geo_id capacity commissioning_date | |
| BW01 Bayswater CNSW 660.0 NaN | |
| I/O Example (subset of columns): | |
| iasr_tables: | |
| existing_committed_anticipated_additional_generator_summary: | |
| IASR ID / DLT names Power Station Technology Type REZ ID Sub-region | |
| BW01 Bayswater Steam Sub Critical NA CNSW | |
| maximum_capacity_existing_committed_anticipated_additional_generators: | |
| IASR ID Installed capacity (MW) Commissioning date | |
| BW01 660.0 NaN | |
| ... plus the other property tables (see _GENERATORS_EXISTING_PLANNED_PROPERTY_MAP) | |
| regional_granularity: "nem_regions" | |
| sub_regional_geography: | |
| geo_id geo_type region_id | |
| CNSW subregion NSW | |
| returns: | |
| name power_station geo_id capacity commissioning_date | |
| BW01 Bayswater NSW 660.0 NaN # CNSW -> NSW via sub_regional_geography |
| Only generators whose ``technology`` appears in the table's own 'Technology Type' | ||
| column are fuzzy-matched and | ||
|
|
||
| I/O Example (property_spec = _COAL_MINIMUM_LOAD_PROPERTY; value_col abbreviated, |
There was a problem hiding this comment.
I think I have a preference for expanding the I/O examples. So in this case showing property_spec and maybe nesting coal_minimum_stable_level under an iasr_tables: headning. Up to you if you think its worth it.
| """ | ||
| resolved = _fuzzy_match_names( | ||
| names, | ||
| table_keys, |
There was a problem hiding this comment.
Maybe this is over kill. But the fuzzy matching would be slightly hardended by filtering out known storage unit names from table_keys before passing to _fuzzy_match_names.
| ValueError: PHES properties station(s) not found in the summary: | ||
| ['Some New Station'] | ||
| """ | ||
| if summary.empty: |
There was a problem hiding this comment.
Not a big one, but I reckon, given that this is the templater we could assume summary always contains some data. I think if you did change this is would just impact a couple of tests.
| BW01 Bayswater 660.0 NaN 10.05 | ||
| HUNTER1 Hunter Power Station 375.0 2025-08-01 10.93 | ||
| """ | ||
| if generators.empty: |
There was a problem hiding this comment.
I might leave this one for the moment - but definitely open to remove in future - just for a little (low cost?) hedge. Because this is catching only when the 'generators' part of the summary table is empty rather than the whole summary table it's just a tiny bit less of a given(?). Though I totally accept it's defensive and we're not at the stage of pre-filtering inputs or anything so if you do feel like the cons outweigh the (admittedly minute) pros I will happily delete hahaha 😆
| ANGAS1 Reciprocating engine 3.0 | ||
| Q1G1 Large scale Solar PV NaN # neither coal nor gas | ||
| """ | ||
| if generators.empty: |
| "closure_year": dict( | ||
| table="expected_closure_years", | ||
| key_col="IASR ID", | ||
| value_col="Expected Closure Year (Calendar year)", |
There was a problem hiding this comment.
Could replace numeric with a declared dtype so that the closure can be output as a int rather than a float. Maybe a nitpick, hmmm.
| value_col="Expected Closure Year (Calendar year)", | |
| value_col="Expected Closure Year (Calendar year)", | |
| dtype="int" |
There was a problem hiding this comment.
I have also been wondering about this - at the moment I'm not 100% convinced that the benefits of doing something like this (non-float looking year values is kinda the main one) outweighs the added complexity due to NaN presence. I'm also thinking about the round-trip when tables get saved to and read from csv (with potentially still empty columns) and needing to keep re-setting dtypes at those boundaries, + validation probably will (/could) have some type-setting role I imagine.
There's also a kind of question about where this mapping 'faces'/what it's kind of telling you about, which at the moment has been pretty much all source-related. A new field that's telling you what will happen to the output is a slightly different direction - but that's a very very small difference (and could be argued that it's still telling about the source too) imo.
Anyway I might open an issue just to keep this comment + thoughts somewhere and am totally open to implementing this if we decide it's worth it!
| geo_id (REZ ID with Sub-region fallback — see helpers._set_geo_id) and relabels | ||
| it to ``regional_granularity`` (REZ-located rows stay untouched at every | ||
| granularity, matching new_entrants.py's convention — see | ||
| _relabel_geo_id_to_granularity). |
There was a problem hiding this comment.
Not sure _relabel_geo_id_to_granularity is used anywhere.
| _get_property_value_map, | ||
| _group_properties_by_source, | ||
| _is_storage_row, | ||
| _is_subregion_geo_id, |
There was a problem hiding this comment.
_is_subregion_geo_id unused?
Addresses some comments from PR review: others have been addressed in a new issue (#145) or in-file comments/notes to be revisited in upcoming PRs.
Adds
generators_existing_plannedto the new-format templater, and lays thegenerator/storage split that
storage_existing_plannedwill build on. A bit chunkierthan ideal prob but should be pretty similar to the new entrants so (hope) not too
much review work...
Everything comes from one IASR table —
existing_committed_anticipated_additional_generator_summary— which already lists onerow per real generating unit (DUID-level), generators and storage together. So the module
splits that table in two, then merges per-unit properties onto the generator side from six
further IASR tables.
storage_existing_plannedis left at just the split and column renamehere; property merges for that one coming up.
Things worth knowing before reading the diff
PHES routing can't rely on
Technology Typealone. Wivenhoe and Shoalhaven arelabelled plain "Hydro" in the summary, but have real entries in the PHES properties table
(and the numbers cross-check — Wivenhoe's 3000 MWh / 285 MW ≈ the properties table's 10h).
Routing by technology string alone would send them to generators and silently never read
their PHES properties. So the split is a technology match union name-presence in the PHES
properties table -> noting it could just be one or the other
_validate_phes_routingthen fails loud if any PHES station is missing from the summaryaltogether — the only way to catch a future Wivenhoe-shaped surprise, since routing's own
presence check can't distinguish "absent" from "missed".
Lower Tumutis the onedocumented tolerated exception: it's Tumut 3's reversible pump-turbine subset (units 1-3),
see #131 comment thread.
Two things to note:
minimum_loadfor coal now comes from Typical Lowest Band. See #142KiataWF1vsKIATAWF1is handled as a named-constant correction, not by fuzzymatching —
fuzz.ratioscores it 50, well below any threshold that would be safe to applyacross 642 IDs. It's the only ID mismatch in the whole set. More detail in comments.
Known gap - not fixed here 25 REZ-located rows use sub-zone ids (
Q8a/Q8b/Q8c) thatrenewable_energy_zonesdoesn't carry — only the parentQ8. Same class asthe existing
#133non-REZ placeholder gap, different cause, so it follows the sameprecedent: named as an exception in the CLI-level geo_id coverage test rather than solved
here. Fix to come shortly - also flagging a future need to reassess when we move to v7.8
workbook which introduces a (differently-purposed/different coverage and impact) split up of
N9->N9a/N9b.Refactors
Mostly consolidation, so both new-format modules draw on one set of helpers instead of
reaching sideways into each other:
helpers.py:_set_geo_id(was duplicated in both modules),geography.py,_assert_table_validand the property-map machinery fromnew_entrants.py, and_build_geo_region_lookupfromtransmission.py_map_geo_id_to_granularitynow handles all three granularities and leaves REZ geo_idsuntouched in every case, so that rule lives in one place rather than being reimplemented
per module.
technology_col→key_col, so one merge helperserves both modules. The maps gain a
numericflag (default true) controlling whether amerged value is coerced with
pd.to_numeric(errors="raise")— an input-side typo guard;only
commissioning_dateopts out.dtype enforcement (whether to implement here or let validator apply) - my thought is
towards implementing here to keep validator's scope tight but it's a small fix thing
so just left simple as-is for the moment.
commissioning_dategets an explicit reformat: the parser emits ISO strings, the schemadeclares
%d/%m/%Y.