Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1018 +/- ##
==========================================
+ Coverage 81.07% 81.53% +0.46%
==========================================
Files 52 52
Lines 6331 6484 +153
Branches 6331 6484 +153
==========================================
+ Hits 5133 5287 +154
- Misses 942 944 +2
+ Partials 256 253 -3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
dalonsoa
left a comment
There was a problem hiding this comment.
I think this makes sense and is OK. There are some aspects that are not fully clear, but I think I grasp the gist of it.
src/input/process/availability.rs
Outdated
| left.parse::<f64>() | ||
| .ok() | ||
| .with_context(|| format!("Invalid lower availability limit: {left}"))? | ||
| .into() | ||
| }; |
There was a problem hiding this comment.
As far as I can tell, this returns a normal float, not a Dimensionless object, as we are expecting. Is rust happy with this?
There was a problem hiding this comment.
It's because the .into() will convert it to the required type (if possible, which in this case it is), but I can rewrite this to make it more explicit
src/process.rs
Outdated
| // Ensure that the new limit is compatible with the current limit | ||
| ensure!( | ||
| *limit.start() <= *current_limit.end() && *limit.end() >= *current_limit.start(), | ||
| "Availability limit for season {season} clashes with time slice limits", | ||
| ); |
There was a problem hiding this comment.
It took me a while to get this. In other words, there must be overlap between the old and new ranges for the new one to be valid, right?
There was a problem hiding this comment.
Yes exactly, otherwise it's impossible to satisfy the seasonal limit within the timeslice-level constraints
Unfortunately I don't think there's a way to make the code clearer (no inbuilt function to do this comparison), but I'll add a comment to clarify
| let mut keys = Vec::new(); | ||
| let capacity_vars: IndexMap<&AssetRef, highs::Col> = variables.iter_capacity_vars().collect(); | ||
| for (asset, time_slice, activity_var) in variables.iter_activity_vars() { | ||
|
|
There was a problem hiding this comment.
I'm not entirely sure if the loops in these two cases add the same number of elements to the problem or not. I think the new way is more readable, first going through all the assets and adding capacity or activity constrains, in each case iterating through the timeslices and limits within. But I don't fully understand why the same terms to the problem could not be added following the previous approach. Could you elaborate?
There was a problem hiding this comment.
Previously, we were iterating over the activity variables (activity of asset X in timeslice Y) and adding one constraint per activity variable (or two for flexible capacity assets).
Now, we still have one/two constraints for each activity variable representing timeslice-level availability (we need these even if no availability constraints are defined, to make sure activity respects the timeslice length), but we may also have seasonal and annual constraints. These will involve multiple activity variables, so we can't simply iterate over the activity variables one by one like before
| let mut keys = Vec::new(); | ||
| let capacity_vars: IndexMap<&AssetRef, highs::Col> = variables.iter_capacity_vars().collect(); | ||
| for (asset, time_slice, activity_var) in variables.iter_activity_vars() { | ||
|
|
There was a problem hiding this comment.
Previously, we were iterating over the activity variables (activity of asset X in timeslice Y) and adding one constraint per activity variable (or two for flexible capacity assets).
Now, we still have one/two constraints for each activity variable representing timeslice-level availability (we need these even if no availability constraints are defined, to make sure activity respects the timeslice length), but we may also have seasonal and annual constraints. These will involve multiple activity variables, so we can't simply iterate over the activity variables one by one like before
src/process.rs
Outdated
| // Ensure that the new limit is compatible with the current limit | ||
| ensure!( | ||
| *limit.start() <= *current_limit.end() && *limit.end() >= *current_limit.start(), | ||
| "Availability limit for season {season} clashes with time slice limits", | ||
| ); |
There was a problem hiding this comment.
Yes exactly, otherwise it's impossible to satisfy the seasonal limit within the timeslice-level constraints
Unfortunately I don't think there's a way to make the code clearer (no inbuilt function to do this comparison), but I'll add a comment to clarify
src/input/process/availability.rs
Outdated
| left.parse::<f64>() | ||
| .ok() | ||
| .with_context(|| format!("Invalid lower availability limit: {left}"))? | ||
| .into() | ||
| }; |
There was a problem hiding this comment.
It's because the .into() will convert it to the required type (if possible, which in this case it is), but I can rewrite this to make it more explicit
Description
This is based on #971, however there were enough problems with this that it was easier to write something from scratch (some snippets/ideas have been directly copied from that, however - I certainly don't want to pass this off as entirely my work!)
The main goal of this PR is to allow availability constraints to be added at different levels of time-granularity. We already allow users to provide availability constraints for e.g. "winter.night" or "winter" as a whole, but our approach for the latter is incorrect - we currently apply the constraint to all individual timeslices in winter, whereas actually we should be constraining availability across winter as a whole (i.e. availability can exceed the limit in any individual time slices, as long as the limit is respected across winter as a whole).
Most of the work is done by the new
ActivityLimitsstruct. This stores the limits, does some validation, flags incompatible limits, removes redundant limits, and allows limits to be retrieved (either for a specific timeslice selection, or an iterator of limits that covers the whole year). Limits are first read in from the input data, then we create anActivityLimitsfor every process/region/year combination.I've then reworked the optimisation constraints to apply to the sum of all the activity variables in the selection. For timeslice-level constraints, this is still a single activity variable, but for seasonal/annual this is potentially many. Unsurprisingly, this changes all of the results, as all models have some processes with annual constraints - previously this was applied individually to every time slice in the year, whereas now it applies to the sum of all time slices.
A second goal is to allow users to provide both lower and upper limits on availability - we currently allow either but not both. Alex had the idea of using range syntax in the input files (similar to what we already do for years), so I had a go at implementing this as well. It was easier to do this in this PR because it makes the validation a bit easier. I've changed all the examples to follow this format, and updated the documentation. We also no longer mandate that all processes have explicitly defined availability limits, so I've deleted a few 0..1 annual limits which aren't needed.
I'm still a bit unsure about some of the validation. The rule I've gone for is that if any timeslices/seasons are provided for a particular process/region/year, then ALL timeslices/seasons must be provided for that particular process/region/year. This ended up being the easiest to implement, and I think makes some logical sense (catches the case where the user accidentally omits a time slice). I don't currently have any constraints on which years/regions are provided (i.e. if a user provides data for a process in one particular region/year, then they don't necessarily need to provide data for all regions/years that the process is active - if not provided it will assume there's no limit beyond the time slice length). I think ideally this would be more strict, so the user has to provide all milestone years and commission years for defined assets, but unfortunately I couldn't find a clean/easy way to do this within the framework I've written here.
Finally, I've also reworked some of the tests a bit by adding more fixtures which makes it a little bit easier to construct processes with unique properties.
Fixes #957
Fixes #958
Fixes #743
Fixes #363
Type of change
Key checklist
$ cargo test$ cargo docFurther checks