Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1212 +/- ##
==========================================
+ Coverage 89.35% 89.74% +0.39%
==========================================
Files 57 57
Lines 8263 8195 -68
Branches 8263 8195 -68
==========================================
- Hits 7383 7355 -28
+ Misses 582 544 -38
+ Partials 298 296 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Refactors src/simulation/prices.rs to reduce duplication by extracting shared logic for cost-based price calculation into reusable helper iterators, and updates documentation/tests to remove outdated “shadow price” wording for marginal/full cost pricing.
Changes:
- Extracts reusable helpers for computing selection-level prices from existing assets (max/average) and candidate assets (min).
- Reworks marginal/full cost and average-price paths to use these helpers, then expands selection-level prices to time-slice prices.
- Updates docstrings/tests terminology from “shadow price” to “price” where appropriate.
💡 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.
There was a problem hiding this comment.
Pull request overview
Refactors src/simulation/prices.rs to reduce repetition in cost-based pricing by extracting reusable iterator helpers for computing selection-level prices (max/avg over existing assets, min over candidates), while also cleaning up outdated “shadow price” references in docs/tests.
Changes:
- Extracts shared pricing logic into
iter_existing_asset_max_prices,iter_candidate_asset_min_prices, anditer_existing_asset_average_prices. - Rewires marginal/full cost pricing functions to use the new helpers and then expand selection-level prices back to per-timeslice entries.
- Updates documentation/tests terminology from “shadow price” to “price” where the cost-based strategies use existing prices as inputs.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
alexdewar
left a comment
alexdewar
left a comment
There was a problem hiding this comment.
LGTM! I've made some small comments. I don't have any ideas about how to simplify this further. Maybe this is the sweet spot 😄
I actually like having the type param in WeightedAverageAccumulator. I feel like it makes sense to be explicit about the type of thing we're accumulating and to enforce it.
| #[test] | ||
| fn weighted_average_accumulator_different_weights() { | ||
| let mut accum = WeightedAverageAccumulator::default(); | ||
| let mut accum = WeightedAverageAccumulator::<Dimensionless>::default(); |
There was a problem hiding this comment.
You could make the type param for WeightedAverageAccumulator default to Dimensionless and then you could write this more tersely, but maybe it's good to be explicit.
| } | ||
|
|
||
| /// Weighted average accumulator with a backup weighting path for `MoneyPerFlow` prices. | ||
| #[derive(Clone, Copy, Debug, Default)] |
There was a problem hiding this comment.
Weird that this doesn't work with the type param... not sure I understand why.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 3 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| PricingStrategy::MarginalCost => assert!( | ||
| annual_activities.is_none(), | ||
| "Cannot provide annual_activities with marginal pricing strategy" | ||
| ), | ||
| PricingStrategy::FullCost => assert!( | ||
| annual_activities.is_some(), | ||
| "annual_activities must be provided for full pricing strategy" | ||
| ), | ||
| _ => panic!("Invalid pricing strategy"), |
There was a problem hiding this comment.
Using assert!/panic! for pricing-strategy validation can crash production runs (these functions are used in simulation paths). Prefer returning an error (e.g., change the helper to return Result<impl Iterator<...>> / collect into a Result<IndexMap<...>>), or if you want to keep the API shape, use debug_assert! plus an explicit early-return behavior (e.g., return an empty iterator) with a logged error. At minimum, replace the panic!(\"Invalid pricing strategy\") with unreachable!() only if the type system guarantees other variants are impossible here; otherwise return/propagate an error.
| PricingStrategy::MarginalCost => assert!( | |
| annual_activities.is_none(), | |
| "Cannot provide annual_activities with marginal pricing strategy" | |
| ), | |
| PricingStrategy::FullCost => assert!( | |
| annual_activities.is_some(), | |
| "annual_activities must be provided for full pricing strategy" | |
| ), | |
| _ => panic!("Invalid pricing strategy"), | |
| PricingStrategy::MarginalCost => { | |
| // In debug builds, enforce that no annual activities are provided. | |
| debug_assert!( | |
| annual_activities.is_none(), | |
| "Cannot provide annual_activities with marginal pricing strategy" | |
| ); | |
| // In release builds, if the invariant is violated, log and return an empty iterator. | |
| if annual_activities.is_some() { | |
| eprintln!( | |
| "iter_existing_asset_max_prices: annual_activities must not be provided \ | |
| when using MarginalCost pricing strategy; returning empty iterator" | |
| ); | |
| return std::iter::empty(); | |
| } | |
| } | |
| PricingStrategy::FullCost => { | |
| // In debug builds, enforce that annual activities are provided. | |
| debug_assert!( | |
| annual_activities.is_some(), | |
| "annual_activities must be provided for full pricing strategy" | |
| ); | |
| // In release builds, if the invariant is violated, log and return an empty iterator. | |
| if annual_activities.is_none() { | |
| eprintln!( | |
| "iter_existing_asset_max_prices: annual_activities must be provided \ | |
| when using FullCost pricing strategy; returning empty iterator" | |
| ); | |
| return std::iter::empty(); | |
| } | |
| } | |
| _ => { | |
| // This function currently only supports MarginalCost and FullCost pricing strategies. | |
| debug_assert!( | |
| false, | |
| "Invalid or unsupported pricing strategy for iter_existing_asset_max_prices" | |
| ); | |
| eprintln!( | |
| "iter_existing_asset_max_prices: unsupported pricing strategy; returning empty iterator" | |
| ); | |
| return std::iter::empty(); | |
| } |
|
|
||
| // When using full cost pricing, skip assets with zero activity across the year, since | ||
| // we cannot calculate a fixed cost per flow. | ||
| let annual_activity = annual_activities.map(|activities| activities[asset]); |
There was a problem hiding this comment.
Indexing activities[asset] will panic if annual_activities is missing an entry for any asset emitted by activity_for_existing. Since this is runtime data-dependent, consider using activities.get(asset) and either (a) skip the asset with a clear message, or (b) expect with a more actionable panic message that includes the asset id/region to aid debugging.
| let annual_activity = annual_activities.map(|activities| activities[asset]); | |
| let annual_activity = annual_activities.map(|activities| { | |
| *activities | |
| .get(asset) | |
| .unwrap_or_else(|| { | |
| panic!( | |
| "Missing annual activity for asset {:?} in region {:?} when using full cost pricing", | |
| asset, | |
| region_id | |
| ) | |
| }) | |
| }); |
| assert!(matches!( | ||
| pricing_strategy, | ||
| PricingStrategy::MarginalCost | PricingStrategy::FullCost | ||
| )); |
There was a problem hiding this comment.
This assert! has no message, which makes failures harder to diagnose. Add an explicit message indicating the invalid pricing_strategy value and where it came from, or avoid panicking by returning an error as suggested elsewhere.
| assert!(matches!( | |
| pricing_strategy, | |
| PricingStrategy::MarginalCost | PricingStrategy::FullCost | |
| )); | |
| assert!( | |
| matches!( | |
| pricing_strategy, | |
| PricingStrategy::MarginalCost | PricingStrategy::FullCost | |
| ), | |
| "Unsupported pricing_strategy {:?} in iter_candidate_asset_min_prices for year {}", | |
| pricing_strategy, | |
| year | |
| ); |
| // Input-stage validation should ensure that this limit is never zero | ||
| let annual_fixed_costs_per_flow = | ||
| annual_fixed_costs.entry(asset.clone()).or_insert_with(|| { | ||
| asset.get_annual_fixed_costs_per_flow(annual_activity_limit.unwrap()) |
There was a problem hiding this comment.
The unwrap() here will panic if annual_activity_limit is unexpectedly None. Even if upstream logic/validation should guarantee presence, using .expect(\"annual activity limit must be computed for FullCost candidate pricing\") (or restructuring to avoid an Option once FullCost is selected) will produce a much more actionable failure mode.
| asset.get_annual_fixed_costs_per_flow(annual_activity_limit.unwrap()) | |
| asset.get_annual_fixed_costs_per_flow( | |
| annual_activity_limit.expect( | |
| "annual activity limit must be computed for FullCost candidate pricing", | |
| ), | |
| ) |
Description
I think this is a bit tidier. Less repetitive at least. The main change is to break the core logic for cost-based price calculation into three re-usable function:
iter_existing_asset_max_prices: calculates prices from existing assets based on marginal/full cost, taking the max across assetsiter_candidate_asset_min_prices: calculates prices from candidate assets based on marginal/full cost, taking the min across assets, and skipping any prices already covered by existing assetsiter_existing_asset_average_prices: calculates prices from existing assets based on marginal/full cost, taking the average across assetsI've gone for the approach suggested by @alexdewar in #1196 of passing an optional annual activities map to signal the type of cost calculation, but also passing
PricingStrategyas an argument just to be extra explicit.There's still some similarity between these three functions, but I think they are are sufficiently different that any attempts to combine them further would probably be counterproductive.
Also:
WeightedAverageAccumulator. I don't know if this is necessarily improvement so let me know what you think.strategiessubmodule to store the core price calculation logic, but I haven't done this hereFixes # (issue)
Type of change
Key checklist
$ cargo test$ cargo docpresent in the previous release
Further checks