Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1125 +/- ##
==========================================
- Coverage 87.48% 87.30% -0.18%
==========================================
Files 55 57 +2
Lines 7660 7706 +46
Branches 7660 7706 +46
==========================================
+ Hits 6701 6728 +27
- Misses 657 673 +16
- Partials 302 305 +3 ☔ 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 :) )
There was a problem hiding this comment.
I'd try not to make too much work for yourself, so I'd maybe leave it without the \text{}s and change yours so they're consistent (even if it's not quite right).
Good idea to assign it to Copilot! Maybe let's open an issue and try that.
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.
There was a problem hiding this comment.
Oh, if it's negative I feel like we should just set it to f64::EPSILON, i.e. use number.max(f64::EPSILON). I think this would be a cleaner approach than just always adding this constant. Maybe this should be done in a separate PR though...
Do you have thoughts @tsmbland?
There was a problem hiding this comment.
Won't it lose information if we force all the coefficients to be positive though? Because the activity coefficients can be negative, and the optimiser will try and arrange the activity to favour positive within the constraints. With the epsilon value we only want to give some numerical wiggle-room where the coefficient is very close to zero.
|
|
||
| - 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.
| 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.
I removed the general forms
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.
| \\[ | ||
| \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.
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.
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.
| 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\\{ \\\\ |
| &- \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 ! |
|
Can you send me the link to where this is in the docs so I can see context?
…________________________________
From: Aurashk ***@***.***>
Sent: 03 March 2026 12:03
To: EnergySystemsModellingLab/MUSE2 ***@***.***>
Cc: Hawkes, Adam D ***@***.***>; Mention ***@***.***>
Subject: Re: [EnergySystemsModellingLab/MUSE2] update appraisal documentation (PR #1125)
[https://avatars.githubusercontent.com/u/9390150?s=20&v=4]Aurashk left a comment (EnergySystemsModellingLab/MUSE2#1125)<#1125 (comment)>
@ahawkes<https://github.com/ahawkes> Is there a commonly accepted terminology for the second and third terms in the (NPV) activity coefficient equation below? In the code we call them flows_cost and revenue_from_flows respectively
image.png (view on web)<https://github.com/user-attachments/assets/4427c2ee-0248-4e80-923d-6628a8a802d1>
—
Reply to this email directly, view it on GitHub<#1125 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AC37JLLY7LWINY4PI3V47YT4O3CZVAVCNFSM6AAAAACU5MU5VGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZTSOJQGU3DSNRWGY>.
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
On the investment appraisal page scrolling down to pre-calculation of metrics |
|
@Aurashk the 2nd term is process_and_commodity_specific_flow_costs, and the 3rd term is commodity_price_flow_costs |
alexdewar
left a comment
alexdewar
left a comment
There was a problem hiding this comment.
Last round, I think!
I've made a lot of small suggestions, but they're all basically the same thing: with mdbook you should escape underscores in math mode. Unfortunately, if you don't, it can sometimes be interpreted as meaning "italics" by the markdown parser. It renders fine for me currently, but I've had problems in the past where suddenly large blocks of the text end up italicised and the only solution seems to be to escape all the
underscores.
Thank you so much for sticking with this, btw. It's ended up being a much bigger task than I originally envisioned!
| \\(\text{AFC}\\). If \\(\text{AFC} = 0\\), this is always preferred over options with | ||
| \\(\text{AFC} > 0\\) so the latter are discarded as investment options. |
| Suppose the optimiser determines \\( cap = 100 \\) MW, \\( act_{t_{0}} = 150 \\) MWh, | ||
| and \\( act_{t_1} = 80 \\) MWh are the optimal capacity and activity levels: |
There was a problem hiding this comment.
| Suppose the optimiser determines \\( cap = 100 \\) MW, \\( act_{t_{0}} = 150 \\) MWh, | |
| and \\( act_{t_1} = 80 \\) MWh are the optimal capacity and activity levels: | |
| Suppose the optimiser determines \\( cap = 100 \\) MW, \\( act\_{t\_{0}} = 150 \\) MWh, | |
| and \\( act\_{t\_1} = 80 \\) MWh are the optimal capacity and activity levels: |
Co-authored-by: Alex Dewar <alexdewar@users.noreply.github.com> Co-authored-by: Copilot <175728472+Copilot@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.
| \frac{\sum\_t act\_t \cdot \text{AC}\_t^{\text{NPV}}}{\text{AFC} \cdot \text{cap}} | ||
| \\] | ||
|
|
||
| - **If \\(\text{AFC} = 0\\), Use the total annualised surplus metric \\(\text{TAS}\\):** | ||
| \\[ | ||
| \text{TAS} = | ||
| \sum\_t act\_t \cdot \text{AC}\_t^{\text{NPV}} |
There was a problem hiding this comment.
Similarly, the TAS formula uses (AC_t^{NPV}) without the (\varepsilon) adjustment, but the code computes total annualised surplus from the same adjusted activity coefficients used in the optimisation. Consider reflecting that in the formula or clarifying that (\varepsilon) is included in the coefficients used for TAS.
| \frac{\sum\_t act\_t \cdot \text{AC}\_t^{\text{NPV}}}{\text{AFC} \cdot \text{cap}} | |
| \\] | |
| - **If \\(\text{AFC} = 0\\), Use the total annualised surplus metric \\(\text{TAS}\\):** | |
| \\[ | |
| \text{TAS} = | |
| \sum\_t act\_t \cdot \text{AC}\_t^{\text{NPV}} | |
| \frac{\sum\_t act\_t \cdot \left(\text{AC}\_t^{\text{NPV}} + \varepsilon\right)}{\text{AFC} \cdot \text{cap}} | |
| \\] | |
| - **If \\(\text{AFC} = 0\\), Use the total annualised surplus metric \\(\text{TAS}\\):** | |
| \\[ | |
| \text{TAS} = | |
| \sum\_t act\_t \cdot \left(\text{AC}\_t^{\text{NPV}} + \varepsilon\right) |
| - Calculate the specific process and commodity flow costs (SPCF): | ||
|
|
||
| \\[ | ||
| \text{SPCF} = \sum\_{c} \Big( cost\_{\text{input}}[c] \cdot input\_{\text{coeff}}[c] + | ||
| cost\_{\text{output}}[c] \cdot output\_{\text{coeff}}[c] \Big) |
There was a problem hiding this comment.
The SPCF term is defined without any time-slice dependence, and cost_input/cost_output aren’t defined elsewhere in this doc. In the implementation, operating costs include per-flow costs plus levies/incentives that are keyed by (region, year, time_slice), so this component can vary by t (and cost_var is not time-slice indexed in code). Consider either indexing SPCF/cost terms by t (and defining them) or explicitly stating the assumptions under which they’re time-invariant.
| \frac{\sum\_t act\_t \cdot \text{AC}\_t^{\text{NPV}}}{\text{AFC} \cdot \text{cap}} | ||
| \\] | ||
|
|
||
| - **If \\(\text{AFC} = 0\\), Use the total annualised surplus metric \\(\text{TAS}\\):** | ||
| \\[ | ||
| \text{TAS} = | ||
| \sum\_t act\_t \cdot \text{AC}\_t^{\text{NPV}} |
There was a problem hiding this comment.
The PI formula here uses (AC_t^{NPV}) without the (\varepsilon) adjustment. In the implementation, (\varepsilon) is added directly to the NPV activity coefficients before optimisation and the profitability index is computed from those adjusted coefficients, so PI will (slightly) include (\varepsilon). Consider reflecting that in the formula or adding a note that the metric uses the adjusted coefficients.
| \frac{\sum\_t act\_t \cdot \text{AC}\_t^{\text{NPV}}}{\text{AFC} \cdot \text{cap}} | |
| \\] | |
| - **If \\(\text{AFC} = 0\\), Use the total annualised surplus metric \\(\text{TAS}\\):** | |
| \\[ | |
| \text{TAS} = | |
| \sum\_t act\_t \cdot \text{AC}\_t^{\text{NPV}} | |
| \frac{\sum\_t act\_t \cdot \left(\text{AC}\_t^{\text{NPV}} + \varepsilon\right)}{\text{AFC} \cdot \text{cap}} | |
| \\] | |
| - **If \\(\text{AFC} = 0\\), Use the total annualised surplus metric \\(\text{TAS}\\):** | |
| \\[ | |
| \text{TAS} = | |
| \sum\_t act\_t \cdot \left(\text{AC}\_t^{\text{NPV}} + \varepsilon\right) |
| 3. If there is still a tie, the first option in the data structure storing the metrics is selected, | ||
| which is an arbitrary choice. A warning is emitted when this occurs. | ||
|
|
There was a problem hiding this comment.
This list claims a final tie-breaker of selecting the first option in the data structure and emitting a warning. The current implementation only falls back to comparing (is_commissioned, commission_year) and explicitly notes that it does not guarantee all ties will be resolved; it also does not emit a warning on unresolved ties. Please update this section to reflect the actual fallback behaviour (and avoid implying deterministic selection or warnings that don’t occur).
| 3. If there is still a tie, the first option in the data structure storing the metrics is selected, | |
| which is an arbitrary choice. A warning is emitted when this occurs. | |
| After applying these rules, remaining ties may still exist. The current implementation does not | |
| guarantee that all such ties are resolved deterministically and does not emit a warning when they | |
| occur. |
| sub-problem to minimise the asset's annualised cost, subject to its operational rules and the specific | ||
| demand tranche it is being asked to serve. |
There was a problem hiding this comment.
This line exceeds the configured markdownlint MD013 line-length limit (100 chars) and will be flagged by the repo’s markdownlint pre-commit hook. Please wrap the sentence (e.g., break after a comma) to keep each line within the limit.
| sub-problem to minimise the asset's annualised cost, subject to its operational rules and the specific | |
| demand tranche it is being asked to serve. | |
| sub-problem to minimise the asset's annualised cost, subject to its operational rules | |
| and the specific demand tranche it is being asked to serve. |
change warning message to log message
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| - **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 | ||
| \\(AC_t \\) to allow assets which are breakeven (or very close to breakeven) to be dispatched. | ||
| demand tranche it is being asked to serve. \\(\varepsilon \approx 1\times 10^{-14}\\) is added to | ||
| each \\(AC_{t}^{NPV} \\) to allow assets which are breakeven (or very close to breakeven) to be | ||
| dispatched. |
There was a problem hiding this comment.
NPV optimisation adds \varepsilon to each activity coefficient (as written here), and in the implementation the same epsilon-adjusted coefficients are also used when computing total annualised surplus / profitability index. Consider clarifying in the subsequent PI/TAS definitions whether AC_t^{NPV} is meant to include this \varepsilon term (even if it’s negligible), so the documentation matches the metric calculation exactly.
alexdewar
left a comment
alexdewar
left a comment
There was a problem hiding this comment.
I spotted a couple more unescaped underscores, but otherwise all good 🎉!
| their annualised CAPEX plus FOM. For existing assets, the relevant fixed cost is its FOM. | ||
|
|
||
| - Calculate the specific process and commodity flow costs: | ||
| - Calculate the specific process and commodity flow costs (\\(\text{SPCF}\_{t})\\): |
There was a problem hiding this comment.
I guess this doesn't really need to be a math block, but it's ok this way too.
| | Primary output (Electricity) | \\( output\_{coeff}[c_{primary}] \\) | 1.0 MWh per unit activity | Main commodity produced | | ||
| | By-product output (Waste heat) | \\( output\_{coeff}[c_{heat}] \\) | 0.5 MWh per unit activity | Co-product from generation | | ||
| | 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 | |
There was a problem hiding this comment.
It worked for me when I ran npx markdown-table-formatter docs/model/investment.md.
Anyways, I've committed the result and pushed it to this branch.
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.
| rules are applied in order: | ||
|
|
||
| 1. Assets which are already commissioned are preferred over new candidate assets. | ||
| 2. Newer (commissioned later) assets are preferred over older assets. | ||
| 3. If there is still a tie, the first option in the data structure storing the metrics is selected, | ||
| which is an arbitrary choice. A `debug` level log message is emitted in this case. |
There was a problem hiding this comment.
The “Equal-Metric Fallback” section claims that if there is still a tie then “the first option in the data structure … is selected” and that a debug log is emitted. The current implementation sorts with a fallback on commissioned status / commission year, but does not emit a log and does not guarantee preserving original order when all compared fields are equal (Rust’s sort_by is not stable). Please adjust the wording to match the actual behavior (ties may remain unresolved / ordering unspecified), or update the implementation if deterministic tie-breaking + logging is required.
| rules are applied in order: | |
| 1. Assets which are already commissioned are preferred over new candidate assets. | |
| 2. Newer (commissioned later) assets are preferred over older assets. | |
| 3. If there is still a tie, the first option in the data structure storing the metrics is selected, | |
| which is an arbitrary choice. A `debug` level log message is emitted in this case. | |
| preferences are applied: | |
| 1. Assets which are already commissioned are preferred over new candidate assets. | |
| 2. Newer (commissioned later) assets are preferred over older assets. | |
| 3. If there is still a tie after applying these preferences, the relative ordering of the remaining | |
| options is implementation-defined and may not be deterministic (for example, it may depend on the | |
| underlying sort algorithm). No specific log message is guaranteed to be emitted in this case. |
| AC_{t\_1}^{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.
In the NPV example for the off-peak period, the rendered unit value is written as \text{£} -10 \text{/MWh}, which is likely to typeset awkwardly (currency symbol separated from the negative sign/value). Consider formatting it as a single unit/value (e.g., -\text{£}10/\text{MWh} or \text{£-10/MWh}) for readability.
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 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| &+ \varepsilon \\\\ | ||
| \end{aligned} | ||
| \\] | ||
| \\(\varepsilon \approx 1\times 10^{-14}\\) is added to |
There was a problem hiding this comment.
The epsilon magnitude here doesn’t match the implementation. In src/simulation/investment/appraisal/coefficients.rs, the NPV activity coefficients add MoneyPerActivity(f64::EPSILON * 100.0) (≈ 2.22e-14), not ~1×10^-14. Please update the docs to reflect the actual constant (or describe it in terms of f64::EPSILON * 100).
| \\(\varepsilon \approx 1\times 10^{-14}\\) is added to | |
| \\(\varepsilon = \texttt{f64::EPSILON} \times 100 \approx 2.22\times 10^{-14}\\) is added to |
| This method is used when the decision rule is `single` and the objective is annualised profit for | ||
| agents serving commodity \\( c \\). It iteratively builds a supply portfolio by selecting | ||
| options that offer the highest annualised profit for serving the current commodity demand. The | ||
| economic evaluation uses \\( \pi_{prevMSY} \\) prices and takes account of asset-specific |
There was a problem hiding this comment.
This section refers to \( \pi_{prevMSY} \) prices, but earlier the document defines and uses previous-milestone prices as \( \lambda_{c,r,t} \). If these are the same concept, please use one symbol consistently; if they differ, add a short clarification of the relationship to avoid ambiguity.
| economic evaluation uses \\( \pi_{prevMSY} \\) prices and takes account of asset-specific | |
| economic evaluation uses previous-milestone prices \\( \lambda_{c,r,t} \\) and takes account of asset-specific |
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