Skip to content

Fix prices calculation#1148

Merged
tsmbland merged 15 commits intomainfrom
prices_fix
Mar 4, 2026
Merged

Fix prices calculation#1148
tsmbland merged 15 commits intomainfrom
prices_fix

Conversation

@tsmbland
Copy link
Collaborator

@tsmbland tsmbland commented Feb 26, 2026

Description

This fixes a number of things to do with commodity price calculations, particularly when using the "marginal" and "full" strategies:

  • As suggested by Adam, calculating prices sequentially in the reverse of the investment order, instead of all in one go. E.g. if processes producing electricity consumes gas, then we must first calculate the price of gas then use this value when calculating the price of electricity. We were previously using shadow prices as inputs for all price calculations but this could lead to inconsistencies (e.g. commodity A being cheaper than commodity B, despite B being an input to A)
  • We were previously not including fixed_operating_costs in the full-cost calculation - this was a mistake
  • When using the "marginal" and "full" methods, we were previously excluding any timeslices with zero activity. In other words, if an asset had zero activity in a particular timeslice then it wasn't allowed to set the price for that timeslice. This let to some weird cases of missing prices, or unusual price variations (see this comment and the response from Adam). I've changed this so that an asset can set a price for a timeslice as long as it could produce in that timeslice (i.e. if there's an activity variable). This just came down to removing an if statement. 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:

  • Since this changes the order in which prices are calculated, this also changes the order of rows in the results files. Otherwise I don't think anything has changed since all models were using shadow prices. I was thinking it might be nice to have a regression test using the full/marginal strategies, but couldn't get this to work since these are blocked behind please_give_me_broken_results and I don't think there's any way of turning this on for regression tests with our current API
  • Since were using the InvestmentSet in a context unrelated to investments, I wonder if it makes sense to rebrand this into something more generic?
  • This sequential approach breaks down for models with circularities. I don't know what to do about this. For now it's flagging this at the validation stage, but I think long term we'd want this to work.
  • Adam also had comments about missing prices in the final year (Prices are odd #1130) which this PR doesn't address

Fixes #1128
Partially fxes #1130

Type of change

  • Bug fix (non-breaking change to fix an issue)
  • New feature (non-breaking change to add functionality)
  • Refactoring (non-breaking, non-functional change to improve maintainability)
  • Optimization (non-breaking change to speed up the code)
  • Breaking change (whatever its nature)
  • Documentation (improve or add documentation)

Key checklist

  • All tests pass: $ cargo test
  • The documentation builds and looks OK: $ cargo doc
  • Update release notes for the latest release if this PR adds a new feature or fixes a bug
    present in the previous release

Further checks

  • Code is commented, particularly in hard-to-understand areas
  • Tests added that prove fix is effective or that feature works

@codecov
Copy link

codecov bot commented Mar 2, 2026

Codecov Report

❌ Patch coverage is 54.33071% with 58 lines in your changes missing coverage. Please review.
✅ Project coverage is 87.30%. Comparing base (9dd37de) to head (e20a1f3).
⚠️ Report is 35 commits behind head on main.

Files with missing lines Patch % Lines
src/simulation/prices.rs 48.75% 33 Missing and 8 partials ⚠️
src/simulation/optimisation.rs 0.00% 12 Missing ⚠️
src/graph/investment.rs 87.50% 0 Missing and 3 partials ⚠️
src/asset.rs 88.88% 1 Missing ⚠️
src/input.rs 50.00% 0 Missing and 1 partial ⚠️
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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@tsmbland tsmbland marked this pull request as ready for review March 3, 2026 11:40
Copilot AI review requested due to automatic review settings March 3, 2026 11:40
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_costs are now included in the full-cost calculation (via get_annual_fixed_costs_per_flow replacing get_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.

Copilot AI review requested due to automatic review settings March 3, 2026 15:42
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@tsmbland tsmbland requested review from Aurashk and alexdewar March 3, 2026 15:57
@tsmbland tsmbland mentioned this pull request Mar 3, 2026
Copy link
Collaborator

@alexdewar alexdewar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@tsmbland
Copy link
Collaborator Author

tsmbland commented Mar 4, 2026

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?

(a) This is still outstanding. See #992
(b) This is quite vague so I'm not sure. I believe prices should now be sensible when using the "marginal" and "full" strategies, but may still be strange when using the "shadow" and "scarcity" methods
(c) This should fix the issue of specific timeslices, but we still have missing prices in the final year (#1166)

Agree that InvestmentSet could have a better name... MarketSet?

Yeah that's better. I think there's more refactoring to do here such as moving this away from the simulation::investment module, so I'll leave this for another PR

@tsmbland tsmbland merged commit 50f31d2 into main Mar 4, 2026
8 checks passed
@tsmbland tsmbland deleted the prices_fix branch March 4, 2026 11:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

"Full cost" method cannot use shadow prices as input

3 participants