Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1311 +/- ##
==========================================
+ Coverage 89.69% 89.70% +0.01%
==========================================
Files 58 58
Lines 8470 8481 +11
Branches 8470 8481 +11
==========================================
+ Hits 7597 7608 +11
Misses 566 566
Partials 307 307 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@alexdewar Could also go further than this and add a region field to Maybe best to leave this as a draft for now and see how we get on with the rest of it |
There was a problem hiding this comment.
Pull request overview
Adds an explicit RegionID dimension to the optimisation FlowMap key so flows can be attributed to a (potentially different) region than the asset’s region, in preparation for cross-region/trade flow handling (Fixes #1309).
Changes:
- Extend
FlowMapkey from(AssetRef, CommodityID, TimeSliceID)to(AssetRef, CommodityID, RegionID, TimeSliceID). - Update flow-map construction and downstream consumers (investment net demand update, flow CSV writer, and related tests) to handle the new key shape.
- Adjust parent/child flow copying and map retention logic for the new tuple arity.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/simulation/optimisation.rs |
Updates FlowMap type and populates RegionID in flow keys during flow-map creation and parent→child copying. |
src/simulation/investment.rs |
Uses the RegionID from FlowMap keys when updating the net demand map. |
src/output.rs |
Updates flow writing/test to destructure the new FlowMap key shape (but currently ignores the region value). |
Comments suppressed due to low confidence (1)
src/output.rs:637
write_flowsdestructures and then discardsregion_id(_region_id). IfFlowMapnow allows flow regions to differ fromasset.region_id(), the CSV output will silently lose that information and may misattribute flows when trade is introduced. Consider adding aregion_id(orflow_region_idto disambiguate) column toCommodityFlowRowand populating it from theFlowMapkey, updating the corresponding CSV test expectations as well.
for ((asset, commodity_id, _region_id, time_slice), flow) in flow_map {
let row = CommodityFlowRow {
milestone_year,
asset_id: asset.id().unwrap(),
commodity_id: commodity_id.clone(),
time_slice: time_slice.clone(),
flow: *flow,
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| fn update_net_demand_map(demand: &mut AllDemandMap, flows: &FlowMap, assets: &[AssetRef]) { | ||
| for ((asset, commodity_id, time_slice), flow) in flows { | ||
| for ((asset, commodity_id, region_id, time_slice), flow) in flows { | ||
| if assets.contains(asset) { | ||
| let key = ( | ||
| commodity_id.clone(), | ||
| asset.region_id().clone(), | ||
| time_slice.clone(), | ||
| ); | ||
| let key = (commodity_id.clone(), region_id.clone(), time_slice.clone()); | ||
|
|
Description
Adds a region field to
FlowMapto allow for the possibility of flow regions differing from the region of the asset. This will be necessary for trade as we'll probably want to store trade flows in this map as well. (Or will we... this map needs an asset key, so if we wanted trade processes to exist without assets, then trade flows would need to be stored in a different map. Not sure!)Fixes #1309
Type of change
Key checklist
$ cargo test$ cargo docpresent in the previous release
Further checks