Ensure that child assets have equal capacity #1070
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1070 +/- ##
==========================================
+ Coverage 82.09% 82.12% +0.02%
==========================================
Files 53 53
Lines 7310 7316 +6
Branches 7310 7316 +6
==========================================
+ Hits 6001 6008 +7
+ Misses 1019 1018 -1
Partials 290 290 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This pull request ensures that all child assets created from parent asset division have equal capacity by implementing a rounding-up strategy. When dividing an asset with capacity C by unit size U, the code now creates ceil(C / U) children, each with capacity U, which may result in total child capacity exceeding parent capacity if C is not an exact multiple of U.
Changes:
- Modified
divide_asset()to use ceiling division, ensuring all children have equal capacity - Updated documentation to clearly explain the rounding behavior and its implications
- Added comprehensive tests for both exact division and rounding cases
- Removed obsolete test assertions that checked for capacity equality between parent and children
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/asset.rs | Refactored divide_asset() implementation to use ceil(C/U) formula, ensuring all children have equal unit_size capacity; added tests for exact and rounded division scenarios |
| schemas/input/processes.yaml | Updated documentation to explain the rounding behavior and warn users about performance implications |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
alexdewar
left a comment
alexdewar
left a comment
There was a problem hiding this comment.
This all seems ok, except that I've got some questions about the rounding up.
For assets invested in by MUSE: will it create problems if we round up? I.e. could we end up with oversupply or something if we exceed the capacity suggested by HiGHS for the appraisal?
For assets in assets.csv: I actually think we should round down here, otherwise we're giving users more capacity than they requested, which doesn't seem right. This could be really stark if the unit size is 100 and they only put a capacity of 1, for example. We might want to allow a bit of leeway by using approx_eq!. Also: would it be worth warning users if the capacity they request isn't (approximately) divisible by unit size, just to avoid surprises?
No you shouldn't get oversupply. This is determined by the dispatch, and so long as there's a cost associated with activity then there should still only be as much activity as there needs to be, even if that results in unused capacity. In any case this is only a short term problem. Longer term we'll want the appraisal to only consider assets that are a multiple of the unit size, which brings us to #1044
My thinking was that users will have calibrated their inputs to make sure that there's enough capacity to meet demands in the base year. If we decrease capacities then there's a chance we'll break that calibration, whereas I don't think increasing capacities will ever cause problems.
I think a good idea to have a warning in cases like this. The reason I didn't include that in this PR is that currently this would also be raised for assets invested in by MUSE. Longer term, once we ensure that investment appraisal respects the unit size, this will only be relevant for input assets, so we can add it then. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Ok, that makes sense.
I might be missing something here, but I kind of think it's beholden on the user to do the rounding up here; i.e., I'd expect users to put the absolute minimum capacity required, so I'm not sure it makes sense to give them additional capacity, otherwise they might think they need less than they do (assuming they're doing it a bit by trial and error). Does that make sense? Possibly not 🙃. You obvs have a lot more experience working with users with MUSE1 though, so I may be not grasping what a typical workflow looks like. I don't think this is such a problem if we emit a warning saying that this is what's happening. There is an edge case we might want to consider: when a capacity in
I was just thinking we could just raise a warning in the input layer, if the remainder isn't zero (but see above) or isn't approx equal to the unit size. Unless you think there's a cleaner approach? |
Honestly you might be right. It's a small difference that we can easily change as we go along.
Good idea to do in the input layer. I'll do that. |
Happy with that for now. At least if there's a warning it hopefully won't surprise users.
👍 I just remembered you mentioned about adding a test for an example but with divisible assets... I think that's a good idea before we start mucking around with things too much. Would you like to do that in this PR? I'm also happy to do it as it's similar to what I was going to do for #1062 anyway. |
I'll give it a go |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/asset.rs
Outdated
| // Calculate the number of units corresponding to the asset's capacity | ||
| // Safe because capacity and unit_size are both positive finite numbers |
There was a problem hiding this comment.
The comment 'Safe because capacity and unit_size are both positive finite numbers' on line 863 doesn't fully explain why the cast is safe. Consider clarifying that ceil() returns a finite positive number that fits in usize for realistic asset capacities.
| // Calculate the number of units corresponding to the asset's capacity | |
| // Safe because capacity and unit_size are both positive finite numbers | |
| // Calculate the number of units corresponding to the asset's capacity. | |
| // `capacity` and `unit_size` are positive, finite quantities, so their ratio is a | |
| // positive, finite `f64`. Applying `ceil()` preserves finiteness and positivity, | |
| // yielding a finite positive number of units. The asset model constrains capacities | |
| // and unit sizes to realistic values, so this number of units is expected to be | |
| // well within the range of `usize` on all supported targets, making the cast safe | |
| // under these domain assumptions. |
There was a problem hiding this comment.
@alexdewar do you think u32 would be better here or is usize okay? Not really sure when to use one over the other...
| if asset.is_divisible() { | ||
| let children = asset.divide_asset(); | ||
| for mut child in children { | ||
| for mut child in asset.divide_asset() { |
There was a problem hiding this comment.
The inlined call to divide_asset() may result in cloning the child asset unnecessarily on each iteration due to the Rc wrapping. While this matches the previous behavior, consider whether a more efficient approach is needed if performance becomes a concern.
There was a problem hiding this comment.
@alexdewar Do you have an opinion on this? Not sure I understand the problem
There was a problem hiding this comment.
It took me a while to figure out what Copilot meant here, but I think the problem it's actually talking about is to do with the next lines:
child.make_mut().commission(
AssetID(self.next_id),
Some(AssetGroupID(self.next_group_id)),
"user input",
);
self.next_id += 1;
self.active.push(child);If there is more than one ref to child, then this will result in cloning it (that's what Rc::make_mut does). As the children are all clones, this will happen every time. But this isn't really going to be a big deal, performance-wise.
Anyway, we'll get rid of divide_asset() at some point when we've finished doing all this refactoring.
|
@alexdewar I've added a regression test for a patched version of the simple model with divisible gas boilers. I went for gas boilers because they're both in the input assets and invested in by MUSE in future years, so we get coverage of both scenarios. I purposefully went for a large Some points/complications:
Still to do: input validation/warning |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@alexdewar Ready for another look I think, although see comments above. I've kept the rounding up behaviour for now because, as long as this is used for assets invested in by MUSE, rounding down could break things due to insufficient capacity. I think I agree with you though that rounding down would be better behaviour longer term. |
There was a problem hiding this comment.
Looks good!
@alexdewar I've added a regression test for a patched version of the simple model with divisible gas boilers. I went for gas boilers because they're both in the input assets and invested in by MUSE in future years, so we get coverage of both scenarios. I purposefully went for a large
unit_size(completely arbitrary) so we don't get hundreds of child assets which would take ages to run and clog up the output files, but we can revisit this as we go through.
👍
[snip]
- I've currently had to generate the results files manually because I couldn't think of an easy way to build this into
regenerate_all_data.sh. Do you have any ideas?
Hmm... The only way I can think to do it is to add some kind of helper command to muse2 to run these extra example models. We wouldn't have to expose it to users and could even not compile it in by default. Anyway, seems a bit fiddly and is out of scope for this PR. I'll open an issue for it and we can do it later.
I've kept the rounding up behaviour for now because, as long as this is used for assets invested in by MUSE, rounding down could break things due to insufficient capacity. I think I agree with you though that rounding down would be better behaviour longer term.
Ok, this is good for now.
Description
This change ensures that all child assets created from parent asset division will have equal capacity. The solution for now is just to round up the number of units if the overall capacity is not a multiple of the unit size.
This is relevant in two places:
assets.csvLonger term, I think this will remain as the best solution for point 1 (it seems overly strict to mandate that capacities must be multiples of the unit size), but for point 2 I imagine we'll end up with a solution whereby this doesn't apply (i.e. only ever considering assets that are multiples of the unit size, or commissioning unit by unit).
In any case, with this now in place, we can consider refactoring changes to fix performance without changing the behaviour (at least in terms of pre-defined assets).
Side note - I think it would be good to have a regression test for a variant of the simple model that makes use of this feature - at least while we develop it. Should we add that in this PR, or another? We were going to do something similar for NPV anyway so once that's in place this will be very similar.
Fixes #1041
Type of change
Key checklist
$ cargo test$ cargo docFurther checks