Use parent assets rather than children for dispatch#1124
Conversation
The reason for this is because we want to be able to convert parent assets back to child assets for the callers.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1124 +/- ##
==========================================
- Coverage 87.55% 87.52% -0.03%
==========================================
Files 55 55
Lines 7580 7626 +46
Branches 7580 7626 +46
==========================================
+ Hits 6637 6675 +38
- Misses 640 649 +9
+ Partials 303 302 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| solution.flow_map.set(Some(create_flow_map( | ||
| self.existing_assets, | ||
| &self.model.time_slice_info, | ||
| solution.iter_activity(), |
There was a problem hiding this comment.
create_flow_map(...) is populated from solution.iter_activity(), which includes candidate assets when dispatch is run with candidates. That means the stored FlowMap can include assets without an AssetID (e.g., Candidate), contradicting the method docs (“existing assets”) and risking panics if it’s ever passed to DataWriter::write_flows() (which does asset.id().unwrap()). Consider restricting the activity iterator to existing assets only (or filtering out non-commissioned assets) when building the flow map.
| solution.flow_map.set(Some(create_flow_map( | |
| self.existing_assets, | |
| &self.model.time_slice_info, | |
| solution.iter_activity(), | |
| // Only include activities for existing assets in the stored flow map. | |
| let existing_assets = self.existing_assets; | |
| let activity_iter = solution | |
| .iter_activity() | |
| .filter(|(asset, ..)| existing_assets.contains(asset)); | |
| solution.flow_map.set(Some(create_flow_map( | |
| self.existing_assets, | |
| &self.model.time_slice_info, | |
| activity_iter, |
There was a problem hiding this comment.
Ha, this doc comment has been wrong for ages, but Copilot is the first to notice.
| for commodity_id in asset.iter_flows().map(|flow| &flow.commodity.id) { | ||
| for time_slice in time_slice_info.iter_ids() { | ||
| let flow = flows[&(parent.clone(), commodity_id.clone(), time_slice.clone())]; | ||
| flows.insert( | ||
| (asset.clone(), commodity_id.clone(), time_slice.clone()), | ||
| flow, | ||
| ); |
There was a problem hiding this comment.
In the child-flow copy loop, flows[&(parent.clone(), commodity_id.clone(), time_slice.clone())] will panic if the parent/time-slice/commodity key is missing. If assumptions change (e.g., differing flow sets between parent/child, or future filtering), this will become a hard-to-diagnose crash. Consider using get(...).expect(...) with a clearer message, or handling the missing-key case explicitly.
There was a problem hiding this comment.
Looks ok to me. Just thinking out loud but maybe in future we want to put an intermediate data structure here which extracts whatever we need from the AssetRefs e.g activity coefficient , (asset, time) key for variable map. Then we feed the new struct to new_with_activity_vars. Then it might be easier to separate what the optimiser needs to know about the assets from the data stored about assets in general.
| /// | ||
| /// Child assets are converted to their parents and non-divisible assets are returned as is. Each | ||
| /// parent asset is returned only once. | ||
| fn convert_assets_to_parents(assets: &[AssetRef]) -> impl Iterator<Item = AssetRef> { |
There was a problem hiding this comment.
maybe change the name of this to divide_assets or something, to make clearer that it's retaining all the non divisible ones. Or even prepare_assets_for_dispatch, because it's essentially converting to a format the dispatch API will understand
There was a problem hiding this comment.
Good point. I went with get_parent_or_self in the end.
If I've understood you correctly, I think the problem with this is that we need the actual |
|
Oh crap, I forgot to push my changes before clicking merge 🙈. |
That's a good point, I think if you wanted to do it you'd update these maps so they use the new struct (say |
2 participants
Description
For divisible assets, we currently include every child asset separately in the dispatch optimisation, but now that divided assets all have parent assets, we can use these parent assets instead. This means that we assume all the flows will be the same for each child asset, which seems reasonable, although note that previously HiGHS would settle on solutions where they would be different.
The implementation I've come up with is a bit horrible, frankly, but I'm hoping that we can improve on it as we incrementally change the rest of the code to work with parent assets rather than children. I've left the external API for dispatch the same -- you still input child assets and get child assets out the other side -- but, internally, it converts the child assets to parents before running dispatch. Once child assets have been yeeted (#1123), we can remove all this converting back and forth.
I benchmarked a model with 10,000 child assets and there was an ~8% speedup. We'll likely get an even better speedup with #1045, but it's not bad.
Closes #1043.
Type of change
Key checklist
$ cargo test$ cargo docpresent in the previous release
Further checks