From ad8f3d27362a0703eb0765896d0be113d7357a02 Mon Sep 17 00:00:00 2001 From: Alex Dewar Date: Thu, 19 Mar 2026 12:56:11 +0000 Subject: [PATCH 1/3] Fix: Docs wrongly states that NPV is broken NPV is now not broken and users are free to use it, though in the docs for input files it wrongly states that NPV is broken. Update docs accordingly. --- schemas/input/agent_objectives.yaml | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/schemas/input/agent_objectives.yaml b/schemas/input/agent_objectives.yaml index 2d920dfc6..04c778c93 100644 --- a/schemas/input/agent_objectives.yaml +++ b/schemas/input/agent_objectives.yaml @@ -17,18 +17,16 @@ fields: type: string description: The year(s) to which this entry applies notes: - One or more milestone years separated by semicolons, `all` to select all years or a year - range in the form 'start..end' to select all valid years within range, inclusive. Either 'start' + One or more milestone years separated by semicolons, `all` to select all years or a year range + in the form 'start..end' to select all valid years within range, inclusive. Either 'start' 'end' or both can be omitted, which will set the corresponding limit to the minimum or maximum valid year, respectively. - name: objective_type type: string - enum: [lcox, npv] + enum: [npv, lcox] description: The type of objective notes: | - Must be `npv` (net present value) or `lcox` (levelised cost of X). Note that support for NPV - is [currently broken](https://github.com/EnergySystemsModellingLab/MUSE2/issues/716), so don't - enable this option unless you know what you're doing. + Must be `npv` (net present value) or `lcox` (levelised cost of X). - name: decision_weight type: number description: Weight for weighted sum decision rule From e174be83e23a9c680959e876eb0f45c621725d7c Mon Sep 17 00:00:00 2001 From: Alex Dewar Date: Thu, 19 Mar 2026 12:57:06 +0000 Subject: [PATCH 2/3] investment.rs: Remove stale comment referring to #716 --- src/simulation/investment.rs | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/simulation/investment.rs b/src/simulation/investment.rs index 5e9716a7f..a1a449336 100644 --- a/src/simulation/investment.rs +++ b/src/simulation/investment.rs @@ -801,11 +801,8 @@ fn select_best_assets( // Check if all options have zero capacity. If so, we cannot meet demand, so have to bail // out. // - // This may happen if: - // - the asset has zero activity limits for all time slices with + // This may happen if the asset has zero activity limits for all time slices with // demand. - // - known issue with the NPV objective - // (see https://github.com/EnergySystemsModellingLab/MUSE2/issues/716). if outputs_for_opts.is_empty() { let remaining_demands: Vec<_> = demand .iter() From b77107938b8249a67bd49298993239bba3610300 Mon Sep 17 00:00:00 2001 From: Alex Dewar Date: Thu, 19 Mar 2026 15:33:59 +0000 Subject: [PATCH 3/3] Add comments and rename function to clarify appraisal output filtering behaviour --- src/simulation/investment.rs | 10 +++---- src/simulation/investment/appraisal.rs | 40 +++++++++++++------------- 2 files changed, 24 insertions(+), 26 deletions(-) diff --git a/src/simulation/investment.rs b/src/simulation/investment.rs index a1a449336..30777a306 100644 --- a/src/simulation/investment.rs +++ b/src/simulation/investment.rs @@ -20,7 +20,7 @@ pub mod appraisal; use appraisal::coefficients::calculate_coefficients_for_assets; use appraisal::{ AppraisalOutput, appraise_investment, count_equal_and_best_appraisal_outputs, - sort_appraisal_outputs_by_investment_priority, + sort_and_filter_appraisal_outputs, }; /// A map of demand across time slices for a specific market @@ -796,13 +796,11 @@ fn select_best_assets( &demand, )?; - sort_appraisal_outputs_by_investment_priority(&mut outputs_for_opts); + // Sort by investment priority and discord non-feasible options + sort_and_filter_appraisal_outputs(&mut outputs_for_opts); - // Check if all options have zero capacity. If so, we cannot meet demand, so have to bail + // Check if there are any remaining options. If not, we cannot meet demand, so have to bail // out. - // - // This may happen if the asset has zero activity limits for all time slices with - // demand. if outputs_for_opts.is_empty() { let remaining_demands: Vec<_> = demand .iter() diff --git a/src/simulation/investment/appraisal.rs b/src/simulation/investment/appraisal.rs index acbb303cb..c32a0e9f3 100644 --- a/src/simulation/investment/appraisal.rs +++ b/src/simulation/investment/appraisal.rs @@ -358,18 +358,18 @@ fn compare_asset_fallback(asset1: &Asset, asset2: &Asset) -> Ordering { .cmp(&(asset1.is_commissioned(), asset1.commission_year())) } -/// Sort appraisal outputs by their investment priority. +/// Sort appraisal outputs by their investment priority and exclude non-feasible options. /// -/// 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. +/// Investment priority is primarily decided by 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. /// -/// 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) { +/// 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_and_filter_appraisal_outputs(outputs_for_opts: &mut Vec) { 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 @@ -626,7 +626,7 @@ mod tests { let mut outputs = appraisal_outputs_with_investment_priority_invariant_to_assets(metrics, &asset); - sort_appraisal_outputs_by_investment_priority(&mut outputs); + sort_and_filter_appraisal_outputs(&mut outputs); 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); @@ -653,7 +653,7 @@ mod tests { let mut outputs = appraisal_outputs_with_investment_priority_invariant_to_assets(metrics, &asset); - sort_appraisal_outputs_by_investment_priority(&mut outputs); + sort_and_filter_appraisal_outputs(&mut outputs); // Higher profitability index is better, so should be sorted: 3.0, 2.0, 1.5 assert_approx_eq!(f64, outputs[0].metric.as_ref().unwrap().value(), 3.0); // Best (highest PI) @@ -685,7 +685,7 @@ mod tests { let mut outputs = appraisal_outputs_with_investment_priority_invariant_to_assets(metrics, &asset); - sort_appraisal_outputs_by_investment_priority(&mut outputs); + sort_and_filter_appraisal_outputs(&mut outputs); // Zero AFC should be first despite lower absolute surplus value assert_approx_eq!(f64, outputs[0].metric.as_ref().unwrap().value(), 50.0); // Zero AFC (uses surplus) @@ -709,7 +709,7 @@ mod tests { 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); + sort_and_filter_appraisal_outputs(&mut outputs); } /// Test that when metrics are equal, commissioned assets are sorted by commission year (newer first) @@ -745,7 +745,7 @@ mod tests { ]; let mut outputs = appraisal_outputs(assets, metrics); - sort_appraisal_outputs_by_investment_priority(&mut outputs); + sort_and_filter_appraisal_outputs(&mut outputs); // Should be sorted by commission year, newest first: 2020, 2015, 2010 assert_eq!(outputs[0].asset.commission_year(), 2020); @@ -782,7 +782,7 @@ mod tests { ]; let mut outputs = appraisal_outputs(assets.clone(), metrics); - sort_appraisal_outputs_by_investment_priority(&mut outputs); + sort_and_filter_appraisal_outputs(&mut outputs); // Verify order is preserved - should match the original agent_ids array for (&expected_id, output) in agent_ids.iter().zip(outputs) { @@ -838,7 +838,7 @@ mod tests { ]; let mut outputs = appraisal_outputs(assets, metrics); - sort_appraisal_outputs_by_investment_priority(&mut outputs); + sort_and_filter_appraisal_outputs(&mut outputs); // Commissioned assets should be prioritised first assert!(outputs[0].asset.is_commissioned()); @@ -901,7 +901,7 @@ mod tests { ]; let mut outputs = appraisal_outputs(assets, metrics); - sort_appraisal_outputs_by_investment_priority(&mut outputs); + sort_and_filter_appraisal_outputs(&mut outputs); // non-commissioned asset prioritised because it has a slightly better metric assert_approx_eq!( @@ -934,7 +934,7 @@ mod tests { }) .collect(); - sort_appraisal_outputs_by_investment_priority(&mut outputs); + sort_and_filter_appraisal_outputs(&mut outputs); // All zero capacity outputs should be filtered out assert_eq!(outputs.len(), 0); @@ -953,7 +953,7 @@ mod tests { }; let mut outputs = vec![output]; - sort_appraisal_outputs_by_investment_priority(&mut outputs); + sort_and_filter_appraisal_outputs(&mut outputs); // The invalid output should have been filtered out assert_eq!(outputs.len(), 0);