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 8189 -74
Branches 8263 8189 -74
==========================================
- Hits 7383 7349 -34
+ Misses 582 542 -40
Partials 298 298 ☔ 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.
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