Use integer variables in appraisal for divisible assets #1075

tsmbland · 2026-01-14T13:08:12Z

Description

This PR modifies the appraisal optimisation to use integer variables for divisible assets, to ensure that capacities are valid multiples of the unit size.

This was a bit fiddly as there was lots of conversion between capacities in absolute terms and capacities as unit counts, but I think it's all correct. I also had to round capacity limits up to the nearest multiple of the unit size to ensure that sufficient capacity can be installed.

Hopefully once we've got a better way of representing divisible assets we'll be able to tidy this up a bit. I didn't want to polish this too much until then, hence the lack of tests etc.

Fixes #1044 I guess this is enough to close the issue. There are some more points in there, but my main issue was that we were appraising unrealistic capacities (e.g. half a wind turbine), which this does fix. Note, this only affects initial appraisal for commissioning new assets - when it comes to reappraisal assets will have already been split into individual units which are appraised individually.

Type of change

Bug fix (non-breaking change to fix an issue)
New feature (non-breaking change to add functionality)
Refactoring (non-breaking, non-functional change to improve maintainability)
Optimization (non-breaking change to speed up the code)
Breaking change (whatever its nature)
Documentation (improve or add documentation)

Key checklist

All tests pass: $ cargo test
The documentation builds and looks OK: $ cargo doc
Update release notes for the latest release if this PR adds a new feature or fixes a bug
present in the previous release

Further checks

Code is commented, particularly in hard-to-understand areas
Tests added that prove fix is effective or that feature works

codecov · 2026-01-14T15:22:01Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 82.26%. Comparing base (94993c1) to head (c500f3f).
⚠️ Report is 11 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1075      +/-   ##
==========================================
+ Coverage   82.18%   82.26%   +0.07%     
==========================================
  Files          53       53              
  Lines        7383     7416      +33     
  Branches     7383     7416      +33     
==========================================
+ Hits         6068     6101      +33     
  Misses       1025     1025              
  Partials      290      290

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copilot

Pull request overview

This PR modifies the appraisal optimization to use integer variables for divisible assets, ensuring that appraised capacities are valid multiples of the unit size. The change prevents unrealistic capacity recommendations (e.g., half a wind turbine) during initial appraisal of new assets.

Changes:

Modified optimization to use unit count as the decision variable for divisible assets, with proper conversion between unit counts and total capacity
Updated capacity constraints to handle unit-based variables, including division of capacity limits by unit size
Adjusted activity constraints to account for unit-based capacity representation
Added helper methods to Asset for converting between capacity and unit counts

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File	Description
src/simulation/investment/appraisal/optimisation.rs	Modified variable creation to use unit count for divisible assets and convert results back to total capacity
src/simulation/investment/appraisal/constraints.rs	Updated capacity and activity constraints to handle unit-based capacity variables
src/simulation/investment.rs	Added rounding of capacity limits to nearest unit size multiple for divisible assets
src/asset.rs	Added helper methods for capacity-to-units conversion and rounding capacity to unit size

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

src/simulation/investment/appraisal/constraints.rs

Copilot

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

src/asset.rs

Copilot · 2026-01-14T16:00:39Z

src/simulation/investment/appraisal/constraints.rs

+        // Sanity check: capacity_limit should be a whole number of units (i.e pre-adjusted
+        // capacity limit was a multiple of unit size)
+        assert!(approx_eq!(f64, capacity_limit, capacity_limit.round()));


This assertion can fail for commissioned assets with divisible processes whose capacity is not an exact multiple of the unit size. The code in src/input/asset.rs allows such assets to be loaded (with a warning), but when these assets are appraised, this assertion will panic. For commissioned assets, the capacity should be used as-is without requiring it to be a perfect multiple of unit_size, or the capacity should be rounded to the nearest unit count before the assertion.

Nope. Assets in the input will get split into units by divide_asset prior to reaching this point

alexdewar

I've suggested a couple of small stylistic things, but otherwise LGTM 😄

src/simulation/investment/appraisal/optimisation.rs

alexdewar · 2026-01-15T14:26:54Z

src/simulation/investment.rs

+                // For divisible assets, round up to the nearest multiple of the process unit size
+                if asset.is_divisible() {
+                    max_capacity = asset.round_capacity_to_unit_size(max_capacity);
+                }


This threw me when I first read it because I assumed it was rounding the capacity of the asset and didn't notice there was an extra argument.

I'm wondering if it might be clearer to have round_capacity_to_unit_size be a standalone function in this file instead. Also, it could handle the case of non-divisible assets by just returning the capacity argument rather than panicking. I think this might be more readable.

This is obvs a bit subjective, so up to you.

I'm going to get rid of this in #1079 (if we agree to go down that route), so I'll leave it for now

Co-authored-by: Alex Dewar <[email protected]>

Copilot

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-01-15T14:46:21Z

src/asset.rs

+    pub fn capacity_to_units(&self, capacity: Capacity) -> u32 {
+        let unit_size = self.unit_size().expect("Asset must be divisible");
+        #[allow(clippy::cast_possible_truncation, clippy::cast_sign_loss)]
+        {
+            (capacity / unit_size).value().ceil() as u32
+        }
+    }


The new method capacity_to_units() lacks test coverage. Consider adding unit tests to verify correct behavior for edge cases like exact multiples, rounding up, and values smaller than unit_size.

Copilot · 2026-01-15T14:46:22Z

src/asset.rs

+    pub fn round_capacity_to_unit_size(&self, capacity: Capacity) -> Capacity {
+        let unit_size = self.unit_size().expect("Asset must be divisible");
+        let n_units = self.capacity_to_units(capacity);
+        Capacity(unit_size.value() * n_units as f64)
+    }


The new method round_capacity_to_unit_size() lacks test coverage. Consider adding unit tests to verify correct rounding behavior for various capacity and unit_size combinations.

Copilot · 2026-01-15T14:46:22Z

tests/data/simple_divisible/assets.csv

+2,WNDFRM,GBR,A0_ELC,,2020,,3.964844
+3,GASCGT,GBR,A0_ELC,,2020,,2.43


The removal of decommission years (2040) for assets 2 and 3 is not explained in the PR description. If this change is intentional and related to the integer variable modifications, it should be documented; otherwise, it may be an unintended side effect.

Suggested change

2,WNDFRM,GBR,A0_ELC,,2020,,3.964844

3,GASCGT,GBR,A0_ELC,,2020,,2.43

2,WNDFRM,GBR,A0_ELC,,2020,2040,3.964844

3,GASCGT,GBR,A0_ELC,,2020,2040,2.43

Copilot · 2026-01-15T14:46:23Z

tests/data/simple_divisible/assets.csv

+8,RELCHP,GBR,A0_RES,,2030,,255.83840587648046
+9,GASCGT,GBR,A0_ELC,,2030,2040,3.1192651014219064


Assets 8 and 9 show updated capacities and different asset types/IDs compared to the original data. The change from RGASBR to RELCHP for asset 8 and the significant capacity changes (1000.0 to 255.83840587648046) should be verified as intentional outcomes of the integer optimization.

tsmbland and others added 5 commits January 14, 2026 10:13

Use integer variables in dispatch for divisible assets

192d999

Merge branch 'child_asset_capacity' into divisible_appraisal

ce77870

Adjust input and output capacities for the appraisal

9ff532c

Simplify get_demand_limiting_units

1d3ba5d

Add helpers

f820ab9

Base automatically changed from child_asset_capacity to main January 14, 2026 15:18

Merge branch 'main' into divisible_appraisal

6998cef

tsmbland marked this pull request as ready for review January 14, 2026 15:36

Copilot AI review requested due to automatic review settings January 14, 2026 15:36

Copilot started reviewing on behalf of tsmbland January 14, 2026 15:37 View session

Copilot AI reviewed Jan 14, 2026

View reviewed changes

src/simulation/investment/appraisal/constraints.rs Outdated Show resolved Hide resolved

src/simulation/investment/appraisal/constraints.rs Outdated Show resolved Hide resolved

Actually use integer variables! (+ copilot suggestions)

acb993c

tsmbland requested review from Aurashk and alexdewar January 14, 2026 15:51

Merge branch 'main' into divisible_appraisal

43d3b50

Copilot AI review requested due to automatic review settings January 14, 2026 15:56

Copilot started reviewing on behalf of tsmbland January 14, 2026 15:57 View session

Copilot AI reviewed Jan 14, 2026

View reviewed changes

tsmbland changed the title ~~Use integer variables in dispatch for divisible assets~~ Use integer variables in appraisal for divisible assets Jan 14, 2026

tsmbland mentioned this pull request Jan 14, 2026

Use integer capacity variables for dispatch #1078

Open

11 tasks

alexdewar approved these changes Jan 15, 2026

View reviewed changes

Update src/simulation/investment/appraisal/optimisation.rs

5491a02

Co-authored-by: Alex Dewar <[email protected]>

Copilot AI review requested due to automatic review settings January 15, 2026 14:43

Merge branch 'main' into divisible_appraisal

c500f3f

Copilot started reviewing on behalf of tsmbland January 15, 2026 14:44 View session

tsmbland enabled auto-merge January 15, 2026 14:44

Copilot AI reviewed Jan 15, 2026

View reviewed changes

tsmbland merged commit 25f3813 into main Jan 15, 2026
8 checks passed

tsmbland deleted the divisible_appraisal branch January 15, 2026 15:03

		2,WNDFRM,GBR,A0_ELC,,2020,,3.964844
		3,GASCGT,GBR,A0_ELC,,2020,,2.43

		8,RELCHP,GBR,A0_RES,,2030,,255.83840587648046
		9,GASCGT,GBR,A0_ELC,,2030,2040,3.1192651014219064

Use integer variables in appraisal for divisible assets #1075

Use integer variables in appraisal for divisible assets #1075

Uh oh!

Conversation

tsmbland commented Jan 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Type of change

Key checklist

Further checks

Uh oh!

codecov bot commented Jan 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Copilot AI Jan 14, 2026

Choose a reason for hiding this comment

Uh oh!

tsmbland Jan 14, 2026

Choose a reason for hiding this comment

Uh oh!

alexdewar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

alexdewar Jan 15, 2026

Choose a reason for hiding this comment

Uh oh!

tsmbland Jan 15, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Jan 15, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 15, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 15, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 15, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

tsmbland commented Jan 14, 2026 •

edited

Loading

codecov bot commented Jan 14, 2026 •

edited

Loading