Merge pull request #1214 from EnergySystemsModellingLab/fix-schema-npv-not-broken

Fix: Docs wrongly states that NPV is broken

Author: alexdewar

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

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,16 +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.
-        // - 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()

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<AppraisalOutput>) {
+/// 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<AppraisalOutput>) {
     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);