Improvement of PeriodDemandSink#12
Conversation
Zetison
left a comment
Zetison
left a comment
There was a problem hiding this comment.
I suggest to bump the version to 0.4.0 already in this PR to showcase that the breaking change is present here (and rather instead skip it in the next PR).
Ideally, this issue should be resolved before a merge of this PR. This would also enable simpler rereview of this PR.
The change from period_length to period_duration should be more consistently updated in docstrings and comments.
In general, I strongly suggest to use Copilot review to do the coarse review (it picks up on most of these issues). Note that this can be done locally in VS Code.
Co-authored-by: Jon Vegard Venås <jon.venas@ife.no>
JulStraus
left a comment
JulStraus
left a comment
There was a problem hiding this comment.
I included all comments with the following exception:
- Based on the approaches used so far in
EMX, I do not bump the version number directly to a breaking change. This can lead to an error with respect to a registration before everything is included in main. later bugfixes for version 0.3. will anyhow checkout from the tag.
I added in addition a legacy constructor for the original approach.
Zetison
left a comment
Zetison
left a comment
There was a problem hiding this comment.
It looks good enough to be merged.
2 participants
The existing version of
PeriodDemandSinkwas 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 thePeriodDemandSinkto the new format.Important
This PR is breaking as we change the fields of
PeriodDemandSink. I still decided not to increase the version number yet as these updates will also be included forCapacityCostLink. In addition, a new subtyoe ofAbstractPeriodDemandSinkwill be included.