Repository navigation
Combine region graphs into single graph for each year - #1569
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1569 +/- ##
==========================================
+ Coverage 90.88% 90.90% +0.01%
==========================================
Files 61 61
Lines 9121 9116 -5
Branches 9121 9116 -5
==========================================
- Hits 8290 8287 -3
+ Misses 509 507 -2
Partials 322 322 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
🟡 Changes recommended
Repeated demand-map and graph-node scans introduce validation scaling regressions as model size increases.
2 open issues
What changed in this PR
Combines regional commodity graphs into one graph per year, simplifying graph handling and preparing MUSE2 for future inter-region trade.
Changes:
- Adds region identities to commodity nodes and process edges.
- Adapts validation and investment ordering to year-keyed graphs.
- Saves one DOT file per year and updates affected tests.
| File | Description |
|---|---|
tests/cli.rs |
Expects the year-only DOT filename. |
src/input.rs |
Returns commodity graphs keyed by year. |
src/graph/validate.rs |
Uses regional edge availability and demand in combined graphs. |
src/graph/investment.rs |
Derives investment order from a single yearly graph. |
src/graph.rs |
Builds region-qualified yearly graphs and updates DOT output. |
Review effort: Balanced
| for ((region_id, dem_year, dem_ts_sel), &flow) in &commodity.demand { | ||
| if *dem_year != year || dem_ts_sel != time_slice_selection || flow <= Flow(0.0) { |
| let commodity_node_index = filtered_graph | ||
| .node_indices() | ||
| .find(|&idx| filtered_graph.node_weight(idx) == Some(&commodity_node)) | ||
| .unwrap_or_else(|| { | ||
| filtered_graph.add_node(GraphNode::Commodity(commodity_id.clone())) | ||
| }); | ||
| .unwrap_or_else(|| filtered_graph.add_node(commodity_node)); |
35a8154 to
5e22b4a
Compare
alexdewar
left a comment
alexdewar
left a comment
There was a problem hiding this comment.
Makes sense and seems to simplify things a bit. I've added some comments.
I don't think performance is likely to be impacted much unless the model is huge (though I suppose you never know). I'm not sure how the GitHub benchmark action is configured (I couldn't find any files related to it in the repo), but it's odd that it's reporting a massive performance regression (~30%). I tried reproducing this locally and I don't see such large differences, BUT the results were really variable -- most benchmarks reported no difference most of the time, but they often indicated performance improvements or regressions, seemingly at random.
Maybe we need to tweak the configuration a bit. I'm seeing a lot of warnings like this:
Warning: Unable to complete 20 samples in 5.0s. You may wish to increase target time to 18.4s, or reduce sample count to 10.
It seems like the examples benchmarks take ages to complete and if cargo bench isn't able to collect many samples within the timeframe, that would explain why we're seeing such variability.
| Commodity(CommodityID), | ||
| /// A node representing a commodity in a specific region | ||
| #[display("{_0} | {_1}")] | ||
| Commodity(CommodityID, RegionID), |
There was a problem hiding this comment.
Should this be renamed to CommodityMarket?
I've also just noticed that MarketSet::Single has a tuple as its inner value, rather than two separate values, like here. Should we change that one for consistency?
There was a problem hiding this comment.
Good point. Not sure why MarketSet::Single has a tuple. I'll change it, but will defer to avoid conflicts with other PRs
|
@alexdewar Thanks. I have another PR tidying up the |
Co-authored-by: Alex Dewar <alexdewar@users.noreply.github.com>
Description
We currently build separate commodity graphs for each region, which we then combine into a single investment graph to get the investment order. This works ok for now, as there aren't any edges between regions (that would imply trade), but this is surely something that we'll want. The way I see it, there's no reason to have multi-region models if we don't have trade, so either way building separate graphs for different regions is a bit dumb. Not a high priority, but I think there will be some changes incoming to the graph code, and I wanted to get this out of the way. I think it's also a bit more consistent with how the rest of the code works.
I first tried this when we were discussing trade (https://github.com/EnergySystemsModellingLab/MUSE2/tree/rough_trade_implementation), and I've mostly just copied over some of the changes from that. There are some other changes in that branch about allowing flows between regions, and prettifying the graph, which I haven't copied over, but these can come later.
Copilot has flagged some performance issues, which I think I agree with, although they're probably minor.
Fixes # (issue)
Type of change
Key checklist
$ cargo test$ cargo docpresent in the previous release
Further checks