Merge branch 'main' into prices_tidy

Author: tsmbland

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

schemas/input/commodities.yaml

@@ -30,15 +30,13 @@ fields:
   - name: pricing_strategy
     type: string
     enum:
-      [
-        shadow,
-        marginal,
-        marginal_average,
-        full,
-        full_average,
-        scarcity,
-        unpriced,
-      ]
+      - shadow
+      - marginal
+      - marginal_average
+      - full
+      - full_average
+      - scarcity
+      - unpriced
     description: The pricing strategy for this commodity
     notes: |
       Optional. If specified, must be one of `shadow` (priced using shadow prices from supply-demand
@@ -49,8 +47,8 @@ fields:
       production across assets), `scarcity` (priced adjusted for scarcity), or `unpriced` (not
       priced at all).
 
-      For `svd` and `sed` commodities, this must be one of `shadow`, `marginal`,
-      `marginal_average`, `full` or `full_average`. For `oth` commodities, this must be
+      For `svd` and `sed` commodities, it must be one of `shadow`, `marginal`,
+      `marginal_average`, `full` or `full_average`. For `oth` commodities, it *must* be
       `unpriced`.
 
       If unspecified, the commodity will use the default pricing strategy according to the commodity

src/asset.rs

@@ -1100,6 +1100,41 @@ impl AssetRef {
             f(Some(&self), child);
         }
     }
+
+    /// Get an [`AssetRef`] representing a subset of this parent's children.
+    ///
+    /// # Panics
+    ///
+    /// Panics if this asset is not a parent asset or `num_units` is zero or exceeds the total
+    /// capacity of this asset.
+    pub fn make_partial_parent(&self, num_units: u32) -> Self {
+        assert!(
+            self.is_parent(),
+            "Cannot make a partial parent from a non-parent asset"
+        );
+        assert!(
+            num_units > 0,
+            "Cannot make a partial parent with zero units"
+        );
+
+        let (max_num_units, unit_size) = match self.capacity() {
+            AssetCapacity::Discrete(max_num_units, unit_size) => (max_num_units, unit_size),
+            // We know asset capacity type is discrete as this is a parent asset
+            AssetCapacity::Continuous(_) => unreachable!(),
+        };
+        match num_units.cmp(&max_num_units) {
+            // Make a new Asset with fewer units
+            Ordering::Less => Self::from(Asset {
+                capacity: Cell::new(AssetCapacity::Discrete(num_units, unit_size)),
+                ..Asset::clone(self)
+            }),
+            // Same number of units as self
+            Ordering::Equal => self.clone(),
+            Ordering::Greater => {
+                panic!("Cannot make a partial parent with more units than original")
+            }
+        }
+    }
 }
 
 impl From<Rc<Asset>> for AssetRef {
@@ -1429,6 +1464,64 @@ mod tests {
         assert_eq!(count, 1);
     }
 
+    #[fixture]
+    fn parent_asset(asset_divisible: Asset) -> AssetRef {
+        let asset = AssetRef::from(asset_divisible);
+        let mut parent = None;
+
+        asset.into_for_each_child(&mut 0, |maybe_parent, _| {
+            if parent.is_none() {
+                parent = maybe_parent.cloned();
+            }
+        });
+
+        parent.expect("Divisible asset should create a parent")
+    }
+
+    #[rstest]
+    #[case::subset_of_children(2, false)]
+    #[case::all_children(3, true)]
+    fn make_partial_parent(
+        parent_asset: AssetRef,
+        #[case] num_units: u32,
+        #[case] expect_same_asset: bool,
+    ) {
+        let parent = parent_asset;
+        assert!(parent.is_parent());
+
+        let partial_parent = parent.make_partial_parent(num_units);
+
+        assert!(partial_parent.is_parent());
+        assert_eq!(
+            partial_parent.capacity(),
+            AssetCapacity::Discrete(num_units, Capacity(4.0))
+        );
+        assert_eq!(partial_parent.num_children(), Some(num_units));
+        assert_eq!(partial_parent.group_id(), parent.group_id());
+        assert_eq!(partial_parent.agent_id(), parent.agent_id());
+        assert_eq!(Rc::ptr_eq(&partial_parent.0, &parent.0), expect_same_asset);
+        assert_eq!(parent.capacity(), AssetCapacity::Discrete(3, Capacity(4.0)));
+    }
+
+    #[rstest]
+    #[should_panic(expected = "Cannot make a partial parent from a non-parent asset")]
+    fn make_partial_parent_panics_for_non_parent_asset(asset_divisible: Asset) {
+        let asset = AssetRef::from(asset_divisible);
+        asset.make_partial_parent(1);
+    }
+
+    #[rstest]
+    #[should_panic(expected = "Cannot make a partial parent with zero units")]
+    fn make_partial_parent_panics_for_zero_units(parent_asset: AssetRef) {
+        parent_asset.make_partial_parent(0);
+    }
+
+    #[rstest]
+    #[should_panic(expected = "Cannot make a partial parent with more units than original")]
+    fn make_partial_parent_panics_for_too_many_units(parent_asset: AssetRef) {
+        parent_asset.make_partial_parent(4);
+    }
+
     #[rstest]
     fn asset_commission(process: Process) {
         // Test successful commissioning of Future asset

src/asset/capacity.rs

@@ -13,6 +13,23 @@ pub enum AssetCapacity {
     Discrete(u32, Capacity),
 }
 
+impl AssetCapacity {
+    /// Return the smaller of `self` or `other`.
+    ///
+    /// # Panics
+    ///
+    /// Panics if the comparison is not meaningful. This happens if either `AssetCapacity` contains
+    /// a NaN value, one is discrete and the other continuous or if both are discrete and the unit
+    /// size differs.
+    pub fn min(self, other: AssetCapacity) -> AssetCapacity {
+        match self.partial_cmp(&other) {
+            None => panic!("Comparing invalid AssetCapacity values ({self:?} and {other:?})"),
+            Some(Ordering::Greater) => other,
+            _ => self,
+        }
+    }
+}
+
 impl Add for AssetCapacity {
     type Output = Self;
 
@@ -49,23 +66,15 @@ impl Sub for AssetCapacity {
     }
 }
 
-impl Eq for AssetCapacity {}
-
 impl PartialOrd for AssetCapacity {
     fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
-        Some(self.cmp(other))
-    }
-}
-
-impl Ord for AssetCapacity {
-    fn cmp(&self, other: &Self) -> Ordering {
         match (self, other) {
-            (AssetCapacity::Continuous(a), AssetCapacity::Continuous(b)) => a.total_cmp(b),
+            (AssetCapacity::Continuous(a), AssetCapacity::Continuous(b)) => a.partial_cmp(b),
             (AssetCapacity::Discrete(units1, size1), AssetCapacity::Discrete(units2, size2)) => {
-                Self::check_same_unit_size(*size1, *size2);
-                units1.cmp(units2)
+                // NB: Also returns `None` if either is NaN
+                (*size1 == *size2).then(|| units1.cmp(units2))
             }
-            _ => panic!("Cannot compare different types of AssetCapacity ({self:?} and {other:?})"),
+            _ => None,
         }
     }
 }
@@ -213,4 +222,111 @@ mod tests {
         let got = orig.apply_limit_factor(factor);
         assert_eq!(got, AssetCapacity::Discrete(expected_units, unit_size));
     }
+
+    #[rstest]
+    #[case::less(
+        AssetCapacity::Continuous(Capacity(4.0)),
+        AssetCapacity::Continuous(Capacity(6.0)),
+        Some(Ordering::Less)
+    )]
+    #[case::equal(
+        AssetCapacity::Continuous(Capacity(4.0)),
+        AssetCapacity::Continuous(Capacity(4.0)),
+        Some(Ordering::Equal)
+    )]
+    #[case::greater(
+        AssetCapacity::Continuous(Capacity(6.0)),
+        AssetCapacity::Continuous(Capacity(4.0)),
+        Some(Ordering::Greater)
+    )]
+    fn partial_cmp_continuous(
+        #[case] left: AssetCapacity,
+        #[case] right: AssetCapacity,
+        #[case] expected: Option<Ordering>,
+    ) {
+        assert_eq!(left.partial_cmp(&right), expected);
+        assert_eq!(left == right, expected == Some(Ordering::Equal));
+    }
+
+    #[rstest]
+    #[case::less(
+        AssetCapacity::Discrete(2, Capacity(3.0)),
+        AssetCapacity::Discrete(4, Capacity(3.0)),
+        Some(Ordering::Less)
+    )]
+    #[case::equal(
+        AssetCapacity::Discrete(4, Capacity(3.0)),
+        AssetCapacity::Discrete(4, Capacity(3.0)),
+        Some(Ordering::Equal)
+    )]
+    #[case::greater(
+        AssetCapacity::Discrete(5, Capacity(3.0)),
+        AssetCapacity::Discrete(4, Capacity(3.0)),
+        Some(Ordering::Greater)
+    )]
+    fn partial_cmp_discrete_with_matching_unit_size(
+        #[case] left: AssetCapacity,
+        #[case] right: AssetCapacity,
+        #[case] expected: Option<Ordering>,
+    ) {
+        assert_eq!(left.partial_cmp(&right), expected);
+        assert_eq!(left == right, expected == Some(Ordering::Equal));
+    }
+
+    #[rstest]
+    #[case::mixed_types(
+        AssetCapacity::Continuous(Capacity(4.0)),
+        AssetCapacity::Discrete(4, Capacity(1.0))
+    )]
+    #[case::different_unit_sizes(
+        AssetCapacity::Discrete(4, Capacity(1.0)),
+        AssetCapacity::Discrete(4, Capacity(2.0))
+    )]
+    #[case::nan_continuous(
+        AssetCapacity::Continuous(Capacity(f64::NAN)),
+        AssetCapacity::Continuous(Capacity(4.0))
+    )]
+    fn partial_cmp_returns_none_for_invalid_comparisons(
+        #[case] left: AssetCapacity,
+        #[case] right: AssetCapacity,
+    ) {
+        assert_eq!(left.partial_cmp(&right), None);
+        assert!(left != right);
+    }
+
+    #[rstest]
+    #[case::continuous(
+        AssetCapacity::Continuous(Capacity(4.0)),
+        AssetCapacity::Continuous(Capacity(6.0)),
+        AssetCapacity::Continuous(Capacity(4.0))
+    )]
+    #[case::discrete(
+        AssetCapacity::Discrete(2, Capacity(3.0)),
+        AssetCapacity::Discrete(4, Capacity(3.0)),
+        AssetCapacity::Discrete(2, Capacity(3.0))
+    )]
+    fn min_returns_smaller_capacity(
+        #[case] left: AssetCapacity,
+        #[case] right: AssetCapacity,
+        #[case] expected: AssetCapacity,
+    ) {
+        assert_eq!(left.min(right), expected);
+    }
+
+    #[rstest]
+    #[case::mixed_types(
+        AssetCapacity::Continuous(Capacity(4.0)),
+        AssetCapacity::Discrete(4, Capacity(1.0))
+    )]
+    #[case::different_unit_sizes(
+        AssetCapacity::Discrete(4, Capacity(1.0)),
+        AssetCapacity::Discrete(4, Capacity(2.0))
+    )]
+    #[should_panic(expected = "Comparing invalid AssetCapacity values")]
+    fn min_panics_for_invalid_comparisons(
+        #[case] left: AssetCapacity,
+        #[case] right: AssetCapacity,
+    ) {
+        let _ = left.min(right);
+    }
 }

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