Enable total capacity constraints#1417
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1417 +/- ##
==========================================
- Coverage 89.44% 86.84% -2.60%
==========================================
Files 59 59
Lines 8364 8424 +60
Branches 8364 8424 +60
==========================================
- Hits 7481 7316 -165
- Misses 562 791 +229
+ Partials 321 317 -4 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| /// limits and total capacity limits, this will have more complex logic which will depend on the | ||
| /// current total capacity. | ||
| pub fn get_addition_limit(&self) -> Option<Capacity> { | ||
| #[allow(unused_variables)] |
There was a problem hiding this comment.
The normal way would be to prefix the argument name with _. That said, contrary to what I originally said, I don't think we should pass a capacity here (see discussion above)
tsmbland
left a comment
tsmbland
left a comment
There was a problem hiding this comment.
Good start.
I've realised that my original plan wasn't as solid as I thought, so the actual implementation will be quite different. The input/validation code is nearly there though so I'd focus on tidying that up in this PR. If you put a note in the schema about the new parameter, mentioning that it's currently unused, then we could merge this and worry about the implementation later
| #[serde( | ||
| default, | ||
| deserialize_with = "ProcessInvestmentConstraintRaw::parse_empty_limit" | ||
| )] | ||
| total_capacity_limit: Option<Capacity>, |
There was a problem hiding this comment.
| #[serde( | |
| default, | |
| deserialize_with = "ProcessInvestmentConstraintRaw::parse_empty_limit" | |
| )] | |
| total_capacity_limit: Option<Capacity>, | |
| total_capacity_limit: Option<Capacity>, |
The Option<Capacity> does everything you need
| ); | ||
|
|
||
| // Validate total_capacity_limit: must be in range [0, inf) | ||
| if let Some(limit) = self.total_capacity_limit { |
There was a problem hiding this comment.
This looks good. Long run, I think we'd want to make addition_limit optional as well. We should allow either total_capacity_limit or addition_limit to be empty, but not both (if both were empty, the user should just omit the row for that process/region/year)
| } | ||
|
|
||
| #[test] | ||
| fn validate_total_capacity_limit() { |
There was a problem hiding this comment.
Realise you've copied the format of the above test so this is absolutely fine. If you wanted to improve this, look into using case from rstest (used throughout the code).
I might two functions testing both addition and total capacity limits: validate_constraint_valid and validate_constraint_invalid.
There was a problem hiding this comment.
If you wanted to improve this, look into using case from rstest (used throughout the code).
Cool, I'd assumed (hoped) there would be a way to do this haha
| .get(&(self.region_id.clone(), self.commission_year)) | ||
| .and_then(|c| c.get_addition_limit().map(|l| l * commodity_portion)) | ||
| .and_then(|c| { | ||
| c.get_addition_limit(self.capacity()) |
There was a problem hiding this comment.
The capacity here isn't what we need in this context. This is the capacity of an individual candidate asset, which is the amount of capacity that we consider installing at a time (we may install multiple of these).
What we'd need to know is the total amount of capacity already installed, so we know how much wiggle room we have above this to install in the current year.
That said, this is more complicated than I originally thought. Because we decide how much of the existing capacity to keep (and how much to get rid of) as part of the investment process, which hasn't happened yet, we have a bit of a chicken and egg situation as we don't know how much capacity is installed (or, rather, how much of this will actually be kept).
Hope is not all lost, but this will require a different approach.
If you look at what happens currently with the addition limits, we store these in a map keyed by asset, and gradually trim from this as we install new capacity (i.e. coming from candidate assets). One way to implement the total capacity limit would be to have another map, keyed by process instead of asset, then gradually trim from this with all selected assets (both new assets and those chosen to be retained).
If this doesn't make any sense, or you don't want to worry about this, you can just focus this PR on the inputs/validation, but I'd remove the self.capacity() arg in any case as it's not right.
| /// limits and total capacity limits, this will have more complex logic which will depend on the | ||
| /// current total capacity. | ||
| pub fn get_addition_limit(&self) -> Option<Capacity> { | ||
| #[allow(unused_variables)] |
There was a problem hiding this comment.
The normal way would be to prefix the argument name with _. That said, contrary to what I originally said, I don't think we should pass a capacity here (see discussion above)
2 participants
Description
This PR enables the option to provide total capacity constraints.
total_capacity_limitcolumntotal_capacity_limitentries and treats them as None.total_capacity_limitfield of theProcessInvestmentConstraintRawstruct.Noneare non-negative and finite.ProcessInvestmentConstraintnow has atotal_capacity_limitfield, for use inget_addition_limit(not yet implemented)get_addition_limitnow also takes anAsset's current capacity for use in the calculationexamples/muse1_default/process_investment_constraints.csvFixes #1414
Type of change
Key checklist
$ cargo test$ cargo docpresent in the previous release
Further checks