WindowManager Continued #10874
WindowManager Continued #10874
Conversation
|
|
| @@ -2025,18 +2025,6 @@ void ConstructionProps::setArraysBasedOnMaxSolidWinLayers(EnergyPlusData &state) | |||
| this->rbBareVisCoef.allocate(state.dataHeatBal->MaxSolidWinLayers); | |||
| this->afBareSolCoef.allocate(state.dataHeatBal->MaxSolidWinLayers); | |||
| this->abBareSolCoef.allocate(state.dataHeatBal->MaxSolidWinLayers); | |||
| for (int Layer = 1; Layer <= state.dataHeatBal->MaxSolidWinLayers; ++Layer) { | |||
There was a problem hiding this comment.
Don't need to be allocated because they are std::arrays now.
src/EnergyPlus/Construction.hh
Outdated
| @@ -216,32 +216,32 @@ namespace Construction { | |||
| Real64 AbsDiffShade = 0.0; // Diffuse solar absorptance for shade | |||
| Real64 AbsDiffBackShade = 0.0; // Diffuse back solar absorptance for shade | |||
| Real64 ShadeAbsorpThermal = 0.0; // Diffuse back thermal absorptance of shade | |||
| Array1D<Array1D<Real64>> AbsBeamCoef; // Coefficients of incidence-angle polynomial for solar | |||
| Array1D<std::array<Real64, DataSurfaces::MaxPolyCoeff>> AbsBeamCoef; // Coefficients of incidence-angle polynomial for solar | |||
There was a problem hiding this comment.
Polynomial coefficients are now held in std::array because the maximum number is known at compile time.
src/EnergyPlus/General.hh
Outdated
| @@ -82,6 +82,13 @@ namespace General { | |||
| Real64 X_0, // 1st bound of interval that contains the solution | |||
| Real64 X_1); // 2nd bound of interval that contains the solution | |||
|
|
|||
| constexpr Real64 POLYF(Real64 const X, // Cosine of angle of incidence | |||
There was a problem hiding this comment.
std::array version of this function.
src/EnergyPlus/HeatBalanceManager.cc
Outdated
| Array1D<Real64> tvisFit(10); // Fitted visible transmittance vs incidence angle | ||
| Array1D<Real64> rfsolFit(10); // Fitted solar front reflectance vs incidence angle | ||
| Array2D<Real64> solabsFit(5, 10); // Fitted solar absorptance vs incidence angle for each glass layer | ||
| Array1D<Real64> TsolTemp(Window::numPhis+1); // Solar transmittance vs incidence angle; diffuse trans. |
There was a problem hiding this comment.
Need temporary Array1D's in order to read from the W5 input file. Can probably move these to local variables now that I think about it.
| @@ -4134,18 +4138,6 @@ namespace HeatBalanceManager { | |||
| if (NextLine.eof) goto Label1000; | |||
| ++FileLineCount; | |||
|
|
|||
| // Pre-calculate constants | |||
There was a problem hiding this comment.
cosPhis is now a constexpr std::array in the Window module header file.
| @@ -75,9 +74,16 @@ namespace Window { | |||
|
|
|||
| int constexpr maxGlassLayers = 5; | |||
| int constexpr maxGapLayers = 5; | |||
| int constexpr maxIncidentAngles = 20; | |||
There was a problem hiding this comment.
This is never 20, number of elements always hardwired to 10 during use. So why allocate all the arrays to 20?
src/EnergyPlus/WindowManager.hh
Outdated
| Real64 constexpr dPhiDeg = 10.0; | ||
| Real64 constexpr dPhiRad = dPhiDeg * Constant::DegToRad; | ||
|
|
||
| constexpr std::array<Real64, numPhis> cosPhis = |
There was a problem hiding this comment.
Bake cos(phi) at 10.0 degree increments into the executable, no need to compute at runtime.
src/EnergyPlus/WindowManager.hh
Outdated
| int N1, // First and last data points used | ||
| int N2, | ||
| Array1A<Real64> CoeffsCurve // Polynomial coeffients from fit | ||
| void W5LsqFit(std::array<Real64, numPhis> const &ivars, // Independent variables |
There was a problem hiding this comment.
Version of this function using std::array. Array size and polynomial order arguments removed because they were always the same.
| ShowFatalError(state, "Errors found getting inputs. Previous error(s) cause program termination."); | ||
| } | ||
|
|
||
| // Loop over incidence angle from 0 to 90 deg in 10 deg increments. | ||
| // Get glass layer properties, then glazing system properties (which include the | ||
| // effect of inter-reflection among glass layers) at each incidence angle. | ||
|
|
||
| for (int IPhi = 1; IPhi <= TotalIPhi; ++IPhi) { |
There was a problem hiding this comment.
No need to calculate this anymore.
| @@ -2545,121 +2517,6 @@ namespace Window { | |||
|
|
|||
| //**************************************************************************** | |||
|
|
|||
| #ifdef GET_OUT | |||
There was a problem hiding this comment.
This giant thing was not used.
amirroth
added
DoNotPublish
These diffs make no sense. None of them are in windows related files. These almost look like they belong to a different PR. |
src/EnergyPlus/WindowManager.cc
Outdated
| @@ -642,27 +636,22 @@ namespace Window { | |||
| } | |||
| } // End of loop over glass layers in the construction for front calculation | |||
|
|
|||
| if (TotalIPhi > maxIncidentAngles) { | |||
| if (TotalIPhi > numPhis) { | |||
| ShowSevereError(state, | |||
| format("WindowManage::InitGlassOpticalCalculations = {}, Invalid maximum value of common incident angles = {}.", | |||
There was a problem hiding this comment.
|
|
|
I can't reproduce these diffs locally, in either debug or release. Did something go wrong with how this branch is attached to the repository? I did change the computer I was using to work on it. |
|
Is this branch set up properly? @Myoldmopar do you have a theory about why my local copy is not seeing seeing these diffs? Did switching to a different machine break something? |
|
I don't see anything immediately wrong with the branch. I can concur that I don't see diffs locally. One odd thing is that when I check out this branch, I am not on the same SHA as the CI instance is running. The branch is up to date with develop, so I would have thought the merge SHA would be the same as the HEAD commit on the branch (a4c6541). But on GHA it is running 7ce4cfd as the merge commit for this branch. That's odd. Unfortunately this didn't reveal much because even when I fetch this merge commit, build it, and run it, it still doesn't show diffs. But then I ran the diffs with the gha_regressions script instead of the regression tool GUI, and I get one diff: |
|
|
|
|
OK I'm going to try to baby step my other PR forward and see what happens as it goes. As of right now, this PR has eight commits in it. Two are format and my GHA tweak, so those aren't relevant. Two of them are just merges from develop, so for now I will assume those are also not relevant. That leaves four meaningful commits: I'm taking each of these one at a time and building up my PR #10885. As of typing this, I have already cherry picked 71257, and I am getting diffs, just like this PR saw last week when that commit was pushed here. I'll report back as things progress. |
|
|
@amirroth I pulled in develop, and attempted to resolve the conflicts, but I may need to revisit them, because there are a few tests that fail. I think I can get them going, but I'll reach out if I get stuck. At this point, I am still able to reproduce the diffs on my Apple Silicon. However, @rraustad reminded me I should evaluate the impact of the diffs by reporting at the runperiod interval. Once I switch to that reporting, all the big diffs go away. There are small diffs, that's fine. This should pass format/integrity checks, Decent should show a few test failures, GHA should show the same test failures, as well as the regressions. Once those tests are resolved, we can probably merge this in and move on. |
|
This quick PR transitions all window arrays indexed by incident angle and polynomial coefficient from
Array1Dtostd::array. In a following PR will move glass layer intostd::arrayas well moving this module to C-based storage and indexing.