-
Notifications
You must be signed in to change notification settings - Fork 1
Only consider primary flows in the topological sort #949
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #949 +/- ##
==========================================
+ Coverage 84.88% 84.96% +0.07%
==========================================
Files 50 50
Lines 5364 5419 +55
Branches 5364 5419 +55
==========================================
+ Hits 4553 4604 +51
- Misses 579 581 +2
- Partials 232 234 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
dalonsoa
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks really good, and I think it makes sense thinking on circularities.
alexdewar
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense to me!
Description
Since non-primary outputs are not considered in the investment algorithm (i.e. agents only consider processes for which the demanded commodity is the primary output), we don't need to include non-primary flows in the topological sort. This may help a little bit with the circularities problem because some flow cycles may disappear (or shrink) if you only consider primary flows. See the discussion in #914
Another benefit of this PR is that we can annotate the outputted DOT graphs to highlight primary vs non-primary flows. Here I've made it so non-primary flows appear as dashed lines.