Add fallback code to calculate metrics for non-dispatched options#1363
Add fallback code to calculate metrics for non-dispatched options#1363alexdewar wants to merge 6 commits into
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1363 +/- ##
==========================================
- Coverage 89.75% 88.55% -1.20%
==========================================
Files 58 58
Lines 8529 8649 +120
Branches 8529 8649 +120
==========================================
+ Hits 7655 7659 +4
- Misses 561 676 +115
- Partials 313 314 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR introduces a fallback appraisal path to produce investment metrics for options that fail to dispatch during appraisal, by reappraising under an assumption of maximum activity in every time slice. This is intended to prevent the investment loop from failing when all remaining options would otherwise yield non-comparable metrics.
Changes:
- Added a “max activity” appraisal mode and unmet-demand reconstruction logic for non-dispatched options.
- Added a model parameter flag to enable/disable investing in non-dispatched options, and integrated the fallback into investment selection.
- Moved the “negative annual fixed cost” assertion into
finance::profitability_indexand improved appraisal-output filtering to report how many non-feasible options were removed.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/simulation/investment/appraisal.rs | Adds max-activity appraisal function and unmet-demand computation; updates filtering to return number of removed outputs. |
| src/simulation/investment.rs | Adds fallback reappraisal path when no option dispatches; improves diagnostics when no feasible options remain. |
| src/model/parameters.rs | Adds allow_investment_in_non_dispatched_options parameter with default true. |
| src/finance.rs | Centralises a fixed-cost validity assertion inside profitability_index. |
| schemas/input/model.yaml | Adds schema entry for the new parameter (currently needs key-name alignment with Rust field). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let time_slices: Vec<_> = ts_selection.iter(time_slice_info).collect(); | ||
| let demand_for_selection: Flow = time_slices.iter().map(|(ts, _)| demand[*ts]).sum(); | ||
| let supply_for_selection: Flow = time_slices | ||
| .iter() | ||
| .map(|(ts, _)| activity[*ts] * flow_coeff) | ||
| .sum(); | ||
|
|
||
| #[allow(clippy::cast_precision_loss)] | ||
| let unmet_per_slice = (demand_for_selection - supply_for_selection).max(Flow(0.0)) | ||
| / Dimensionless(time_slices.len() as f64); | ||
| for (time_slice, _) in &time_slices { | ||
| unmet_demand.insert((*time_slice).clone(), unmet_per_slice); | ||
| } |
There was a problem hiding this comment.
I don't think Copilot is right here, but I'd be interested to know what you think @tsmbland.
Thinking about this a bit more, I'm wondering if it might be more principled to have is_any_remaining_demand check whether the sum of unmet demand is above the threshold rather than each individual time slice.
There was a problem hiding this comment.
I don't think there's a problem here. It's cleaner in #1329 though, if we want to go down that route
| /// The exact per-time-slice distribution is arbitrary: all downstream consumers sum values back up | ||
| /// to the selection level before using them (e.g. the next round's demand constraint, and | ||
| /// `is_any_remaining_demand`), so only the selection-level total needs to be correct. |
| if model.parameters.allow_investment_in_non_dispatched_options | ||
| && !outputs_for_opts | ||
| .iter() | ||
| .any(|output| output.metric.is_some()) | ||
| { | ||
| warn!( | ||
| "None of the investment options dispatched. Reappraising assuming maximum activity." | ||
| ); | ||
| outputs_for_opts = reappraise_outputs_assuming_max_activity( |
There was a problem hiding this comment.
Once we merge #1319, we will have coverage of this branch.
alexdewar
force-pushed
the
dispatch-nonfeasible-options
branch
from
bc84585 to
2923a8e
Compare
tsmbland
left a comment
tsmbland
left a comment
There was a problem hiding this comment.
I know this was partly my idea, but I didn't really think it through properly, and I'm not so convinced any more:
- The "max activity" calculation is wrong/flawed. This may be ok for calculating metrics, since these are essentially just timeslice weights, but it isn't fair to subtract these from the unmet demand profile as they doesn't account for seasonal/annual limits. To get timeslice-level max activities that abide by seasonal and annual constraints, I think would require a linear optimisation.
- The appraisal doesn't take any account of remaining demand, which most likely will only exist in a handful of timeslices (as seen in the failing examples). Imagine there's only demand left in night. Solar technologies may end up as the best technology based on full utilisation, but selecting solar won't reduce unmet demand at all. It could end up infinitely selecting solar.
I think we need to discuss before going any further. One idea (also not very well thought out) might be to appraise these with a normal optimisation, but setting all activity coefficients to epsilon.
Still, I raised another point here that in these situations a "better" approach might be to utilise already-selected assets more, rather than forcing it to invest in new capacity. Don't really have a plan for how to implement that though
| let max_act = *asset.get_activity_per_capacity_limits(&time_slice).end(); | ||
| (time_slice, max_act * capacity) | ||
| }) | ||
| } |
There was a problem hiding this comment.
I don't think this correctly gives you "maximum activity", unfortunately. This will respect individual timeslice limits, but not necessarily seasonal or annual limits.
Unfortunately this means we could end up in a situation where appraisal "eliminates" unmet demand, but when it comes to dispatch, where seasonal/annual limits are enforced, it could fail to meet demands at that point.
| let cost_index = lcox( | ||
| capacity.total_capacity(), | ||
| coefficients.capacity_coefficient, | ||
| &results.activity, |
There was a problem hiding this comment.
I'm not really sure it makes sense to appraise using max activity in all timeslices, when in reality there may only be demand in one or two timeslices, and we want to appraise based on ability to meet those demands
Description
This PR adds fallback code to calculate metrics (NPV/LCOX) for assets which failed to dispatch during appraisal. It does this by assuming maximum activity for each time slice.
I've tested this code with #1319 and it's sufficient to get the failing models to work 🥳 (though perhaps we shouldn't need this hack).
I've tried not to make my changes too invasive to reduce the pain of rebasing #1319 and #1357, but unfortunately some merge conflicts were inevitable. The code could use a refactor, but I decided to hold off until everything outstanding was merged first. (@tsmbland: I'm happy to try to rebase #1357 for you once this is done.)
Closes #1347.
Type of change
Key checklist
$ cargo test$ cargo docpresent in the previous release
Further checks