From a20bc5d0215e1329339a9472b60674a52e8f660d Mon Sep 17 00:00:00 2001 From: Aurash Karimi Date: Tue, 20 Jan 2026 14:04:57 +0000 Subject: [PATCH 01/11] add simple lcox metric test --- src/fixture.rs | 58 +++++++++++++++++++++++++++++++++++- src/simulation/investment.rs | 51 +++++++++++++++++++++++++------ 2 files changed, 99 insertions(+), 10 deletions(-) diff --git a/src/fixture.rs b/src/fixture.rs index aa43feb6..ea0d4cfa 100644 --- a/src/fixture.rs +++ b/src/fixture.rs @@ -16,7 +16,7 @@ use crate::process::{ use crate::region::RegionID; use crate::simulation::investment::appraisal::LCOXMetric; use crate::simulation::investment::appraisal::{ - AppraisalOutput, coefficients::ObjectiveCoefficients, + AppraisalOutput, MetricTrait, coefficients::ObjectiveCoefficients, }; use crate::time_slice::{TimeSliceID, TimeSliceInfo, TimeSliceLevel}; use crate::units::{ @@ -396,6 +396,62 @@ pub fn appraisal_output(asset: Asset, time_slice: TimeSliceID) -> AppraisalOutpu } } +/// Creates appraisal outputs from assets with corresponding metrics. If no assets provided, +/// creates default candidate assets based on the number of metrics. +/// +/// # Panics +/// Panics if `assets` and `metrics` have different lengths +#[fixture] +pub fn appraisal_outputs( + #[default(vec![])] assets: Vec, + #[default(vec![])] metrics: Vec>, + process: Process, + region_id: RegionID, +) -> Vec { + // If no assets provided, create default candidate assets based on number of metrics + let assets = if assets.is_empty() { + let process_rc = Rc::new(process); + (0..metrics.len()) + .map(|i| { + Asset::new_candidate( + // Use the same process with different reference count + Rc::clone(&process_rc), + region_id.clone(), + Capacity(10.0), + u32::try_from(2010 + i).unwrap(), // Different commission years + ) + .unwrap() + }) + .collect() + } else { + assets + }; + + assert_eq!( + assets.len(), + metrics.len(), + "assets and metrics must have the same length" + ); + + assets + .into_iter() + .zip(metrics) + .map(|(asset, metric)| AppraisalOutput { + asset: AssetRef::from(asset), + capacity: AssetCapacity::Continuous(Capacity(10.0)), + coefficients: ObjectiveCoefficients { + capacity_coefficient: MoneyPerCapacity(0.0), + activity_coefficients: IndexMap::new(), + unmet_demand_coefficient: MoneyPerFlow(0.0), + }, + activity: IndexMap::new(), + demand: IndexMap::new(), + unmet_demand: IndexMap::new(), + metric, + }) + .collect() +} + #[cfg(test)] mod tests { use super::*; diff --git a/src/simulation/investment.rs b/src/simulation/investment.rs index 4cf6c189..6c9255c4 100644 --- a/src/simulation/investment.rs +++ b/src/simulation/investment.rs @@ -735,15 +735,8 @@ fn select_best_assets( )?; // Sort assets by appraisal metric - let assets_sorted_by_metric = outputs_for_opts - .into_iter() - .filter(|output| output.capacity.total_capacity() > Capacity(0.0)) - .sorted_by(|output1, output2| match output1.compare_metric(output2) { - // If equal, we fall back on comparing asset properties - Ordering::Equal => compare_asset_fallback(&output1.asset, &output2.asset), - cmp => cmp, - }) - .collect_vec(); + let assets_sorted_by_metric = + sort_appraisal_outputs_by_investment_priority(outputs_for_opts); // Check if all options have zero capacity. If so, we cannot meet demand, so have to bail // out. @@ -818,6 +811,26 @@ fn compare_asset_fallback(asset1: &Asset, asset2: &Asset) -> Ordering { .cmp(&(asset1.is_commissioned(), asset1.commission_year())) } +/// Sort appraisal outputs by their investment priority. +/// Primarily this will be decided by their appraisal metric. There +/// is a tie-breaker fallback to handle the most common cases of equal metrics. But +/// the function does not guarantee all ties will be resolved. +/// Assets with zero capacity are filtered out (their metric would be NaN and cause the program to panic) +/// +pub fn sort_appraisal_outputs_by_investment_priority( + outputs_for_opts: Vec, +) -> Vec { + outputs_for_opts + .into_iter() + .filter(|output| output.capacity.total_capacity() > Capacity(0.0)) + .sorted_by(|output1, output2| match output1.compare_metric(output2) { + // If equal, we fall back on comparing asset properties + Ordering::Equal => compare_asset_fallback(&output1.asset, &output2.asset), + cmp => cmp, + }) + .collect_vec() +} + /// Update capacity of chosen asset, if needed, and update both asset options and chosen assets fn update_assets( mut best_asset: AssetRef, @@ -863,10 +876,12 @@ fn update_assets( #[cfg(test)] mod tests { + use super::appraisal::{LCOXMetric, MetricTrait}; use super::*; use crate::agent::AgentID; use crate::asset::Asset; use crate::commodity::Commodity; + use crate::fixture::appraisal_outputs; use crate::fixture::{ agent_id, asset, process, process_activity_limits_map, process_flows_map, region_id, svd_commodity, time_slice, time_slice_info, time_slice_info2, @@ -874,7 +889,9 @@ mod tests { use crate::process::{ActivityLimits, FlowType, Process, ProcessFlow}; use crate::region::RegionID; use crate::time_slice::{TimeSliceID, TimeSliceInfo}; + use crate::units::MoneyPerActivity; use crate::units::{Capacity, Flow, FlowPerActivity, MoneyPerFlow}; + use float_cmp::assert_approx_eq; use indexmap::indexmap; use rstest::rstest; use std::rc::Rc; @@ -988,4 +1005,20 @@ mod tests { assert!(compare_asset_fallback(&asset3, &asset2).is_lt()); assert!(compare_asset_fallback(&asset2, &asset3).is_gt()); } + + #[rstest] + fn sort_by_lcox_metric(process: Process, region_id: RegionID) { + let metrics: Vec> = vec![ + Box::new(LCOXMetric::new(MoneyPerActivity(5.0))), + Box::new(LCOXMetric::new(MoneyPerActivity(3.0))), + Box::new(LCOXMetric::new(MoneyPerActivity(7.0))), + ]; + + let outputs = appraisal_outputs(vec![], metrics, process, region_id); + let sorted = sort_appraisal_outputs_by_investment_priority(outputs); + + assert_approx_eq!(f64, sorted[0].metric.value(), 3.0); // Best (lowest) + assert_approx_eq!(f64, sorted[1].metric.value(), 5.0); + assert_approx_eq!(f64, sorted[2].metric.value(), 7.0); // Worst (highest) + } } From 2734d1f07e61daba50e171e9d02fb54e8357c4ee Mon Sep 17 00:00:00 2001 From: Aurash Karimi Date: Wed, 21 Jan 2026 10:22:19 +0000 Subject: [PATCH 02/11] move some appraisal logic and tests to appraisal.rs from investment.rs simplified appraisal_outputs fixture --- src/fixture.rs | 58 +------- src/simulation/investment.rs | 90 +------------ src/simulation/investment/appraisal.rs | 126 +++++++++++++++++- .../investment/appraisal/coefficients.rs | 2 +- 4 files changed, 130 insertions(+), 146 deletions(-) diff --git a/src/fixture.rs b/src/fixture.rs index ea0d4cfa..aa43feb6 100644 --- a/src/fixture.rs +++ b/src/fixture.rs @@ -16,7 +16,7 @@ use crate::process::{ use crate::region::RegionID; use crate::simulation::investment::appraisal::LCOXMetric; use crate::simulation::investment::appraisal::{ - AppraisalOutput, MetricTrait, coefficients::ObjectiveCoefficients, + AppraisalOutput, coefficients::ObjectiveCoefficients, }; use crate::time_slice::{TimeSliceID, TimeSliceInfo, TimeSliceLevel}; use crate::units::{ @@ -396,62 +396,6 @@ pub fn appraisal_output(asset: Asset, time_slice: TimeSliceID) -> AppraisalOutpu } } -/// Creates appraisal outputs from assets with corresponding metrics. If no assets provided, -/// creates default candidate assets based on the number of metrics. -/// -/// # Panics -/// Panics if `assets` and `metrics` have different lengths -#[fixture] -pub fn appraisal_outputs( - #[default(vec![])] assets: Vec, - #[default(vec![])] metrics: Vec>, - process: Process, - region_id: RegionID, -) -> Vec { - // If no assets provided, create default candidate assets based on number of metrics - let assets = if assets.is_empty() { - let process_rc = Rc::new(process); - (0..metrics.len()) - .map(|i| { - Asset::new_candidate( - // Use the same process with different reference count - Rc::clone(&process_rc), - region_id.clone(), - Capacity(10.0), - u32::try_from(2010 + i).unwrap(), // Different commission years - ) - .unwrap() - }) - .collect() - } else { - assets - }; - - assert_eq!( - assets.len(), - metrics.len(), - "assets and metrics must have the same length" - ); - - assets - .into_iter() - .zip(metrics) - .map(|(asset, metric)| AppraisalOutput { - asset: AssetRef::from(asset), - capacity: AssetCapacity::Continuous(Capacity(10.0)), - coefficients: ObjectiveCoefficients { - capacity_coefficient: MoneyPerCapacity(0.0), - activity_coefficients: IndexMap::new(), - unmet_demand_coefficient: MoneyPerFlow(0.0), - }, - activity: IndexMap::new(), - demand: IndexMap::new(), - unmet_demand: IndexMap::new(), - metric, - }) - .collect() -} - #[cfg(test)] mod tests { use super::*; diff --git a/src/simulation/investment.rs b/src/simulation/investment.rs index 6c9255c4..3bb82564 100644 --- a/src/simulation/investment.rs +++ b/src/simulation/investment.rs @@ -13,13 +13,14 @@ use anyhow::{Context, Result, ensure}; use indexmap::IndexMap; use itertools::{Itertools, chain}; use log::debug; -use std::cmp::Ordering; use std::collections::HashMap; use std::fmt::Display; pub mod appraisal; use appraisal::coefficients::calculate_coefficients_for_assets; -use appraisal::{AppraisalOutput, appraise_investment}; +use appraisal::{ + AppraisalOutput, appraise_investment, sort_appraisal_outputs_by_investment_priority, +}; /// A map of demand across time slices for a specific market type DemandMap = IndexMap; @@ -801,36 +802,6 @@ fn is_any_remaining_demand(demand: &DemandMap) -> bool { demand.values().any(|flow| *flow > Flow(0.0)) } -/// Compare assets as a fallback if metrics are equal. -/// -/// Commissioned assets are ordered before uncommissioned and newer before older. -/// -/// Used as a fallback to sort assets when they have equal appraisal tool outputs. -fn compare_asset_fallback(asset1: &Asset, asset2: &Asset) -> Ordering { - (asset2.is_commissioned(), asset2.commission_year()) - .cmp(&(asset1.is_commissioned(), asset1.commission_year())) -} - -/// Sort appraisal outputs by their investment priority. -/// Primarily this will be decided by their appraisal metric. There -/// is a tie-breaker fallback to handle the most common cases of equal metrics. But -/// the function does not guarantee all ties will be resolved. -/// Assets with zero capacity are filtered out (their metric would be NaN and cause the program to panic) -/// -pub fn sort_appraisal_outputs_by_investment_priority( - outputs_for_opts: Vec, -) -> Vec { - outputs_for_opts - .into_iter() - .filter(|output| output.capacity.total_capacity() > Capacity(0.0)) - .sorted_by(|output1, output2| match output1.compare_metric(output2) { - // If equal, we fall back on comparing asset properties - Ordering::Equal => compare_asset_fallback(&output1.asset, &output2.asset), - cmp => cmp, - }) - .collect_vec() -} - /// Update capacity of chosen asset, if needed, and update both asset options and chosen assets fn update_assets( mut best_asset: AssetRef, @@ -876,22 +847,15 @@ fn update_assets( #[cfg(test)] mod tests { - use super::appraisal::{LCOXMetric, MetricTrait}; use super::*; - use crate::agent::AgentID; - use crate::asset::Asset; use crate::commodity::Commodity; - use crate::fixture::appraisal_outputs; use crate::fixture::{ - agent_id, asset, process, process_activity_limits_map, process_flows_map, region_id, - svd_commodity, time_slice, time_slice_info, time_slice_info2, + asset, process, process_activity_limits_map, process_flows_map, svd_commodity, time_slice, + time_slice_info, time_slice_info2, }; use crate::process::{ActivityLimits, FlowType, Process, ProcessFlow}; - use crate::region::RegionID; use crate::time_slice::{TimeSliceID, TimeSliceInfo}; - use crate::units::MoneyPerActivity; use crate::units::{Capacity, Flow, FlowPerActivity, MoneyPerFlow}; - use float_cmp::assert_approx_eq; use indexmap::indexmap; use rstest::rstest; use std::rc::Rc; @@ -977,48 +941,4 @@ mod tests { // Maximum = 20.0 assert_eq!(result, Capacity(20.0)); } - - #[rstest] - fn compare_assets_fallback(process: Process, region_id: RegionID, agent_id: AgentID) { - let process = Rc::new(process); - let capacity = Capacity(2.0); - let asset1 = Asset::new_commissioned( - agent_id.clone(), - process.clone(), - region_id.clone(), - capacity, - 2015, - ) - .unwrap(); - let asset2 = - Asset::new_candidate(process.clone(), region_id.clone(), capacity, 2015).unwrap(); - let asset3 = - Asset::new_commissioned(agent_id, process, region_id.clone(), capacity, 2010).unwrap(); - - assert!(compare_asset_fallback(&asset1, &asset1).is_eq()); - assert!(compare_asset_fallback(&asset2, &asset2).is_eq()); - assert!(compare_asset_fallback(&asset3, &asset3).is_eq()); - assert!(compare_asset_fallback(&asset1, &asset2).is_lt()); - assert!(compare_asset_fallback(&asset2, &asset1).is_gt()); - assert!(compare_asset_fallback(&asset1, &asset3).is_lt()); - assert!(compare_asset_fallback(&asset3, &asset1).is_gt()); - assert!(compare_asset_fallback(&asset3, &asset2).is_lt()); - assert!(compare_asset_fallback(&asset2, &asset3).is_gt()); - } - - #[rstest] - fn sort_by_lcox_metric(process: Process, region_id: RegionID) { - let metrics: Vec> = vec![ - Box::new(LCOXMetric::new(MoneyPerActivity(5.0))), - Box::new(LCOXMetric::new(MoneyPerActivity(3.0))), - Box::new(LCOXMetric::new(MoneyPerActivity(7.0))), - ]; - - let outputs = appraisal_outputs(vec![], metrics, process, region_id); - let sorted = sort_appraisal_outputs_by_investment_priority(outputs); - - assert_approx_eq!(f64, sorted[0].metric.value(), 3.0); // Best (lowest) - assert_approx_eq!(f64, sorted[1].metric.value(), 5.0); - assert_approx_eq!(f64, sorted[2].metric.value(), 7.0); // Worst (highest) - } } diff --git a/src/simulation/investment/appraisal.rs b/src/simulation/investment/appraisal.rs index 5eef02cf..77269c72 100644 --- a/src/simulation/investment/appraisal.rs +++ b/src/simulation/investment/appraisal.rs @@ -1,12 +1,12 @@ //! Calculation for investment tools such as Levelised Cost of X (LCOX) and Net Present Value (NPV). use super::DemandMap; use crate::agent::ObjectiveType; -use crate::asset::{AssetCapacity, AssetRef}; +use crate::asset::{Asset, AssetCapacity, AssetRef}; use crate::commodity::Commodity; use crate::finance::{ProfitabilityIndex, lcox, profitability_index}; use crate::model::Model; use crate::time_slice::TimeSliceID; -use crate::units::{Activity, Money, MoneyPerActivity, MoneyPerCapacity}; +use crate::units::{Activity, Capacity, Money, MoneyPerActivity, MoneyPerCapacity}; use anyhow::Result; use costs::annual_fixed_cost; use erased_serde::Serialize as ErasedSerialize; @@ -22,6 +22,7 @@ mod optimisation; use coefficients::ObjectiveCoefficients; use float_cmp::approx_eq; use float_cmp::{ApproxEq, F64Margin}; +use itertools::Itertools; use optimisation::perform_optimisation; /// Compares two values with approximate equality checking. @@ -325,12 +326,48 @@ pub fn appraise_investment( appraisal_method(model, asset, max_capacity, commodity, coefficients, demand) } +/// Compare assets as a fallback if metrics are equal. +/// +/// Commissioned assets are ordered before uncommissioned and newer before older. +/// +/// Used as a fallback to sort assets when they have equal appraisal tool outputs. +fn compare_asset_fallback(asset1: &Asset, asset2: &Asset) -> Ordering { + (asset2.is_commissioned(), asset2.commission_year()) + .cmp(&(asset1.is_commissioned(), asset1.commission_year())) +} + +/// Sort appraisal outputs by their investment priority. +/// Primarily this will be decided by their appraisal metric. There +/// is a tie-breaker fallback to handle the most common cases of equal metrics. But +/// the function does not guarantee all ties will be resolved. +/// Assets with zero capacity are filtered out (their metric would be NaN and cause the program to panic) +/// +pub fn sort_appraisal_outputs_by_investment_priority( + outputs_for_opts: Vec, +) -> Vec { + outputs_for_opts + .into_iter() + .filter(|output| output.capacity.total_capacity() > Capacity(0.0)) + .sorted_by(|output1, output2| match output1.compare_metric(output2) { + // If equal, we fall back on comparing asset properties + Ordering::Equal => compare_asset_fallback(&output1.asset, &output2.asset), + cmp => cmp, + }) + .collect_vec() +} + #[cfg(test)] mod tests { use super::*; + use crate::agent::AgentID; use crate::finance::ProfitabilityIndex; + use crate::fixture::{agent_id, asset, process, region_id}; + use crate::process::Process; + use crate::region::RegionID; use crate::units::{Money, MoneyPerActivity}; - use rstest::rstest; + use float_cmp::assert_approx_eq; + use rstest::{fixture, rstest}; + use std::rc::Rc; /// Parametrised tests for LCOX metric comparison. #[rstest] @@ -465,4 +502,87 @@ mod tests { "Failed comparison for case: {description}" ); } + + /// Creates appraisal outputs from assets with corresponding metrics. If no assets provided, + /// creates default candidate assets based on the number of metrics. + /// + /// # Panics + /// Panics if `assets` and `metrics` have different lengths + #[fixture] + pub fn appraisal_outputs( + #[default(vec![])] assets: Vec, + #[default(vec![])] metrics: Vec>, + asset: Asset, + ) -> Vec { + // If no assets provided, create default candidate assets based on number of metrics + let assets = if assets.is_empty() { + vec![asset.clone(); metrics.len()] + } else { + assets + }; + + assert_eq!( + assets.len(), + metrics.len(), + "assets and metrics must have the same length" + ); + + assets + .into_iter() + .zip(metrics) + .map(|(asset, metric)| AppraisalOutput { + asset: AssetRef::from(asset), + capacity: AssetCapacity::Continuous(Capacity(10.0)), + coefficients: ObjectiveCoefficients::default(), + activity: IndexMap::new(), + demand: IndexMap::new(), + unmet_demand: IndexMap::new(), + metric, + }) + .collect() + } + + #[rstest] + fn compare_assets_fallback(process: Process, region_id: RegionID, agent_id: AgentID) { + let process = Rc::new(process); + let capacity = Capacity(2.0); + let asset1 = Asset::new_commissioned( + agent_id.clone(), + process.clone(), + region_id.clone(), + capacity, + 2015, + ) + .unwrap(); + let asset2 = + Asset::new_candidate(process.clone(), region_id.clone(), capacity, 2015).unwrap(); + let asset3 = + Asset::new_commissioned(agent_id, process, region_id.clone(), capacity, 2010).unwrap(); + + assert!(compare_asset_fallback(&asset1, &asset1).is_eq()); + assert!(compare_asset_fallback(&asset2, &asset2).is_eq()); + assert!(compare_asset_fallback(&asset3, &asset3).is_eq()); + assert!(compare_asset_fallback(&asset1, &asset2).is_lt()); + assert!(compare_asset_fallback(&asset2, &asset1).is_gt()); + assert!(compare_asset_fallback(&asset1, &asset3).is_lt()); + assert!(compare_asset_fallback(&asset3, &asset1).is_gt()); + assert!(compare_asset_fallback(&asset3, &asset2).is_lt()); + assert!(compare_asset_fallback(&asset2, &asset3).is_gt()); + } + + #[rstest] + fn sort_by_lcox_metric(asset: Asset) { + let metrics: Vec> = vec![ + Box::new(LCOXMetric::new(MoneyPerActivity(5.0))), + Box::new(LCOXMetric::new(MoneyPerActivity(3.0))), + Box::new(LCOXMetric::new(MoneyPerActivity(7.0))), + ]; + + let outputs = appraisal_outputs(vec![], metrics, asset); + let sorted = sort_appraisal_outputs_by_investment_priority(outputs); + + assert_approx_eq!(f64, sorted[0].metric.value(), 3.0); // Best (lowest) + assert_approx_eq!(f64, sorted[1].metric.value(), 5.0); + assert_approx_eq!(f64, sorted[2].metric.value(), 7.0); // Worst (highest) + } } diff --git a/src/simulation/investment/appraisal/coefficients.rs b/src/simulation/investment/appraisal/coefficients.rs index 0c17f2b7..b5ad85a3 100644 --- a/src/simulation/investment/appraisal/coefficients.rs +++ b/src/simulation/investment/appraisal/coefficients.rs @@ -14,7 +14,7 @@ use std::collections::HashMap; /// These coefficients are calculated according to the agent's `ObjectiveType` and are used by /// the investment appraisal routines. The map contains the per-capacity and per-activity cost /// coefficients used in the appraisal optimisation, together with the unmet-demand penalty. -#[derive(Clone)] +#[derive(Clone, Default)] pub struct ObjectiveCoefficients { /// Cost per unit of capacity pub capacity_coefficient: MoneyPerCapacity, From 9976bd4e828af702533b3905759864dfacd284ab Mon Sep 17 00:00:00 2001 From: Aurash Karimi Date: Wed, 21 Jan 2026 11:03:09 +0000 Subject: [PATCH 03/11] add appraisal output ordering tests --- src/simulation/investment/appraisal.rs | 164 ++++++++++++++++++++++++- 1 file changed, 163 insertions(+), 1 deletion(-) diff --git a/src/simulation/investment/appraisal.rs b/src/simulation/investment/appraisal.rs index 77269c72..dd458bc6 100644 --- a/src/simulation/investment/appraisal.rs +++ b/src/simulation/investment/appraisal.rs @@ -570,8 +570,9 @@ mod tests { assert!(compare_asset_fallback(&asset2, &asset3).is_gt()); } + /// Test sorting by LCOX metric when invariant to asset properties #[rstest] - fn sort_by_lcox_metric(asset: Asset) { + fn appraisal_sort_by_lcox_metric(asset: Asset) { let metrics: Vec> = vec![ Box::new(LCOXMetric::new(MoneyPerActivity(5.0))), Box::new(LCOXMetric::new(MoneyPerActivity(3.0))), @@ -585,4 +586,165 @@ mod tests { assert_approx_eq!(f64, sorted[1].metric.value(), 5.0); assert_approx_eq!(f64, sorted[2].metric.value(), 7.0); // Worst (highest) } + + /// Test sorting by NPV profitability index when invariant to asset properties + #[rstest] + fn appraisal_sort_by_npv_metric(asset: Asset) { + let metrics: Vec> = vec![ + Box::new(NPVMetric::new(ProfitabilityIndex { + total_annualised_surplus: Money(200.0), + annualised_fixed_cost: Money(100.0), + })), + Box::new(NPVMetric::new(ProfitabilityIndex { + total_annualised_surplus: Money(300.0), + annualised_fixed_cost: Money(100.0), + })), + Box::new(NPVMetric::new(ProfitabilityIndex { + total_annualised_surplus: Money(150.0), + annualised_fixed_cost: Money(100.0), + })), + ]; + + let outputs = appraisal_outputs(vec![], metrics, asset); + let sorted = sort_appraisal_outputs_by_investment_priority(outputs); + + // Higher profitability index is better, so should be sorted: 3.0, 2.0, 1.5 + assert_approx_eq!(f64, sorted[0].metric.value(), 3.0); // Best (highest PI) + assert_approx_eq!(f64, sorted[1].metric.value(), 2.0); + assert_approx_eq!(f64, sorted[2].metric.value(), 1.5); // Worst (lowest PI) + } + + /// Test that NPV metrics with zero annual fixed cost are prioritised above all others + /// when invariant to asset properties + #[rstest] + fn appraisal_sort_by_npv_metric_zero_afc_prioritised(asset: Asset) { + let metrics: Vec> = vec![ + // Very high profitability index but non-zero AFC + Box::new(NPVMetric::new(ProfitabilityIndex { + total_annualised_surplus: Money(1000.0), + annualised_fixed_cost: Money(100.0), + })), + // Zero AFC with modest surplus - should be prioritised first + Box::new(NPVMetric::new(ProfitabilityIndex { + total_annualised_surplus: Money(50.0), + annualised_fixed_cost: Money(0.0), + })), + // Another high profitability index but non-zero AFC + Box::new(NPVMetric::new(ProfitabilityIndex { + total_annualised_surplus: Money(500.0), + annualised_fixed_cost: Money(50.0), + })), + ]; + + let outputs = appraisal_outputs(vec![], metrics, asset); + let sorted = sort_appraisal_outputs_by_investment_priority(outputs); + + // Zero AFC should be first despite lower absolute surplus value + assert_approx_eq!(f64, sorted[0].metric.value(), 50.0); // Zero AFC (uses surplus) + assert_approx_eq!(f64, sorted[1].metric.value(), 10.0); // PI = 1000/100 + assert_approx_eq!(f64, sorted[2].metric.value(), 10.0); // PI = 500/50 + } + + /// Test that mixing LCOX and NPV metrics causes a runtime panic during comparison + #[rstest] + #[should_panic(expected = "Cannot compare metrics of different types")] + fn appraisal_sort_by_mixed_metrics_panics(asset: Asset) { + let metrics: Vec> = vec![ + Box::new(LCOXMetric::new(MoneyPerActivity(5.0))), + Box::new(NPVMetric::new(ProfitabilityIndex { + total_annualised_surplus: Money(200.0), + annualised_fixed_cost: Money(100.0), + })), + Box::new(LCOXMetric::new(MoneyPerActivity(3.0))), + ]; + + let outputs = appraisal_outputs(vec![], metrics, asset); + // This should panic when trying to compare different metric types + sort_appraisal_outputs_by_investment_priority(outputs); + } + + /// Test that when metrics are equal, commissioned assets are sorted by commission year (newer first) + #[rstest] + fn appraisal_sort_by_commission_year_when_metrics_equal( + process: Process, + region_id: RegionID, + agent_id: AgentID, + ) { + let process_rc = Rc::new(process); + let capacity = Capacity(10.0); + let commission_years = [2015, 2020, 2010]; + + let assets: Vec<_> = commission_years + .iter() + .map(|&year| { + Asset::new_commissioned( + agent_id.clone(), + process_rc.clone(), + region_id.clone(), + capacity, + year, + ) + .unwrap() + }) + .collect(); + + // All metrics have the same value + let metrics: Vec> = vec![ + Box::new(LCOXMetric::new(MoneyPerActivity(5.0))), + Box::new(LCOXMetric::new(MoneyPerActivity(5.0))), + Box::new(LCOXMetric::new(MoneyPerActivity(5.0))), + ]; + + let outputs = appraisal_outputs( + assets, + metrics, + Asset::new_commissioned(agent_id, process_rc, region_id, capacity, 2015).unwrap(), + ); + let sorted = sort_appraisal_outputs_by_investment_priority(outputs); + + // Should be sorted by commission year, newest first: 2020, 2015, 2010 + assert_eq!(sorted[0].asset.commission_year(), 2020); + assert_eq!(sorted[1].asset.commission_year(), 2015); + assert_eq!(sorted[2].asset.commission_year(), 2010); + } + + /// Test that when metrics and commission years are equal, the original order is preserved + #[rstest] + fn appraisal_sort_maintains_order_when_all_equal(process: Process, region_id: RegionID) { + let process_rc = Rc::new(process); + let capacity = Capacity(10.0); + let commission_year = 2015; + let agent_ids = ["agent1", "agent2", "agent3"]; + + let assets: Vec<_> = agent_ids + .iter() + .map(|&id| { + Asset::new_commissioned( + AgentID(id.into()), + process_rc.clone(), + region_id.clone(), + capacity, + commission_year, + ) + .unwrap() + }) + .collect(); + + let metrics: Vec> = vec![ + Box::new(LCOXMetric::new(MoneyPerActivity(5.0))), + Box::new(LCOXMetric::new(MoneyPerActivity(5.0))), + Box::new(LCOXMetric::new(MoneyPerActivity(5.0))), + ]; + + let outputs = appraisal_outputs(assets.clone(), metrics, assets[0].clone()); + let sorted = sort_appraisal_outputs_by_investment_priority(outputs); + + // Verify order is preserved - should match the original agent_ids array + for (i, &expected_id) in agent_ids.iter().enumerate() { + assert_eq!( + sorted[i].asset.agent_id(), + Some(&AgentID(expected_id.into())) + ); + } + } } From 701b7084548bdd00b63fb026d17c34d98cefa7e2 Mon Sep 17 00:00:00 2001 From: Aurash Karimi Date: Wed, 21 Jan 2026 11:13:00 +0000 Subject: [PATCH 04/11] rename variable for clarity --- src/simulation/investment.rs | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/src/simulation/investment.rs b/src/simulation/investment.rs index 3bb82564..0ed9a44a 100644 --- a/src/simulation/investment.rs +++ b/src/simulation/investment.rs @@ -735,8 +735,7 @@ fn select_best_assets( &outputs_for_opts, )?; - // Sort assets by appraisal metric - let assets_sorted_by_metric = + let assets_sorted_by_investment_priority = sort_appraisal_outputs_by_investment_priority(outputs_for_opts); // Check if all options have zero capacity. If so, we cannot meet demand, so have to bail @@ -748,20 +747,23 @@ fn select_best_assets( // - known issue with the NPV objective // (see https://github.com/EnergySystemsModellingLab/MUSE2/issues/716). ensure!( - !assets_sorted_by_metric.is_empty(), + !assets_sorted_by_investment_priority.is_empty(), "No feasible investment options for commodity '{}' after appraisal", &commodity.id ); // Warn if there are multiple equally good assets warn_on_equal_appraisal_outputs( - &assets_sorted_by_metric, + &assets_sorted_by_investment_priority, &agent.id, &commodity.id, region_id, ); - let best_output = assets_sorted_by_metric.into_iter().next().unwrap(); + let best_output = assets_sorted_by_investment_priority + .into_iter() + .next() + .unwrap(); // Log the selected asset debug!( From a0b9e66fbb027bdf190c322483e19ceb6df07c24 Mon Sep 17 00:00:00 2001 From: Aurash Karimi Date: Wed, 21 Jan 2026 11:21:11 +0000 Subject: [PATCH 05/11] add a test for zero capacity sorting of appraisal outputs --- src/simulation/investment/appraisal.rs | 42 +++++++++++++++++++++++--- 1 file changed, 38 insertions(+), 4 deletions(-) diff --git a/src/simulation/investment/appraisal.rs b/src/simulation/investment/appraisal.rs index dd458bc6..1124cb0e 100644 --- a/src/simulation/investment/appraisal.rs +++ b/src/simulation/investment/appraisal.rs @@ -337,10 +337,15 @@ fn compare_asset_fallback(asset1: &Asset, asset2: &Asset) -> Ordering { } /// Sort appraisal outputs by their investment priority. -/// Primarily this will be decided by their appraisal metric. There -/// is a tie-breaker fallback to handle the most common cases of equal metrics. But -/// the function does not guarantee all ties will be resolved. -/// Assets with zero capacity are filtered out (their metric would be NaN and cause the program to panic) +/// +/// Primarily this is decided by their appraisal metric. +/// When appraisal metrics are equal, a tie-breaker fallback is used. Commissioned assets +/// are preferred over uncommissioned assets, and newer assets are preferred over older +/// ones. The function does not guarantee that all ties will be resolved. +/// +/// Assets with zero capacity are filtered out before sorting, +/// as their metric would be `NaN` and could cause the program to panic. So the length +/// of the returned vector may be less than the input. /// pub fn sort_appraisal_outputs_by_investment_priority( outputs_for_opts: Vec, @@ -747,4 +752,33 @@ mod tests { ); } } + + /// Test that appraisal outputs with zero capacity are filtered out during sorting. + #[rstest] + fn appraisal_sort_filters_zero_capacity_outputs(asset: Asset) { + let metrics: Vec> = vec![ + Box::new(LCOXMetric::new(MoneyPerActivity(f64::NAN))), + Box::new(LCOXMetric::new(MoneyPerActivity(f64::NAN))), + Box::new(LCOXMetric::new(MoneyPerActivity(f64::NAN))), + ]; + + // Create outputs with zero capacity + let outputs: Vec = metrics + .into_iter() + .map(|metric| AppraisalOutput { + asset: AssetRef::from(asset.clone()), + capacity: AssetCapacity::Continuous(Capacity(0.0)), + coefficients: ObjectiveCoefficients::default(), + activity: IndexMap::new(), + demand: IndexMap::new(), + unmet_demand: IndexMap::new(), + metric, + }) + .collect(); + + let sorted = sort_appraisal_outputs_by_investment_priority(outputs); + + // All zero capacity outputs should be filtered out + assert_eq!(sorted.len(), 0); + } } From 80bca5054465a20b80ee8903e6c8daa8dd927b2b Mon Sep 17 00:00:00 2001 From: Aurash Karimi Date: Wed, 21 Jan 2026 11:28:59 +0000 Subject: [PATCH 06/11] improve doc wording --- src/simulation/investment/appraisal.rs | 62 +++++++++++++------------- 1 file changed, 31 insertions(+), 31 deletions(-) diff --git a/src/simulation/investment/appraisal.rs b/src/simulation/investment/appraisal.rs index 1124cb0e..36808898 100644 --- a/src/simulation/investment/appraisal.rs +++ b/src/simulation/investment/appraisal.rs @@ -508,18 +508,46 @@ mod tests { ); } + #[rstest] + fn compare_assets_fallback(process: Process, region_id: RegionID, agent_id: AgentID) { + let process = Rc::new(process); + let capacity = Capacity(2.0); + let asset1 = Asset::new_commissioned( + agent_id.clone(), + process.clone(), + region_id.clone(), + capacity, + 2015, + ) + .unwrap(); + let asset2 = + Asset::new_candidate(process.clone(), region_id.clone(), capacity, 2015).unwrap(); + let asset3 = + Asset::new_commissioned(agent_id, process, region_id.clone(), capacity, 2010).unwrap(); + + assert!(compare_asset_fallback(&asset1, &asset1).is_eq()); + assert!(compare_asset_fallback(&asset2, &asset2).is_eq()); + assert!(compare_asset_fallback(&asset3, &asset3).is_eq()); + assert!(compare_asset_fallback(&asset1, &asset2).is_lt()); + assert!(compare_asset_fallback(&asset2, &asset1).is_gt()); + assert!(compare_asset_fallback(&asset1, &asset3).is_lt()); + assert!(compare_asset_fallback(&asset3, &asset1).is_gt()); + assert!(compare_asset_fallback(&asset3, &asset2).is_lt()); + assert!(compare_asset_fallback(&asset2, &asset3).is_gt()); + } + /// Creates appraisal outputs from assets with corresponding metrics. If no assets provided, - /// creates default candidate assets based on the number of metrics. + /// Uses a default asset for each metric. /// /// # Panics /// Panics if `assets` and `metrics` have different lengths #[fixture] - pub fn appraisal_outputs( + fn appraisal_outputs( #[default(vec![])] assets: Vec, #[default(vec![])] metrics: Vec>, asset: Asset, ) -> Vec { - // If no assets provided, create default candidate assets based on number of metrics + // If no assets provided, repeat the default asset for each metric. let assets = if assets.is_empty() { vec![asset.clone(); metrics.len()] } else { @@ -547,34 +575,6 @@ mod tests { .collect() } - #[rstest] - fn compare_assets_fallback(process: Process, region_id: RegionID, agent_id: AgentID) { - let process = Rc::new(process); - let capacity = Capacity(2.0); - let asset1 = Asset::new_commissioned( - agent_id.clone(), - process.clone(), - region_id.clone(), - capacity, - 2015, - ) - .unwrap(); - let asset2 = - Asset::new_candidate(process.clone(), region_id.clone(), capacity, 2015).unwrap(); - let asset3 = - Asset::new_commissioned(agent_id, process, region_id.clone(), capacity, 2010).unwrap(); - - assert!(compare_asset_fallback(&asset1, &asset1).is_eq()); - assert!(compare_asset_fallback(&asset2, &asset2).is_eq()); - assert!(compare_asset_fallback(&asset3, &asset3).is_eq()); - assert!(compare_asset_fallback(&asset1, &asset2).is_lt()); - assert!(compare_asset_fallback(&asset2, &asset1).is_gt()); - assert!(compare_asset_fallback(&asset1, &asset3).is_lt()); - assert!(compare_asset_fallback(&asset3, &asset1).is_gt()); - assert!(compare_asset_fallback(&asset3, &asset2).is_lt()); - assert!(compare_asset_fallback(&asset2, &asset3).is_gt()); - } - /// Test sorting by LCOX metric when invariant to asset properties #[rstest] fn appraisal_sort_by_lcox_metric(asset: Asset) { From bcf5afa95dd09548b67f6c782c9f7ccca514bbfc Mon Sep 17 00:00:00 2001 From: Aurash Karimi Date: Fri, 23 Jan 2026 09:51:23 +0000 Subject: [PATCH 07/11] swap appraisal output copy for mut ref --- src/simulation/investment.rs | 18 ++---- src/simulation/investment/appraisal.rs | 76 ++++++++++++-------------- 2 files changed, 39 insertions(+), 55 deletions(-) diff --git a/src/simulation/investment.rs b/src/simulation/investment.rs index a2748233..c5caea73 100644 --- a/src/simulation/investment.rs +++ b/src/simulation/investment.rs @@ -13,7 +13,6 @@ use anyhow::{Context, Result, ensure}; use indexmap::IndexMap; use itertools::{Itertools, chain}; use log::debug; -use std::cmp::Ordering; use std::collections::{HashMap, HashSet}; use std::fmt::Display; @@ -747,8 +746,7 @@ fn select_best_assets( &outputs_for_opts, )?; - let assets_sorted_by_investment_priority = - sort_appraisal_outputs_by_investment_priority(outputs_for_opts); + sort_appraisal_outputs_by_investment_priority(&mut outputs_for_opts); // Check if all options have zero capacity. If so, we cannot meet demand, so have to bail // out. @@ -759,23 +757,15 @@ fn select_best_assets( // - known issue with the NPV objective // (see https://github.com/EnergySystemsModellingLab/MUSE2/issues/716). ensure!( - !assets_sorted_by_investment_priority.is_empty(), + !outputs_for_opts.is_empty(), "No feasible investment options for commodity '{}' after appraisal", &commodity.id ); // Warn if there are multiple equally good assets - warn_on_equal_appraisal_outputs( - &assets_sorted_by_investment_priority, - &agent.id, - &commodity.id, - region_id, - ); + warn_on_equal_appraisal_outputs(&outputs_for_opts, &agent.id, &commodity.id, region_id); - let best_output = assets_sorted_by_investment_priority - .into_iter() - .next() - .unwrap(); + let best_output = outputs_for_opts.into_iter().next().unwrap(); // Log the selected asset debug!( diff --git a/src/simulation/investment/appraisal.rs b/src/simulation/investment/appraisal.rs index 36808898..4542863e 100644 --- a/src/simulation/investment/appraisal.rs +++ b/src/simulation/investment/appraisal.rs @@ -22,7 +22,6 @@ mod optimisation; use coefficients::ObjectiveCoefficients; use float_cmp::approx_eq; use float_cmp::{ApproxEq, F64Margin}; -use itertools::Itertools; use optimisation::perform_optimisation; /// Compares two values with approximate equality checking. @@ -347,18 +346,13 @@ fn compare_asset_fallback(asset1: &Asset, asset2: &Asset) -> Ordering { /// as their metric would be `NaN` and could cause the program to panic. So the length /// of the returned vector may be less than the input. /// -pub fn sort_appraisal_outputs_by_investment_priority( - outputs_for_opts: Vec, -) -> Vec { - outputs_for_opts - .into_iter() - .filter(|output| output.capacity.total_capacity() > Capacity(0.0)) - .sorted_by(|output1, output2| match output1.compare_metric(output2) { - // If equal, we fall back on comparing asset properties - Ordering::Equal => compare_asset_fallback(&output1.asset, &output2.asset), - cmp => cmp, - }) - .collect_vec() +pub fn sort_appraisal_outputs_by_investment_priority(outputs_for_opts: &mut Vec) { + outputs_for_opts.retain(|output| output.capacity.total_capacity() > Capacity(0.0)); + outputs_for_opts.sort_by(|output1, output2| match output1.compare_metric(output2) { + // If equal, we fall back on comparing asset properties + Ordering::Equal => compare_asset_fallback(&output1.asset, &output2.asset), + cmp => cmp, + }); } #[cfg(test)] @@ -584,12 +578,12 @@ mod tests { Box::new(LCOXMetric::new(MoneyPerActivity(7.0))), ]; - let outputs = appraisal_outputs(vec![], metrics, asset); - let sorted = sort_appraisal_outputs_by_investment_priority(outputs); + let mut outputs = appraisal_outputs(vec![], metrics, asset); + sort_appraisal_outputs_by_investment_priority(&mut outputs); - assert_approx_eq!(f64, sorted[0].metric.value(), 3.0); // Best (lowest) - assert_approx_eq!(f64, sorted[1].metric.value(), 5.0); - assert_approx_eq!(f64, sorted[2].metric.value(), 7.0); // Worst (highest) + assert_approx_eq!(f64, outputs[0].metric.value(), 3.0); // Best (lowest) + assert_approx_eq!(f64, outputs[1].metric.value(), 5.0); + assert_approx_eq!(f64, outputs[2].metric.value(), 7.0); // Worst (highest) } /// Test sorting by NPV profitability index when invariant to asset properties @@ -610,13 +604,13 @@ mod tests { })), ]; - let outputs = appraisal_outputs(vec![], metrics, asset); - let sorted = sort_appraisal_outputs_by_investment_priority(outputs); + let mut outputs = appraisal_outputs(vec![], metrics, asset); + sort_appraisal_outputs_by_investment_priority(&mut outputs); // Higher profitability index is better, so should be sorted: 3.0, 2.0, 1.5 - assert_approx_eq!(f64, sorted[0].metric.value(), 3.0); // Best (highest PI) - assert_approx_eq!(f64, sorted[1].metric.value(), 2.0); - assert_approx_eq!(f64, sorted[2].metric.value(), 1.5); // Worst (lowest PI) + assert_approx_eq!(f64, outputs[0].metric.value(), 3.0); // Best (highest PI) + assert_approx_eq!(f64, outputs[1].metric.value(), 2.0); + assert_approx_eq!(f64, outputs[2].metric.value(), 1.5); // Worst (lowest PI) } /// Test that NPV metrics with zero annual fixed cost are prioritised above all others @@ -641,13 +635,13 @@ mod tests { })), ]; - let outputs = appraisal_outputs(vec![], metrics, asset); - let sorted = sort_appraisal_outputs_by_investment_priority(outputs); + let mut outputs = appraisal_outputs(vec![], metrics, asset); + sort_appraisal_outputs_by_investment_priority(&mut outputs); // Zero AFC should be first despite lower absolute surplus value - assert_approx_eq!(f64, sorted[0].metric.value(), 50.0); // Zero AFC (uses surplus) - assert_approx_eq!(f64, sorted[1].metric.value(), 10.0); // PI = 1000/100 - assert_approx_eq!(f64, sorted[2].metric.value(), 10.0); // PI = 500/50 + assert_approx_eq!(f64, outputs[0].metric.value(), 50.0); // Zero AFC (uses surplus) + assert_approx_eq!(f64, outputs[1].metric.value(), 10.0); // PI = 1000/100 + assert_approx_eq!(f64, outputs[2].metric.value(), 10.0); // PI = 500/50 } /// Test that mixing LCOX and NPV metrics causes a runtime panic during comparison @@ -663,9 +657,9 @@ mod tests { Box::new(LCOXMetric::new(MoneyPerActivity(3.0))), ]; - let outputs = appraisal_outputs(vec![], metrics, asset); + let mut outputs = appraisal_outputs(vec![], metrics, asset); // This should panic when trying to compare different metric types - sort_appraisal_outputs_by_investment_priority(outputs); + sort_appraisal_outputs_by_investment_priority(&mut outputs); } /// Test that when metrics are equal, commissioned assets are sorted by commission year (newer first) @@ -700,17 +694,17 @@ mod tests { Box::new(LCOXMetric::new(MoneyPerActivity(5.0))), ]; - let outputs = appraisal_outputs( + let mut outputs = appraisal_outputs( assets, metrics, Asset::new_commissioned(agent_id, process_rc, region_id, capacity, 2015).unwrap(), ); - let sorted = sort_appraisal_outputs_by_investment_priority(outputs); + sort_appraisal_outputs_by_investment_priority(&mut outputs); // Should be sorted by commission year, newest first: 2020, 2015, 2010 - assert_eq!(sorted[0].asset.commission_year(), 2020); - assert_eq!(sorted[1].asset.commission_year(), 2015); - assert_eq!(sorted[2].asset.commission_year(), 2010); + assert_eq!(outputs[0].asset.commission_year(), 2020); + assert_eq!(outputs[1].asset.commission_year(), 2015); + assert_eq!(outputs[2].asset.commission_year(), 2010); } /// Test that when metrics and commission years are equal, the original order is preserved @@ -741,13 +735,13 @@ mod tests { Box::new(LCOXMetric::new(MoneyPerActivity(5.0))), ]; - let outputs = appraisal_outputs(assets.clone(), metrics, assets[0].clone()); - let sorted = sort_appraisal_outputs_by_investment_priority(outputs); + let mut outputs = appraisal_outputs(assets.clone(), metrics, assets[0].clone()); + sort_appraisal_outputs_by_investment_priority(&mut outputs); // Verify order is preserved - should match the original agent_ids array for (i, &expected_id) in agent_ids.iter().enumerate() { assert_eq!( - sorted[i].asset.agent_id(), + outputs[i].asset.agent_id(), Some(&AgentID(expected_id.into())) ); } @@ -763,7 +757,7 @@ mod tests { ]; // Create outputs with zero capacity - let outputs: Vec = metrics + let mut outputs: Vec = metrics .into_iter() .map(|metric| AppraisalOutput { asset: AssetRef::from(asset.clone()), @@ -776,9 +770,9 @@ mod tests { }) .collect(); - let sorted = sort_appraisal_outputs_by_investment_priority(outputs); + sort_appraisal_outputs_by_investment_priority(&mut outputs); // All zero capacity outputs should be filtered out - assert_eq!(sorted.len(), 0); + assert_eq!(outputs.len(), 0); } } From 7f06bda65fcf0b6b7b61d78d6b3aa6531c76a416 Mon Sep 17 00:00:00 2001 From: Aurash Karimi Date: Fri, 23 Jan 2026 11:09:23 +0000 Subject: [PATCH 08/11] make appraisal output tests cleaner --- src/simulation/investment/appraisal.rs | 49 ++++++++++--------- .../investment/appraisal/coefficients.rs | 3 +- 2 files changed, 27 insertions(+), 25 deletions(-) diff --git a/src/simulation/investment/appraisal.rs b/src/simulation/investment/appraisal.rs index 4542863e..59b65a6d 100644 --- a/src/simulation/investment/appraisal.rs +++ b/src/simulation/investment/appraisal.rs @@ -365,7 +365,7 @@ mod tests { use crate::region::RegionID; use crate::units::{Money, MoneyPerActivity}; use float_cmp::assert_approx_eq; - use rstest::{fixture, rstest}; + use rstest::rstest; use std::rc::Rc; /// Parametrised tests for LCOX metric comparison. @@ -530,24 +530,14 @@ mod tests { assert!(compare_asset_fallback(&asset2, &asset3).is_gt()); } - /// Creates appraisal outputs from assets with corresponding metrics. If no assets provided, - /// Uses a default asset for each metric. + /// Creates appraisal from corresponding assets and metrics /// /// # Panics /// Panics if `assets` and `metrics` have different lengths - #[fixture] fn appraisal_outputs( - #[default(vec![])] assets: Vec, - #[default(vec![])] metrics: Vec>, - asset: Asset, + assets: Vec, + metrics: Vec>, ) -> Vec { - // If no assets provided, repeat the default asset for each metric. - let assets = if assets.is_empty() { - vec![asset.clone(); metrics.len()] - } else { - assets - }; - assert_eq!( assets.len(), metrics.len(), @@ -569,6 +559,17 @@ mod tests { .collect() } + /// Creates appraisal outputs with given metrics. + /// Copies the provided default asset for each metric. + fn appraisal_outputs_with_investment_priority_invariant_to_assets( + metrics: Vec>, + asset: &Asset, + ) -> Vec { + // If no assets provided, repeat the default asset for each metric. + let assets = vec![asset.clone(); metrics.len()]; + appraisal_outputs(assets, metrics) + } + /// Test sorting by LCOX metric when invariant to asset properties #[rstest] fn appraisal_sort_by_lcox_metric(asset: Asset) { @@ -578,7 +579,8 @@ mod tests { Box::new(LCOXMetric::new(MoneyPerActivity(7.0))), ]; - let mut outputs = appraisal_outputs(vec![], metrics, asset); + let mut outputs = + appraisal_outputs_with_investment_priority_invariant_to_assets(metrics, &asset); sort_appraisal_outputs_by_investment_priority(&mut outputs); assert_approx_eq!(f64, outputs[0].metric.value(), 3.0); // Best (lowest) @@ -604,7 +606,8 @@ mod tests { })), ]; - let mut outputs = appraisal_outputs(vec![], metrics, asset); + let mut outputs = + appraisal_outputs_with_investment_priority_invariant_to_assets(metrics, &asset); sort_appraisal_outputs_by_investment_priority(&mut outputs); // Higher profitability index is better, so should be sorted: 3.0, 2.0, 1.5 @@ -635,7 +638,8 @@ mod tests { })), ]; - let mut outputs = appraisal_outputs(vec![], metrics, asset); + let mut outputs = + appraisal_outputs_with_investment_priority_invariant_to_assets(metrics, &asset); sort_appraisal_outputs_by_investment_priority(&mut outputs); // Zero AFC should be first despite lower absolute surplus value @@ -657,7 +661,8 @@ mod tests { Box::new(LCOXMetric::new(MoneyPerActivity(3.0))), ]; - let mut outputs = appraisal_outputs(vec![], metrics, asset); + let mut outputs = + appraisal_outputs_with_investment_priority_invariant_to_assets(metrics, &asset); // This should panic when trying to compare different metric types sort_appraisal_outputs_by_investment_priority(&mut outputs); } @@ -694,11 +699,7 @@ mod tests { Box::new(LCOXMetric::new(MoneyPerActivity(5.0))), ]; - let mut outputs = appraisal_outputs( - assets, - metrics, - Asset::new_commissioned(agent_id, process_rc, region_id, capacity, 2015).unwrap(), - ); + let mut outputs = appraisal_outputs(assets, metrics); sort_appraisal_outputs_by_investment_priority(&mut outputs); // Should be sorted by commission year, newest first: 2020, 2015, 2010 @@ -735,7 +736,7 @@ mod tests { Box::new(LCOXMetric::new(MoneyPerActivity(5.0))), ]; - let mut outputs = appraisal_outputs(assets.clone(), metrics, assets[0].clone()); + let mut outputs = appraisal_outputs(assets.clone(), metrics); sort_appraisal_outputs_by_investment_priority(&mut outputs); // Verify order is preserved - should match the original agent_ids array diff --git a/src/simulation/investment/appraisal/coefficients.rs b/src/simulation/investment/appraisal/coefficients.rs index b5ad85a3..e09511e5 100644 --- a/src/simulation/investment/appraisal/coefficients.rs +++ b/src/simulation/investment/appraisal/coefficients.rs @@ -14,7 +14,8 @@ use std::collections::HashMap; /// These coefficients are calculated according to the agent's `ObjectiveType` and are used by /// the investment appraisal routines. The map contains the per-capacity and per-activity cost /// coefficients used in the appraisal optimisation, together with the unmet-demand penalty. -#[derive(Clone, Default)] +#[derive(Clone)] +#[cfg_attr(test, derive(Default))] pub struct ObjectiveCoefficients { /// Cost per unit of capacity pub capacity_coefficient: MoneyPerCapacity, From 5144c77f730bd879c3867f567227d470c05b4dd9 Mon Sep 17 00:00:00 2001 From: Aurash Karimi Date: Fri, 23 Jan 2026 11:24:33 +0000 Subject: [PATCH 09/11] add more test coverage of sort_appraisal_outputs_by_investment_priority --- src/simulation/investment/appraisal.rs | 124 ++++++++++++++++++++++++- 1 file changed, 119 insertions(+), 5 deletions(-) diff --git a/src/simulation/investment/appraisal.rs b/src/simulation/investment/appraisal.rs index 59b65a6d..bd25b2d4 100644 --- a/src/simulation/investment/appraisal.rs +++ b/src/simulation/investment/appraisal.rs @@ -740,14 +740,128 @@ mod tests { sort_appraisal_outputs_by_investment_priority(&mut outputs); // Verify order is preserved - should match the original agent_ids array - for (i, &expected_id) in agent_ids.iter().enumerate() { - assert_eq!( - outputs[i].asset.agent_id(), - Some(&AgentID(expected_id.into())) - ); + for (&expected_id, output) in agent_ids.iter().zip(outputs) { + assert_eq!(output.asset.agent_id(), Some(&AgentID(expected_id.into()))); } } + /// Test that commissioned assets are prioritised over non-commissioned assets when metrics are equal + #[rstest] + fn appraisal_sort_commissioned_before_uncommissioned_when_metrics_equal( + process: Process, + region_id: RegionID, + agent_id: AgentID, + ) { + let process_rc = Rc::new(process); + let capacity = Capacity(10.0); + + // Create a mix of commissioned and candidate (non-commissioned) assets + let commissioned_asset_newer = Asset::new_commissioned( + agent_id.clone(), + process_rc.clone(), + region_id.clone(), + capacity, + 2020, + ) + .unwrap(); + + let commissioned_asset_older = Asset::new_commissioned( + agent_id.clone(), + process_rc.clone(), + region_id.clone(), + capacity, + 2015, + ) + .unwrap(); + + let candidate_asset = + Asset::new_candidate(process_rc.clone(), region_id.clone(), capacity, 2020).unwrap(); + + let assets = vec![ + candidate_asset.clone(), + commissioned_asset_older.clone(), + candidate_asset.clone(), + commissioned_asset_newer.clone(), + ]; + + // All metrics have identical values to test fallback ordering + let metrics: Vec> = vec![ + Box::new(LCOXMetric::new(MoneyPerActivity(5.0))), + Box::new(LCOXMetric::new(MoneyPerActivity(5.0))), + Box::new(LCOXMetric::new(MoneyPerActivity(5.0))), + Box::new(LCOXMetric::new(MoneyPerActivity(5.0))), + ]; + + let mut outputs = appraisal_outputs(assets, metrics); + sort_appraisal_outputs_by_investment_priority(&mut outputs); + + // Commissioned assets should be prioritised first + assert!(outputs[0].asset.is_commissioned()); + assert!(outputs[0].asset.commission_year() == 2020); + assert!(outputs[1].asset.is_commissioned()); + assert!(outputs[1].asset.commission_year() == 2015); + + // Non-commissioned assets should come after + assert!(!outputs[2].asset.is_commissioned()); + assert!(!outputs[3].asset.is_commissioned()); + } + + /// Test that appraisal metric is prioritised over asset properties when sorting + #[rstest] + fn appraisal_metric_is_priorited_over_asset_properties( + process: Process, + region_id: RegionID, + agent_id: AgentID, + ) { + let process_rc = Rc::new(process); + let capacity = Capacity(10.0); + + // Create a mix of commissioned and candidate (non-commissioned) assets + let commissioned_asset_newer = Asset::new_commissioned( + agent_id.clone(), + process_rc.clone(), + region_id.clone(), + capacity, + 2020, + ) + .unwrap(); + + let commissioned_asset_older = Asset::new_commissioned( + agent_id.clone(), + process_rc.clone(), + region_id.clone(), + capacity, + 2015, + ) + .unwrap(); + + let candidate_asset = + Asset::new_candidate(process_rc.clone(), region_id.clone(), capacity, 2020).unwrap(); + + let assets = vec![ + candidate_asset.clone(), + commissioned_asset_older.clone(), + candidate_asset.clone(), + commissioned_asset_newer.clone(), + ]; + + // Make one metric slightly better than all others + let baseline_metric_value = 5.0; + let best_metric_value = baseline_metric_value - 1e-5; + let metrics: Vec> = vec![ + Box::new(LCOXMetric::new(MoneyPerActivity(best_metric_value))), + Box::new(LCOXMetric::new(MoneyPerActivity(baseline_metric_value))), + Box::new(LCOXMetric::new(MoneyPerActivity(baseline_metric_value))), + Box::new(LCOXMetric::new(MoneyPerActivity(baseline_metric_value))), + ]; + + let mut outputs = appraisal_outputs(assets, metrics); + sort_appraisal_outputs_by_investment_priority(&mut outputs); + + // non-commissioned asset prioritised because it has a slightly better metric + assert_approx_eq!(f64, outputs[0].metric.value(), best_metric_value); + } + /// Test that appraisal outputs with zero capacity are filtered out during sorting. #[rstest] fn appraisal_sort_filters_zero_capacity_outputs(asset: Asset) { From f8892221eb3449d1ba5728552de311e06f4fb666 Mon Sep 17 00:00:00 2001 From: Aurash Karimi Date: Fri, 23 Jan 2026 11:25:41 +0000 Subject: [PATCH 10/11] remove misleading comment --- src/simulation/investment/appraisal.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/simulation/investment/appraisal.rs b/src/simulation/investment/appraisal.rs index bd25b2d4..8899cfe4 100644 --- a/src/simulation/investment/appraisal.rs +++ b/src/simulation/investment/appraisal.rs @@ -565,7 +565,6 @@ mod tests { metrics: Vec>, asset: &Asset, ) -> Vec { - // If no assets provided, repeat the default asset for each metric. let assets = vec![asset.clone(); metrics.len()]; appraisal_outputs(assets, metrics) } From e7ff372c297f9655742aaaa0efa4d5cf42dfcd06 Mon Sep 17 00:00:00 2001 From: Aurashk Date: Fri, 23 Jan 2026 11:30:07 +0000 Subject: [PATCH 11/11] Update src/simulation/investment/appraisal.rs Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- src/simulation/investment/appraisal.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/simulation/investment/appraisal.rs b/src/simulation/investment/appraisal.rs index 8899cfe4..85e97808 100644 --- a/src/simulation/investment/appraisal.rs +++ b/src/simulation/investment/appraisal.rs @@ -807,7 +807,7 @@ mod tests { /// Test that appraisal metric is prioritised over asset properties when sorting #[rstest] - fn appraisal_metric_is_priorited_over_asset_properties( + fn appraisal_metric_is_prioritised_over_asset_properties( process: Process, region_id: RegionID, agent_id: AgentID,