Clarification of Zone and Surface Opaque Conduction Output Variables #11110
Conversation
Corrected various problems with report variables relating to inside and outside face conduction variables. Some variables simply were not set up to report. Others were not listed in the docs. Also did a name clean-up for consistency between similar values.
Added a unit test to verify that the proper values are being reported for some of the variables that were added as part of this fix.
|
|
Well, clarification--I forgot that because variables were added that some of the files will show a change in NumofRVariables. There are no differences in actual results from EnergyPlus. |
mjwitte
left a comment
There was a problem hiding this comment.
Tested with 1ZoneUncontrolled, the zone-level outputs agree with the sums of the surface-level outputs.
|
|
||
| Output:Variable,*,Surface Outside Face Convection Heat Transfer Coefficient,daily; | ||
|
|
||
| Output:Variable,*,Surface Inside Face Conduction Heat Transfer Rate,hourly; |
There was a problem hiding this comment.
1ZoneUncontrolled has an rvi file in \testfiles. The new variables need to be added there as well.
There was a problem hiding this comment.
I can fix this when the crash issue you mentioned gets resolved.
There was a problem hiding this comment.
Had to make another commit because I forgot to do this before I committed the fix to resolve the initialization issue, but a revised .rvi is in the very latest commit.
| EXPECT_DOUBLE_EQ(srdSurfacesTemp_result, surface.SrdSurfTemp); | ||
| } | ||
|
|
||
| TEST_F(EnergyPlusFixture, HeatBalanceSurfaceManager_ZoneFaceConductionVariableTest) |
There was a problem hiding this comment.
This unit test crashes in SolarShading::ReportSurfaceShading. Using Debug build locally on windows.
There was a problem hiding this comment.
Ummm...well, I just tried this and it doesn't crash on Mac. Do you know what line it is crashing on?
@Myoldmopar Any idea why it's fine on the Mac but not on Windows? Not sure I've ever had this happen...or at least not recently.
There was a problem hiding this comment.
Ummm...well, I just tried this and it doesn't crash on Mac. Do you know what line it is crashing on?
@Myoldmopar Any idea why it's fine on the Mac but not on Windows? Not sure I've ever had this happen...or at least not recently.
Sorry, forgot to paste in the screen shot of the error and line. Here:
SurfSunlitFrac is nan, probably never initialized.
Why it doesn't crash on your Mac?
- I've merged in develop.
- In VS for windows, I've long ago had to manually activate exceptions for uninitialized variables and floating point exceptions.
There was a problem hiding this comment.
Or 3. Mac c++ might autoinitialize everything to zero?
There was a problem hiding this comment.
Huh, interesting. Well, to avoid things relying on the OS, I just made a new commit that specifically initializes the Sunlit variables to zero. Does this take care of the crash on Windows?
There was a problem hiding this comment.
Huh, interesting. Well, to avoid things relying on the OS, I just made a new commit that specifically initializes the Sunlit variables to zero. Does this take care of the crash on Windows?
@RKStrand Thanks, that fixed the crash. I still need to review docs, then this should be good to go.
|
In Windows, one of these was not being initialized and this resulted in a crash. Zeroing them all out on definition in this unit test now to hopefully avoid this.
Another review comment involved the RVI for 1ZoneUncontrolled. This resolves this comment hopefully.
|
|
…fyZoneOpaqueSurfaceConductionOutputVars
|
|
@mjwitte Oh, yes! Thank you for correcting those. I saw them somewhere along the way as I was looking at this and then forgot to correct them. Glad you caught it before the merge! 😊 |
Pull request overview
Description of the purpose of this PR
Several output variables that were listed in the Input/Output Reference relating to zone and surface opaque conduction values were not actually set up to allow them to be produced. A full review of these variables revealed that there were additional variables that were neither set-up for reporting in the code nor mentioned in the Input/Output Reference but were defined and calculated within the code for reporting purposes. In addition, there was some naming inconsistencies noted in these variables as well. Changes include making all variables that were set-up for reporting available by adding code to allow this, correcting and enhancing the Input/Output Reference for these variables, providing variable name change rules for four variables where the name was changed for clarify/consistency, an existing test file IDF was modified to request all of these variables, and a unit test to verify that these variables are being reported correctly. The only output changes would be in the file that had additional variables requested.
Pull Request Author
Reviewer