Include secondary edges in investment graphs#1361
Open
tsmbland wants to merge 1 commit into
Open
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1361 +/- ##
=======================================
Coverage 89.75% 89.75%
=======================================
Files 58 58
Lines 8529 8529
Branches 8529 8529
=======================================
Hits 7655 7655
Misses 561 561
Partials 313 313 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the construction of InvestmentGraphs so they retain secondary (non-primary) flows rather than filtering them out, ensuring that the derived topological order can reflect secondary outputs (notably for commodity price-calculation ordering, per #1331).
Changes:
- Stop filtering
CommodityGraphedges to onlyGraphEdge::Primarywhen initialising anInvestmentGraph. - Update module/function documentation to reflect that only SVD/SED commodity-node filtering remains.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
75
to
88
| @@ -84,7 +84,7 @@ fn init_investment_graph_for_year( | |||
| } | |||
| _ => None, | |||
| }, | |||
| |_, e| matches!(e, GraphEdge::Primary(_)).then_some(e.clone()), | |||
| |_, e| Some(e.clone()), | |||
| ); | |||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
We use (what we're currently calling)
InvestmentGraphs for two purposes:Both of these orders are based on the same topological sorting of
InvestmentGraphs, just in reverse (investment are calculated for downstream commodities first, prices are calculated for upstream commodities first). At the moment, we exclude non-primary flows from these graphs (i.e. flows that don't contribute to production of the primary commodity). This made sense when we were only using these graphs to solve the investment order, since investments (for now) only target demands of primary output commodities. However, it doesn't make sense for price calculations as we do want secondary outputs to be able to contribute to prices (kind of making an assumption here, but it would feel strange/inconsistent if they couldn't). I don't want to have to create different graphs for investments/price calculations, so the simplest solution is just to no longer exclude non-primary flows fromInvestmentGraphs.This does have some implications for investments, but I think they're small:
Layer, could now be pushed to separateLayers if there's a secondary flow between them. I think the only implication of this is a potential performance cost (could require more dispatch rounds). Since investments still only target primary outputs, I don't think there's any functional change.Cycles that weren't there before. This could change results as these go through an extra "cycle balancing" step to equilibrate demands/supply. If anything, I think this could result in excess capacity invested in as a result of ignoring secondary outputs being "trimmed away", which may be a good thing anyway.Fixes #1331
Type of change
Key checklist
$ cargo test$ cargo docpresent in the previous release
Further checks