Make varying period lenghts possible #957
Conversation
This matrix represent the temporal distance between each period point to each other. It is necessary to determine the endogenous decommissioning with the multi-period approach.
This is necessary to fix a bug when using multi-period with varying period lengths. Now various period lengths are possible and even multiple investment periods can decomission in one period.
This changes are similiar to _old_capacity_rule_end but indexes for constraints are different. This constraint takes care of the storage capacity (MWh).
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
...s/test_scripts/test_solph/test_multi_period_model/test_multi_period_varying_period_length.py
Fixed
Show resolved
Hide resolved
There was a problem hiding this comment.
Thank you, @nailend! This looks sound to me. Congrats to coming up with a well-thought thorugh, though quite complicated fix for that. I guess the reason for having a rather complicated fix is that the issue itself brings that complication, right? ;-)
I suggested some minor code modifications, but would not alter the essence of it. Also, I think, we need to remove all that inline commenting and document it somewhere central. I guess, I'd rather prefer having a dedicated section on that in the sphinx documentation, compared to blowing up the docstrings for that constraint formulation, but we can argue about that.
You're correct about that. It is basically the same as for any investment flow. We might address these code "duplications" at some point in the future, but that's another issue. Also you might look into the open black issues. |
Concerning that, my preference would be to have a dedicated subsection under the multi-period mode in the |
This test is not necessary anymore as there is a constraint test covering the same.
Further components are not necessary to create the LP-file.
Investment blocks are implemented in this componenents separately. To cover all code changes, these components need to be added as well.
|
@jokochems Thanks a lot for your review. I took care of the modifications you suggested and a few other things like the constraint test. I tried to reduce the inline comments, but as you said yourself, it is quite complex, even if you are the person who knows best about it. To make the code understandable and open to changes by developers, I would prefer to keep them. Whereas, I don't think this level of detail is necessary for the user, so I just added a few words to the Would you like to double-check the LP-file again, or do you think it's ready to merge? (To check the constraints ending with Thanks again for your support. |
There was a problem hiding this comment.
I did not work through each and every detail again. It's pretty much the same approach for each component that is eligible for investments, right?
I suggest to alter the wording to "bookkeeping" instead of "memory".
In terms of the inline comments, I'd be fine with it, but I'd like to hear @p-snft's take on that and would like to leave a final review and approval up to him.
There was a problem hiding this comment.
I had another look into the decailts: To me it looks all good and I understand the logics as well as the analogies (you might say duplications) that are there.
The minor remarks on the wording hold for all code occurences, i.e. _generic_storage.py, _sink_dsm.py as well as _investment_flow_block.py.
One thing that was a bit counter-intuitive is that in the constraint test, the setup method gets somewhat "overwritten" as an energy system is set up twice (you'll notice when debugging and putting a break point into the new _extract_periods_matrix method). But I'd say, that's also okay, because this is an experts user area anyways and the test does what it is supposed to do and succeeds.
|
Thanks a lot!
Yes, this is done by purpose as the setup method only has three periods defined and there were more needed to check the various decommissioning cases. |
p-snft
force-pushed
the
features/make-varying-period-lenghts-possible
branch
from
5acd141
to
889a81a
Compare
There was a problem hiding this comment.
This is definitely an improvement. It might not be optimal, but as we still have the disclaimer for multi-period optimisation
CAUTION! (…) Please be aware that the feature is experimental as of now.
I am very okay with that. For the future, we should talk about refactoring the tests, so that understanding the tests is easier. (I did not go through the lengthy LP files.) But that's a general issue with solph and not specific to the current PR.
Changes
This feature fixes #955 and introduces the capability to utilize multi-periods with varying lengths. To achieve this, the
_old_capacity_rule_endin theinvestment_flow_blockand_old_storage_capacity_rule_endofgeneric_storagewere reworked. It seems that the same approach needs to be applied toSinkDSMas well.Challenges
The methodology behind this implementation can be quite complex to grasp. I have attempted to provide as many comments as possible to aid understanding. The major challenges encountered were as follows:
Description
The method is based on an upper-right matrix that describes the temporal distance between each of the periods. Let's consider an example with the following periods:
The period matrix appears as follows:
In this matrix, the values represent the age of a component based on the period it was invested in (rows) and decommissioned (columns). The value at the top-right corner indicates that a component would be 95 years old if it was invested in period 0 and decommissioned in period 7.
To match investment periods with decommissioning periods, we need to select, for each row, the smallest value that is greater than or equal to 20 (the lifetime). We then obtain the respective indexes, where the row index corresponds to the investment period and the column index corresponds to the decommissioning period.
Additionally, it is necessary to address periods without decommissioning (depicted in blue boxes) and periods where multiple investment periods are decommissioned (depicted in green boxes).
Remarks
It's probably possible to do this without this complicated for-loop but I did't find a solution to group duplicated investment periods to chain and add them to the constraint.
Help
I guess the LP-file test should be sufficient. Please double-check the logic for
c_e_GenericInvestmentStorageBlock_old_rule_endandc_e_InvestmentFlowBlock_old_rule_end.Thanks for your help
TODO:
investment_flow_blockgeneric_storageSinkDSMSinkDSMto LP-file test