Conversation
There was a problem hiding this comment.
Pull request overview
Fixes dispatch capacity accounting when dispatch is run with only a subset of a divisible parent asset’s children (e.g., during staged investment selection), preventing the parent from being treated as if it represents all children.
Changes:
- Adjust dispatch asset normalization to create a “partial parent” when only some children are present.
- Add
AssetRef::make_partial_parentto represent a parent with reduced discrete unit count (while keeping Hash/Eq compatible with the original parent). - Update golden test output CSVs for the
simple_divisiblefixture to reflect corrected dispatch results.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
src/simulation/optimisation.rs |
Reworks get_parent_or_self to count children per parent and emit a partial-parent AssetRef with matching unit count. |
src/asset.rs |
Adds AssetRef::make_partial_parent to construct an AssetRef representing a subset of a parent’s discrete units. |
tests/data/simple_divisible/commodity_flows.csv |
Updates expected commodity flow outputs to align with corrected dispatch capacity handling. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1211 +/- ##
=======================================
Coverage 89.31% 89.31%
=======================================
Files 57 57
Lines 7974 8004 +30
Branches 7974 8004 +30
=======================================
+ Hits 7122 7149 +27
- Misses 555 558 +3
Partials 297 297 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Maybe I should add a test which covers the case where we need a parent asset with less than its full capacity, like the one we have in #1131. Currently those lines appear not to be covered... |
Indeed! Though we will still need to consider how we represent a subset of an asset's capacity for this sort of case when we do that... but that's a future problem 😆 |
Description
Since #1124, we've been using parent assets to stand in for their children during dispatch, which allows us to avoid having separate variables for each child. Unfortunately, I didn't account for the fact that it is possible that only a subset of a parent's children will be included in the dispatch run. While the capacity of parent assets is permanently reduced at the point where a child is decommissioned, it is also possible that we are running dispatch with a subset of children, because only some have been selected for investment so far. In that case, we should treat the parent asset as if it represents only a number of units representing the selected children.
The way I implemented this was to add a helper to
AssetRefto create new objects representing part of a parent's capacity. As we don't include capacity inAssets' hashes, these "fake" assets can be used interchangeably with the real parents when they're a key (or part of a key) in a map.Fixes #1131.
Type of change
Key checklist
$ cargo test$ cargo docpresent in the previous release
Further checks