Add price strategies using load-weighted averaging#1205
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1205 +/- ##
==========================================
+ Coverage 89.31% 89.35% +0.03%
==========================================
Files 57 57
Lines 7974 8263 +289
Branches 7974 8263 +289
==========================================
+ Hits 7122 7383 +261
- Misses 555 582 +27
- Partials 297 298 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Adds two new commodity pricing strategies—marginal_average and full_average—to complement the existing marginal and full strategies by using load-weighted averaging rather than “highest-cost producer wins”. This extends the model’s ability to represent observed market pricing behaviors (per #1200) and adds regression coverage via new patched examples.
Changes:
- Introduces
PricingStrategy::MarginalCostAverageandPricingStrategy::FullCostAverageand wires them into price calculation. - Updates circular-dependency validation to forbid the new cost-based average strategies in SCC cycles.
- Adds patched examples + regression fixtures for
simple_marginal_averageandsimple_full_average, plus schema/docs updates for the new strategy names.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
src/commodity.rs |
Adds new PricingStrategy enum variants with serde names. |
src/simulation/prices.rs |
Implements marginal/full cost average pricing and integrates into calculate_prices. |
src/graph/investment.rs |
Extends cycle validation to include the new average strategies. |
src/example/patches.rs |
Adds two new patched examples selecting the new strategies. |
schemas/input/commodities.yaml |
Documents and validates the new pricing_strategy enum values. |
docs/model/investment.md |
Fixes pricing strategy names to match current inputs (shadow, scarcity). |
tests/regression.rs |
Adds regression tests for the two new patched examples. |
tests/data/simple_marginal_average/* |
Regression fixtures for marginal-average pricing example. |
tests/data/simple_full_average/* |
Regression fixtures for full-average pricing example. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
|
I'm going to hold off on reviewing this until #1196 is merged, if that's ok, as I think the refactoring there will affect this PR. |
@alexdewar Sure. I've done some small refactoring in that PR, but I think any larger refactoring would need to be conscious of the methods introduced here, so might actually be better done as part of this PR |
alexdewar
left a comment
There was a problem hiding this comment.
This looks ok for now, though I do think it would be better if we could dedup some of this code -- this file's getting rather long. Aside from that, the functions themselves are also long, which makes it hard to keep track of everything they're doing. I see you're working on that already though (#1212), so maybe let's hold off discussion until then.
One option would be to define an Accumulator trait and implement it for the weighted average and the min cases (with a wrapper struct for the latter), then you could make these functions generic... I think. There are probably some fiddly details there. Anyway, I'll hold off judgment until #1212!
One thing I've only just thought of is that the WeightedAverageAccumulator uses Dimensionless for the denominator, though in practice we're actually using various other unit types. How about adding a type parameter for the denominator type? That way we can enforce that the same unit type is being supplied to add(), but without mandating that it always be Dimensionless. The compiler will infer the type in practice, so it shouldn't make using the class any uglier.
| "ELCTRI,Electricity,sed,daynight,marginal_average,PJ", | ||
| "RSHEAT,Residential heating,svd,daynight,shadow,PJ", | ||
| "CO2EMT,CO2 emitted,oth,annual,unpriced,ktCO2", | ||
| ])], |
There was a problem hiding this comment.
This is perhaps a bit out of scope for this PR, but I'm wondering whether we should include the pricing_strategy column in the examples even though it will be empty in every case (for now). It might be good as a kind of documentation.
| let output_coeff = asset | ||
| .get_flow(&commodity_id) | ||
| .expect("Commodity should be an output flow for this asset") | ||
| .coeff; |
There was a problem hiding this comment.
Would it also be worth asserting that this is an output flow, not an input?
Co-authored-by: Alex Dewar <alexdewar@users.noreply.github.com>
Co-authored-by: Alex Dewar <alexdewar@users.noreply.github.com>
Co-authored-by: Alex Dewar <alexdewar@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds two new cost-based pricing strategies—marginal_average and full_average—that compute load-weighted average prices across contributing assets, extending the model’s pricing options as requested in #1200.
Changes:
- Introduce
PricingStrategy::MarginalCostAverageandPricingStrategy::FullCostAverage(including schema/docs updates). - Implement average-price calculation paths in
src/simulation/prices.rsand integrate them into the pricing pipeline. - Add regression fixtures and example patches for the two new strategies.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/regression.rs | Adds regression test cases for the new strategies. |
| tests/data/simple_marginal_average/* | New regression outputs/fixtures for marginal_average. |
| tests/data/simple_full_average/* | New regression outputs/fixtures for full_average. |
| src/simulation/prices.rs | Implements marginal/full load-weighted average pricing and wires them into calculate_prices. |
| src/graph/investment.rs | Extends cycle validation to also forbid the new cost-based strategies in SCC cycles. |
| src/example/patches.rs | Adds example patches enabling the new strategies in the “simple” scenario. |
| src/commodity.rs | Extends PricingStrategy enum with the two new variants and serde names. |
| schemas/input/commodities.yaml | Updates allowed pricing_strategy enum values and documentation text. |
| docs/model/investment.md | Corrects pricing strategy naming in documentation (shadow, scarcity). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
I had considered this, but we use min/max/average in such different contexts that I don't think we can/should make a generic function to cover all of these cases, and at that point the
Yeah I guess this is a good idea. We'd still have to convert the denominator to |
Description
Adds two new pricing strategies as per the instructions in #1200
These are very similar to the existing marginal and full strategies, and indeed they recycle a lot of code. We could think about ways to reduce repetition, although I don't want to over-engineer too much as I want each strategy to be understood in isolation.
The approach here is a bit simpler than the previous strategies. Since we're averaging over all assets, we only need one accumulator per (commodity, region, ts selection), rather than an accumulator for each asset. Candidate assets are treated identically is in the previous strategies, i.e. averaging based on activity limits taking the min. This seems appropriate to me as candidate assets don't have any output over which to average, and I've been thinking of this as representing the price if a small amount of demand was added, in which case the cheapest candidate asset would win as the sole provider of this small amount.
Fixes #1200
Type of change
Key checklist
$ cargo test$ cargo docpresent in the previous release
Further checks