Fix Reheat Coil Multiplier report #11029
Conversation
|
| if (IsNotOK) { | ||
| ShowContinueError(state, format("In {} = {}", airTerm.sysType, airTerm.SysName)); | ||
| ErrorsFound = true; | ||
| if (airTerm.ReheatComp_Num == HeatingCoilType::Gas || airTerm.ReheatComp_Num == HeatingCoilType::Electric) { |
There was a problem hiding this comment.
These are the changes. They occur in about four different places.
| if (IsNotOK) { | ||
| ShowContinueError(state, format("In {} = {}", airTerm.sysType, airTerm.SysName)); | ||
| ErrorsFound = true; | ||
| if (airTerm.ReheatComp_Num == HeatingCoilType::Gas || airTerm.ReheatComp_Num == HeatingCoilType::Electric) { |
There was a problem hiding this comment.
Here they are again.
|
|
|
Diffs (eplustbl.htm.absdiff.htm) in ASHRAE901_OutPatientHealthCare_STD2019_Denver are because of a change in rows in the table. This makes sense because these code changes did not change heating coil performance. Not sure why the diff dates look different compared to the actual table dates when I used the same weather file. add_simulation_test(IDF_FILE ASHRAE901_OutPatientHealthCare_STD2019_Denver.idf EPW_FILE USA_CO_Denver-Aurora-Buckley.AFB.724695_TMY3.epw) LOCATION,Denver-Aurora-Buckley AFB,CO,USA,TMY3,724695,39.72,-104.75,-7.0,1726.0 Diffs:
Develop:
This branch:
Typical diffs in many files change the TU reheat coil multiplier from -999 to 1.
|
|
|
|
OK, the code changes do look good. I'm going to jump back to tablediff and try to get the reordered/diffs stuff worked out prior to merging this though, since it's such a good example. |
|
@amirroth I was able to make some great changes to table diff. The full set of changes is here: NREL/EnergyPlusRegressionTool#117, but the crux of it is that table diff should now intelligently allow row reordering in most cases, and also ignore the silliest of table diffs that are not useful as regression results. Unit tests over on the regression tool are all happy, and I am cautiously optimistic that it will come back clean here with fewer, and more meaningful regressions. Assuming it does, I'll tag a release of the regression tool, point this branch back, and then get this merged in. Thanks for your patience, this resulted in some really nice improvements on table diff. FYI @mjwitte |
|
|
OK, so the table big diff count didn't go down, because there are still actual diffs from the multiplier. However, it is cleaner. In the previous regression run, this was in the outpatient 90.1 table diff summary:
And the actual diff was simply the rows reordered:
With the improved table diff, it's properly handling the changed row order:
Looks good to me. Unless there is an objection, I'll tag a new release of the regression tool, update the pip install command in this branch, and then get this merged in. Actually..... @mjwitte, if you have a quick fix for the enum cleanup test failure, I can toss it in here. |
|
Alright, patched in the unit test fix suggested by @mjwitte (thanks!) and got the new regression script installing from PyPi. This should come back fully happy and this will merge in. |
|
|
Same as before, good to go. Thanks @amirroth |
5 participants
Pull request overview
This PR fixes #11030 (yes, I created the fix before reporting the issue).
The fix is simple. The Reheat Coil needs to be stored via
GetCoilIndexwhen theAirTerminalobject is created. Simply validating that that the coil exists viaValidateComponentis not enough (ValidateComponentneeds to go but that is a story for a different day).There are a few diffs. In addition to the expected desired diff of reported reheat coil multipliers, there are now ordering diffs in
RDD,MDD,MTR,BND,EIOand a number of other files because objects are being created in different orders than they used to be (GetCoilIndexactually reads coil inputs and creates coil objects whereasValidateComponentdoes not).