Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1148 +/- ##
==========================================
- Coverage 87.48% 87.30% -0.18%
==========================================
Files 55 57 +2
Lines 7660 7706 +46
Branches 7660 7706 +46
==========================================
+ Hits 6701 6728 +27
- Misses 657 673 +16
- Partials 302 305 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR fixes commodity price calculations for "marginal" and "full" pricing strategies. Previously, all prices were calculated simultaneously using shadow prices as inputs, which could produce inconsistencies (e.g., a downstream commodity being cheaper than an upstream one). The fix calculates prices sequentially in reverse investment order so that upstream prices are already computed when needed by downstream price calculations. Additionally, it fixes a bug where fixed_operating_costs were not included in the full-cost calculation, and removes an incorrect filter that excluded zero-activity time slices from price setting.
Changes:
- Prices are now calculated sequentially in reverse investment order, using previously computed upstream prices instead of shadow prices for marginal/full cost calculations
fixed_operating_costsare now included in the full-cost calculation (viaget_annual_fixed_costs_per_flowreplacingget_annual_capital_cost_per_flow)- Zero-activity time slices are no longer excluded from marginal/full cost price setting
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
src/simulation/prices.rs |
Main logic change: sequential pricing loop using investment order in reverse, replacing shadow prices with upstream prices for marginal/full cost strategies, and removing zero-activity filter |
src/simulation/optimisation.rs |
Added iter_activity_keys_for_existing and iter_activity_keys_for_candidates helpers that strip activity values from existing iterators |
src/asset.rs |
Renamed get_annual_capital_cost_per_flow to get_annual_fixed_costs_per_flow and updated it to include fixed_operating_cost in addition to capital cost; added Year to imports |
tests/data/two_regions/commodity_prices.csv |
Updated expected CSV output to reflect new ordering of price results |
tests/data/two_outputs/commodity_prices.csv |
Updated expected CSV output to reflect new ordering and newly computed prices |
tests/data/muse1_default/commodity_prices.csv |
Updated expected CSV output to reflect new ordering |
tests/data/missing_commodity/commodity_prices.csv |
Updated expected CSV output to reflect new ordering |
tests/data/circularity/commodity_prices.csv |
Updated expected CSV output to reflect new ordering and newly computed prices |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
alexdewar
left a comment
There was a problem hiding this comment.
The code seems ok to me and I'll have to take the economics on faith 😄.
Adam mentioned a few problems in #1130:
Issues I'm seeing are:
(a) there are zeros for price in some time slices for "shadow" and "scarcity" methods, and
(b) prices are often odd in any case, either too high or too low to be plausible, and
(c) sometimes prices are missing entirely for a year, or just for some time slices.
To be clear, which of these problems are fixed and which are still outstanding?
Agree that InvestmentSet could have a better name... MarketSet?
(a) This is still outstanding. See #992
Yeah that's better. I think there's more refactoring to do here such as moving this away from the |
Description
This fixes a number of things to do with commodity price calculations, particularly when using the "marginal" and "full" strategies:
fixed_operating_costsin the full-cost calculation - this was a mistakeifstatement. Since activity values aren't actually used in the price calculation (apart from annual activity in the case of full cost), nothing else needed to be done.Notes:
please_give_me_broken_resultsand I don't think there's any way of turning this on for regression tests with our current APIInvestmentSetin a context unrelated to investments, I wonder if it makes sense to rebrand this into something more generic?Fixes #1128
Partially fxes #1130
Type of change
Key checklist
$ cargo test$ cargo docpresent in the previous release
Further checks