Revert "Use TimeSliceSelection for demand maps" - #1546
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1546 +/- ##
=======================================
Coverage 90.83% 90.84%
=======================================
Files 61 61
Lines 9014 9019 +5
Branches 9014 9019 +5
=======================================
+ Hits 8188 8193 +5
Misses 507 507
Partials 319 319 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
🟡 Changes recommended
A critical unmet-demand tolerance issue and missing regression coverage remain unresolved.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Reverts #1540, restoring time-slice-keyed demand handling across investment appraisal and market selection.
Changes:
- Restores per-time-slice demand flattening and appraisal handling.
- Updates constraints, output, fixtures, benchmarks, and regression data.
- Restores prior time-slice serialization behavior.
File summaries
| File | Summary |
|---|---|
tests/data/simple/debug_appraisal_results.csv |
Updates regression expectations. |
src/time_slice.rs |
Restores prior enum serialization behavior. |
src/simulation/market.rs |
Uses time-slice demand lookups. |
src/simulation/investment/appraisal/optimisation.rs |
Computes per-time-slice unmet demand. |
src/simulation/investment/appraisal/constraints.rs |
Aggregates demand for constraints. |
src/simulation/investment.rs |
Restores time-slice demand maps and flattening. |
src/output.rs |
Restores per-slice appraisal output fields. |
src/fixture.rs |
Updates appraisal fixtures. |
benches/assets.rs |
Updates benchmark demand handling. |
Review details
Suppressed comments (2)
src/simulation/investment.rs:163
- This conversion is the only path that turns annual/seasonal input demand into the per-time-slice map consumed by appraisal, but the unit tests in this module only exercise already-flattened maps. Please add parameterised coverage for at least annual and seasonal selections, asserting that each selection's per-slice values sum to the original demand and that separate selections do not overwrite one another; otherwise a regression here can silently change investment demand.
// We split the demand equally over all time slices in the selection
// NOTE: since demands will only be balanced to the time slice level of the commodity
// it doesn't matter how we do this distribution, only the total matters.
#[allow(clippy::cast_precision_loss)]
let n_time_slices = time_slice_selection.iter(time_slice_info).count() as f64;
let demand_per_slice = *demand / Dimensionless(n_time_slices);
for (time_slice, _) in time_slice_selection.iter(time_slice_info) {
demand_map.insert(
(commodity_id.clone(), region_id.clone(), time_slice.clone()),
demand_per_slice,
);
}
src/simulation/investment/appraisal/constraints.rs:56
- This flow lookup is now inside the time-slice loop even though the coefficient is constant for the whole selection. Appraisal runs this constraint construction for every candidate and round, so this adds one
IndexMaplookup per time slice unnecessarily; hoist the lookup before the inner loop.
let flow_coeff = asset.get_flow(&commodity.id).unwrap().coeff;
- Files reviewed: 9/10 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| let unmet_per_slice = (demand_for_selection - supply_for_selection).max(Flow(0.0)) | ||
| / Dimensionless(time_slices.len() as f64); |
Reverts #1540