Make assets divisible

[dalonsoa]

Description

Implements the division of assets when they are commissioned. For now, we are allowing fractional unit sizes, but otherwise, all children assets are identical. They keep a reference to the parent asset, in case we want to group them at some point. The parent asset becomes Divided, its capacity made zero and store in a separate pool.

TODO

• Address the assets commissioned during investment.
• Add tests.

Fixes #936

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

Further checks

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

[codecov]

Codecov Report

❌ Patch coverage is 87.87879% with 12 lines in your changes missing coverage. Please review.
✅ Project coverage is 80.85%. Comparing base (b55c6c1) to head (ba13835).
⚠️ Report is 67 commits behind head on main.

Files with missing lines	Patch %	Lines
src/asset.rs	90.00%	9 Missing ⚠️
src/input/process.rs	50.00%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1030      +/-   ##
==========================================
+ Coverage   80.71%   80.85%   +0.13%     
==========================================
  Files          52       52              
  Lines        6924     6998      +74     
  Branches     6924     6998      +74     
==========================================
+ Hits         5589     5658      +69     
- Misses       1063     1068       +5     
  Partials      272      272              

☔ 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.

[dalonsoa]

There's a couple of missing bits, but it will be good to have some early feedback.

I've run it making divisible one of the process of the simple model and, not surprisingly, while it works, I get:

[09:21:18 WARN muse2::simulation::investment] Could not resolve deadlock between equally good appraisals for Agent id: A0_GEX, Commodity: 'GASPRD', Region: GBR. Options: [Process id: 'GASDRV' (State: Commissioned, Asset id: 14), Process id: 'GASDRV' (State: Commissioned, Asset id: 15), Process id: 'GASDRV' (State: Commissioned, Asset id: 16), Process id: 'GASDRV' (State: Commissioned, Asset id: 17), Process id: 'GASDRV' (State: Commissioned, Asset id: 18), Process id: 'GASDRV' (State: Commissioned, Asset id: 19), Process id: 'GASDRV' (State: Commissioned, Asset id: 20)]. Selecting first option.
[09:21:18 WARN muse2::simulation::investment] Could not resolve deadlock between equally good appraisals for Agent id: A0_GEX, Commodity: 'GASPRD', Region: GBR. Options: [Process id: 'GASDRV' (State: Commissioned, Asset id: 15), Process id: 'GASDRV' (State: Commissioned, Asset id: 16), Process id: 'GASDRV' (State: Commissioned, Asset id: 17), Process id: 'GASDRV' (State: Commissioned, Asset id: 18), Process id: 'GASDRV' (State: Commissioned, Asset id: 19), Process id: 'GASDRV' (State: Commissioned, Asset id: 20)]. Selecting first option.
[09:21:18 WARN muse2::simulation::investment] Could not resolve deadlock between equally good appraisals for Agent id: A0_GEX, Commodity: 'GASPRD', Region: GBR. Options: [Process id: 'GASDRV' (State: Commissioned, Asset id: 16), Process id: 'GASDRV' (State: Commissioned, Asset id: 17), Process id: 'GASDRV' (State: Commissioned, Asset id: 18), Process id: 'GASDRV' (State: Commissioned, Asset id: 19), Process id: 'GASDRV' (State: Commissioned, Asset id: 20)]. Selecting first option.
[09:21:18 WARN muse2::simulation::investment] Could not resolve deadlock between equally good appraisals for Agent id: A0_GEX, Commodity: 'GASPRD', Region: GBR. Options: [Process id: 'GASDRV' (State: Commissioned, Asset id: 17), Process id: 'GASDRV' (State: Commissioned, Asset id: 18), Process id: 'GASDRV' (State: Commissioned, Asset id: 19), Process id: 'GASDRV' (State: Commissioned, Asset id: 20)]. Selecting first option.
[09:21:18 WARN muse2::simulation::investment] Could not resolve deadlock between equally good appraisals for Agent id: A0_GEX, Commodity: 'GASPRD', Region: GBR. Options: [Process id: 'GASDRV' (State: Commissioned, Asset id: 18), Process id: 'GASDRV' (State: Commissioned, Asset id: 19), Process id: 'GASDRV' (State: Commissioned, Asset id: 20)]. Selecting first option.
[09:21:18 WARN muse2::simulation::investment] Could not resolve deadlock between equally good appraisals for Agent id: A0_GEX, Commodity: 'GASPRD', Region: GBR. Options: [Process id: 'GASDRV' (State: Commissioned, Asset id: 19), Process id: 'GASDRV' (State: Commissioned, Asset id: 20)]. Selecting first option.

Children are equally good, so now the solver has problems to decide which one to chose during the appraisal. I guess we need to think how to best resolve this before moving forward.

[alexdewar]

Looking good on the whole 😄.

I'm not totally sure if we need "parent" assets though. Is that purely so we can group the children? If so, I'm wondering if it would be better to have a separate group ID for this, or to just not bother grouping them for now (we can always add it later if we really need to keep track). Unless I'm mistaken, the parent assets will never be decommissioned, which isn't exactly the end of the world, but it may confuse users when they look at their output file and it looks like there are assets with zero capacity that have lived for the whole simulation. The alternative is to decommission parent assets when their children have been decommissioned, but that would require keeping count of the number of children that are still operating, which would be annoying. The parents effectively don't exist in the simulation anyway, so I think we can just do away with them and make our lives easier. I'd change the signature of divide_asset so it consumes self instead (i.e. change the &mut self arg to just self).

I'm guessing when you talk about missing bits, you mean that this will currently only work with divisible assets in assets.csv, not with divisible assets created from processes in the investment steps? I guess these will currently just be created as one big asset instead of lots of little ones. Just double-checking that that's the plan -- I'm a bit out of the loop!

[alexdewar]

PS -- In terms of the warning, I'm wondering if it would be easiest just to remove it. Is there a particular reason we think it would be helpful for users to know if there are two equally good assets? It doesn't seem especially dangerous on the face of it.

[dalonsoa]

I'm not totally sure if we need "parent" assets though. Is that purely so we can group the children? If so, I'm wondering if it would be better to have a separate group ID for this, or to just not bother grouping them for now (we can always add it later if we really need to keep track). Unless I'm mistaken, the parent assets will never be decommissioned, which isn't exactly the end of the world, but it may confuse users when they look at their output file and it looks like there are assets with zero capacity that have lived for the whole simulation. The alternative is to decommission parent assets when their children have been decommissioned, but that would require keeping count of the number of children that are still operating, which would be annoying. The parents effectively don't exist in the simulation anyway, so I think we can just do away with them and make our lives easier. I'd change the signature of divide_asset so it consumes self instead (i.e. change the &mut self arg to just self).

Indeed, we can ditch the parent altogether, but I'd keep the parent_id - maybe renamed group_id - so we can track those assets comming from the same parent. I'm pretty sure we will want to use that at some point. The main reason I left it there in a separate status is because it is not a decomissioned asset, in reality, as it was never commissioned, and leaving it disapear without trace didn't look right.

I'm guessing when you talk about missing bits, you mean that this will currently only work with divisible assets in assets.csv, not with divisible assets created from processes in the investment steps? I guess these will currently just be created as one big asset instead of lots of little ones. Just double-checking that that's the plan -- I'm a bit out of the loop!

Yes, so far I've addressed only the assets coming from Future not from Selected. I still need to do those. And add dedicated tests to check this functionality.

[alexdewar]

Indeed, we can ditch the parent altogether, but I'd keep the parent_id - maybe renamed group_id - so we can track those assets comming from the same parent. I'm pretty sure we will want to use that at some point. The main reason I left it there in a separate status is because it is not a decomissioned asset, in reality, as it was never commissioned, and leaving it disapear without trace didn't look right.

Ok cool. How about a group ID for these types of assets then? We could just use a number and auto-increment it for new groups, like we do with asset IDs. That would solve the problem of keeping track of groups, without having extra assets with strange behaviours. I guess we'll probably want to write this field to the assets.csv output file? I suppose we could give non-divisible assets group IDs too, but it's probably clearer if the group ID is None for those...

Yes, so far I've addressed only the assets coming from Future not from Selected. I still need to do those. And add dedicated tests to check this functionality.

👍

[dalonsoa]

Ok cool. How about a group ID for these types of assets then? We could just use a number and auto-increment it for new groups, like we do with asset IDs. That would solve the problem of keeping track of groups, without having extra assets with strange behaviours.

I'm not sure I follow this. a group_id in the children assets is what I suggested, but I'm not sure if you mean keeping the parent asset as a normal, commissioned asset containing the group ID. My confusion is with the extra assets with strange behaviours bit.

[alexdewar]

I'm not sure I follow this. a group_id in the children assets is what I suggested, but I'm not sure if you mean keeping the parent asset as a normal, commissioned asset containing the group ID. My confusion is with the extra assets with strange behaviours bit.

Oh, sorry. What I meant was that if we have a group ID we won't need parent assets anymore, which means we don't have to worry about the special handling for them (e.g. when to decommission them).

[tsmbland]

I think this is a good place to start, and nice that this is achievable without massive changes to the code. I worry a bit about performance, but there are probably some tricks we can use:

• The fact that all assets with the same parent will be identical (apart from the little fractional asset at the end) means that we should never need to appraise all of them at any one time - we can just, for example, appraise the one with the lowest id number each round as we only select one each round anyway (would also fix the problem with equal best assets)
• Similarly, when it comes to dispatch we don't need activity variables for all assets with the same parent - we could just aggregate them, run the dispatch, and distribute the flows equally afterwards

Not that we need to do anything about this right away, but long term we definitely do need to consider performance so should think ahead of ways to make our lives easier.

[alexdewar]

I've also tagged Copilot for review. The stylistic stuff it picks up is usually not that helpful, but sometimes it clocks logic errors, which is handy.

[Copilot]

Pull request overview

This PR implements asset division functionality for commissioned assets. When a process has a unit_size defined, assets of that process are divided into multiple smaller assets upon commissioning, with each child asset having a capacity of unit_size (or less for the remainder). All children assets share a common group_id to enable potential future regrouping.

Key changes:

• Added unit_size field to Process struct to specify divisible capacity units
• Implemented divide_asset() method to split assets into multiple children based on unit_size
• Added group_id field to commissioned assets to track assets created from the same parent
• Updated asset commissioning logic in AssetPool to handle division automatically

Reviewed changes

Copilot reviewed 13 out of 13 changed files in this pull request and generated 8 comments.

Show a summary per file

File	Description
src/process.rs	Added unit_size field to Process struct with documentation
src/input/process.rs	Added unit_size parsing and validation (must be > 0 if present)
src/asset.rs	Core implementation: added group_id to AssetState::Commissioned, implemented divide_asset() and is_divisible() methods, updated AssetPool commissioning logic to divide assets, updated mothball/unmothball to handle group_id, added comprehensive tests
src/output.rs	Added group_id field to AssetRow output structure
src/fixture.rs	Updated test fixture to include unit_size field (set to None)
src/input/agent/search_space.rs	Updated test process creation to include unit_size field
schemas/input/processes.yaml	Added unit_size field documentation to input schema
schemas/output/assets.yaml	Added group_id field documentation to output schema
tests/data/*/assets.csv	Updated all test CSV files to include empty group_id column

---

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

[Copilot]

Typo in assertion message: "Divisbile" should be "Divisible".

Suggested change

	"Divisbile asset cannot be divided!"
	"Divisible asset cannot be divided!"

[alexdewar]

Actually, you don't really need a message at all for a test, I think.

[dalonsoa]

But it is fun!

[alexdewar]

There are a few lines you forgot to delete, but other than that I think we're good to go 😄

[tsmbland]

This is only to give myself chance to review this properly before it gets merged! Will have a look today

[tsmbland]

Sorry for holding this up.

I have some reservations, and I guess what I'm struggling to decide is whether this PR is a step in the right direction and my concerns can all be addressed by building on top of this, or whether we might be better off completely re-thinking the problem and exploring other ideas.

My main concerns:

• Performance, as mentioned above. I've just tried running this on the "simple" example with a unit size of 1 for all processes (perhaps silly, but also not entirely unreasonable) and well over an hour later it was still running investments for the first year (dispatch, while slow, was actually not as bad as I was expecting given that there are presumably 10s of thousands of variables). I think we can mostly address this with my suggestions above, but until then I think that we should hide this behind the "please_allow_broken_results" option, or at the very least warn users in the documentation, especially if we might want to make a release before we fix this up.

• The little "stub" assets when capacity isn't divisible by the unit size. I know these will be small and few, but it's still a bit unsatisfying to have assets that violate the unit size (e.g. half a wind turbine). It's one thing to do this with input assets, as the user has some responsibility there, but I don't really think we should be doing this for assets installed by MUSE. A simple fix might be to change the line child.set_capacity(unit_size.min(capacity)) to child.set_capacity(unit_size), but I think there might be better approaches.

• Tranching. The current approach is to progressively select asset capacity in chunks, add these chunks together, then break the capacity up according to the unit size. It all seems a bit convoluted to me. Also, since tranche size and unit size are independent, you might, for example, have a unit size is 1 and a tranche capacity of 1.5, so you'd be appraising e.g. 1.5 wind turbines, which is a bit wrong. An easy fix might be to modify the tranche capacity (determined by get_demand_limiting_capacity) so that it's consistent with the unit size (i.e. rounding up), but, again, I think there might be better approaches.

• Circularities. This throws a little bit of a spanner in the works for my solution to the circularities problem, where I'm allowing all new assets to freely change their capacities (which would allow divided assets to deviate from the unit size). I guess it's up to me to think of solutions, and circularities are already hidden behind "please_allow_broken_results" so it's not the end of the world, but I do wonder if there might be an alternative approach to this problem that would make my life a bit easier.

• Outputs. No user, unless they're debugging, is going to want to keep track of thousands of tiny assets. All most users will want to know is when investment/decommissioning decisions were made, how much capacity was added/removed, total capacities of each process in each region/year, and flows over these total capacities. If we're going down this route then I think we should reconsider the format of the output data we present to users. (Not unique to this PR though, as presumably any solution to the partial decommissioning problem would require us to change the output format).

I'm a bit fuzzy on alternative approaches, but some vague ideas:

• It feels a little wrong to me that assets from the same parent aren't grouped together within an object, and only connected by a shared attribute (group_id). Thinking about my ideas for performance mentioned earlier - appraise only the first unit in each group, and run dispatch over the group as a whole - it seems to me that these would be much easier if assets from the same group were actually grouped together in some way. Likewise for the circularity and outputs problems, it seems to me that any solutions would be made easier by having groups actually grouped together in some way. I don't really have any genius suggestions off the top of my head, it might just require some experimenting to see what works.
• Integer variables in the appraisal optimisation: When we appraise new assets, the optimisation has a capacity variable that can vary freely to an upper limit (determined by the tranche size). Instead of allowing this to take any value, we could constrain it to multiples of unit_size by framing capacity as unit_count * unit_size, where unit_count is an integer variable. You can do this quite easily with highs (e.g. I did this with order_sccs in the graph module)
• That said, I think we should do-away with tranching for divisible assets and just appraise one unit at a time (with fixed capacity). Then we don't have to worry about merging and splitting up capacity, and it would also fix my concern about "stub" assets. It could be very slow if the unit size is small, but still, since we're retaining existing assets one unit at a time, I don't see why we shouldn't go one unit at a time when we're initially commissioning new assets.

After writing all this, I think I'm leaning towards going ahead with this PR and trying to address all of these issues on top of this. My only request to officially approve this would be to, at least, add a note in the documentation warning that this is an experimental feature.

[alexdewar]

My main concerns:

• Performance, as mentioned above. I've just tried running this on the "simple" example with a unit size of 1 for all processes (perhaps silly, but also not entirely unreasonable) and well over an hour later it was still running investments for the first year (dispatch, while slow, was actually not as bad as I was expecting given that there are presumably 10s of thousands of variables). I think we can mostly address this with my suggestions above, but until then I think that we should hide this behind the "please_allow_broken_results" option, or at the very least warn users in the documentation, especially if we might want to make a release before we fix this up.

Wondering if you ran it in release mode? I imagine it would still be slow as hell, but the speedups can be considerable, and it's what end users will be using, so it's more realistic. Bit off topic, but I thought worth mentioning. Random example: https://www.reddit.com/r/rust/comments/1afix7t/how_are_release_builds_so_fast/

I don't think it's actually a broken option, in that results will be correct (provided that MUSE2 ever finishes...), but it is potentially a gotcha, so maybe we could just warn users who set any unit_sizes and point them to a GitHub issue about the performance problem? Just a thought.

• The little "stub" assets when capacity isn't divisible by the unit size. I know these will be small and few, but it's still a bit unsatisfying to have assets that violate the unit size (e.g. half a wind turbine). It's one thing to do this with input assets, as the user has some responsibility there, but I don't really think we should be doing this for assets installed by MUSE. A simple fix might be to change the line child.set_capacity(unit_size.min(capacity)) to child.set_capacity(unit_size), but I think there might be better approaches.

Good point. Another advantage of having all of the assets have exactly the same capacity is it would make it simpler to lump them into one object (you don't have to keep track of the capacity of the stub asset).

I'm a bit fuzzy on alternative approaches, but some vague ideas:

• It feels a little wrong to me that assets from the same parent aren't grouped together within an object, and only connected by a shared attribute (group_id). Thinking about my ideas for performance mentioned earlier - appraise only the first unit in each group, and run dispatch over the group as a whole - it seems to me that these would be much easier if assets from the same group were actually grouped together in some way. Likewise for the circularity and outputs problems, it seems to me that any solutions would be made easier by having groups actually grouped together in some way. I don't really have any genius suggestions off the top of my head, it might just require some experimenting to see what works.
• Integer variables in the appraisal optimisation: When we appraise new assets, the optimisation has a capacity variable that can vary freely to an upper limit (determined by the tranche size). Instead of allowing this to take any value, we could constrain it to multiples of unit_size by framing capacity as unit_count * unit_size, where unit_count is an integer variable. You can do this quite easily with highs (e.g. I did this with order_sccs in the graph module)
• That said, I think we should do-away with tranching for divisible assets and just appraise one unit at a time (with fixed capacity). Then we don't have to worry about merging and splitting up capacity, and it would also fix my concern about "stub" assets. It could be very slow if the unit size is small, but still, since we're retaining existing assets one unit at a time, I don't see why we shouldn't go one unit at a time when we're initially commissioning new assets.

After writing all this, I think I'm leaning towards going ahead with this PR and trying to address all of these issues on top of this. My only request to officially approve this would be to, at least, add a note in the documentation warning that this is an experimental feature.

I agree that it's a step in the right direction -- I kind of imagined we'd do something simple but inefficient as a first pass though! Like you say, there are a lot of things to consider if we want to do something more sophisticated. I think it makes sense to open one or more issues for these and tackle them later.

[tsmbland]

My main concerns:

• Performance, as mentioned above. I've just tried running this on the "simple" example with a unit size of 1 for all processes (perhaps silly, but also not entirely unreasonable) and well over an hour later it was still running investments for the first year (dispatch, while slow, was actually not as bad as I was expecting given that there are presumably 10s of thousands of variables). I think we can mostly address this with my suggestions above, but until then I think that we should hide this behind the "please_allow_broken_results" option, or at the very least warn users in the documentation, especially if we might want to make a release before we fix this up.

Wondering if you ran it in release mode? I imagine it would still be slow as hell, but the speedups can be considerable, and it's what end users will be using, so it's more realistic. Bit off topic, but I thought worth mentioning. Random example: https://www.reddit.com/r/rust/comments/1afix7t/how_are_release_builds_so_fast/

No you're right it was debug mode, but even in release mode it's snail pace. No biggie, just means that someone has the exciting job in the future of speeding it up 10,000x!

I don't think it's actually a broken option, in that results will be correct (provided that MUSE2 ever finishes...), but it is potentially a gotcha, so maybe we could just warn users who set any unit_sizes and point them to a GitHub issue about the performance problem? Just a thought.

Yeah I guess so (apart from the "stub" assets which are a little bit wrong). Agree though that we need some kind of warning or note in the documentation

• The little "stub" assets when capacity isn't divisible by the unit size. I know these will be small and few, but it's still a bit unsatisfying to have assets that violate the unit size (e.g. half a wind turbine). It's one thing to do this with input assets, as the user has some responsibility there, but I don't really think we should be doing this for assets installed by MUSE. A simple fix might be to change the line child.set_capacity(unit_size.min(capacity)) to child.set_capacity(unit_size), but I think there might be better approaches.

Good point. Another advantage of having all of the assets have exactly the same capacity is it would make it simpler to lump them into one object (you don't have to keep track of the capacity of the stub asset).

Yeah good point. At that point you only really need one main object for asset properties and a count or set of ids

I'm a bit fuzzy on alternative approaches, but some vague ideas:

• It feels a little wrong to me that assets from the same parent aren't grouped together within an object, and only connected by a shared attribute (group_id). Thinking about my ideas for performance mentioned earlier - appraise only the first unit in each group, and run dispatch over the group as a whole - it seems to me that these would be much easier if assets from the same group were actually grouped together in some way. Likewise for the circularity and outputs problems, it seems to me that any solutions would be made easier by having groups actually grouped together in some way. I don't really have any genius suggestions off the top of my head, it might just require some experimenting to see what works.
• Integer variables in the appraisal optimisation: When we appraise new assets, the optimisation has a capacity variable that can vary freely to an upper limit (determined by the tranche size). Instead of allowing this to take any value, we could constrain it to multiples of unit_size by framing capacity as unit_count * unit_size, where unit_count is an integer variable. You can do this quite easily with highs (e.g. I did this with order_sccs in the graph module)
• That said, I think we should do-away with tranching for divisible assets and just appraise one unit at a time (with fixed capacity). Then we don't have to worry about merging and splitting up capacity, and it would also fix my concern about "stub" assets. It could be very slow if the unit size is small, but still, since we're retaining existing assets one unit at a time, I don't see why we shouldn't go one unit at a time when we're initially commissioning new assets.

After writing all this, I think I'm leaning towards going ahead with this PR and trying to address all of these issues on top of this. My only request to officially approve this would be to, at least, add a note in the documentation warning that this is an experimental feature.

I agree that it's a step in the right direction -- I kind of imagined we'd do something simple but inefficient as a first pass though! Like you say, there are a lot of things to consider if we want to do something more sophisticated. I think it makes sense to open one or more issues for these and tackle them later.

Sounds good. I'll open up some more issues tomorrow if I have time

[tsmbland]

Assuming it's a 2 minute job to add a note/warning for users, happy to go ahead with this

[dalonsoa]

@tsmbland how about this (9d7d69c) . I think the process.yml schema file is the right place to explain this, but I'm not sure if it would be better to add it to another place as well.

[tsmbland]

@tsmbland how about this (9d7d69c) . I think the process.yml schema file is the right place to explain this, but I'm not sure if it would be better to add it to another place as well.

Thanks. I think this is good enough for now

[tsmbland]

I've opened #1041, #1042, #1043, #1044 and #1045 related to my points above. I don't have a specific issue about grouping sibling assets into a single object, but I imagine we'll end up doing this as part of the solution for these issues.

I also realised that my point about circularities isn't quite right because we do the flexible capacity stuff on the parent assets (i.e. before division), but I think we will need to make changes here after #1041 and #1044, depending on what solution we come up with for these