Use graphs for commodity flow validation#771
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #771 +/- ##
==========================================
- Coverage 86.77% 86.53% -0.24%
==========================================
Files 46 46
Lines 4787 4828 +41
Branches 4787 4828 +41
==========================================
+ Hits 4154 4178 +24
- Misses 423 437 +14
- Partials 210 213 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
alexdewar
left a comment
There was a problem hiding this comment.
This is great! I've made some small suggestions.
Requesting changes mostly because I think you should use enums for edges/nodes rather than IDs with sentinel values.
|
|
||
| // Add demand edges | ||
| // We add edges to _DEMAND for commodities that are demanded in the selection | ||
| // NOTE: we only do this for commodities with the same time_slice_level as the selection |
There was a problem hiding this comment.
Is this a limitation? Will things break for commodities with a different time_slice_level?
There was a problem hiding this comment.
Nope. We only validate SVD commodities at their appropriate time_slice_level, and other commodities won't be affected by the presence/absence of demand nodes
|
@alexdewar Thanks for the comments, I think this is ready for another look |
alexdewar
left a comment
There was a problem hiding this comment.
I've suggested a few small tweaks, but otherwise all good!
Description
This replaces the code for validating commodity flows with a new and (hopefully) more robust method using the graph library that we're already using for commodity ordering.
The point of this is to make sure that each commodity abides by the rules for its type (
SVDvsSEDvsOTH), and ultimately to ensure that there's a viable pathway to meet commodity demands.SVD): if these are demanded, there must also be a process that can produce themSED): if there are processes that can consume these commodities, there must also be processes that can produce themOTH): these can only be consumed or produced but not both (these are usually side-products such as CO2, renewables such as solar energy, or raw materials that we don't model the production of)The process is as follows:
Annual-level commoditiesSeason-level commoditiesDayNight-level commoditiesIf it passes all of this then we're good to go!
I've tried to be faithful to the original code by keeping roughly the same checks that were originally in place. If we're being smart, then we probably don't need to do validation checks for every commodity but just need to ensure that each
_DEMANDnode is reachable from at least one_SOURCEorOTHnode via a directed path through the graph, which is ultimately what we care about. (Seehas_connecting_pathorall_simple_paths)(Ultimately, I wonder if we actually need the "commodity type" concept at all, but that's a conversation for another day)
I think as the model gets more complicated the benefits of this approach will become stronger. For example, commodity trade where a commodity may be produced in one region and consumed in another (in this case we'll probably have to combine all regions into a single graph)
Fixes #737 I'm not directly addressing this issue, but I've deleted the part of the code where the panic occurred, and checked that it no longer panics in the new version (it gives a nice error message instead)
Re #743 I can't actually reproduce the panicking (I remember it being a bit random as to whether it panics or gives an error message), but the part of the code where it supposedly did panic is now gone and I've been careful not to include the same mistake in the new code. The error message persists (unrelated to this code) which is still a problem so I'll leave the issue up and change the description
Type of change
Key checklist
$ cargo test$ cargo docFurther checks