Group commissioned divided assets under parent assets #1121

alexdewar · 2026-02-10T11:46:41Z

Description

Currently divisible assets are broken into small individual assets when commissioned, which is messy and bad for performance. As a step on the road to replacing these many sub-assets with a single super-asset representing all of them, I've added parent assets to divided assets (commissioned ones only). This will allow us to use parent assets for variables in the dispatch optimisation (#1043). We still store the individual child assets in the AssetPool, not the parents.

As parent assets need to have their capacity updated as children are decommissioned, I've made the capacity field mutable by wrapping it in a Cell (this is called "interior mutability" in Rust-speak). As this is potentially dangerous, however, there's only one private method which allows for changing the capacity of an immutable Asset and it's only used for parent assets.

In the longer run, we will be able to get rid of child assets altogether and just have parents. There are a few other issues we need to figure out first, though (e.g. #1042).

Closes #1100.

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-02-10T11:48:05Z

Codecov Report

❌ Patch coverage is 90.00000% with 11 lines in your changes missing coverage. Please review.
✅ Project coverage is 87.57%. Comparing base (5595bd8) to head (5b9736a).

Files with missing lines	Patch %	Lines
src/asset.rs	90.74%	9 Missing and 1 partial ⚠️
src/simulation/investment.rs	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1121      +/-   ##
==========================================
+ Coverage   87.54%   87.57%   +0.02%     
==========================================
  Files          55       55              
  Lines        7460     7500      +40     
  Branches     7460     7500      +40     
==========================================
+ Hits         6531     6568      +37     
- Misses        631      633       +2     
- Partials      298      299       +1

☔ 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 introduces “parent” assets for commissioned divided assets to enable grouping in downstream optimisation (per #1043/#1100), while continuing to store only the child assets in the active AssetPool. It also introduces controlled interior mutability for Asset.capacity so parent capacity can be updated as child units are decommissioned.

Changes:

Add AssetState::Parent and store parent: Option<AssetRef> on commissioned child assets; derive group_id from the parent.
Make Asset.capacity mutable via Cell<AssetCapacity> and decrement parent unit count when a child asset is decommissioned.
Add Asset::total_capacity() and update call sites; add Clippy configuration to ignore interior mutability for AssetRef.

Reviewed changes

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

File	Description
`src/asset.rs`	Adds parent/child relationships for divided assets, switches capacity storage to `Cell`, updates commissioning/decommissioning and tests.
`src/simulation/investment.rs`	Switches capacity logging to use `Asset::total_capacity()`.
`src/output.rs`	Outputs asset capacity via `Asset::total_capacity()`.
`clippy.toml`	Configures Clippy to ignore interior mutability warnings for `AssetRef`.

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

Copilot · 2026-02-10T11:54:01Z

src/asset.rs

-        for child in children.clone() {
+        let mut count = 0;
+        let mut total_child_capacity = Capacity(0.0);
+        asset.clone().into_for_each_child(&mut 0, |parent, child| {


This test won’t compile because &mut 0 attempts to take a mutable reference to an integer literal. Introduce a let mut next_group_id = 0u32; variable and pass &mut next_group_id instead.

Suggested change

asset.clone().into_for_each_child(&mut 0, |parent, child| {

let mut next_group_id = 0u32;

asset.clone().into_for_each_child(&mut next_group_id, |parent, child| {

...and yet it compiles 😆. You wouldn't get away with this in C/C++ though...

Copilot · 2026-02-10T11:54:02Z

src/asset.rs

+
+        let asset = AssetRef::from(asset);
+        let mut count = 0;
+        asset.clone().into_for_each_child(&mut 0, |parent, child| {


Same issue as above: &mut 0 is an invalid mutable reference to a literal and will fail to compile. Use a mutable local u32 for next_group_id.

src/asset.rs

Copilot · 2026-02-10T11:54:02Z

src/asset.rs

+        // Create a child of size `unit_size`
+        let child = AssetRef::from(Asset {
+            capacity: Cell::new(AssetCapacity::Discrete(1, unit_size)),
+            ..Asset::clone(&self)
+        });
+
+        // Turn this asset into a parent
+        let agent_id = self.agent_id().unwrap().clone();
+        self.make_mut().state = AssetState::Parent {
+            agent_id,
+            group_id: AssetGroupID(*next_group_id),
+        };
+        *next_group_id += 1;
+
+        // Run `f` over each child
+        for child in iter::repeat_n(child, n_units as usize) {
+            f(Some(&self), child);
+        }


iter::repeat_n(child, n_units) repeats the same AssetRef (shared Rc) and relies on downstream code calling make_mut() to force clones. If the closure ever stores/uses children without mutating them, all “children” will alias the same underlying asset. Consider constructing a fresh AssetRef inside the loop (or cloning the underlying Asset) so each child is distinct by construction and doesn’t depend on Rc::make_mut semantics.

I did this on purpose. It would be fine if a caller stored the identical child assets anyway though, I think!

src/asset.rs

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Copilot

Pull request overview

Copilot reviewed 4 out of 4 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

Pull request overview

Copilot reviewed 4 out of 4 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-02-10T12:26:28Z

src/asset.rs

    /// Hash an asset according to its state:
    /// - Commissioned assets are hashed based on their ID alone
-    /// - Selected assets are hashed based on `process_id`, `region_id`, `commission_year` and `agent_id`
+    /// - Selected assets are hashed based on `process_id`, `region_id`, `commission_year` and
+    ///   `agent_id`
    /// - Candidate assets are hashed based on `process_id`, `region_id` and `commission_year`
+    /// - Parent assets are hashed based on `agent_id` and `group_id`
    /// - Future and Decommissioned assets cannot currently be hashed


The Hash impl doc comment doesn’t match the implementation: Candidate, Selected, and Parent all hash process.id, region_id, commission_year, agent_id(), and group_id(), but the bullets describe different field sets (e.g. "Parent assets are hashed based on agent_id and group_id"). Please update the comment to reflect the current hashing logic.

Sort of, except that they don't all have these fields, in which case the result is None. I don't think it needs amending.

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Aurashk

As we discussed, I feel uneasy already about how many distinct forms an Asset can take through the AssetState, I would prefer a separate ParentAsset struct even if it repeats a lot of the Asset struct (which could be simplified later on). It looks like you can adapt AssetPool with a separate field Vec[ParentAsset] and handle commissions with a dedicated function. That being said I'm approving this and will leave up to your judgement whether that would fit well into the wider refactoring for divisible assets.

alexdewar added 6 commits February 10, 2026 09:56

Add Asset::total_capacity() helper method

c329e8f

Wrap Asset::capacity in Cell for interior mutability

554aa4c

asset.rs: Tweak hashing code

1330c3b

Give divided assets parents

8af4687

Refactor to replace divide_asset

e371fe6

Make parent assets hashable

0e0b41c

Copilot AI review requested due to automatic review settings February 10, 2026 11:46

Copilot started reviewing on behalf of alexdewar February 10, 2026 11:47 View session

Copilot AI reviewed Feb 10, 2026

View reviewed changes

Doc comment fixes

b0ef494

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Copilot AI review requested due to automatic review settings February 10, 2026 11:58

Copilot started reviewing on behalf of alexdewar February 10, 2026 11:58 View session

Copilot AI reviewed Feb 10, 2026

View reviewed changes

src/asset.rs Outdated Show resolved Hide resolved

src/asset.rs Show resolved Hide resolved

alexdewar added 2 commits February 10, 2026 12:16

Add test for when we decommission child asset

85d36d7

Update doc comment for AssetRef::hash()

240422a

Copilot AI review requested due to automatic review settings February 10, 2026 12:19

Copilot started reviewing on behalf of alexdewar February 10, 2026 12:19 View session

alexdewar requested a review from Aurashk February 10, 2026 12:22

Copilot AI reviewed Feb 10, 2026

View reviewed changes

Fix doc comment for AssetRef::into_for_each_child()

5b9736a

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

alexdewar force-pushed the parent-assets branch from 039dc1a to 5b9736a Compare February 10, 2026 12:29

Aurashk approved these changes Feb 10, 2026

View reviewed changes

	asset.clone().into_for_each_child(&mut 0, \|parent, child\| {
	let mut next_group_id = 0u32;
	asset.clone().into_for_each_child(&mut next_group_id, \|parent, child\| {

Group commissioned divided assets under parent assets #1121

Are you sure you want to change the base?

Group commissioned divided assets under parent assets #1121

Conversation

alexdewar commented Feb 10, 2026

Description

Type of change

Key checklist

Further checks

Uh oh!

codecov bot commented Feb 10, 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!

Copilot AI Feb 10, 2026

Choose a reason for hiding this comment

Uh oh!

alexdewar Feb 10, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 10, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI Feb 10, 2026

Choose a reason for hiding this comment

Uh oh!

alexdewar Feb 10, 2026

Choose a reason for hiding this comment

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!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Copilot AI Feb 10, 2026

Choose a reason for hiding this comment

Uh oh!

alexdewar Feb 10, 2026

Choose a reason for hiding this comment

Uh oh!

Aurashk left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

codecov bot commented Feb 10, 2026 •

edited

Loading