Repository navigation
Template existing planned storage - #149
Conversation
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.
Hey Ellie,
I'm happy with this, looking well thought through and organised. A lot going on but I think that's just the nature of the beast.
One further comment on how you handled Wivenhoe and Shoalhaven. I think I'd prefer fixing the the data up front and keeping the logic in the code simple. But the method you opted for works so I don't see a need to change.
| return storage[_STORAGE_COLUMNS] | ||
|
|
||
|
|
||
| # --- generator/storage split --- |
There was a problem hiding this comment.
Very minor, but in _is_existing_planned_storage_row I/O example, in the phes_properties table, Borumba should be QEJP - Borumba, otherwise you'd get a raise, right?
| phes["efficiency_charge"] = one_way_efficiency | ||
| phes["efficiency_discharge"] = one_way_efficiency | ||
| return phes | ||
| return phes.drop(columns=["round_trip_efficiency"]) |
There was a problem hiding this comment.
Very minor, but this could get reflected in the docstring of _merge_phes_properties in src/ispypsa/templater/new_entrants.py:601-602
There was a problem hiding this comment.
| unit's ``name`` is resolved against the property table's own 'key' column (see |
Wraps up the existing/planned templating started in #143 by adding the
storage_existing_plannedtable, and moves the technology/station-keyed merge intohelpers.py, so new entrants and existing/planned storage share one function (sorry for not splitting out into smaller PR chunks whoops).Storage properties (the ones templated in this PR) come from three places:
IASR ID): capacity, storage capacity, commissioning date, closure year. Uses the same unit-keyed merge as generators.battery_properties): charge/discharge efficiencypumped_hydro_..._properties): round-trip efficiency, split into symmetric charge/discharge legsWhere the changes live:
Some design choices to flag
_merge_storage_type_split_propertiesI've split by complement from battery rows, but we could instead manually edit thetechnologyvalue for these units from the start. I just did this for the moment to avoid getting too into the hydro-phes weeds while it's still a bit in the air - I imagine we might make other hydro-related decisions that could change what the ideal templater output shape for those units. But am very open to thoughts+opinions.exclude_unit_keys: Added based on suggestion from Nick in review of Template existing and planned (ECAA) generators #143 to tighten scope of fuzzy-matching for the unit-level property merges. Property tables otherwise have both generator and storage units all listed in the same table so if there were ever a typo'd generator name that made it a closer match to a storage unit name that would create an incorrect mapping (potentially problematic and kinda annoying to locate if it happened I reckon).Some other data notes to flag - see #131:
Note on the category merge function and property maps:
I thought about adding a field to the mapping dicts that would indicate which 'category' column should be used to merge onto for different properties (see 'df_key_col' in '_merge_category_keyed_properties'), but in the end decided against. Because: those mappings are currently IASR-table facing only; they just describe the input table features important for the property merge. I didn't want to broaden that context to make those maps also describe the part-templated summary table. BUT as always open to persuasion/other preferences.