Template new format new entrants identity cols#124
Conversation
EllieKallmier
added
type: feature
Codecov Report✅ All modified and coverable lines are covered by tests.
... and 2 files with indirect coverage changes 🚀 New features to boost your workflow:
|
nick-gorman
left a comment
nick-gorman
left a comment
There was a problem hiding this comment.
I think the structure is good, very readable, and a nice size for review. Minor comments only.
| # Extraction pattern for the resource-quality code embedded between underscores in | ||
| # a VRE IASR ID, e.g. "WFX" in "N10_WFX_Hunter Coast". |
There was a problem hiding this comment.
| # Extraction pattern for the resource-quality code embedded between underscores in | |
| # a VRE IASR ID, e.g. "WFX" in "N10_WFX_Hunter Coast". | |
| # Regex extracting the resource-quality code embedded between underscores in a VRE # IASR ID, e.g. "WFX" in "N10_WFX_Hunter Coast". Derived from the code map, it | |
| # expands to "_(WFX|WFL|SAT|...)_" — one capture group over the known codes, # sorted longest-first so a short code can't shadow a longer one it prefixes. |
| pd.testing.assert_frame_equal(storage, expected_storage) | ||
|
|
||
|
|
||
| def test_filter_to_technology_group_raises_unknown_group(csv_str_to_df, caplog): |
There was a problem hiding this comment.
Nit picking / totally fine the way it is. But since _filter_to_technology_group is a private function (and you know it will only be fed storage or generators) you could probably not have the raise and assocaited tests.
| ) | ||
|
|
||
|
|
||
| def _set_geo_id(new_entrants: pd.DataFrame) -> pd.DataFrame: |
There was a problem hiding this comment.
Somewhat duplicates custom_constraints_from_plexos.py:938 _pick_location. We could extract to a helper, but it doesn't concern me too much,
There was a problem hiding this comment.
Could also guard against future REZ ID being NaN rather than "Not Applicable", but that goes against our lets just assume the templater inputs are fixed principal, on the other hand its a cheap bit of robustness.
| ``_STORAGE_TECHNOLOGY_STRINGS`` substring (battery, pumped hydro), matched | ||
| case-insensitively; generators are every other row. The two groups partition | ||
| the summary, so this single predicate is the only place the generator/storage | ||
| boundary is defined. |
There was a problem hiding this comment.
custom_constraints_from_plexos.py:935 _is_battery_row ("Batter"); and static_new_generator_properties.py:65 (r"battery|pumped hydro") are also doing this work. Not suggesting we extract helpers (I don't think). But maybe loosen the language in the comment?
|
Actually, on thing that might have slipped past me in the review (and pervious ones) is adding new format templater tables to this CLI test test_create_ispypsa_inputs_new_table_formats.py::test_create_ispypsa_inputs_new_format, which runs real IASR data through the templater using the CLI and then checks the results are roughly the shape we expect. Not a blocker, and something it might be easier to circle back to when the new templater is being finalised. |
|
Some notes from post-review updates:
Re: your comment about the CLI tests - yep good point, and I think I will circle back after I add the next prs with more detail onto this one and actually wire the new_entrants outputs to be templater outputs as well. So I'll do a check + add for everything that I've missed that on so far at that point if ok :) |
2 participants
This PR is the first of a few to come in the process of templating new entrants - both generators and storage.
Scope:
create_ispypsa_inputs_templateto feed into connection costs templatingComing up in next PR(s):
For the moment I've set up both new entrant groups to live in the same file, but this might change/split in future as more detail gets added to each - and as existing/planned technologies get templated too. Happy to take on any suggestions around this kind of structural stuff if there are any :)