Add commodity constraints input and validation - #1487
Conversation
|
@tsmbland I've taken a stab at this but some feedback would be helpful before I add tests |
tsmbland
left a comment
tsmbland
left a comment
There was a problem hiding this comment.
Good start, but a few comments
- Cleaner parsing of region_id - Change units of limits from money to flow - Store time slice selection and balance type in `CommodityConstraint` struct - Store vectors of `CommodityConstraint`s in `CommodityConstraintMap`
c872f0a to
273b323
Compare
273b323 to
328243d
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1487 +/- ##
==========================================
+ Coverage 90.31% 90.35% +0.03%
==========================================
Files 60 61 +1
Lines 8663 8850 +187
Branches 8663 8850 +187
==========================================
+ Hits 7824 7996 +172
- Misses 525 529 +4
- Partials 314 325 +11 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
be131f2 to
03f77ef
Compare
tsmbland
left a comment
tsmbland
left a comment
There was a problem hiding this comment.
Looks very good on the whole, just a few suggestions
| let time_slice1 = TimeSliceID { | ||
| season: "summer".into(), | ||
| time_of_day: "all-day".into(), | ||
| }; | ||
| let time_slice2 = TimeSliceID { | ||
| season: "summer".into(), | ||
| time_of_day: "all-day".into(), | ||
| }; |
There was a problem hiding this comment.
Good spot, one should have been "winter" - perhaps unnecessary though?
| season: "summer".into(), | ||
| time_of_day: "all-day".into(), | ||
| }; | ||
| let time_slice_info = TimeSliceInfo { |
There was a problem hiding this comment.
You could consider using the time_slice_info fixture instead of building one from scratch
There was a problem hiding this comment.
Ah, that would have been handy haha
There was a problem hiding this comment.
I guess it doesn't matter here, but technically this TimeSliceInfo isn't valid because:
- season lengths should sum to 1
- time slice lengths should sum to 1 (and be consistent with season lengths)
seasonsandtimes_of_dayoptions should match those in thetime_slicesentries ("all day" vs "day"/"night")
There was a problem hiding this comment.
I seeee, yeah I guessed "all-day" was day+night, didn't realise there was also morning, evening, peak etc. Had no idea what any of this time-slicing was to be honest.
There was a problem hiding this comment.
So something like this: 3363674?
Commodities specify seasonal time slice levels, constraints are over whole seasons (two of them). Time slices are defined for those two seasons, with both day and night specified as the time of day for each. Then time_slice_info contains the two seasons (1/2 year each), and the four time slices themselves (1/4 year each).
tsmbland
left a comment
tsmbland
left a comment
There was a problem hiding this comment.
Looks good!
Still a small error in the tests. As I don't think it affects the validity of the tests I'll approve, but probably worth fixing up for the sake of correctness!
2 participants
Description
This PR adds a new module for reading and validating an input csv file for commodity constraints.
Commodity constraints are stored in a new
CommodityConstraintstruct, and the set of constraints provided are read and stored in aCommodityConstraintsMaptype, which mapsCommodityConstraints by region ID and year.CommodityConstraints have a single field,limits, which stores the range of values to which the commodity is constrained.An example commodity_constraints.csv file has been added to the "simple" setup for demonstration, and a corresponding schema has been added.
Fixes #1448
Type of change
Key checklist
$ cargo test$ cargo docpresent in the previous release
Further checks