Change LCOX to use same optimisation as NPV#1319
Draft
alexdewar wants to merge 6 commits into
Draft
Conversation
We shouldn't be using the result of the capacity var for NPV.
It's always zero now, so we don't need to store it.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors the LCOX investment appraisal so that it uses the same optimisation formulation as the NPV appraisal (maximise activity surplus with fixed capacity equal to the supplied max_capacity), instead of the previous LCOX-specific formulation that minimised annualised cost with capacity as a decision variable and a VoLL-priced unmet-demand term. LCOX-specific per-time-slice costs are now computed separately and used only for the final LCOX metric calculation. This addresses issue #1199 where the old LCOX approach overinvested capacity and relied on VoLL to clear demand.
Changes:
- Removed the capacity decision variable, the unmet-demand objective penalty, and the LCOX-specific
Sense::Minimisepath from the appraisal optimisation; both LCOX and NPV now call a single sharedperform_optimisationthat maximises activity surplus. - Restructured
ObjectiveCoefficientsto hold a sharedactivity_coefficientsmap plus an optionallcox_costsmap;calculate_coefficients_for_assetsnow always builds the activity coefficients via a single helper and fillslcox_costsonly when the objective is LCOX. - Propagated removal of capacity/unmet-demand coefficients through callers (
appraisal.rs,output.rs,fixture.rs), soAppraisalOutput.capacityis now simply the suppliedmax_capacityand the LCOX metric usesannual_fixed_cost(asset)directly.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/simulation/investment/appraisal/optimisation.rs | Drops capacity variable, hard-codes Sense::Maximise, and remaps solution columns to activity + unmet-demand only. |
| src/simulation/investment/appraisal/constraints.rs | Removes add_capacity_constraint and candidate-specific activity constraints; activity bounds are now derived from max_capacity.total_capacity(). |
| src/simulation/investment/appraisal/coefficients.rs | Unifies coefficient calculation; adds lcox_costs and removes capacity_coefficient/unmet_demand_coefficient. |
| src/simulation/investment/appraisal.rs | calculate_lcox/calculate_npv now share the same optimisation call and use max_capacity directly for metrics. |
| src/output.rs | Removes capacity_coefficient column from appraisal debug output. |
| src/fixture.rs | Updates test fixture to the new ObjectiveCoefficients shape. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
13
to
24
| /// Map storing cost coefficients for an asset. | ||
| /// | ||
| /// These coefficients are calculated according to the agent's `ObjectiveType` and are used by | ||
| /// the investment appraisal routines. The map contains the per-capacity and per-activity cost | ||
| /// coefficients used in the appraisal optimisation, together with the unmet-demand penalty. | ||
| #[derive(Clone)] | ||
| pub struct ObjectiveCoefficients { | ||
| /// Cost per unit of capacity | ||
| pub capacity_coefficient: MoneyPerCapacity, | ||
| /// Cost per unit of activity in each time slice | ||
| pub activity_coefficients: IndexMap<TimeSliceID, MoneyPerActivity>, | ||
| /// Unmet demand coefficient | ||
| pub unmet_demand_coefficient: MoneyPerFlow, | ||
| /// Costs for LCOX | ||
| pub lcox_costs: IndexMap<TimeSliceID, MoneyPerActivity>, | ||
| } |
Comment on lines
+55
to
+56
| /// activity coefficients so that assets with near-zero net value still appear in dispatch. Capacity | ||
| /// costs and unmet-demand penalties are set to zero. |
| use crate::agent::ObjectiveType; | ||
| use crate::asset::AssetRef; | ||
| use crate::model::Model; | ||
|
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
The following models are currently failing to run to completion:
Examples:
circularity: FAILED
missing_commodity: succeeded
muse1_default: FAILED
simple: succeeded
two_outputs: succeeded
two_regions: FAILED
Patched examples:
simple_divisible: succeeded
simple_full: succeeded
simple_full_average: succeeded
simple_ironing_out: succeeded
simple_marginal: FAILED
simple_marginal_average: FAILED
simple_npv: succeeded
Closes #1199.
Type of change
Key checklist
$ cargo test$ cargo docpresent in the previous release
Further checks