From 2da91a057bdea01d6130cf6f0495d358db1c35f5 Mon Sep 17 00:00:00 2001 From: Alex Dewar Date: Fri, 13 Feb 2026 14:39:15 +0000 Subject: [PATCH 1/9] Drop `demand` field from `AppraisalOutput` The demand map is the same for all `AppraisalOutput`s for a given appraisal iteration, so there's no need to store it. If many assets are being appraised, it could result in many clones. --- src/fixture.rs | 2 -- src/output.rs | 7 ++++++- src/simulation/investment.rs | 1 + src/simulation/investment/appraisal.rs | 6 ------ 4 files changed, 7 insertions(+), 9 deletions(-) diff --git a/src/fixture.rs b/src/fixture.rs index a3ef113d5..2e9188bf2 100644 --- a/src/fixture.rs +++ b/src/fixture.rs @@ -382,7 +382,6 @@ pub fn time_slice_info2() -> TimeSliceInfo { pub fn appraisal_output(asset: Asset, time_slice: TimeSliceID) -> AppraisalOutput { let activity_coefficients = indexmap! { time_slice.clone() => MoneyPerActivity(0.5) }; let activity = indexmap! { time_slice.clone() => Activity(10.0) }; - let demand = indexmap! { time_slice.clone() => Flow(100.0) }; let unmet_demand = indexmap! { time_slice.clone() => Flow(5.0) }; AppraisalOutput { asset: AssetRef::from(asset), @@ -393,7 +392,6 @@ pub fn appraisal_output(asset: Asset, time_slice: TimeSliceID) -> AppraisalOutpu unmet_demand_coefficient: MoneyPerFlow(10000.0), }, activity, - demand, unmet_demand, metric: Box::new(LCOXMetric::new(MoneyPerActivity(4.14))), } diff --git a/src/output.rs b/src/output.rs index edd3df829..67f403183 100644 --- a/src/output.rs +++ b/src/output.rs @@ -467,11 +467,12 @@ impl DebugDataWriter { milestone_year: u32, run_description: &str, appraisal_results: &[AppraisalOutput], + demand: &IndexMap, ) -> Result<()> { for result in appraisal_results { for (time_slice, activity) in &result.activity { let activity_coefficient = result.coefficients.activity_coefficients[time_slice]; - let demand = result.demand[time_slice]; + let demand = demand[time_slice]; let unmet_demand = result.unmet_demand[time_slice]; let row = AppraisalResultsTimeSliceRow { milestone_year, @@ -564,6 +565,7 @@ impl DataWriter { milestone_year: u32, run_description: &str, appraisal_results: &[AppraisalOutput], + demand: &IndexMap, ) -> Result<()> { if let Some(wtr) = &mut self.debug_writer { wtr.write_appraisal_results(milestone_year, run_description, appraisal_results)?; @@ -571,6 +573,7 @@ impl DataWriter { milestone_year, run_description, appraisal_results, + demand, )?; } @@ -1006,6 +1009,7 @@ mod tests { let milestone_year = 2020; let run_description = "test_run".to_string(); let dir = tempdir().unwrap(); + let demand = indexmap! {time_slice.clone() => Flow(100.0) }; // Write appraisal time slice results { @@ -1015,6 +1019,7 @@ mod tests { milestone_year, &run_description, &[appraisal_output], + &demand, ) .unwrap(); writer.flush().unwrap(); diff --git a/src/simulation/investment.rs b/src/simulation/investment.rs index 2a2f13157..688bd524d 100644 --- a/src/simulation/investment.rs +++ b/src/simulation/investment.rs @@ -796,6 +796,7 @@ fn select_best_assets( year, &format!("{} {} round {}", &commodity.id, &agent.id, round), &outputs_for_opts, + &demand, )?; sort_appraisal_outputs_by_investment_priority(&mut outputs_for_opts); diff --git a/src/simulation/investment/appraisal.rs b/src/simulation/investment/appraisal.rs index 906bff3cb..380314f95 100644 --- a/src/simulation/investment/appraisal.rs +++ b/src/simulation/investment/appraisal.rs @@ -61,8 +61,6 @@ pub struct AppraisalOutput { pub metric: Box, /// Capacity and activity coefficients used in the appraisal pub coefficients: ObjectiveCoefficients, - /// Demand profile used in the appraisal - pub demand: DemandMap, } impl AppraisalOutput { @@ -252,7 +250,6 @@ fn calculate_lcox( unmet_demand: results.unmet_demand, metric: Box::new(LCOXMetric::new(cost_index)), coefficients: coefficients.clone(), - demand: demand.clone(), }) } @@ -299,7 +296,6 @@ fn calculate_npv( unmet_demand: results.unmet_demand, metric: Box::new(NPVMetric::new(profitability_index)), coefficients: coefficients.clone(), - demand: demand.clone(), }) } @@ -557,7 +553,6 @@ mod tests { unmet_demand_coefficient: MoneyPerFlow(0.0), }, activity: IndexMap::new(), - demand: IndexMap::new(), unmet_demand: IndexMap::new(), metric, }) @@ -887,7 +882,6 @@ mod tests { unmet_demand_coefficient: MoneyPerFlow(0.0), }, activity: IndexMap::new(), - demand: IndexMap::new(), unmet_demand: IndexMap::new(), metric, }) From ab8137f3d374d38812aad5429bb56873e1720043 Mon Sep 17 00:00:00 2001 From: Alex Dewar Date: Fri, 13 Feb 2026 14:47:44 +0000 Subject: [PATCH 2/9] Make `ObjectiveCoefficients` `Rc` to remove need for deep copy --- src/fixture.rs | 4 +-- src/simulation/investment/appraisal.rs | 29 ++++++++++--------- .../investment/appraisal/coefficients.rs | 5 ++-- 3 files changed, 20 insertions(+), 18 deletions(-) diff --git a/src/fixture.rs b/src/fixture.rs index 2e9188bf2..a9e02c391 100644 --- a/src/fixture.rs +++ b/src/fixture.rs @@ -386,11 +386,11 @@ pub fn appraisal_output(asset: Asset, time_slice: TimeSliceID) -> AppraisalOutpu AppraisalOutput { asset: AssetRef::from(asset), capacity: AssetCapacity::Continuous(Capacity(42.0)), - coefficients: ObjectiveCoefficients { + coefficients: Rc::new(ObjectiveCoefficients { capacity_coefficient: MoneyPerCapacity(2.14), activity_coefficients, unmet_demand_coefficient: MoneyPerFlow(10000.0), - }, + }), activity, unmet_demand, metric: Box::new(LCOXMetric::new(MoneyPerActivity(4.14))), diff --git a/src/simulation/investment/appraisal.rs b/src/simulation/investment/appraisal.rs index 380314f95..6221eaac1 100644 --- a/src/simulation/investment/appraisal.rs +++ b/src/simulation/investment/appraisal.rs @@ -14,6 +14,7 @@ use indexmap::IndexMap; use serde::Serialize; use std::any::Any; use std::cmp::Ordering; +use std::rc::Rc; pub mod coefficients; mod constraints; @@ -60,7 +61,7 @@ pub struct AppraisalOutput { /// The comparison metric to compare investment decisions pub metric: Box, /// Capacity and activity coefficients used in the appraisal - pub coefficients: ObjectiveCoefficients, + pub coefficients: Rc, } impl AppraisalOutput { @@ -223,7 +224,7 @@ fn calculate_lcox( asset: &AssetRef, max_capacity: Option, commodity: &Commodity, - coefficients: &ObjectiveCoefficients, + coefficients: &Rc, demand: &DemandMap, ) -> Result { let results = perform_optimisation( @@ -263,7 +264,7 @@ fn calculate_npv( asset: &AssetRef, max_capacity: Option, commodity: &Commodity, - coefficients: &ObjectiveCoefficients, + coefficients: &Rc, demand: &DemandMap, ) -> Result { let results = perform_optimisation( @@ -311,7 +312,7 @@ pub fn appraise_investment( max_capacity: Option, commodity: &Commodity, objective_type: &ObjectiveType, - coefficients: &ObjectiveCoefficients, + coefficients: &Rc, demand: &DemandMap, ) -> Result { let appraisal_method = match objective_type { @@ -526,6 +527,14 @@ mod tests { assert!(compare_asset_fallback(&asset2, &asset3).is_gt()); } + fn objective_coeffs() -> Rc { + Rc::new(ObjectiveCoefficients { + capacity_coefficient: MoneyPerCapacity(0.0), + activity_coefficients: IndexMap::new(), + unmet_demand_coefficient: MoneyPerFlow(0.0), + }) + } + /// Creates appraisal from corresponding assets and metrics /// /// # Panics @@ -547,11 +556,7 @@ mod tests { .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), - }, + coefficients: objective_coeffs(), activity: IndexMap::new(), unmet_demand: IndexMap::new(), metric, @@ -876,11 +881,7 @@ mod tests { .map(|metric| AppraisalOutput { asset: AssetRef::from(asset.clone()), capacity: AssetCapacity::Continuous(Capacity(0.0)), - coefficients: ObjectiveCoefficients { - capacity_coefficient: MoneyPerCapacity(0.0), - activity_coefficients: IndexMap::new(), - unmet_demand_coefficient: MoneyPerFlow(0.0), - }, + coefficients: objective_coeffs(), activity: IndexMap::new(), unmet_demand: IndexMap::new(), metric, diff --git a/src/simulation/investment/appraisal/coefficients.rs b/src/simulation/investment/appraisal/coefficients.rs index 0c17f2b74..286b93535 100644 --- a/src/simulation/investment/appraisal/coefficients.rs +++ b/src/simulation/investment/appraisal/coefficients.rs @@ -8,6 +8,7 @@ use crate::time_slice::{TimeSliceID, TimeSliceInfo}; use crate::units::{MoneyPerActivity, MoneyPerCapacity, MoneyPerFlow}; use indexmap::IndexMap; use std::collections::HashMap; +use std::rc::Rc; /// Map storing cost coefficients for an asset. /// @@ -31,7 +32,7 @@ pub fn calculate_coefficients_for_assets( assets: &[AssetRef], prices: &CommodityPrices, year: u32, -) -> HashMap { +) -> HashMap> { assets .iter() .map(|asset| { @@ -47,7 +48,7 @@ pub fn calculate_coefficients_for_assets( calculate_coefficients_for_npv(asset, &model.time_slice_info, prices, year) } }; - (asset.clone(), coefficient) + (asset.clone(), Rc::new(coefficient)) }) .collect() } From f792bbcdb02cdd6ddba9edd559ad31c0b495d1f5 Mon Sep 17 00:00:00 2001 From: Alex Dewar Date: Mon, 2 Mar 2026 13:35:49 +0000 Subject: [PATCH 3/9] Remove use of NaN metrics in test --- src/simulation/investment/appraisal.rs | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/simulation/investment/appraisal.rs b/src/simulation/investment/appraisal.rs index 6221eaac1..b8538b92a 100644 --- a/src/simulation/investment/appraisal.rs +++ b/src/simulation/investment/appraisal.rs @@ -869,10 +869,11 @@ 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))), + let metric = LCOXMetric::new(MoneyPerActivity(1.0)); + let metrics = [ + Box::new(metric.clone()), + Box::new(metric.clone()), + Box::new(metric), ]; // Create outputs with zero capacity From a6311af5a6c0eb866e5b2b452a32f1633cf97e1a Mon Sep 17 00:00:00 2001 From: Alex Dewar Date: Mon, 2 Mar 2026 13:48:35 +0000 Subject: [PATCH 4/9] Add `AppraisalOutput::new()` constructor and use --- src/simulation/investment/appraisal.rs | 45 +++++++++++++++++--------- 1 file changed, 29 insertions(+), 16 deletions(-) diff --git a/src/simulation/investment/appraisal.rs b/src/simulation/investment/appraisal.rs index b8538b92a..81605b40d 100644 --- a/src/simulation/investment/appraisal.rs +++ b/src/simulation/investment/appraisal.rs @@ -11,6 +11,7 @@ use anyhow::Result; use costs::annual_fixed_cost; use erased_serde::Serialize as ErasedSerialize; use indexmap::IndexMap; +use optimisation::ResultsMap; use serde::Serialize; use std::any::Any; use std::cmp::Ordering; @@ -65,6 +66,22 @@ pub struct AppraisalOutput { } impl AppraisalOutput { + /// Create a new `AppraisalOutput` + pub fn new( + asset: AssetRef, + results: ResultsMap, + metric: T, + coefficients: Rc, + ) -> Self { + Self { + asset, + capacity: results.capacity, + activity: results.activity, + unmet_demand: results.unmet_demand, + metric: Box::new(metric), + coefficients, + } + } /// Compare this appraisal to another on the basis of the comparison metric. /// /// Note that if the metrics are approximately equal (as determined by the [`approx_eq!`] macro) @@ -244,14 +261,12 @@ fn calculate_lcox( &coefficients.activity_coefficients, ); - Ok(AppraisalOutput { - asset: asset.clone(), - capacity: results.capacity, - activity: results.activity, - unmet_demand: results.unmet_demand, - metric: Box::new(LCOXMetric::new(cost_index)), - coefficients: coefficients.clone(), - }) + Ok(AppraisalOutput::new( + asset.clone(), + results, + LCOXMetric::new(cost_index), + coefficients.clone(), + )) } /// Calculate NPV for a hypothetical investment in the given asset. @@ -290,14 +305,12 @@ fn calculate_npv( &coefficients.activity_coefficients, ); - Ok(AppraisalOutput { - asset: asset.clone(), - capacity: results.capacity, - activity: results.activity, - unmet_demand: results.unmet_demand, - metric: Box::new(NPVMetric::new(profitability_index)), - coefficients: coefficients.clone(), - }) + Ok(AppraisalOutput::new( + asset.clone(), + results, + NPVMetric::new(profitability_index), + coefficients.clone(), + )) } /// Appraise the given investment with the specified objective type From ea17f45e7d1fe621eaa943286c41dffe78f3ac36 Mon Sep 17 00:00:00 2001 From: Alex Dewar Date: Mon, 2 Mar 2026 13:37:20 +0000 Subject: [PATCH 5/9] Add `AppraisalOutput::is_valid` helper to check whether output should be filtered --- src/simulation/investment/appraisal.rs | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/src/simulation/investment/appraisal.rs b/src/simulation/investment/appraisal.rs index 81605b40d..fc2af370e 100644 --- a/src/simulation/investment/appraisal.rs +++ b/src/simulation/investment/appraisal.rs @@ -91,12 +91,23 @@ impl AppraisalOutput { /// depending on the user's platform (e.g. macOS ARM vs. Windows). We want to avoid this, if /// possible, which is why we use a more approximate comparison. pub fn compare_metric(&self, other: &Self) -> Ordering { + assert!( + self.is_valid() && other.is_valid(), + "Cannot compare non-valid outputs" + ); assert!( !(self.metric.value().is_nan() || other.metric.value().is_nan()), "Appraisal metric cannot be NaN" ); self.metric.compare(other.metric.as_ref()) } + + /// Whether this [`AppraisalOutput`] is a valid output. + /// + /// Specifically, it checks whether the calculated capacity is greater than zero. + pub fn is_valid(&self) -> bool { + self.capacity.total_capacity() > Capacity(0.0) + } } /// Supertrait for appraisal metrics that can be serialised and compared. @@ -355,9 +366,8 @@ fn compare_asset_fallback(asset1: &Asset, asset2: &Asset) -> Ordering { /// 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: &mut Vec) { - outputs_for_opts.retain(|output| output.capacity.total_capacity() > Capacity(0.0)); + outputs_for_opts.retain(AppraisalOutput::is_valid); 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), From bdfc3adf0aa54ffe2eb7b78887d5414e390571d0 Mon Sep 17 00:00:00 2001 From: Alex Dewar Date: Mon, 2 Mar 2026 14:06:15 +0000 Subject: [PATCH 6/9] AppraisalOutput: Make `metric` field optional When `None`, it indicates that a valid metric could not be computed. --- src/fixture.rs | 2 +- src/output.rs | 6 +-- src/simulation/investment/appraisal.rs | 54 ++++++++++++++------------ 3 files changed, 34 insertions(+), 28 deletions(-) diff --git a/src/fixture.rs b/src/fixture.rs index a9e02c391..d959ba997 100644 --- a/src/fixture.rs +++ b/src/fixture.rs @@ -393,7 +393,7 @@ pub fn appraisal_output(asset: Asset, time_slice: TimeSliceID) -> AppraisalOutpu }), activity, unmet_demand, - metric: Box::new(LCOXMetric::new(MoneyPerActivity(4.14))), + metric: Some(Box::new(LCOXMetric::new(MoneyPerActivity(4.14)))), } } diff --git a/src/output.rs b/src/output.rs index 67f403183..5d1f3f84a 100644 --- a/src/output.rs +++ b/src/output.rs @@ -220,7 +220,7 @@ struct AppraisalResultsRow { region_id: RegionID, capacity: Capacity, capacity_coefficient: MoneyPerCapacity, - metric: f64, + metric: Option, } /// Represents the appraisal results in a row of the appraisal results CSV file @@ -453,7 +453,7 @@ impl DebugDataWriter { region_id: result.asset.region_id().clone(), capacity: result.capacity.total_capacity(), capacity_coefficient: result.coefficients.capacity_coefficient, - metric: result.metric.value(), + metric: result.metric.as_ref().map(|m| m.value()), }; self.appraisal_results_writer.serialize(row)?; } @@ -989,7 +989,7 @@ mod tests { region_id: asset.region_id().clone(), capacity: Capacity(42.0), capacity_coefficient: MoneyPerCapacity(2.14), - metric: 4.14, + metric: Some(4.14), }; let records: Vec = csv::Reader::from_path(dir.path().join(APPRAISAL_RESULTS_FILE_NAME)) diff --git a/src/simulation/investment/appraisal.rs b/src/simulation/investment/appraisal.rs index fc2af370e..bcab58a6c 100644 --- a/src/simulation/investment/appraisal.rs +++ b/src/simulation/investment/appraisal.rs @@ -60,7 +60,7 @@ pub struct AppraisalOutput { /// The hypothetical unmet demand following investment in this asset pub unmet_demand: DemandMap, /// The comparison metric to compare investment decisions - pub metric: Box, + pub metric: Option>, /// Capacity and activity coefficients used in the appraisal pub coefficients: Rc, } @@ -70,7 +70,7 @@ impl AppraisalOutput { pub fn new( asset: AssetRef, results: ResultsMap, - metric: T, + metric: Option, coefficients: Rc, ) -> Self { Self { @@ -78,7 +78,7 @@ impl AppraisalOutput { capacity: results.capacity, activity: results.activity, unmet_demand: results.unmet_demand, - metric: Box::new(metric), + metric: metric.map(|m| Box::new(m) as Box), coefficients, } } @@ -95,18 +95,20 @@ impl AppraisalOutput { self.is_valid() && other.is_valid(), "Cannot compare non-valid outputs" ); - assert!( - !(self.metric.value().is_nan() || other.metric.value().is_nan()), - "Appraisal metric cannot be NaN" - ); - self.metric.compare(other.metric.as_ref()) + + // We've already checked the metrics aren't `None` in `is_valid` + self.metric + .as_ref() + .unwrap() + .compare(other.metric.as_ref().unwrap().as_ref()) } /// Whether this [`AppraisalOutput`] is a valid output. /// - /// Specifically, it checks whether the calculated capacity is greater than zero. + /// Specifically, it checks whether the metric is a valid value (not `None`) and that the + /// calculated capacity is greater than zero. pub fn is_valid(&self) -> bool { - self.capacity.total_capacity() > Capacity(0.0) + self.metric.is_some() && self.capacity.total_capacity() > Capacity(0.0) } } @@ -275,7 +277,7 @@ fn calculate_lcox( Ok(AppraisalOutput::new( asset.clone(), results, - LCOXMetric::new(cost_index), + Some(LCOXMetric::new(cost_index)), coefficients.clone(), )) } @@ -319,7 +321,7 @@ fn calculate_npv( Ok(AppraisalOutput::new( asset.clone(), results, - NPVMetric::new(profitability_index), + Some(NPVMetric::new(profitability_index)), coefficients.clone(), )) } @@ -582,7 +584,7 @@ mod tests { coefficients: objective_coeffs(), activity: IndexMap::new(), unmet_demand: IndexMap::new(), - metric, + metric: Some(metric), }) .collect() } @@ -610,9 +612,9 @@ mod tests { 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) - assert_approx_eq!(f64, outputs[1].metric.value(), 5.0); - assert_approx_eq!(f64, outputs[2].metric.value(), 7.0); // Worst (highest) + assert_approx_eq!(f64, outputs[0].metric.as_ref().unwrap().value(), 3.0); // Best (lowest) + assert_approx_eq!(f64, outputs[1].metric.as_ref().unwrap().value(), 5.0); + assert_approx_eq!(f64, outputs[2].metric.as_ref().unwrap().value(), 7.0); // Worst (highest) } /// Test sorting by NPV profitability index when invariant to asset properties @@ -638,9 +640,9 @@ mod tests { 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, 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) + assert_approx_eq!(f64, outputs[0].metric.as_ref().unwrap().value(), 3.0); // Best (highest PI) + assert_approx_eq!(f64, outputs[1].metric.as_ref().unwrap().value(), 2.0); + assert_approx_eq!(f64, outputs[2].metric.as_ref().unwrap().value(), 1.5); // Worst (lowest PI) } /// Test that NPV metrics with zero annual fixed cost are prioritised above all others @@ -670,9 +672,9 @@ mod tests { sort_appraisal_outputs_by_investment_priority(&mut outputs); // Zero AFC should be first despite lower absolute surplus value - 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 + assert_approx_eq!(f64, outputs[0].metric.as_ref().unwrap().value(), 50.0); // Zero AFC (uses surplus) + assert_approx_eq!(f64, outputs[1].metric.as_ref().unwrap().value(), 10.0); // PI = 1000/100 + assert_approx_eq!(f64, outputs[2].metric.as_ref().unwrap().value(), 10.0); // PI = 500/50 } /// Test that mixing LCOX and NPV metrics causes a runtime panic during comparison @@ -886,7 +888,11 @@ mod tests { 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); + assert_approx_eq!( + f64, + outputs[0].metric.as_ref().unwrap().value(), + best_metric_value + ); } /// Test that appraisal outputs with zero capacity are filtered out during sorting. @@ -908,7 +914,7 @@ mod tests { coefficients: objective_coeffs(), activity: IndexMap::new(), unmet_demand: IndexMap::new(), - metric, + metric: Some(metric), }) .collect(); From 965accaf343938e867c983f82dbf5b41eb280100 Mon Sep 17 00:00:00 2001 From: Alex Dewar Date: Mon, 2 Mar 2026 14:08:16 +0000 Subject: [PATCH 7/9] lcox: Return `None` if total activity is zero Fixes #1126. --- src/finance.rs | 23 ++++++++++++++++------- src/simulation/investment/appraisal.rs | 21 ++++++++++++++++++++- 2 files changed, 36 insertions(+), 8 deletions(-) diff --git a/src/finance.rs b/src/finance.rs index 495baa54b..bdb296576 100644 --- a/src/finance.rs +++ b/src/finance.rs @@ -74,12 +74,14 @@ pub fn profitability_index( } /// Calculates annual LCOX based on capacity and activity. +/// +/// If the total activity is zero, then it returns `None`, otherwise `Some` LCOX value. pub fn lcox( capacity: Capacity, annual_fixed_cost: MoneyPerCapacity, activity: &IndexMap, activity_costs: &IndexMap, -) -> MoneyPerActivity { +) -> Option { // Calculate the annualised fixed costs let annualised_fixed_cost = annual_fixed_cost * capacity; @@ -92,7 +94,8 @@ pub fn lcox( total_activity_costs += activity_cost * *activity; } - (annualised_fixed_cost + total_activity_costs) / total_activity + (total_activity > Activity(0.0)) + .then(|| (annualised_fixed_cost + total_activity_costs) / total_activity) } #[cfg(test)] @@ -223,20 +226,26 @@ mod tests { 100.0, 50.0, vec![("winter", "day", 10.0), ("summer", "night", 20.0)], vec![("winter", "day", 5.0), ("summer", "night", 3.0)], - 170.33333333333334 // (100*50 + 10*5 + 20*3) / (10+20) = 5110/30 + Some(170.33333333333334) // (100*50 + 10*5 + 20*3) / (10+20) = 5110/30 )] #[case( 50.0, 100.0, vec![("winter", "day", 25.0)], vec![("winter", "day", 0.0)], - 200.0 // (50*100 + 25*0) / 25 = 5000/25 + Some(200.0) // (50*100 + 25*0) / 25 = 5000/25 + )] + #[case( + 50.0, 100.0, + vec![("winter", "day", 0.0)], + vec![("winter", "day", 0.0)], + None // (50*0 + 25*0) / 0 = not feasible )] fn lcox_works( #[case] capacity: f64, #[case] annual_fixed_cost: f64, #[case] activity_data: Vec<(&str, &str, f64)>, #[case] cost_data: Vec<(&str, &str, f64)>, - #[case] expected: f64, + #[case] expected: Option, ) { let activity = activity_data .into_iter() @@ -271,7 +280,7 @@ mod tests { &activity_costs, ); - let expected = MoneyPerActivity(expected); - assert_approx_eq!(MoneyPerActivity, result, expected); + let expected = expected.map(MoneyPerActivity); + assert_approx_eq!(Option, result, expected); } } diff --git a/src/simulation/investment/appraisal.rs b/src/simulation/investment/appraisal.rs index bcab58a6c..895c40d82 100644 --- a/src/simulation/investment/appraisal.rs +++ b/src/simulation/investment/appraisal.rs @@ -277,7 +277,7 @@ fn calculate_lcox( Ok(AppraisalOutput::new( asset.clone(), results, - Some(LCOXMetric::new(cost_index)), + cost_index.map(LCOXMetric::new), coefficients.clone(), )) } @@ -923,4 +923,23 @@ mod tests { // All zero capacity outputs should be filtered out assert_eq!(outputs.len(), 0); } + + /// Test that appraisal outputs with an invalid metric are filtered out + #[rstest] + fn appraisal_sort_filters_invalid_metric(asset: Asset) { + let output = AppraisalOutput { + asset: AssetRef::from(asset), + capacity: AssetCapacity::Continuous(Capacity(1.0)), // non-zero capacity + coefficients: objective_coeffs(), + activity: IndexMap::new(), + unmet_demand: IndexMap::new(), + metric: None, + }; + let mut outputs = vec![output]; + + sort_appraisal_outputs_by_investment_priority(&mut outputs); + + // The invalid output should have been filtered out + assert_eq!(outputs.len(), 0); + } } From f431775be3d52dcc62ca205332a9bd1c147ed86a Mon Sep 17 00:00:00 2001 From: Alex Dewar Date: Mon, 2 Mar 2026 15:17:58 +0000 Subject: [PATCH 8/9] Fix doc comment Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- src/simulation/investment/appraisal.rs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/simulation/investment/appraisal.rs b/src/simulation/investment/appraisal.rs index 895c40d82..f5d240dd8 100644 --- a/src/simulation/investment/appraisal.rs +++ b/src/simulation/investment/appraisal.rs @@ -365,9 +365,10 @@ fn compare_asset_fallback(asset1: &Asset, asset2: &Asset) -> Ordering { /// 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. +/// Before sorting, outputs are filtered using [`AppraisalOutput::is_valid`], which +/// excludes entries with invalid metrics (e.g. `None`) as well as zero capacity. This +/// avoids meaningless or `NaN` appraisal metrics that 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: &mut Vec) { outputs_for_opts.retain(AppraisalOutput::is_valid); outputs_for_opts.sort_by(|output1, output2| match output1.compare_metric(output2) { From ab2fa47acae5514487214a4fa1f8940d963f5096 Mon Sep 17 00:00:00 2001 From: Alex Dewar Date: Mon, 2 Mar 2026 15:18:49 +0000 Subject: [PATCH 9/9] Fix: Make `AppraisalOutput::new()` private --- 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 f5d240dd8..7fbf42d68 100644 --- a/src/simulation/investment/appraisal.rs +++ b/src/simulation/investment/appraisal.rs @@ -67,7 +67,7 @@ pub struct AppraisalOutput { impl AppraisalOutput { /// Create a new `AppraisalOutput` - pub fn new( + fn new( asset: AssetRef, results: ResultsMap, metric: Option,