Input Equation #5
Conversation
|
The new input equation function provides the same output, but instead of finding which load corresponds to given time for each sample, the function generates a matrix (time x number of samples) where every column corresponds to a different future profile according to the input parameters. Then, the input time is used as an index in such as way that the output for the time t is a vector that contains all columns in the t row. The matrix is only generated at the first prediction time step or if the input parameters change. Otherwise, the function just throws a row of the matrix as output. |
|
This should be instead a commit to the develop branch, not the master branch. |
There was a problem hiding this comment.
Main comments:
-
Contributions should be submitted to the develop branch, not the master branch. Please resubmit as such.
-
Variable naming needs to be updated and comments added.
-
This should instead be an update to the current version of InputEqn. See also the comments about sticking with the segment-based matrix rather than the time-based matrix, which should be even more efficient.
| @@ -0,0 +1,28 @@ | |||
| function Output = InputEqn(parameters,t,inputParameters) | |||
There was a problem hiding this comment.
Function name should match filename. In any case, since this is an improvement to Battery.InputEqn, then it should be submitted as a modification to that file instead of a new one.
The code also needs to be fully commented and follow variable naming conventions in the code base.
| @@ -0,0 +1,28 @@ | |||
| function Output = InputEqn(parameters,t,inputParameters) | |||
|
|
|||
| persistent input inputparameters time; | |||
There was a problem hiding this comment.
These variable names are not clear. Especially inputparameters vs inputParameters and time vs t. Suggest renaming to a more descriptive name.
I am also wary of using persistent variables, as this may have unintended consequences. However I realize this is needed to get the speed improvement.
|
|
||
| persistent input inputparameters time; | ||
|
|
||
| if isempty(input) || ~isequal(inputparameters,inputParameters) |
There was a problem hiding this comment.
The function should handle the case where the inputParameters argument is not specified (see the original InputEqn).
| end | ||
| input(1:length(temp),j)=temp; | ||
| input(length(temp)+1:end,j)=temp(end); | ||
| end |
There was a problem hiding this comment.
The way this works is that a time-based matrix is constructed in the first call to InputEqn (or when inputParameters changes). Precomputing this matrix gives the speedup, because then it is not recomputed at each call to InputEqn. This is the first difference with the original InputEqn.
The second difference is that the matrix used to lookup the load for the current time is time-based, instead of segment-based. For example consider 10 segments, covering 1 hour, with a sample time for the model of 1 second, with 100 prediction samples. The time-based matrix would be 3600x100. For a sampling time of 0.1 seconds, it is 36000x100. This is not scalable and would result in memory and performance issues if the sampling time is decreased and/or the total duration covered by the segments is increased.
In contrast, the segment-based matrix will always be of size (n_segments)x(n_samples). So I would suggest that instead of precomputing the time-based matrix, precompute the segment-based matrix (the 'loads' and 'times' variables in the original InputEqn). Then this would be a more straightforward extension of the original InputEqn where those are precomputed on the first call to InputEqn (or when inputParameters changes) and should be even more efficient than this revision.
|
Follow up- No need to resubmit in response to comment 1. I changed the base branch to develop. The other comments do still need to be resolved, but that can be done in this pull request. |
As shown in the figure, the input equation function proposed to reduce the prediction processing time. The times were measured on an Intel Core i7-2680QM CPU @ 2.50GHz with 8GB of RAM with no other application running at the same time.