Refine time index of Flows #1077
Conversation
An index entry should not be needed for the flow. In preparation for time-series aggegration, a cleanup might make sense, to keep overall complexity managable.
For the refactoring, it should be convenient to allow both. TIMEINDEX can be disused later.
This functionality had no unit test, thus the constreaint test was not fixed in the last commit.
|
Maybe, we should split this into several PRs and merge this as a first step. |
| @@ -743,17 +743,11 @@ def _positive_gradient_flow_constraint(_): | |||
| m.flow[ | |||
| i, | |||
| o, | |||
| m.TIMEINDEX[index][0], | |||
| m.TIMEINDEX[index][1], | |||
| m.TIMESTEPS[index], | |||
There was a problem hiding this comment.
Kind of not directly related, but why does line 740 iterate over a numeric index instead of t as is done everywhere else? I guess there is a good reason for it...
There was a problem hiding this comment.
To my understanding, this is because of the positive_gradient_constr needing the TIMEINDEX (including periods) to be allowed to consider investment (periods) in the ramping. (@jokochems might now better.) There is probably a more elegant way to do this, but I did not want to touch this now.
There was a problem hiding this comment.
Maybe also something to be discussed in a separate issue to not delay the progress too much
There was a problem hiding this comment.
Well if you want to change it, it should rather be p for periods instead of t as the first index of the TIMEINDEX set refers to the periods, the second to the time steps.
What is more, is the previous formulation was just my inconvenient way of working around ramping events crossing periods. This is a nasty thing for the two-dimensional indexing, but vastly simplified when returning back to a one-dimensional flow index.
Maybe to add on that: I have never really used non-convex in combination with multi-period investments, but wanted to have it there in case somebody is willing to use that.
There was a problem hiding this comment.
If it is really the periods, I did something wrong, as m.TIMESTEPS[p] does not make sense.
|
Thank you for the work @p-snft, since the tests are working things look fine for me. However, I am not very knowledgeable yet on how multi-period would work and cannot really check the changes for content. Also, I did not check the lp-files changes... That is simply too much, so #1079 is really welcome! :) |
There was a problem hiding this comment.
Thank you, @p-snft!
It has always bugged me that the multi-period feature brought along this inconsistency between indexing flows and other variables.
I thought about indexing every not investment-related variable with the TIMEINDEX set, but getting rid of the TIMEINDEX and indexing flows in TIMESTEPS instead, seems more user-friendly and intuitive to me. I just did not come up with that because I seem to have been too routine-blinded.
Preserving periods where needed can be achieved by using the TIMEINDEX set for iterations, as you did. Well actually you just did not touch upon that and kept it as is.
Long story short: I am happy to approve this as well. I only had one minor suggestion for renaming.
P.S.: It has been a while since I looked into the code base, but I still have the feeling of understanding it, which is a very good thing. ;-)
| @@ -743,17 +743,11 @@ def _positive_gradient_flow_constraint(_): | |||
| m.flow[ | |||
| i, | |||
| o, | |||
| m.TIMEINDEX[index][0], | |||
| m.TIMEINDEX[index][1], | |||
| m.TIMESTEPS[index], | |||
There was a problem hiding this comment.
Well if you want to change it, it should rather be p for periods instead of t as the first index of the TIMEINDEX set refers to the periods, the second to the time steps.
What is more, is the previous formulation was just my inconvenient way of working around ramping events crossing periods. This is a nasty thing for the two-dimensional indexing, but vastly simplified when returning back to a one-dimensional flow index.
Maybe to add on that: I have never really used non-convex in combination with multi-period investments, but wanted to have it there in case somebody is willing to use that.
|
@p-snft I'm fine with you addressing the two comments according to your preferences (as I already approved it) and then merging it rightaways. Thank you very much! |
|
As the gradient constraint has problems anyway, I will not touch it in this PR. |
As different types of periods will be used for multi-period optimisation (long-term investment planing) and working with aggregated time-series (e.g. using tsam), I suggest to change the indexing. This PR implements Flows that use timesteps [int] instead of timeindex currently ([int, int]).