Template new non vre connection costs#114
Conversation
EllieKallmier
added
type: feature
Codecov Report✅ All modified and coverable lines are covered by tests.
🚀 New features to boost your workflow:
|
nick-gorman
left a comment
nick-gorman
left a comment
There was a problem hiding this comment.
Hey, Ellie this is looking good. A few minor comments. I left my main substaintal comment as a discussion post (#115) because I think it's broader than this PR. However, I think the point raised there would be the biggest thing to reduce test duplication, I'm keen to hear your thoughts on it.
| # connection_capacity_non_vre is in manually_extracted_template_tables/ (sourced from | ||
| # ENOR tables 16-17 and confirmed with AEMO) but is needed as an iasr_tables input, | ||
| # not a template output. | ||
| iasr_tables["connection_capacity_non_vre"] = manually_extracted_tables.pop( |
There was a problem hiding this comment.
In PR112 I've changed the templater to read the manul table directly from disk rather than recieve them as an input. I just thought it was a bit simpler (or hides the complexity). I'm happy to go either way, what do you think?
Also, if its coming as an input, maybe we shouldn't use pop, as this modifies somthing the user parsed in, and I guess its possible they might still want connection_capacity_non_vre in manually_extracted_tables.
There was a problem hiding this comment.
Yeah that sounds fine to me I think! I've taken a look at that PR and can't immediately spot what you're referring to but by this description I'm on board hahah
Re: popping, yeah I didn't love mutating the manual dict so open to not doing that - was just looking at the old version of things and didn't want this table to end up as a templater output, but ofc we can just handle that differently in the new version lol :) will update!
| system_strength_cost are in $/MW; either may be NaN where no cost | ||
| applies for that combination. | ||
| system_strength_cost are in $/MW; system_strength_cost is 0.0 for | ||
| non-IBR technologies and offshore wind. |
There was a problem hiding this comment.
Should there be an explicit setting of offshore wind to zero as for non-IBR (_set_non_ibr_system_strength_cost_to_zero and _set_solar_thermal_system_strength_costs_to_zero)?
There was a problem hiding this comment.
Oh, I think I see now, this is a outcome of there not being any forecast costs for offshore wind.
There was a problem hiding this comment.
yeah, that was the goal I think - but very happy to make explicit too for clarity/self docs purposes.
| normalised_df, canonical_technologies | ||
| ) | ||
| costs_per_mw = _calculate_connection_cost_per_mw(canonicalised_df) | ||
| _warn_nan_connection_costs(costs_per_mw, id_cols=["region_id", "technology"]) |
There was a problem hiding this comment.
Is this currently firing on the 2026 input data? Curious what to make of the MW what values being zero.
There was a problem hiding this comment.
yeahh this is probably a "too flexible" moment tbh - I might go back and simplify some bits here after our chat this morning. There are some rows in the 'capacity' df for VRE that have NaNs, so I thought I'd just cover the divide by 0 case just in case it popped up also - but post-chat this morning that is prob a bit extra and overcomplicating things :)
| connection_cost_forecast_wind_and_solar = csv_str_to_df(""" | ||
| REZ__ID, Scenario, 2024-25 | ||
| Q1, Step__Change, 73000000 | ||
| REZ__ID, Scenario, 2024-25, 2025-26 |
There was a problem hiding this comment.
I don't think we need the __ anymore, and csv_str_to_df should be able to handle just normal spaces.
| @@ -527,7 +476,7 @@ def test_merge_and_filter_system_strength_costs(csv_str_to_df): | |||
| def test_merge_and_filter_system_strength_costs_year_not_in_system_strength_gives_nan( | |||
There was a problem hiding this comment.
Not really a comment on this PR. But just saw this when reading through the test file. It's an interesting case, which might happen in many places. I guess if we wanted to inforce no missing years then the validator would be the place to do this. So yep, I think filling with nan makes sense.
| empty_costs = pd.DataFrame(columns=costs.columns) | ||
|
|
||
| expected = pd.DataFrame(columns=_CONNECTION_ONLY_COST_COLS) | ||
| # A emtpy |
2 participants
Add templating functionality (and tests) for non-VRE new entrant connection costs. Includes:
connection_capacity_non_vre-> currently saved inmanually_extracted_template_tables/7.5but open to moving elsewhere as its function is slightly different to other manual tables. To manage this I've just manually popped the read-in df out of manual table dict and into IASR table dict - again open to suggestions/other preferences for handling*_new_entranttables to the orchestrator functions, rather pre-processing to only pass necessary data through)There are still a few notes that point to open discussions or might need addressing but at this point felt non-urgent and easier to come back to once we are a few steps further into validation and consensus with #103 .
If I can also request in particular feedback on some of the tests - I don't know if I've done a great job on covering all necessary cases without too much double up so if anything sticks out as particularly unnecessary/messy or could be handled more clearly please let me know and I'll fix :)