Enum for asset capacity #1079
Enum for asset capacity #1079
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## divisible_dispatch_flexible_capacity #1079 +/- ##
========================================================================
+ Coverage 82.03% 82.14% +0.11%
========================================================================
Files 53 53
Lines 7364 7444 +80
Branches 7364 7444 +80
========================================================================
+ Hits 6041 6115 +74
- Misses 1033 1042 +9
+ Partials 290 287 -3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR introduces an AssetCapacity enum to represent asset capacities, which can be either continuous (for non-divisible assets) or discrete (for divisible assets with a unit size). This refactoring replaces the previous approach where capacity was always represented as a simple Capacity value, with divisibility determined by checking the process's unit size.
Changes:
- Introduced
AssetCapacityenum withContinuousandDiscretevariants, including arithmetic operations (Add, Sub) and helper methods (min, max, apply_limit_factor, etc.) - Updated all asset construction methods to convert
CapacitytoAssetCapacityusingfrom_capacity()with optional unit size - Modified
asset.capacity()to return&AssetCapacityinstead ofCapacity, requiring call sites to use.total_capacity()when needed
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/asset.rs | Defines AssetCapacity enum with arithmetic operations and conversion methods; updates Asset struct and methods to use AssetCapacity |
| src/simulation/optimisation/constraints.rs | Updates constraint logic to pattern match on AssetCapacity::Discrete instead of checking unit_size() |
| src/simulation/optimisation.rs | Updates capacity variable handling to construct AssetCapacity from optimization results; modifies iter_capacity() return type |
| src/simulation/investment/appraisal/optimisation.rs | Changes ResultsMap.capacity to AssetCapacity and updates optimization result conversion |
| src/simulation/investment/appraisal/constraints.rs | Simplifies capacity constraint logic using AssetCapacity pattern matching |
| src/simulation/investment/appraisal.rs | Updates function signatures and uses .total_capacity() when passing to financial calculations |
| src/simulation/investment.rs | Updates investment logic to use AssetCapacity operations and .total_capacity() for comparisons |
| src/simulation.rs | Renames new_candidate() to new_candidate_for_dispatch() for clarity |
| src/output.rs | Adds .total_capacity() calls when writing capacity values to output |
| src/fixture.rs | Updates test fixture to use AssetCapacity::Continuous |
| AGENTS.md | Documents preference for named format arguments over positional formatting |
💡 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 seems sensible. LGTM!
There are a few places where you can get away without taking a reference to an AssetCapacity. I've also mentioned some more traits you could implement for it, but obvs only do that if it seems useful.
And yes, asset.rs is getting long! Feel free to break it up. I think AssetPool probably deserves its own file, at least.
| let num_units = (capacity / size).value().ceil() as u32; | ||
| AssetCapacity::Discrete(num_units, size) | ||
| } | ||
| None => AssetCapacity::Continuous(capacity), |
There was a problem hiding this comment.
You could also consider deriving From<Capacity> so you could write my_capacity.into() rather than AssetCapacity::from_capacity(my_capacity)
| process: Rc<Process>, | ||
| region_id: RegionID, | ||
| capacity: Capacity, | ||
| capacity: AssetCapacity, |
There was a problem hiding this comment.
Is there a reason why you can't just leave capacity as a Capacity and then create an AssetCapacity using process.unit_size here? I'm guessing there are cases where you wouldn't want that?
There was a problem hiding this comment.
I had it like that originally but then realised we need candidate assets to deviate from unit_size (i.e. a small capacity set by candidate_asset_capacity)
Description
This PR changes how we represent asset capacities to an Enum
AssetCapacity, which can either be a singleCapacityvalue in the case of non-divisible assets (i.e. those without a unit size), or a unit size and count for divisible assets.This now gives us an ability to express multi-unit assets as a single object. However, these multi-unit objects won't yet exist outside of the investment part of the code, because we break them up into individual units as we commission them. Long run the plan is to no longer do this, but we need to figure out a few things first (#1043, #1045, #1042)
With
AssetCapacitycomes a load of tools for adding/subtracting/converting capacities etc., with checks to make sure you're not doing anything silly like adding a continuous capacity to a discrete capacity, for example.I've allowed candidate assets in the dispatch (i.e the "mock" assets with tiny capacity) to have non-unit sized capacities - these will all be
AssetCapacity::Continuous(candidate_asset_capacity)regardless of whether the underlying process is divisible. I think this is the way to go, but it doesn't seem to make a difference.Overall, I think this is cleaner and safer and forces us to be explicit with the way we treat asset capacities
PS: The
asset.rsmodule is getting really large now - do you think it's worth breaking this up?Fixes # (issue)
Type of change
Key checklist
$ cargo test$ cargo docpresent in the previous release
Further checks