Closed joroeder closed 3 years ago
Thanks for your work! Looks quite impressive. A lot of things seem to have happened since my last visit and everything is much more cleaned up.
Installation I started my review by installing this branch on a fresh environment. No problems at that point. I guess there is a minimum version of oemof.solph (probably 0.4.1). If so, it should be set in setup.py.
Examples Then I tried the examples. For some examples, code and data are contained in their own folder (introduction_example, investment_minimal_example), for the rest that is not the case. If this would be tidied up it would help to gain a quick overview.
Another suggestion (just a small thing): To make names shorter, drop the "_example". As we are in the example folder, it should be clear already.
/investment_input
seems to be obsolete. Instead, /investment_input_2
is used.operation.py
does not do much yet. I would expect to see some results of a heat dispatch.
It takes some time to understand which example does what. Would be great if that was more transparent.
To conclude, the folder would look like this
.
| ---- introduction/
| ---- invest_data
| ---- tnw_data
| ---- introduction.py
| ---- investment/
| ...
| ---- operation/
| ...
Details on introduction_example.py
network.results.optimization.edges
Docs
to be continued
Code
I will address specific feedback to the code to the respective line. Here some general remarks:
Thank you for your review so far! I will follow your issues and adapt the code accordingly.
One general thing, I was wondering, if there should be some major change in the structure? Because if so, it would be double work adding docstrings everywhere, and in the next step adapt them again. But if it is fine so far (firstly, it is working quite well ;) , and secondly, some improvements of implementation can be done within the next releases), I would continue adding docstrings. What do you think?
With pleasure! I am only half-way through and am willing to dedicate some time for another review session. Naturally, the more superficial things pop up first and I wanted to write them down. With some more time, I could give some feedback that goes a little deeper. So you are right, it makes sense to wait with the docstrings.
I started my review by installing this branch on a fresh environment. No problems at that point. I guess there is a minimum version of oemof.solph (probably 0.4.1). If so, it should be set in setup.py.
Yes, I think v0.4.0 is fine.
| ---- introduction/ | ---- invest_data | ---- tnw_data | ---- introduction.py | ---- investment/ | ... | ---- operation/ | ...
I agree. There is no operational
at the moment - so, take it out?
network.results.optimization.edges
* contains data on the length, which is not a result, but part of the input data.
Yes, the edges table is extended by the results. In the end, I always appended the results table to the given edges data table to do analysis whatever in qgis ... so it was more convenient to have it directly together.
* what is the difference btw. existing and active and invest_status?
active
: only active pipes are considered. If active==0
, the line element is not considered at all. hopefully this becomes clear from rtdexisting
: this line is not free for optimisation. so a type of heatpipe element must be specifiied and a capacity must be given. And this type must be present in the invest_data -> network -> pipes.csv
rtdinvest_status
: okay, yes, this is not clear ;-) invest_status
is 1, only for nonconvex
pipes (so either there is an existing nonconvex
pipe, or there was an investment in an nonconvex
pipe). I introduced the parameter to correctly re-calculate the costs and losses in this investment case. The costs and losses are re-calculated for the existing and optimized pipes (in order to have a quick access e.g. to the total heat loss.)* What are these variables? pipe-typ-A.dir-1 pipe-typ-A.size-1 pipe-typ-A.dir-2 pipe-typ-A.size-2 pipe-typ-A.size
pipe-typ-A
-> label of your pipe type from the pipes.csv
dir
-> direction of the oemof-solph heatpipeline Transformer
size
-> the results of the oemof.solph investment
Generally, you have the option to choose between bi-directional or uni-direction pipes. (see also general settings: link to rtd) Since the flow direction is not known before the optimisation, the model builder creates two pipes in each direction in the case of the setting bidirectional_pipes=False
). And all these results are given in the results edges.
With pleasure! I am only half-way through and am willing to dedicate some time for another review session. Naturally, the more superficial things pop up first and I wanted to write them down. With some more time, I could give some feedback that goes a little deeper. So you are right, it makes sense to wait with the docstrings.
Take as much time as you need, (or as you have) ;-)
Thanks for your explanations!
I agree. There is no
operational
at the moment - so, take it out?
Yes.
network.results.optimization.edges
* contains data on the length, which is not a result, but part of the input data.
Yes, the edges table is extended by the results. In the end, I always appended the results table to the given edges data table to do analysis whatever in qgis ... so it was more convenient to have it directly together.
Your data is explicitly called 'results' and I would strongly suggest to keep the distinction between input data and results very clear. Clearly for analysis and plotting, you often want to combine both. This is still possible in a second step without a lot of effort as the data structure is the same.
* what is the difference btw. existing and active and invest_status?
* `active`: only active pipes are considered. If `active==0`, the line element is not considered at all. hopefully this becomes clear from [rtd](https://dhnx.readthedocs.io/en/features-integrate_solph_model_builder_merge/optimization_models.html#edges)
I see. I have a suggestion for making it simpler: Instead of setting active==0, you could as well just remove the edge from the network. If you want to include it again later, you can add it again. You could save one variable and gain more clarity. What do you think?
* `existing`: this line is not free for optimisation. so a type of heatpipe element must be specifiied and a capacity must be given. And this type must be present in the `invest_data -> network -> pipes.csv` [rtd](https://dhnx.readthedocs.io/en/features-integrate_solph_model_builder_merge/optimization_models.html#edges)
Ah, ok. I understand how this naming came about. So these pipes exist and they are fixed. It is not really self-explanatory in my view. And there might be situations where there is an existing pipe where you have the option to replace it. For a clearer naming I would suggest to describe the pipes with these three variables:
(The first two would stay as they are. 'investable' this is just the opposite of 'existing', which means that your code probably would not have to be changed much. existing=True -> investable=False and vice versa).
* `invest_status`: okay, yes, this is not clear ;-) `invest_status` is 1, only for `nonconvex` pipes (so either there is an existing `nonconvex` pipe, or there was an investment in an `nonconvex` pipe). I introduced the parameter to correctly re-calculate the costs and losses in this investment case. The costs and losses are re-calculated for the existing and optimized pipes (in order to have a quick access e.g. to the total heat loss.)
I am not sure if I completely understood this. So invest_status=1 for nonconvex pipes, else 0? Doesn't that depend on the type of the pipe? In the invest_data, this is already set. So your method could get it from there I guess?
* What are these variables? pipe-typ-A.dir-1 pipe-typ-A.size-1 pipe-typ-A.dir-2 pipe-typ-A.size-2 pipe-typ-A.size
pipe-typ-A
-> label of your pipe type from thepipes.csv
dir
-> direction of the oemof-solph heatpipeline Transformersize
-> the results of the oemof.solph investmentGenerally, you have the option to choose between bi-directional or uni-direction pipes. (see also general settings: link to rtd) Since the flow direction is not known before the optimisation, the model builder creates two pipes in each direction in the case of the setting
bidirectional_pipes=False
). And all these results are given in the results edges.
So you can give the solver the option to invest in both directions separately. What happens if the solver invests in both directions? That's not realistic I guess? Or is it just that this does not happen in most cases? Or are you ok with it to happen?
Concerning the directions: In the definition of the edges, a direction is already implicit: That's why in the edges dataframe it is called 'from_node' and 'to_node'. So if you can discriminate the two directions by 'forward' and 'backward' or 1 and -1. So you do not need two more columns to define the directions (dir-1 and dir-2).
Here, I want to provide a short overview of what I have changed and considered from the review so far:
active
attribute (check if present, and delete all rows with active ==0)f_invest
and annuity
attribute for precalculation purpose (I think it is more clear at the moment. Everybody could use the absolute or the annuized costs, if he like to do so.)invest_status
- this was not clear at allexisting
vs investable
: I kept the existing
and so far did not introduce investable
additionally. So, all pipes are an option for investment, unless existing==1
. If so, there must be an capacity
and the type of the pipe specified hp_type
. I think this is clear, however, the option of replacing an existing pipe is not given so far.About the dictionary settgs
: Why not rather pass keyword arguments and set default values in the function? As a sketch:
in network.py, you could pass keyword arguments
def optimize_investment(self, invest_options, **kwargs):
oemof_opti_model = setup_optimise_investment(
self, invest_options, **kwargs
)
in optimization.py, you could set the default arguments
setup_optimise_investment(thermal_network, invest_options, heat_demand='scalar', etc.)
With this approach, there would be an error if you pass a setting that does not exist.
In the examples, there seems to be an inconsistent naming, or am I wrong? Maybe it is not too severe, but consistency always helps
When I checkout the branch locally and try to run the tests, I get:
from oemof.solph import helpers
E ImportError: cannot import name 'helpers'
It seems that I have the right oemof.solph v0.4.1 installed though.
Update: When installing DHNx into a fresh environment, it works. Update 2 I ran the examples successfully.
This PR takes the most important features of #29 and prepares the code for merging.