Conversation
There was a problem hiding this comment.
Pull request overview
Follow-up tweaks related to #1124’s internal switch to using parent assets during dispatch optimisation, plus small rustdoc cleanups.
Changes:
- Simplified/trimmed rustdoc around flow map creation.
- Renamed the helper that deduplicates assets by parent (
convert_assets_to_parents→get_parent_or_self) and updated the call site.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /// | ||
| /// Note that this only includes commodity flows which relate to existing assets, so not every | ||
| /// commodity in the simulation will necessarily be represented. | ||
| /// Create a map of commodity flows for each asset's coeffs at every time slice |
There was a problem hiding this comment.
The doc comment for create_flow_map no longer mentions that the returned FlowMap is sparse (it only contains commodities implied by assets/flows, not necessarily every commodity in the simulation). Given the implementation only iterates asset.iter_flows() and then prunes parent assets, callers may incorrectly assume full commodity coverage—please reintroduce/replace that caveat in the rustdoc so the output is not misleading.
| /// Create a map of commodity flows for each asset's coeffs at every time slice | |
| /// Create a map of commodity flows for each asset's coeffs at every time slice. | |
| /// | |
| /// The returned `FlowMap` is *sparse*: it only contains entries for commodities that are | |
| /// implied by the assets' defined flows (via `asset.iter_flows()`) and the provided | |
| /// `activity`/`existing_assets`/`time_slice_info`. It does **not** guarantee coverage of | |
| /// every commodity (or asset/time-slice combination) in the simulation, and parent assets | |
| /// are removed before returning. |
| /// Child assets are converted to their parents and non-divisible assets are returned as is. Each | ||
| /// parent asset is returned only once. |
There was a problem hiding this comment.
The rustdoc for get_parent_or_self says “non-divisible assets are returned as is”, but the function also returns parent assets unchanged (i.e. any asset with parent() == None, including divisible parents). Consider rewording to something like “assets with no parent are returned as-is” to match the actual behavior.
| /// Child assets are converted to their parents and non-divisible assets are returned as is. Each | |
| /// parent asset is returned only once. | |
| /// Child assets are converted to their parent asset, and assets with no parent are returned as-is. | |
| /// Each parent asset is returned at most once. |
| let parent_assets = get_parent_or_self(self.existing_assets).collect_vec(); | ||
|
|
There was a problem hiding this comment.
parent_assets is now built via get_parent_or_self(...), so the vector may contain non-parent assets as well (assets with no parent). Renaming this local to something like existing_assets_deduped / dispatch_assets would better reflect the contents and avoid confusion in later checks like parent_assets.contains(asset).
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1127 +/- ##
=======================================
Coverage 87.52% 87.52%
=======================================
Files 55 55
Lines 7626 7626
Branches 7626 7626
=======================================
Hits 6675 6675
Misses 649 649
Partials 302 302 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
1 participant
Description
A couple of small changes for PR #1124, that I forgot to push before I clicked merge.
Type of change
Key checklist
$ cargo test$ cargo docpresent in the previous release
Further checks