Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1125 +/- ##
==========================================
- Coverage 87.54% 87.52% -0.02%
==========================================
Files 55 55
Lines 7460 7626 +166
Branches 7460 7626 +166
==========================================
+ Hits 6531 6675 +144
- Misses 631 649 +18
- Partials 298 302 +4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR updates the documentation for NPV (Net Present Value) and LCOX (Levelised Cost of X) investment appraisal methods to accurately reflect their implementation in the codebase. The changes fix erroneous negative signs in the formulas, separate NPV and LCOX activity coefficient expressions for clarity, and add a comprehensive worked example using a gas power plant.
Changes:
- Corrected mathematical formulas for NPV and LCOX activity coefficients, removing incorrect negative signs and adding proper superscript notation (AC_t^NPV and AC_t^LCOX)
- Separated NPV and LCOX formulas into distinct expressions to highlight their differences
- Added detailed gas power plant example demonstrating NPV and LCOX calculations across two time periods with complete numerical workthrough
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| \text{Cost Index} = \frac{\text{AFC} \times \text{cap}_r + \sum_t \text{act}_t | ||
| \times \text{AC}_t^{\text{LCOX}}}{\sum_t \text{act}_t} |
There was a problem hiding this comment.
Inconsistent LaTeX formatting for variable names. The cost index formula at lines 224-225 uses '\text{act}_t', '\text{AC}_t', etc. with the \text{} wrapper, but the equivalent cost index formula in the example section at line 386 uses 'act_t', 'AC_t', etc. without the \text{} wrapper. Similarly, line 182 uses '\text{act}_t' for the profitability index, but line 317 uses 'act_t' in the example. For consistency, either use \text{} for all variable names in standalone formulas, or don't use it for any. The most readable option would be to consistently use \text{} for all variable names in metric formulas (lines 224-225, 182) and update the example formulas (lines 317, 325, 386) to match.
There was a problem hiding this comment.
Hmm... It seems like we weren't consistently using \text before this PR, but we probably don't also want to be changing act to \text{act} haphazardly, without updating it everywhere (although the latter is arguably more correct).
Up to you whether you want to revert or whatever. Not esp important.
There was a problem hiding this comment.
should I update the rest of the file to consistently use \text as copilot says? then we can make an issue about making sure it's consistent throughout the docs? (Maybe we can assign copilot to do that too :) )
alexdewar
left a comment
alexdewar
left a comment
There was a problem hiding this comment.
I've suggested some tweaks and I've asked some questions, but this looks really good! The worked example is especially helpful.
Besides that:
- We should mention the fallback behaviour for NPV when AFC==0 somewhere; I don't think we do currently.
- I think you're right about the minus signs. Your new version looks correct to me.
- I feel like we should probably just drop the scopes stuff (the text and the terms in the equations). I'm not convinced the equations are right as they are and we're not planning on implementing this feature any time soon anyway.
| @@ -140,10 +157,10 @@ operational constraints (e.g., minimum load levels) and the balance level of the | |||
| - **Optimise capacity and dispatch to maximise annualised profit:** Solve a small optimisation | |||
| sub-problem to maximise the asset's surplus, subject to its operational rules and the specific | |||
| demand tranche it is being asked to serve. \\(\varepsilon \approx 1×10^{-14}\\) is added to each | |||
There was a problem hiding this comment.
The value of f64::EPSILON * 100. Incidentally, do you know why we're not just using f64::EPSILON?
There was a problem hiding this comment.
I think f64::EPSILON is too small to meaningfully nullify any floating point precision errors which made the activity coefficient tiny and negative (don't think the constant is really intended for that purpose). So it's multiplied by 100 to get it to around 1e-14. I suppose you could argue it's cleaner just to use 1e-14 directly but that would be a bit more arbitrary too.
|
|
||
| - Calculate cost per unit of activity \\( AC_{t}^{LCOX} \\) (Tool B). Note that the commodity | ||
| of interest (primary output \\( c_{primary} \\)) is excluded from the price term: | ||
| \\[ |
There was a problem hiding this comment.
Has this block been changed, other than the indentation?
Co-authored-by: Alex Dewar <alexdewar@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
docs/model/investment.md
Outdated
| AC_{t2}^{NPV} &= (1.0 \times 50) + (0.5 \times 15) + (-2.5 \times 25) - 5 \\\\ | ||
| &= 50 + 7.5 - 62.5 - 5 \\\\ | ||
| &= \text{£} -10 \text{/MWh} | ||
| \end{aligned} |
There was a problem hiding this comment.
The LaTeX rendering for the negative value in this example is likely incorrect: \text{£} -10 \text{/MWh} will typically typeset awkwardly (and may drop spacing/formatting). Prefer a single \text{...} (e.g., \text{£-10/MWh}) or otherwise format the currency/unit consistently with the other example lines.
|
|
||
| | Commodity | Notation | t1 (Peak) | t2 (Off-peak) | | ||
| |-----------|----------|-----------|---------------| | ||
| | Electricity | \\( \lambda\_{c_{elec},r,t} \\) | £90/MWh | £50/MWh | |
There was a problem hiding this comment.
In the example, electricity is introduced as the primary output using c_{primary}, but the market price table switches to c_{elec} in the price notation. Using a single symbol consistently (e.g., c_{primary} everywhere, or explicitly stating c_{primary}=c_{elec}) would prevent ambiguity.
| | Electricity | \\( \lambda\_{c_{elec},r,t} \\) | £90/MWh | £50/MWh | | |
| | Electricity | \\( \lambda\_{c_{primary},r,t} \\) | £90/MWh | £50/MWh | |
| \\[ | ||
| AC_t^{NPV} = \sum_{c} \Big( output\_{coeff}[c] - input\_{coeff}[c] \Big) \cdot \lambda_{c,r,t} - cost\_{var}[t] | ||
| \\] | ||
|
|
There was a problem hiding this comment.
The example’s “general form” for AC_t^{NPV} only includes price terms and cost_{var}[t], while the earlier definition of AC_t^{NPV} also includes flow-specific costs/levies (and scope terms, even if currently unimplemented). Consider stating explicitly that the example assumes those additional terms are zero/omitted for simplicity, so readers don’t infer the implementation ignores them.
| In this illustrative example, any additional flow-specific costs or levies (and any scope-related terms present in the full definition of \(AC_t^{NPV}\)) are assumed to be zero and are therefore omitted for simplicity. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| \\[ | ||
| minimise \Big\\{ | ||
| AF \* cap + \sum_t act_t \* AC_t + VoLL \* UnmetD_t | ||
| AF \times cap + \sum_t act_t \times AC_{t}^{LCOX} + VoLL \times UnmetD_t | ||
| \Big\\} |
There was a problem hiding this comment.
In the LCOX optimisation objective, AF is used for fixed cost (AF \times cap), but the rest of the document defines annualised fixed cost as AFC_{opt,r} and the cost index formula below uses AFC. Please rename AF to AFC here (and keep notation consistent across Tool B).
| \\[ | ||
| \text{Profitability Index} = | ||
| \frac{\sum_t \text{act}_t \cdot \text{AC}_t^{\text{NPV}}}{\text{AFC} \cdot \text{cap}} | ||
| \\] |
There was a problem hiding this comment.
LaTeX variable formatting is inconsistent within this file: the profitability index uses \text{act}_t, \text{AC}_t, \text{AFC}, etc., while surrounding formulas and the worked example use act_t, AC_t, AFC without \text{}. Pick one style for variables in formulas (either remove \text{} here, or update the other formulas) to keep the notation consistent and easier to read.
| \\[ | ||
| \text{cost index} = \frac{AFC \cdot cap + \sum_t act_t \cdot AC_t^{LCOX}}{\sum_t act_t} | ||
| \\] |
There was a problem hiding this comment.
Capitalisation of the metric name is inconsistent: earlier you define \text{Cost Index}, but in the example the formula uses \text{cost index}. Please use consistent capitalisation for the metric label across the document.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| \\[ | ||
| \text{Cost Index} = \frac{\text{AFC} \times \text{cap}_r + \sum_t \text{act}_t | ||
| \times \text{AC}_t^{\text{LCOX}}}{\sum_t \text{act}_t} |
There was a problem hiding this comment.
The cost index formula introduces cap_r, but that symbol isn’t defined elsewhere in the doc (the optimisation and parameter tables use cap). Consider using the same capacity symbol/indices consistently (e.g., cap or cap_{opt,r}) to avoid confusion.
| The net revenue calculation follows the general form: | ||
|
|
||
| \\[ | ||
| AC_t^{NPV} = \sum_{c} \Big( output\_{coeff}[c] - input\_{coeff}[c] \Big) \cdot \lambda_{c,r,t} - cost\_{var}[t] | ||
| \\] |
There was a problem hiding this comment.
In the example, the simplified expression for AC_t^{NPV} drops several terms that appear in the full definition above (e.g., cost_input/cost_output and scope-related terms). It would help to explicitly state that these terms are assumed to be zero/omitted for simplicity in this example, so readers don’t think the implementation differs.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| \\[ | ||
| \text{Cost Index} = \frac{\text{AFC} \times \text{cap}_r + \sum_t \text{act}_t | ||
| \times \text{AC}_t^{\text{LCOX}}}{\sum_t \text{act}_t} |
There was a problem hiding this comment.
In the Cost Index formula, capacity is written as \text{cap}_r, but elsewhere in Tool B (including the objective just above) the decision variable is cap without a regional subscript. Please make the notation consistent (either always subscript capacity by region or omit it throughout) so it’s clear these refer to the same variable.
docs/model/investment.md
Outdated
| cost\_{\text{output}}[c] \cdot output\_{\text{coeff}}[c] \Big) \\\\ | ||
| &+ \sum\_{c} \Big( output\_{\text{coeff}}[c] - input\_{\text{coeff}}[c] \Big) | ||
| \cdot \lambda\_{c,r,t} \\\\ | ||
| &+ \sum\_{s,c} in\\_scope[s] \cdot \Big\\{ \\\\ |
There was a problem hiding this comment.
In the NPV activity-coefficient expression, in\\_scope[s] looks like an accidental extra backslash. In LaTeX, \\ is a linebreak and will likely break rendering here; it should be consistent with other escaped underscores (e.g., cost\_var) and use in\_scope instead.
| &+ \sum\_{s,c} in\\_scope[s] \cdot \Big\\{ \\\\ | |
| &+ \sum\_{s,c} in\_scope[s] \cdot \Big\\{ \\\\ |
docs/model/investment.md
Outdated
| &- \sum\_{c \neq c_{primary}} \Big( output\_{\text{coeff}}[c] - input\_{\text{coeff}} | ||
| [c] \Big) | ||
| \cdot \lambda\_{c,r,t} \\\\ | ||
| &+ \sum\_{s,c} in\\_scope[s] \cdot \Big\\{ \\\\ | ||
| &\quad \quad (cost\_{\text{prod}}[s,c] - \mu\_{s,c}^{\text{prod}}) |
There was a problem hiding this comment.
The AC_t^{LCOX} formula has a line break inside input_{\text{coeff}}[c] (the [c] is on the next line). This is likely to render incorrectly in LaTeX; keep the term on one line (or break between factors) so the subscripted variable isn’t split.
| &- \sum\_{c \neq c_{primary}} \Big( output\_{\text{coeff}}[c] - input\_{\text{coeff}} | |
| [c] \Big) | |
| \cdot \lambda\_{c,r,t} \\\\ | |
| &+ \sum\_{s,c} in\\_scope[s] \cdot \Big\\{ \\\\ | |
| &\quad \quad (cost\_{\text{prod}}[s,c] - \mu\_{s,c}^{\text{prod}}) | |
| &- \sum\_{c \neq c_{primary}} \Big( output\_{\text{coeff}}[c] - input\_{\text{coeff}}[c] \Big) | |
| \cdot \lambda\_{c,r,t} \\\\ | |
| &+ \sum\_{s,c} in\\_scope[s] \cdot \Big\\{ \\\\ | |
| &\quad \quad (cost\_{\text{prod}}[s,c] - \mu\_{s,c}^{\text{prod}}) | |
| &\quad \quad (cost\_{\text{prod}}[s,c] - \mu\_{s,c}^{\text{prod}}) |
|
|
||
| \\[ | ||
| minimise \Big\\{ | ||
| AF \* cap + \sum_t act_t \* AC_t + VoLL \* UnmetD_t | ||
| AF \times cap + \sum_t act_t \times AC_{t}^{LCOX} + VoLL \times UnmetD_t | ||
| \Big\\} |
There was a problem hiding this comment.
Tool B’s optimisation bullet says it ‘minimise[s] annualised cost’ but then describes solving the sub-problem ‘to maximise the asset's surplus’, which is contradictory. Also, the objective uses AF × cap while the rest of the document defines the annualised fixed cost as AFC; please align the wording and notation so the objective is unambiguous and consistent with the earlier AFC_{opt,r} definition and the cost-index formula.
| |-----------|----------|-----------|---------------| | ||
| | Electricity | \\( \lambda\_{c_{elec},r,t} \\) | £90/MWh | £50/MWh | | ||
| | Heat | \\( \lambda\_{c_{heat},r,t} \\) | £25/MWh | £15/MWh | |
There was a problem hiding this comment.
In the example, electricity is introduced as the primary output c_{primary}, but the price table uses \lambda_{c_{elec},r,t}. Either use c_{primary} consistently, or explicitly define c_{elec} = c_{primary} in the example to avoid ambiguity.
the equations look so much less intimidating without the scopes stuff ! |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| | Input (Natural gas) | \\( input\_{coeff}[c_{gas}] \\) | 2.5 MWh per unit activity | Fuel consumption | | ||
| | Variable operating cost | \\( cost\_{var}[t] \\) | £5/MWh of activity | Operating costs per unit activity | | ||
| <!-- markdownlint-enable MD013 --> | ||
|
|
There was a problem hiding this comment.
The example assumes all flow costs (the cost field on each ProcessFlow) are zero, but this should be stated explicitly. The general formulas at lines 78-82 and 90-97 include cost_input and cost_output terms representing these per-flow costs, but they are omitted from the example calculations without explanation. Consider adding a note in the "Asset Parameters" section stating that flow costs are assumed to be zero for simplicity in this example.
| Note: In this illustrative example, all per-flow costs associated with individual ProcessFlows (the `cost` field, represented in the general formulas as \\( cost\_{input} \\) and \\( cost\_{output} \\)) are assumed to be zero; only the variable operating cost \\( cost\_{var}[t] \\) is included in the calculations. |
| 3. If there is still a tie, the first option in the data structure is selected, | ||
| which is non-deterministic. A warning is emitted when this occurs. |
There was a problem hiding this comment.
The documentation states that "A warning is emitted when this occurs" referring to cases where two or more investment options have identical metrics and cannot be distinguished even after applying tie-breaking rules. However, the implementation in sort_appraisal_outputs_by_investment_priority (src/simulation/investment/appraisal.rs:349-356) does not emit any warning when ties remain unresolved. The function comment acknowledges "The function does not guarantee that all ties will be resolved" but no warning is logged. Either the documentation should be updated to remove the claim about warning emission, or the code should be updated to emit a warning when unresolved ties exist.
| \text{Cost Index} = \frac{\text{AFC} \times \text{cap}_r + \sum_t \text{act}_t | ||
| \times \text{AC}_t^{\text{LCOX}}}{\sum_t \text{act}_t} |
There was a problem hiding this comment.
Inconsistent notation for capacity variable. Line 205 uses cap while line 226 uses \text{cap}_r. For consistency, both should use the same notation. Either remove the subscript from line 226 to match line 205, or add a subscript to line 205 to match line 226. The subscript _r for region is not essential here since the context makes it clear we're dealing with a specific asset and region.
2 participants
Description
I went through the documentation for npv and tried to ensure that it accurately reflects what is done in the code. I think there was a couple of erroneous
-signs in the docs but please double check. When we choose the best asset using profitability index we used to negate them so that the smallest is best, but this was just to allow us to deal with NPV and LCOX metrics in the same way. When it comes to calculating activity coefficients, I don't see a negation like this in the code, and it makes sense that we would want to maximise annualised revenue rather than minimise it.I modified/added a couple of bits to try and make the documentation clearer but let me know what you think:
Fixes #1103
Type of change
Key checklist
$ cargo test$ cargo docpresent in the previous release
Further checks