Calculate reduced costs and add `scarcity_pricing` option #684

alexdewar · 2025-07-03T11:09:42Z

Description

Reduced costs are needed for the investment appraisal step.

For candidate assets, we get reduced costs as direct output of the optimisation step, however we need to adjust them because of the adjustment for scarcity prices that we do (i.e. adding in activity duals). For existing assets, we don't get the result from the optimisation and it is a separate calculation step.

After I'd written this code, it became apparent that we probably actually don't want to adjust prices for scarcity anyway, as this approach has problems (#677). In any case, there was an open issue to make the scarcity pricing thing a model option (#625), so rather than just removing the code I've added this option and set the default to true (meaning we don't do this adjustment by default any more). I think this is a good compromise.

Closes #647. Closes #625.

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

Further checks

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

…city

Closes #647.

Closes #625.

Also emit a warning if the user disables it.

codecov · 2025-07-03T11:10:50Z

Codecov Report

Attention: Patch coverage is 51.82927% with 79 lines in your changes missing coverage. Please review.

Project coverage is 85.87%. Comparing base (dac8333) to head (4087d34).
Report is 35 commits behind head on main.

Files with missing lines	Patch %	Lines
src/simulation/prices.rs	48.29%	76 Missing ⚠️
src/model.rs	50.00%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #684      +/-   ##
==========================================
- Coverage   87.78%   85.87%   -1.92%     
==========================================
  Files          38       39       +1     
  Lines        3439     3589     +150     
  Branches     3439     3589     +150     
==========================================
+ Hits         3019     3082      +63     
- Misses        249      337      +88     
+ Partials      171      170       -1

☔ 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.

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

tsmbland

This looks like it's on the right track, but seems very convoluted. Maybe you understand something that I don't, but why are we keeping both the adjusted and unadjusted prices?

Wouldn't it be better to have several price calculation methods, a switch that toggles which one to use, and then proceed using the resulting prices from the chosen method. We calculate the reduced costs using these prices, and then pass the prices and reduced costs to the investment algorithm. Am I missing something?

tsmbland · 2025-07-03T12:41:39Z

src/model.rs

+    ///
+    /// Don't disable unless you know what you're doing.
+    #[serde(default = "return_true")]
+    pub scarcity_pricing: bool,


Funnily enough I'd find this more intuitive if instead we had scarcity_adjusted_pricing (= false by default). Maybe that's just me

I think you're right. It's a bit weird that the code does an extra thing if you set this option to false -- not v intuitive.

How about calling it adjust_prices_for_scarcity, to make it clear the code does something extra in this case?

Actually, I'm wondering if we should have a pricing_strategy option instead. See below.

Yep, I think that's the best option

src/simulation/prices.rs

tsmbland · 2025-07-03T12:56:28Z

src/simulation/investment.rs

+    reduced_costs.extend(reduced_costs_for_existing(
+        &model.time_slice_info,
+        assets,
+        adjusted_prices.unwrap_or(unadjusted_prices),


I think we should move some of this logic outside of this function and have perform_agent_investment take prices and reduced_costs directly as arguments. Would that work?

Especially if we'll want to add more options for calculating prices in the future, this could get very messy otherwise

Good idea.

Tbh it's not entirely clear in my head what the point of the perform_agent_investment function should be, but I left it there because that's the structure we have already. The reduced costs stuff probably shouldn't live in there though.

tsmbland · 2025-07-03T13:03:06Z

src/model.rs

+/// A function which always returns true.
+///
+/// Used for setting options to default to "on".
+const fn return_true() -> bool {


Yuck. But if this is the only way then so be it!

Yeah 😞. Rust isn't always that ergonomic. But if we change the default to be false, then we can just mark the attribute as #[serde(default)], which is less gross (but perhaps not super clear).

src/simulation/prices.rs

tsmbland · 2025-07-03T14:31:38Z

Hmm okay I'm going to give #661 a read and get back to this - think some of my uncertainties are addressed there!

tsmbland · 2025-07-03T14:59:15Z

src/simulation/prices.rs

+}
+
+/// Remove the effect of scarcity on candidate assets' reduced costs
+pub fn remove_scarcity_influence_from_candidate_reduced_costs(


I see there's a way to calculate this directly using the duals. I think we should do this, then we won't need to keep both sets of prices any more and can hopefully make it a bit more modular

I did consider this, but Adam's response (on #661) was:

We subtract the scarcity-inclusive prices, and add back the scarcity-removed prices. It just adjusts RC a bit (to remove scarcity element of price). True maybe there is some other way to represent this with the capacity duals, etc. But I think the way I've done it is more intuitive - working with the prices.

We don't have to take that suggestion, obviously, but I figured it might make sense to implement things in the same way as in the formulation for now and maybe revise it later if we want.

Something that I forgot until after the discussion with Adam is that prices are also influenced by levies. If a commodity has a commodity balance constraint and a levy, then we choose the higher of the two. I'm not sure how we could control for this in the calculation in an obvious way, so maybe Adam's suggestion is easier in that respect too.

Ok, but are we sure the levy should be factored into this calculation? All seems a bit strange

Even still, I think there would be a way to account for this even without keeping both sets of prices.

Bottom line: if we can do some of the other changes suggested, and ensure that only the prices (and reduced costs) corresponding the the chosen method exists outside of the prices module, then I think I'll be happy

tsmbland

Accidentally hit approve, but I do think this needs some work as discussed

alexdewar · 2025-07-07T07:31:25Z

This looks like it's on the right track, but seems very convoluted. Maybe you understand something that I don't, but why are we keeping both the adjusted and unadjusted prices?

Because we need them in the (catchily named) remove_scarcity_influence_from_candidate_reduced_costs function (as an alternative to calculating them directly from the duals -- see other comment).

Wouldn't it be better to have several price calculation methods, a switch that toggles which one to use, and then proceed using the resulting prices from the chosen method. We calculate the reduced costs using these prices, and then pass the prices and reduced costs to the investment algorithm. Am I missing something?

You're not missing anything and this is probably a cleaner way of doing things. Adam did say he'd like the pricing strategy to be modular too, which implies we might want to add another strategy in future.

How about instead of the scarcity_pricing option, we have one called pricing_strategy, with options called shadow_prices and scarcity_adjusted? Then, as you say, we can just pass the prices and reduced costs to perform_agent_investment.

What do you think @ahawkes? Would that flexibility be useful?

…adjustment`

alexdewar · 2025-07-08T13:03:35Z

I've tidied it up and changed the scarcity_pricing option into a more generic pricing_strategy one. I think this is tidier now.

Copilot

Pull Request Overview

Adds a configurable scarcity‐pricing option to the model, refactors how commodity prices and reduced costs are computed, and updates test data accordingly

Introduce PricingStrategy enum with two modes (ShadowPrices, ScarcityAdjusted) and wire it through model and simulation
Refactor CommodityPrices and add get_prices_and_reduced_costs to produce both prices and reduced costs in one call
Update test CSV fixtures to reflect scarcity‐adjusted price values

Reviewed Changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
tests/data/simple_mc/commodity_prices.csv	Updated expected prices for scarcity‐adjusted mode
tests/data/simple/commodity_prices.csv	Same updates for the simpler test dataset
src/simulation/prices.rs	Extracted pricing logic into `get_prices_and_reduced_costs`, added scarcity logic
src/simulation/optimisation.rs	Simplified cost coefficient by delegating to new `get_operating_cost`
src/asset.rs	Added `get_operating_cost` to `Asset`
src/simulation/investment.rs	Updated `perform_agent_investment` signature to accept `ReducedCosts` and `year`
src/simulation.rs	Switched to `get_prices_and_reduced_costs` and pass reduced costs to investment
src/model.rs	Added `pricing_strategy` config, default, and warning for scarcity adjustments

Comments suppressed due to low confidence (3)

src/simulation/prices.rs:215

[nitpick] This function name is quite long and could be more concise; consider renaming it to something like adjust_reduced_costs_for_scarcity to improve readability.

fn remove_scarcity_influence_from_candidate_reduced_costs(

src/simulation/prices.rs:20

Add unit tests to cover both ShadowPrices and ScarcityAdjusted pricing strategies in get_prices_and_reduced_costs to ensure correct price and reduced cost calculations.

pub fn get_prices_and_reduced_costs(

src/simulation/prices.rs:14

[nitpick] Consider adding a doc comment for the ReducedCosts type alias to explain its purpose and units for future maintainers.

pub type ReducedCosts = HashMap<(AssetRef, TimeSliceID), MoneyPerActivity>;

Copilot · 2025-07-08T13:04:06Z

src/simulation/prices.rs

+) -> MoneyPerActivity {
+    let adjusted = adjusted_prices
+        .get(&flow.commodity.id, region_id, time_slice)
+        .expect("No adjusted price found");
+    let unadjusted = unadjusted_prices
+        .get(&flow.commodity.id, region_id, time_slice)
+        .expect("No unadjusted price found");
+    flow.coeff * (unadjusted - adjusted)


Using expect here can cause a panic if a price entry is missing; consider returning a Result or providing a default value to handle missing prices gracefully.

Suggested change

) -> MoneyPerActivity {

let adjusted = adjusted_prices

.get(&flow.commodity.id, region_id, time_slice)

.expect("No adjusted price found");

let unadjusted = unadjusted_prices

.get(&flow.commodity.id, region_id, time_slice)

.expect("No unadjusted price found");

flow.coeff * (unadjusted - adjusted)

) -> Result<MoneyPerActivity, String> {

let adjusted = adjusted_prices

.get(&flow.commodity.id, region_id, time_slice)

.ok_or(format!(

"No adjusted price found for commodity: {:?}, region: {:?}, time slice: {:?}",

flow.commodity.id, region_id, time_slice

))?;

let unadjusted = unadjusted_prices

.get(&flow.commodity.id, region_id, time_slice)

.ok_or(format!(

"No unadjusted price found for commodity: {:?}, region: {:?}, time slice: {:?}",

flow.commodity.id, region_id, time_slice

))?;

Ok(flow.coeff * (unadjusted - adjusted))

tsmbland

This is way better, thanks!

alexdewar · 2025-07-08T13:30:11Z

This is way better, thanks!

It was a bit hacky before 😆

alexdewar added 7 commits July 3, 2025 11:59

Add CommodityPrices::get method

6db2034

Implement FromIterator for CommodityPrices

fe80551

CommodityPrices: Separate out calculation for removing effect of scar…

1f75758

…city

Calculate reduced costs for existing and candidate assets

28f4fdc

Closes #647.

Make removing effect of scarcity pricing a separate step

0c9b013

Add scarcity_pricing option, defaulting to false

1aa4979

Closes #625.

Set scarcity_pricing to default to true

e1a31f8

Also emit a warning if the user disables it.

alexdewar requested review from Copilot and tsmbland July 3, 2025 11:09

This comment was marked as outdated.

Sign in to view

Remove leading underscore from variable name

5e95f96

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

tsmbland reviewed Jul 3, 2025

View reviewed changes

tsmbland approved these changes Jul 3, 2025

View reviewed changes

tsmbland requested changes Jul 3, 2025

View reviewed changes

alexdewar added 5 commits July 7, 2025 08:35

CommodityPrices: Rename without_scarcity_pricing => `with_scarcity_…

9d62974

…adjustment`

Add get_operating_cost method to Asset

73427cd

Tidy reduced_costs_for_existing

c4b904f

Make pricing strategy an option

7d11adc

Wrap pricing strategy-specific behaviour in separate function

4087d34

alexdewar requested review from Copilot and tsmbland July 8, 2025 13:02

Copilot AI reviewed Jul 8, 2025

View reviewed changes

tsmbland approved these changes Jul 8, 2025

View reviewed changes

alexdewar merged commit 4b30fe8 into main Jul 8, 2025
7 checks passed

alexdewar deleted the reduced-costs-and-scarcity-pricing-option branch July 8, 2025 13:30

Calculate reduced costs and add scarcity_pricing option #684

Calculate reduced costs and add scarcity_pricing option #684

Uh oh!

Conversation

alexdewar commented Jul 3, 2025

Description

Type of change

Key checklist

Further checks

Uh oh!

codecov bot commented Jul 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

This comment was marked as outdated.

Uh oh!

tsmbland left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tsmbland commented Jul 3, 2025

Uh oh!

tsmbland Jul 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tsmbland Jul 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tsmbland left a comment

Choose a reason for hiding this comment

Uh oh!

alexdewar commented Jul 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alexdewar commented Jul 8, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Jul 8, 2025

Choose a reason for hiding this comment

Uh oh!

tsmbland left a comment

Choose a reason for hiding this comment

Uh oh!

alexdewar commented Jul 8, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

Calculate reduced costs and add `scarcity_pricing` option #684

Calculate reduced costs and add `scarcity_pricing` option #684

codecov bot commented Jul 3, 2025 •

edited

Loading

tsmbland Jul 3, 2025 •

edited

Loading

tsmbland Jul 8, 2025 •

edited

Loading

alexdewar commented Jul 7, 2025 •

edited

Loading