Removed unnecessary covariance get() #132
Conversation
teubert
added
A: Predictors
| @@ -117,8 +117,7 @@ namespace PCOE { | |||
| Matrix Pxx(model.getStateSize(), model.getStateSize()); | |||
| for (unsigned int xIndex = 0; xIndex < model.getStateSize(); xIndex++) { | |||
| xMean[xIndex][0] = state[xIndex][MEAN]; | |||
| std::vector<double> covariance = state[xIndex].getVec(COVAR()); | |||
| Pxx.row(xIndex, state[xIndex].getVec(COVAR(0))); | |||
| Pxx.row(xIndex, state[xIndex].getVec(COVAR())); | |||
There was a problem hiding this comment.
Are COVAR() and COVAR(0) equivalent in this context?
src/Models/BatteryModel.cpp
Outdated
| @@ -416,7 +416,7 @@ SystemModel::output_type BatteryModel::outputEqn(double, | |||
|
|
|||
| auto z_new = getOutputVector(); | |||
| // Set outputs | |||
| z_new[OUT::TEMP] = Tb - 273.15; | |||
| z_new[OUT::TEMP] = Tb; | |||
There was a problem hiding this comment.
I don't think I like this... Since we don't have proper unit analysis, this silently changes the model's API. How are we going to communicate this change effectively?
There was a problem hiding this comment.
Right now, the main tools we have to communicate the change are the release notes and the documentation. That is, unless we were to incorporate some sort of unit analysis like this library (except supporting C++11):
https://github.com/nholthaus/units
I am also on the fence on this for the same reasons. In the end I wanted to go for it because 1. it's in the most computationally expensive function of the innermost loop of the prediction step. 2. it's the lesser used output (voltage is more important. and 3. At this point we don't have many users.
That said, I am still on the fence, so if you prefer not to make this change I can reverse it.
There was a problem hiding this comment.
I thought some more about this and I think you're right. I reverted this commit.
There was a problem hiding this comment.
Sounds good. I've been working on my own unit analysis library off and on when I have some spare time. Eventually it will be good enough to consider incorporating it into GSAP and other projects.
Just saw this unnecessary line when debugging with Ed. Fixes #130
Also, noticed that we were returning temperature in °C but it's calculated in K. Updated to return in K, removing a single operation from the inner loop. (#133)
Added a few comments describing UKF variables