Make all assets divisible by tsmbland · Pull Request #1483 · EnergySystemsModellingLab/MUSE2

tsmbland · 2026-08-12T10:44:08Z

Description

This PR follows on from #1480, and effectively makes all assets divisible, in line with the plans described in #1441.

The main change to the code is simplifying AssetCapacity so that now all asset capacities consist of a unit count and unit size. "Non-divisible" assets (i.e. assets who's capacities cannot be broken up) can still exist, but these are now represented like all other assets, just with a unit count of 1 (I've created AssetCapacity::single to make it easier to construct these). There are then a lot of changes to the code to remove special treatment of AssetCapacity::Discrete vs AssetCapacity::Continuous capacities. Hopefully this makes things a lot simpler.

The main functional change (and the reason for doing all this to begin with) is that, whereas previously, assets without a defined process unit_size became non-divisible (represented by AssetCapacity::Continuous), they are now divisible, with a unit size calculated based on demand at the time of investment and the capacity_limit_factor (the "trial capacity" that's referred to in the documentation). This should give more realistic behaviour (in reality you'd never invest in one giant indivisible assets to meet all demands that can only be decommissioned in one go).

For now, I've gone for the approach of strictly enforcing unit sizes, which can lead to slight overinvestment as capacities of units meeting the final bit of demand are not capped (see discussion in the issue). Potential to revisit this later. Keeping the "demand limiting capacity" code for now, even though it's now unused, as it's self-contained and might be useful later.

Other changes:

renaming n_units to num_units throughout for consistency
during re-appraisal of existing assets, where we retain one asset at a time, we need to keep track of the original unit count to make sure that we don't retain more units than actually exist. Previously, this was implemented via the same map as process investment constraints, which I didn't like as it mixed distinct concepts, so I've split this into a new map (remaining units). Relevant for tackling Implement calculation for overall addition limit #1428
renamed the simple_divisible model to simple_unit_size (since now all models have divisible assets, but this one has a process with a fixed unit size)
added as_single_unit as a sort of shorthand for with_subset_of_units(1). Not strictly necessary, but I think it makes the code a bit easier to understand as as_single_unit can be far simpler than the more general with_subset_of_units
moves a check against DLC (now irrelevant) and investment limits outside of the main appraisal loop. This has been replaced by remove_candidates_exceeding_limits

Appreciate that this is probably a bit tricky to review. I tried to think of ways to break this up but it was ultimately quite difficult.

Fixes #1441

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-08-12T10:46:00Z

Codecov Report

❌ Patch coverage is 97.36842% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.26%. Comparing base (8691aa8) to head (7b3cf98).

Files with missing lines	Patch %	Lines
src/simulation/optimisation.rs	75.00%	1 Missing and 2 partials ⚠️
src/simulation/investment.rs	98.11%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1483      +/-   ##
==========================================
+ Coverage   90.12%   90.26%   +0.13%     
==========================================
  Files          60       60              
  Lines        8665     8614      -51     
  Branches     8665     8614      -51     
==========================================
- Hits         7809     7775      -34     
+ Misses        537      525      -12     
+ Partials      319      314       -5

☔ View full report in Codecov by Harness.
📢 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.

…te unit.

Copilot

Pull request overview

This PR completes the shift to treating all assets as unit-based (divisible) by removing the continuous-capacity representation and standardising dispatch/investment logic around integer unit counts with a defined unit_size. This aligns the simulation/investment model with the “all assets divisible” plan from #1441 and builds on #1480’s num_units support.

Changes:

Replaced AssetCapacity from a Continuous|Discrete enum to a (num_units, unit_size) representation and updated core asset APIs accordingly.
Updated investment candidate generation and reappraisal to operate on single-unit assets and track limits via remaining candidate capacity / remaining commissioned units.
Updated output schema and docs to reflect that num_units is always present and assets are always unit-composed.

Reviewed changes

Copilot reviewed 18 out of 18 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
src/simulation/optimisation/constraints.rs	Treats dispatch capacity variables as unit-count based and scales limits by `unit_size`.
src/simulation/optimisation.rs	Makes solution capacity iteration and flexible-capacity handling unit-based; updates capacity variable bounds logic.
src/simulation/market.rs	Builds candidate assets as single units, with dynamic `unit_size` when process lacks one.
src/simulation/investment/appraisal/constraints.rs	Uses `asset.total_capacity()` for appraisal constraint scaling.
src/simulation/investment/appraisal.rs	Switches appraisal metrics to use `total_capacity()` and updates tests for unit-based capacities.
src/simulation/investment.rs	Reworks candidate/commissioned appraisal selection around unit counts and capacity limits.
src/simulation.rs	Updates dispatch candidate creation to use the new `Asset::new_candidate` signature.
src/output.rs	Makes `num_units` non-optional in output rows and writes it for all assets.
src/model/parameters.rs	Removes validation for `candidate_asset_capacity` (now needs reintroducing).
src/input/asset.rs	Constructs `AssetCapacity` via `new`/`single` consistently for all assets.
src/fixture.rs	Updates fixtures to use unit-based capacities (including a multi-unit asset fixture).
src/asset/pool.rs	Updates tests/fixtures naming and unit-capacity construction.
src/asset/capacity.rs	Implements new unit-based `AssetCapacity` struct and updates tests.
src/asset.rs	Removes “divisible vs non-divisible” branching and updates asset creation/capacity APIs for unit-only assets.
schemas/output/asset_capacities.yaml	Updates schema so `num_units` is always present.
schemas/input/processes.yaml	Updates `unit_size` documentation to match unit-based model semantics.
schemas/input/assets.yaml	Updates `num_units` semantics/documentation for determining unit sizing.
docs/model/investment.md	Updates investment documentation to reflect unit-based assets and trial capacity behaviour.

Suppressed comments (1)

src/model/parameters.rs:340

candidate_asset_capacity is no longer validated. Because Capacity deserialisation allows negative values, a negative (or zero) candidate_asset_capacity can now make the model panic later (e.g. when used as the unit size for dispatch candidates in AssetCapacity::new, which asserts non-negative). This should be rejected during ModelParameters::validate() like before.

        // milestone_years
        check_milestone_years(&self.milestone_years)?;

        // capacity_limit_factor already validated with deserialise_proportion_nonzero

        // fallback_pricing_strategy already validated by deserialisation

        // commodity_balance_epsilon already validated with deserialise_finite_non_negative

        // value_of_lost_load
        check_value_of_lost_load(self.value_of_lost_load)?;

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Copilot

Pull request overview

Copilot reviewed 18 out of 18 changed files in this pull request and generated no new comments.

Suppressed comments (3)

src/simulation/market.rs:450

A candidate asset is currently constructed with unit_size = 0.0 as a placeholder and then immediately updated. This relies on allowing zero unit sizes and makes it easy to reintroduce divide-by-zero/validation issues later (e.g. if AssetCapacity::new is tightened to require > 0).

            // Create asset with zero capacity, which will be updated below
            let mut asset =
                Asset::new_candidate(process.clone(), region_id.clone(), Capacity(0.0), year)
                    .unwrap();

src/simulation/optimisation.rs:732

add_capacity_variables divides by unit_size when applying capacity_limits. If a flexible-capacity asset were ever created with unit_size == 0, this becomes a divide-by-zero, and even for valid sizes the limit should be converted to an integer unit bound (otherwise fractional bounds can permit slight limit violations due to floating point rounding).

        let unit_size = asset.capacity().unit_size();
        let current_units = asset.capacity().num_units();

        let lower = (current_units as f64 * (1.0 - capacity_margin)).max(0.0);

        let mut upper = current_units as f64 * (1.0 + capacity_margin);
        if let Some(limit) = capacity_limits.and_then(|limits| limits.get(asset)) {
            upper = upper.min((*limit / unit_size).value());
        }

src/input/asset.rs:141

This branch constructs AssetCapacity::single(asset.capacity) directly from the CSV capacity. Because AssetCapacity::new/single now assert that unit sizes are non-negative, a negative capacity value in assets.csv would panic before UserAsset::new can return a validation error. Adding an explicit ensure! here keeps invalid input as a recoverable error instead of a crash.

            // Without a process unit_size, lack of num_units implies the asset is indivisible
            // (consists of a single unit).
            AssetCapacity::single(asset.capacity)
        };

tsmbland · 2026-08-13T14:23:17Z

Hmm this is now giving different results on windows and ubuntu which is unfortunate. Hopefully once #1477 is done this should fix the problem

Edit: yes, I tried rebasing this on #1477 and it indeed fixed the problem

dc2917

Couple of minor suggestions for the documentation, and a bit of confusion over variable naming vs comments for what capacity represents - I think it's just a wording issue in the comments, as the fields in AssetCapacity are clear.

dc2917 · 2026-08-17T07:56:39Z

-      capacity up to the nearest whole unit. For processes without a `unit_size`, if provided it
-      determines the unit size; otherwise the asset is represented as one unit. Must be >0.
+      Optional. If provided, the asset is split into `num_units` units of size
+      `capacity / num_units` (taking precedence over the process's `unit_size`). If omitted and the


Suggested change

`capacity / num_units` (taking precedence over the process's `unit_size`). If omitted and the

`capacity / num_units` (taking precedence over the process' `unit_size`). If omitted and the

Couldn't help myself 😅

I think because I'm referring to one particular process (i.e. the process the asset belongs to), this is correct? Not an expert though!

dc2917 · 2026-08-17T08:29:04Z

            .map(|(asset, capacity_var)| {
-                // If the asset has a defined unit size, the capacity variable represents number of
-                // units, otherwise it represents absolute capacity
+                // The capacity variable represents number of units


I'm not sure what this means. The capacity variable represents the number of units, or the size of a unit? And which "capacity" variable? capacity_var, or asset_capacity?

The capacity variable in the optimisation is number of units. To make that meaningful, we also need to know the size of the unit. AssetCapacity stores both the number of units and the size (Capacity) of each unit

dc2917 · 2026-08-17T08:32:26Z

+        // Since capacity variables are numbers of units, we apply constraints to the unit count
+        let unit_size = asset.capacity().unit_size();


So asset.capacity() is the number of units (but asset.capacity().num_units() isn't?), and asset.capacity().unit_size() is the size of each one?

I agree this is super confusing...

I think that capacity variables refers to the capacity-related variables that go into the solver. We do not solve for the capacity, which is a real, continouous magnitude: we solve for the number of units, which results in a certain discrete capacity. asset.capacity() returns the AssetCapacity object, which in turn has unit_size and num_units attributes.

dc2917 · 2026-08-17T08:37:29Z

-                    upper_limit *= unit_size.value();
-                    lower_limit *= unit_size.value();
-                }
+                // The capacity variable represents number of units, so we need to multiply the


Perhaps it's a wording issue. Something like "capacity variable represents number of units" -> "capacity comprises all units"?

By this I mean that the capacity variable literally represents the number of units. i.e. an integer variable representing the unit count

dalonsoa

I've a few comments asking for clarification, but nothing major (I think). I'll wait to approve until then.

dalonsoa · 2026-08-18T12:56:13Z

-      Optional. For processes with a `unit_size`, if omitted it is calculated by rounding the
-      capacity up to the nearest whole unit. For processes without a `unit_size`, if provided it
-      determines the unit size; otherwise the asset is represented as one unit. Must be >0.
+      Optional. If provided, the asset is split into `num_units` units of size


Same comment as in previous PR :)

dalonsoa · 2026-08-18T13:00:13Z

+            unit_size.is_finite() && unit_size >= Capacity(0.0),
+            "Unit size must be a finite non-negative number"


What's the point of a zero unit_size? Shouldn't it be strictly possitive?

Currently we need it because get_candidate_assets can produce zero capacity candidate assets (i.e. if there's no demand for the commodity), which later get dropped.

I might see if I can improve this as we shouldn't need to create candidate assets if there's no demand

dalonsoa · 2026-08-18T13:03:13Z

+    /// Returns the number of units in this `AssetCapacity`.
+    pub fn num_units(&self) -> u32 {
+        self.num_units
+    }
+
+    /// Returns the unit size of this `AssetCapacity`.
+    pub fn unit_size(&self) -> Capacity {
+        self.unit_size
+    }


My rust is a bit rusty :P but cannot we just access the attributes directly? Why wrapping them in functions that do nothing but returning the attribute itself? Is it to keep them private and ensure they are not modified elsewhere by mistake - or reckless user?

Pretty much. I think it's generally better practice to keep attributes private and access with methods. Alex was more opinionated about this than me

dalonsoa · 2026-08-18T13:36:34Z

+            *remaining_capacity -= best_asset.total_capacity();

-            // If there's no capacity remaining, remove the asset from the options
-            if remaining_capacity.total_capacity() <= Capacity(0.0) {
+            // If there's not enough capacity remaining to install any more units, remove the
+            // asset from the investment options.
+            if *remaining_capacity < best_asset.total_capacity() {


Let's see if I get this right: you first remove the capacity of the asset from the remaining capacity and then, check if enough remains to install another asset in the future with the same capacity, right? But for the current round, the asset can still be installed because there was enough remaining capacity for a round - just not for two.

If that is the case, it is a bit confusing but I think I get it. If it is not this, then I'm not getting it...

dalonsoa · 2026-08-18T13:42:56Z

-            // Otherwise add it to the list of best assets. Selected assets are unmothballed.
-            best_assets.push(best_asset.with_no_mothballed_units());
+    } else {
+        // Commissioned assets: we've appraised a single unit, so remove one unit from the


Probably the comment should go somewhere else, but I leave it here as it is also pertinent: why do we appraise a single unit of commissioned assets? And why that means there are less units remaining? Are we installing one unit at a time in the case of already commissioned assets?

We're deciding unit-by-unit whether to retain the unit, and we can only retain as many as already existed

dalonsoa · 2026-08-18T13:48:37Z

+        // Since capacity variables are numbers of units, we apply constraints to the unit count
+        let unit_size = asset.capacity().unit_size();


I agree this is super confusing...

I think that capacity variables refers to the capacity-related variables that go into the solver. We do not solve for the capacity, which is a real, continouous magnitude: we solve for the number of units, which results in a certain discrete capacity. asset.capacity() returns the AssetCapacity object, which in turn has unit_size and num_units attributes.

dalonsoa · 2026-08-18T13:50:19Z

+            upper = upper.min((*limit / unit_size).value());
+        }
+
+        let var = problem.add_integer_column((coeff * unit_size).value(), lower..=upper);


That is why we use integer columns here, because the number of units are discrete. If we were handling capacity in the solver directly, these would need to be floats.

Originally it had a mix of integer and float variables. Now since all capacities are discrete they are all integer variables, which is simpler

dalonsoa · 2026-08-18T13:54:40Z

+Existing assets (i.e assets that have already been commissioned, whether via `assets.csv` or by
+MUSE) are appraised one unit at a time to decide how many units to retain. This allows partial
+retention — for example, some units of a multi-unit plant may be retained while others are
+mothballed.


How many to retain, and how many to installed, as it affects the remaining_units (see my comment above), right? I'm a bit confused about this.

For existing assets (i.e. already installed), this is about how many to retain. For candidate assets (below section), this is about how many to install

Your comment makes it very clear, but I think that same explanation should be in the code. If remaining_units means different things depending on the case, that should be clarified.

dalonsoa · 2026-08-18T13:56:14Z

+After investment is complete for a given MSY, any previously commissioned assets (or individual
+units making up the asset) that were not selected for retention are *mothballed*: their mothball
+year is recorded and they are removed from the active asset pool. They remain available for
+potential re-selection in future MSYs.


I understand that MSY is milestone year, which does not feel much longer than MSY and it is way clearer...

tsmbland changed the title ~~Store remaining_units count for investments~~ Make all assets divisible Aug 12, 2026

tsmbland force-pushed the assets_n_units branch from db5b58e to 6cef7f2 Compare August 12, 2026 14:00

tsmbland added 9 commits August 12, 2026 15:01

Store remaining_units count for investments

8a2b682

Remove AssetCapacity::Continuous

ebeee58

Fix sign error

d1eb51c

Update comments

415b23b

Remove candidate assets whose investment limit cannot fund one comple…

3adc3ba

…te unit.

Fix test compile issues

b3818ea

Fix partial_cmp

02832fd

Fix most other tests

1f32d9d

AssetCapacity::single

ec47667

tsmbland force-pushed the remove_nondivisible_assets branch from 0f448c1 to ec47667 Compare August 12, 2026 14:01

tsmbland added 8 commits August 12, 2026 15:23

Fix final unit test

b1abd33

Remove "divisible" terminology

57722ba

Add tests for AssetCapacity::new

22aa0db

Small cleanups

0b23a81

Fix old comment

b9733e7

More small tidy-ups

13655af

Update schemas

42931f7

Update docs

ba86143

tsmbland requested a lite review from Copilot August 12, 2026 16:57

Copilot started reviewing on behalf of tsmbland August 12, 2026 17:01 View session

Copilot AI reviewed Aug 12, 2026

View reviewed changes

Comment thread src/simulation/investment.rs Outdated

Comment thread src/simulation/optimisation.rs Outdated

Comment thread src/asset/capacity.rs

tsmbland linked an issue Aug 13, 2026 that may be closed by this pull request

All assets should be "divisible" #1441

Closed

tsmbland added 2 commits August 13, 2026 14:30

Fixes from review

6e8d98c

Restore check for candidate_asset_capacity

744277b

tsmbland requested a lite review from Copilot August 13, 2026 13:56

Copilot started reviewing on behalf of tsmbland August 13, 2026 13:57 View session

Copilot AI reviewed Aug 13, 2026

View reviewed changes

tsmbland added 2 commits August 13, 2026 15:14

Extra check for assets file

0085357

Update results

c2df880

tsmbland mentioned this pull request Aug 13, 2026

num_units column in assets.csv #1480

Merged

11 tasks

tsmbland marked this pull request as ready for review August 14, 2026 10:34

Renamed simple_divisible model

602ac95

tsmbland requested review from AdrianDAlessandro, dalonsoa and dc2917 August 14, 2026 10:50

dc2917 approved these changes Aug 17, 2026

View reviewed changes

dalonsoa reviewed Aug 18, 2026

View reviewed changes

Base automatically changed from assets_n_units to main August 19, 2026 06:47

tsmbland and others added 2 commits August 19, 2026 07:47

Merge branch 'main' into remove_nondivisible_assets

8e11b91

Better docstring for update_assets

3040d0f

tsmbland enabled auto-merge August 19, 2026 06:58

tsmbland disabled auto-merge August 19, 2026 07:00

tsmbland added 2 commits August 19, 2026 08:14

Merge branch 'main' into remove_nondivisible_assets

45eb8c1

Link to mothballing section

7b3cf98

tsmbland enabled auto-merge August 19, 2026 07:18

tsmbland merged commit f0fc937 into main Aug 19, 2026
6 checks passed

tsmbland deleted the remove_nondivisible_assets branch August 19, 2026 07:20

tsmbland mentioned this pull request Aug 19, 2026

Allow for "divisible" assets with continuous capacity rather than discrete #1269

Closed

	`capacity / num_units` (taking precedence over the process's `unit_size`). If omitted and the
	`capacity / num_units` (taking precedence over the process' `unit_size`). If omitted and the

		// Since capacity variables are numbers of units, we apply constraints to the unit count
		let unit_size = asset.capacity().unit_size();

		unit_size.is_finite() && unit_size >= Capacity(0.0),
		"Unit size must be a finite non-negative number"

Uh oh!

Conversation

tsmbland commented Aug 12, 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 Aug 12, 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!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

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

Uh oh!

Uh oh!

dc2917 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dalonsoa left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tsmbland commented Aug 12, 2026 •

edited

Loading

codecov Bot commented Aug 12, 2026 •

edited

Loading

tsmbland commented Aug 13, 2026 •

edited

Loading