Conversation
amirroth added Refactoring 
amirroth left a comment
amirroth left a comment There was a problem hiding this comment.
Quick walk-through. Not much to see. Mostly tedium.
src/EnergyPlus/WindowManager.cc Outdated
| std::array<Real64, maxGapLayers> &hgap, // Gap gas conductive conductance (W/m2-K) | ||
| Array2D<Real64> &Aface, // Coefficient in equation Aface*thetas = Bface | ||
| Array1D<Real64> &Bface // Coefficient in equation Aface*thetas = Bface | ||
| std::array<std::array<Real64, 2 * maxGlassLayers>, 2 * maxGlassLayers> &Aface, // Coefficient in equation Aface*thetas = Bface |
src/EnergyPlus/WindowManager.cc Outdated
| | ||
| void LUdecomposition(EnergyPlusData &state, | ||
| Array2<Real64> &ajac, // As input: matrix to be decomposed; | ||
| std::array<std::array<Real64, 2 * maxGlassLayers>, 2 * maxGlassLayers> &ajac, // As input: matrix to be decomposed; |
There was a problem hiding this comment.
Have to be very careful with index arithmetic here. I think I got this right.
| if (i < wm->nglface) { | ||
| rguess(i + 1) = resgap; | ||
| for (int i = 1; i < wm->nglface; i += 2) { | ||
| rguess[i] = 1.0 / wm->scon[i / 2]; |
There was a problem hiding this comment.
Another index arithmetic gotcha.
| --L; | ||
| } | ||
| } // W5LsqFit2() | ||
| |
There was a problem hiding this comment.
We have not been using this version for quite a while.
| bld_pr[3] = 0.0; | ||
| bld_pr[4] = 0.0; | ||
| bld_pr[5] = 0.0; | ||
| bld_pr[6] = matBlind->slatTAR.Sol.Ft.Bm[0].DfTra; |
There was a problem hiding this comment.
At some point, we can just pass a const reference to this data structure instead of packing everything into an array and unpacking it INTO THE SAME STRUCTURE in the called function.
src/EnergyPlus/WindowManager.cc Outdated
| for (int k = 1; k <= i - 1; ++k) { | ||
| sum -= ajac(k, i) * ajac(j, k); | ||
| for (int j = 0; j < n; ++j) { | ||
| for (int i = 0; i < j; ++i) { // should this be j-1? |
There was a problem hiding this comment.
Risky to say anything here but i < j does look correct. I just substituted values for i, j, and k and it seems the loops do the same thing now.
There was a problem hiding this comment.
I agree. I think i < j is correct.
src/EnergyPlus/WindowManager.cc Outdated
| std::array<std::array<Real64, 6>, 6> F; // View factor array | ||
| std::array<std::array<Real64, 2 * maxGlassLayers>, 2 * maxGlassLayers> X; // Exchange matrix | ||
| std::array<std::array<Real64, 2 * maxGlassLayers>, 2 * maxGlassLayers> Xinv; // Inverse of exchange matrix | ||
| std::array<int, 2 * maxGlassLayers> indx; // LU decomposition indices |
There was a problem hiding this comment.
I've seen these 2 * maxGlassLayers a lot. If int constexpr maxGlassLayers = 5; then would it be beneficial to create int constexpr twoTimesMaxGlassLayers = 10; ?
There was a problem hiding this comment.
FWIW, the compiler reduces any expression that consists only of constants (otherwise, this wouldn't compile because the second argument to the std::array template has to be a constant). Replacing this with twoTimesMaxGlassLayers doesn't save any runtime, makes the code slightly longer, and in my opinion, also slightly less clear. That said, it's six on one hand.
There was a problem hiding this comment.
Thanks for that info. As you can see, I already pushed the change, but it doesn't sound like that's a problem.
| | ||
| indx = 0; | ||
| indx = {0}; | ||
| InvertMatrix(state, X, Xinv, indx, 4); // Autodesk:Note X modified by this call |
There was a problem hiding this comment.
Out of scope but indx is getting initialized twice, once here and once in the function (line 8241).
|
| I'll rerun regressions on this soon. The current results don't make sense to me. |
| Regressions look good now. @jmarrec I don't think I did things out of order on this, so I'm a bit confused on the false positive on the regressions here. Flagging for your awareness. |
4 participants
Transitions most remaining uses of ObjexxFCL
ArrayXtostd::array. The remaining uses are the input processor and something having to do with blind slat profiles where the current implementation looks like an off-by-1 bug but I don't have the time or patience to investigate it.