-
Notifications
You must be signed in to change notification settings - Fork 1
Allow for different metrics in NPV calculation #1027
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
701ab71
f2ebcae
d8dead1
8baf828
bdcf224
e205cc9
1f8a752
0d9a07a
5f5b59b
c1736a8
d411957
5f66991
18034e4
9dc1028
5e8a65e
29e38a3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -3,13 +3,14 @@ use super::DemandMap; | |||||
| use crate::agent::ObjectiveType; | ||||||
| use crate::asset::AssetRef; | ||||||
| use crate::commodity::Commodity; | ||||||
| use crate::finance::{lcox, profitability_index}; | ||||||
| use crate::finance::{ProfitabilityIndex, lcox, profitability_index}; | ||||||
| use crate::model::Model; | ||||||
| use crate::time_slice::TimeSliceID; | ||||||
| use crate::units::{Activity, Capacity}; | ||||||
| use crate::units::{Activity, Capacity, Dimensionless, Money, MoneyPerActivity, MoneyPerCapacity}; | ||||||
| use anyhow::Result; | ||||||
| use costs::annual_fixed_cost; | ||||||
| use indexmap::IndexMap; | ||||||
| use std::any::Any; | ||||||
| use std::cmp::Ordering; | ||||||
|
|
||||||
| pub mod coefficients; | ||||||
|
|
@@ -30,8 +31,8 @@ pub struct AppraisalOutput { | |||||
| pub activity: IndexMap<TimeSliceID, Activity>, | ||||||
| /// The hypothetical unmet demand following investment in this asset | ||||||
| pub unmet_demand: DemandMap, | ||||||
| /// The comparison metric to compare investment decisions (lower is better) | ||||||
| pub metric: f64, | ||||||
| /// The comparison metric to compare investment decisions | ||||||
| pub metric: Box<dyn MetricTrait>, | ||||||
| /// Capacity and activity coefficients used in the appraisal | ||||||
| pub coefficients: ObjectiveCoefficients, | ||||||
| /// Demand profile used in the appraisal | ||||||
|
|
@@ -49,16 +50,149 @@ impl AppraisalOutput { | |||||
| /// possible, which is why we use a more approximate comparison. | ||||||
| pub fn compare_metric(&self, other: &Self) -> Ordering { | ||||||
| assert!( | ||||||
| !(self.metric.is_nan() || other.metric.is_nan()), | ||||||
| !(self.metric.value().is_nan() || other.metric.value().is_nan()), | ||||||
| "Appraisal metric cannot be NaN" | ||||||
| ); | ||||||
| self.metric.compare(other.metric.as_ref()) | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| /// Trait for appraisal metrics that can be compared. | ||||||
| /// | ||||||
| /// Implementers define how their values should be compared to determine | ||||||
| /// which investment option is preferable through the `compare` method. | ||||||
| pub trait MetricTrait: Any + Send + Sync { | ||||||
| /// Returns the numeric value of this metric. | ||||||
| fn value(&self) -> f64; | ||||||
|
|
||||||
| /// Compares this metric with another of the same type. | ||||||
| /// | ||||||
| /// Returns `Ordering::Less` if `self` is better than `other`, | ||||||
| /// `Ordering::Greater` if `other` is better, or `Ordering::Equal` | ||||||
| /// if they are approximately equal. | ||||||
| /// | ||||||
| /// # Panics | ||||||
| /// | ||||||
| /// Panics if `other` is not the same concrete type as `self`. | ||||||
| fn compare(&self, other: &dyn MetricTrait) -> Ordering; | ||||||
|
|
||||||
| /// Helper for downcasting to enable type-safe comparison. | ||||||
| fn as_any(&self) -> &dyn Any; | ||||||
| } | ||||||
|
Comment on lines
+60
to
+81
|
||||||
|
|
||||||
| if approx_eq!(f64, self.metric, other.metric) { | ||||||
| /// Levelised Cost of X (LCOX) metric. | ||||||
| /// | ||||||
| /// Represents the average cost per unit of output. Lower values indicate | ||||||
| /// more cost-effective investments. | ||||||
| #[derive(Debug, Clone)] | ||||||
| pub struct LCOXMetric { | ||||||
| /// The calculated cost value for this LCOX metric | ||||||
| pub cost: MoneyPerActivity, | ||||||
| } | ||||||
|
|
||||||
| impl LCOXMetric { | ||||||
| /// Creates a new `LCOXMetric` with the given cost. | ||||||
| pub fn new(cost: MoneyPerActivity) -> Self { | ||||||
| Self { cost } | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| impl MetricTrait for LCOXMetric { | ||||||
| fn value(&self) -> f64 { | ||||||
| self.cost.value() | ||||||
| } | ||||||
|
|
||||||
| fn compare(&self, other: &dyn MetricTrait) -> Ordering { | ||||||
| let other = other | ||||||
| .as_any() | ||||||
| .downcast_ref::<Self>() | ||||||
| .expect("Cannot compare metrics of different types"); | ||||||
|
|
||||||
| if approx_eq!(MoneyPerActivity, self.cost, other.cost) { | ||||||
| Ordering::Equal | ||||||
| } else { | ||||||
| self.metric.partial_cmp(&other.metric).unwrap() | ||||||
| // Lower cost is better | ||||||
| self.cost.partial_cmp(&other.cost).unwrap() | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| fn as_any(&self) -> &dyn Any { | ||||||
| self | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| /// Net Present Value (NPV) metric | ||||||
| #[derive(Debug, Clone)] | ||||||
| pub struct NPVMetric { | ||||||
| /// Profitability index data for this NPV metric | ||||||
| pub profitability_index: ProfitabilityIndex, | ||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You could also make this a newtype, i.e. not give the field a name, which would make things less verbose: pub struct NPVMetric(ProfitabilityIndex);You can then get at the field with |
||||||
| } | ||||||
|
|
||||||
| impl NPVMetric { | ||||||
| /// Creates a new `NPVMetric` with the given profitability index. | ||||||
| pub fn new(profitability_index: ProfitabilityIndex) -> Self { | ||||||
| Self { | ||||||
| profitability_index, | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| /// Returns true if this metric represents a zero fixed cost case. | ||||||
| fn is_zero_fixed_cost(&self) -> bool { | ||||||
| self.profitability_index.annualised_fixed_cost == Money(0.0) | ||||||
|
||||||
| self.profitability_index.annualised_fixed_cost == Money(0.0) | |
| approx_eq!(Money, self.profitability_index.annualised_fixed_cost, Money(0.0)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tsmbland @alexdewar I was wondering about this the other day too. Even though very small positive denominators don't break the profitability index calculation mathematically, is it practically more stable to still use surplus instead if the denominator is very small?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good point. It probably makes sense to use approx_eq! instead, so that we don't get subtly different behaviours depending on small rounding errors (see also #893).
I don't think we need this as a separate helper method though, as it's only used it one place. See my comment below about refactoring.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bear in mind that this is the value that gets presented in the output file, and it's not ideal that this has a different meaning depending on the value of the denominator. I think we do need something like what Alex suggested here #1027 (comment)
If we're doing this then we probably don't even need a .value() implementation for MetricTrait
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree it would be better to make it clear what the metric actually is, although I'm not totally clear on how you'd avoid having value altogether since it could either be an lcox metric or an npv?
We might just want a metric value() and a metric label() in the output file. And label is implemented to return 'lcox', 'npv annualised surplus' and 'npv profitability index' accordingly in the trait structs. Unless it's useful seeing both values where the profitability index denominator is nonzero
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should implement the Serialize trait for NPVMetric and LCOXMetric rather than having a method for getting a scalar like this. Then we can write the outputs in a custom way, without having to restrict ourselves to a single number, e.g. for NPV, we could write both the numerator and the denominator.
I think the easiest way to implement this is with a supertrait (without extra methods): https://doc.rust-lang.org/rust-by-example/trait/supertraits.html
So we'd have:
- A trait for comparing two metrics, like the current
MetricTrait(maybe rename toComparableMetricor something) - A supertrait which is defined as:
pub trait MetricTrait: ComparableMetric + Serialize {}Does that make sense?
That said, adding custom Serialize implementations is something we could do later, so if you'd rather open an issue for it instead, feel free.
Copilot
AI
Jan 8, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The critical new behavior of NPVMetric comparison with zero fixed costs lacks test coverage. The new logic handles three scenarios: both with zero fixed cost, both with non-zero fixed cost, and mixed cases. Only the panic behavior on calling .value() with zero cost is tested. Consider adding unit tests for NPVMetric.compare() that verify: 1) Comparison of two metrics with zero fixed cost correctly uses total surplus, 2) Comparison of two metrics with non-zero fixed cost correctly uses profitability index, and 3) Zero fixed cost metrics are correctly preferred over non-zero fixed cost metrics.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll wait for your review @alexdewar but I do think some tests would be nice
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed.
I think the logic for this method is a bit convoluted as it is. There will be different ways you could go about it, but here's how I'd split it up:
- Make a helper function for doing approx comparisons (as you're doing this in a bunch of places), returning Ordering::Equal if two values are approx equal. (You might need to make this generic, in which case I think
Twill have trait boundsPartialEq + ApproxEq<Margin=F64>.) - Write a method which tries to compare the metrics based on profitability index, returning
Noneif either has an AFC that's approx zero. - Then this method can just call this method, falling back on comparing AFC, e.g.:
// Using `compare_approx` as I mentioned in 1 and newtypes
self.try_compare_pi(other).unwrap_or_else(|| compare_approx(self.0.annualised_fixed_cost, other.0.annualised_fixed_cost)Does this make sense?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering if we really need this now that we have
NPVMetric. Could just haveprofitability_indexreturn a tuple and store each value inNPVMetric. Not sure if that's better?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I generally prefer structs over tuples but don't feel strongly either way about this