Improvement of CapacityCostLink#13
Conversation
* Constraints implementation adjusted * Test adjusted based on requirements * Checks not yet adjusted * Documentation not yet adjusted
tillho
left a comment
tillho
left a comment
There was a problem hiding this comment.
Looks good to me!
I feel like the update-models section is the part most people will read when getting an error after updating. So I tried to add some context that makes it easier to understand what has changed.
| # old behavior, corresponding to 2 periods | ||
| cap_period_duration = 2 | ||
|
|
||
| # new behavior, corresponding to periods whocse duration sums to at least 4 |
There was a problem hiding this comment.
whocse -> whose
I think it might make more sense to change the numbers so they describe the same thing?
before = 2, after = 10
or
before = 5, after = 4
and add a sentence like:
"Both cases create two periods with a duration of 10, but instead of specifying the number of periods, we now define the duration of each period"
There was a problem hiding this comment.
Yes, I agree. It makes significantly more sense to have both examples the same.
|
|
||
| ```julia | ||
| # time structure | ||
| ts = Twolevel(2, 1, SimpleTimes(10, 2); op_per_strat=8760.0) |
| # old behavior, corresponding to 5 periods a 1752 duration based on `op_per_strat` | ||
| cap_period_duration = [1752, 1752, 1752, 1752, 1752] | ||
|
|
||
| # new behavior, corresponding to 5 periods a 4 duration based on `SimpleTimes` |
There was a problem hiding this comment.
Might be nice to add a comment like this, since the jump from 1752 to 4 looks arbitrary at first glance:
"# With the scaling factor 8760 / (10 * 2) = 438, each new value of 4 corresponds to the same 1752 as before (4 * 438 = 1752), i.e. the same 5 chunks of 2 time steps each."
There was a problem hiding this comment.
Included although I slightly rephrased it.
JulStraus
left a comment
JulStraus
left a comment
There was a problem hiding this comment.
I implemented all changes and will merge once the CI passed.
| # old behavior, corresponding to 2 periods | ||
| cap_period_duration = 2 | ||
|
|
||
| # new behavior, corresponding to periods whocse duration sums to at least 4 |
There was a problem hiding this comment.
Yes, I agree. It makes significantly more sense to have both examples the same.
|
|
||
| ```julia | ||
| # time structure | ||
| ts = Twolevel(2, 1, SimpleTimes(10, 2); op_per_strat=8760.0) |
| # old behavior, corresponding to 5 periods a 1752 duration based on `op_per_strat` | ||
| cap_period_duration = [1752, 1752, 1752, 1752, 1752] | ||
|
|
||
| # new behavior, corresponding to 5 periods a 4 duration based on `SimpleTimes` |
There was a problem hiding this comment.
Included although I slightly rephrased it.
2 participants
The existing version of
CapacityCostLinkwas not really flexible with respect to the chosen time structure and potentially error prone with respect to the chosen durations. As a consequence, its application required carefulness regarding the parameter choices.TimeStructv0.9.12 introduced partitions of periods which allowed to simplify the overall structure. This PR adapts now theCapacityCostLinkto the new format.Important
This PR is breaking as we change the meaning of the fields of
CapacityCostLink. I still decided not to increase the version number yet as a new subtype ofAbstractPeriodDemandSinkwill be included for the next major release.