Skip to content

Tweaks to parent assets PR #1127

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.

Sign up for GitHub

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

Merged
alexdewar merged 2 commits into from
Feb 13, 2026
Merged

Tweaks to parent assets PR #1127

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4bbba7d
Fix a couple of doc comments
alexdewar Feb 13, 2026
5a1a098
Rename: `convert_assets_to_parents` => `get_parent_or_self`
alexdewar Feb 13, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 4 additions & 10 deletions src/simulation/optimisation.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,10 +181,7 @@ impl VariableMap {
}
}

/// Create a map of commodity flows for each asset's coeffs at every time slice.
///
/// 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
Copy link

Copilot AI Feb 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
/// 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.

Copilot uses AI. Check for mistakes.
fn create_flow_map<'a>(
existing_assets: &[AssetRef],
time_slice_info: &TimeSliceInfo,
Expand Down Expand Up @@ -238,9 +235,6 @@ pub struct Solution<'a> {
impl Solution<'_> {
/// Create a map of commodity flows for each asset's coeffs at every time slice.
///
/// Note that this only includes commodity flows which relate to existing assets, so not every
/// commodity in the simulation will necessarily be represented.
///
/// Note: The flow map is actually already created and is taken from `self` when this method is
/// called (hence it can only be called once). The reason for this is because we need to convert
/// back from parent assets to child assets. We can remove this hack once we have updated all
Expand Down Expand Up @@ -420,11 +414,11 @@ fn filter_input_prices(
.collect()
}

/// Get the parent for each asset.
/// Get the parent for each asset, if it has one, or itself.
///
/// Child assets are converted to their parents and non-divisible assets are returned as is. Each
/// parent asset is returned only once.
Comment on lines 419 to 420
Copy link

Copilot AI Feb 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
/// 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.

Copilot uses AI. Check for mistakes.
fn convert_assets_to_parents(assets: &[AssetRef]) -> impl Iterator<Item = AssetRef> {
fn get_parent_or_self(assets: &[AssetRef]) -> impl Iterator<Item = AssetRef> {
let mut parents = HashSet::new();
assets
.iter()
Expand Down Expand Up @@ -611,7 +605,7 @@ impl<'model, 'run> DispatchRun<'model, 'run> {
allow_unmet_demand: bool,
input_prices: Option<&CommodityPrices>,
) -> Result<Solution<'model>, ModelError> {
let parent_assets = convert_assets_to_parents(self.existing_assets).collect_vec();
let parent_assets = get_parent_or_self(self.existing_assets).collect_vec();

Comment on lines +608 to 609
Copy link

Copilot AI Feb 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copilot uses AI. Check for mistakes.
// Set up problem
let mut problem = Problem::default();
Expand Down