ZoneHVAC:HybridUnitaryHVAC ERR object type output #8326

matthew-larson · 2020-09-30T16:04:15Z

Pull request overview

Fixes ZoneHVAC:HybridUnitaryHVAC ERR output malfunctioning #8316
This pull request corrects an inconsistent variable name for the Object type related to the ZoneHVAC:HybridUnitaryHVAC object. This was being referenced as "cCurrentModuleObject" in some places when "CurrentModuleObject" was the original string variable name. I changed all of the references to "cCurrentModuleObject" to match the majority of other Object type variable names for objects.
Input processing for optional fields was fixed so that a blank optional field will not throw an error.
Various error messages were corrected to use the correct variables for referencing field names and values.

Pull Request Author

Add to this list or remove from it as applicable. This is a simple templated set of guidelines.

Reviewer

This will not be exhaustively relevant to every PR.

…ridunitary-erroutput

mjwitte · 2020-10-16T21:39:56Z

@Myoldmopar You disabled some HybridUnitary unit tests in 799b650. They run fine locally. Any reason to keep them disabled?

Myoldmopar · 2020-10-16T21:40:49Z

If they are running fine go ahead and re-enable them.

mjwitte

@matthew-larson Please merge in develop and resolve conflicts and address these other comments.

mjwitte · 2020-10-16T21:54:54Z

src/EnergyPlus/HybridUnitaryAirConditioners.cc

-                    ShowSevereError(RoutineName + cCurrentModuleObject + "=\"" + Alphas(1) + " invalid data");
+                    ShowSevereError(RoutineName + cCurrentModuleObject + " = " + Alphas(1) + " invalid data");
                    ShowContinueError("Invalid-not found" + cAlphaFieldNames(19) + "=\"" + Alphas(19) + "\".");


The object type for the error message is fixed now, but the error message is still basically empty:

** Severe ** GetInputZoneHybridUnitaryAirConditioners: ZoneHVAC:HybridUnitaryHVAC = MUNTERSEPX5000 invalid data ** ~~~ ** Invalid-not found="".

In the call to getObjectItem, the field names are returned in cAlphaFields. Some error messages use this variable, but a few (like the error shown here) are using cAlphaFieldNames which is blank. Replace all of these with cAlphaFields and the error message should show the field name that's in error.

mjwitte · 2020-10-16T21:55:45Z

tst/EnergyPlus/unit/UnitaryHybridAirConditioner.unit.cc

@@ -1265,4 +1265,124 @@ TEST_F(EnergyPlusFixture, DISABLED_Test_UnitaryHybridAirConditioner_ModelOperati
    }
 }

+TEST_F(EnergyPlusFixture, DISABLED_Test_UnitaryHybridAirConditioner_ValidateError)


Please enable this unit test and all of the other hybrid unitary tests. They run fine locally here.

nealkruis · 2020-10-19T14:21:04Z

@mjwitte It may be fine to enable the hybrid unitary tests, but I remember seeing this from @Myoldmopar:

so when I try to run the full unit test suite in debug it is failing on the hybrid unitary tests:
0x000055555629cca5 in EnergyPlus::Psychrometrics::PsyRhFnTdbWPb (TDB=0, dW=0, PB=0, CalledFrom="InitZoneHybridUnitaryAirConditioners")
    at /eplus/repos/myoldmopar/src/EnergyPlus/Psychrometrics.hh:920
920	        Real64 const RHValue(U / (1.0 - (1.0 - U) * (PWS / PB)));
If you run them individually, they pass, but something is not being cleaned up

EnergyPlus/src/EnergyPlus/HybridUnitaryAirConditioners.cc

Line 205 in 9fc1e48

static bool HybridCoolOneTimeFlag(true); // one time flag

this flag is definitely problematic since it is keeping everything from being reinitialized the second time, but I pulled that out to a namespace variable and reset it in clear_state and it is still failing
maybe there is a second static in there causing an issue, I'm not sure, but I'm going to disable all the hybrid unitary tests for right now

@matthew-larson looked into this a bit and couldn't reproduce the issue. Do we still want to move forward with enabling them?

mjwitte · 2020-10-19T14:43:40Z

@nealkruis I ran all of the hybrid unit tests serially and it was fine here (Windows 10). I'm running everything now. Moving that to the namespace likely corrected the problem. Either way, these need to be enabled. If there's still a problem, then it can be fixed.

matthew-larson · 2020-10-19T16:11:47Z

@mjwitte I updated the code based on your comments and also re-enabled the unit tests. Tests all run successfully for me locally.

mjwitte · 2020-10-19T21:09:27Z

@matthew-larson That's better.

   ** Severe  ** GetInputZoneHybridUnitaryAirConditioners: ZoneHVAC:HybridUnitaryHVAC = MUNTERSEPX5000 invalid data
   **   ~~~   ** Invalid-not found Design Specification Outdoor Air Object Name="".

But why is this an error? The IDD implies that this field is optional.

@Myoldmopar Any idea what happened with the windows CI failure?

matthew-larson · 2020-10-20T15:21:08Z

@mjwitte that's a great point about that particular field being optional. Digging into it a little, the ZoneHybridUnitaryAirConditioner(UnitLoop).OutdoorAir method isn't actually being used for anything and if ZoneHybridUnitaryAirConditioner(UnitLoop).OARequirementsPtr is zero, it just sets the minimum ventilation rate to 0 further in the code, which is the intent. Based on this, I'm tempted to just remove the if/else statement altogether here. I can modify the unit test to present a different error the shows the correct module object.

        // A19, \ OA requirement pointer
        ZoneHybridUnitaryAirConditioner(UnitLoop).OARequirementsPtr = UtilityRoutines::FindItemInList(Alphas(19), OARequirements);
        if (ZoneHybridUnitaryAirConditioner(UnitLoop).OARequirementsPtr == 0) {
            ShowSevereError(RoutineName + cCurrentModuleObject + " = " + Alphas(1) + " invalid data");
            ShowContinueError("Invalid-not found " + cAlphaFields(19) + "=\"" + Alphas(19) + "\".");
            ErrorsFound = true;
        } else {
            ZoneHybridUnitaryAirConditioner(UnitLoop).OutdoorAir = true;
        }

mjwitte · 2020-10-20T15:25:57Z

@matthew-larson This still needs to trap an invalid non-blank OA specification object name. For optional fields without a default, the input processing should check if the field is blank (using the logical array that's returned from getObjectItem) then skip it, but if it's not blank, then get the pointer and throw an error if it's still zero.

matthew-larson · 2020-10-20T16:38:17Z

@mjwitte that makes sense. I modified the code as shown below, which I believe now acts as intended to check if the field is blank, otherwise grab the pointer and throw an error if 0.

                // A19, \ OA requirement pointer
                if (!lAlphaBlanks(19)) {
                    ZoneHybridUnitaryAirConditioner(UnitLoop).OARequirementsPtr = UtilityRoutines::FindItemInList(Alphas(19), OARequirements);
                    if (ZoneHybridUnitaryAirConditioner(UnitLoop).OARequirementsPtr == 0) {
                        ShowSevereError(RoutineName + cCurrentModuleObject + " = " + Alphas(1) + " invalid data");
                        ShowContinueError("Invalid-not found " + cAlphaFields(19) + "=\"" + Alphas(19) + "\".");
                        ErrorsFound = true;
                    } else {
                        ZoneHybridUnitaryAirConditioner(UnitLoop).OutdoorAir = true;
                    }
                }

mjwitte · 2020-10-20T16:46:31Z

@matthew-larson Yes, like that. While you're in there, it would be good to review all of the other optional fields in the (massive) object to see if they are all being processed correctly. I suppose a test file with all optional fields left blank should expose any problems.

…ode is required, and update unit test to test optional fields.

…ridunitary-erroutput

matthew-larson · 2020-10-27T14:40:19Z

doc/input-output-reference/src/overview/group-zone-forced-air-units.tex

@@ -4241,7 +4241,7 @@ \subsubsection{Inputs}
 This alpha input field specifies the name of the HVAC system node from which the hybrid unit draws return air. This node name must also be specified as a zone return air node in a \hyperref[zonehvacequipmentconnections]{ZoneHVAC:EquipmentConnections} object. It may also be included in \hyperref[nodelist]{NodeList} object that is specified as a zone return air node in a \hyperref[zonehvacequipmentconnections]{ZoneHVAC:EquipmentConnections} object. (Ref. Group - Zone Equipment \hyperref[zonehvacequipmentconnections]{ZoneHVAC:EquipmentConnections} and Group - Node-Branch Management \hyperref[nodelist]{NodeList}).

 \paragraph{Field: Outdoor Air Node Name}
-This alpha input field specifies the name of the HVAC system node from which the hybrid unit draws outdoor air. This node name must also be specified in an \hyperref[outdoorairnode]{OutdoorAir:Node} or \hyperref[outdoorairnodelist]{OutdoorAir:NodeList} object. If this field is blank, a node name will be created internally. (Ref. Group - Node-Branch Management \hyperref[outdoorairnode]{OutdoorAir:Node} and \hyperref[outdoorairnodelist]{OutdoorAir:NodeList}).
+This alpha input field specifies the name of the HVAC system node from which the hybrid unit draws outdoor air. This node name must also be specified in an \hyperref[outdoorairnode]{OutdoorAir:Node} or \hyperref[outdoorairnodelist]{OutdoorAir:NodeList} object. (Ref. Group - Node-Branch Management \hyperref[outdoorairnode]{OutdoorAir:Node} and \hyperref[outdoorairnodelist]{OutdoorAir:NodeList}).


The IDD has this as a required field so I just removed the statement about leaving the field blank.

Good catch. But the code for this input is wrong. See comment below.

matthew-larson · 2020-10-27T17:48:28Z

@mjwitte I cleaned up a couple more optional fields to prevent incorrect errors and updated the unit test with optional fields left blank.

mjwitte

@matthew-larson Almost there.

mjwitte · 2020-10-28T12:17:00Z

doc/input-output-reference/src/overview/group-zone-forced-air-units.tex

@@ -4241,7 +4241,7 @@ \subsubsection{Inputs}
 This alpha input field specifies the name of the HVAC system node from which the hybrid unit draws return air. This node name must also be specified as a zone return air node in a \hyperref[zonehvacequipmentconnections]{ZoneHVAC:EquipmentConnections} object. It may also be included in \hyperref[nodelist]{NodeList} object that is specified as a zone return air node in a \hyperref[zonehvacequipmentconnections]{ZoneHVAC:EquipmentConnections} object. (Ref. Group - Zone Equipment \hyperref[zonehvacequipmentconnections]{ZoneHVAC:EquipmentConnections} and Group - Node-Branch Management \hyperref[nodelist]{NodeList}).

 \paragraph{Field: Outdoor Air Node Name}
-This alpha input field specifies the name of the HVAC system node from which the hybrid unit draws outdoor air. This node name must also be specified in an \hyperref[outdoorairnode]{OutdoorAir:Node} or \hyperref[outdoorairnodelist]{OutdoorAir:NodeList} object. If this field is blank, a node name will be created internally. (Ref. Group - Node-Branch Management \hyperref[outdoorairnode]{OutdoorAir:Node} and \hyperref[outdoorairnodelist]{OutdoorAir:NodeList}).
+This alpha input field specifies the name of the HVAC system node from which the hybrid unit draws outdoor air. This node name must also be specified in an \hyperref[outdoorairnode]{OutdoorAir:Node} or \hyperref[outdoorairnodelist]{OutdoorAir:NodeList} object. (Ref. Group - Node-Branch Management \hyperref[outdoorairnode]{OutdoorAir:Node} and \hyperref[outdoorairnodelist]{OutdoorAir:NodeList}).


Good catch. But the code for this input is wrong. See comment below.

mjwitte · 2020-10-28T12:44:39Z

src/EnergyPlus/HybridUnitaryAirConditioners.cc

                ZoneHybridUnitaryAirConditioner(UnitLoop).SecondaryInletNode = GetOnlySingleNode(state, Alphas(10),
                                                                                                 ErrorsFound,
-                                                                                                 CurrentModuleObject,
+                                                                                                 cCurrentModuleObject,
                                                                                                 Alphas(1),
                                                                                                 NodeType_Air,
                                                                                                 NodeConnectionType_OutsideAirReference,


This is kind of nit-picky, but NodeConnectionType_OutsideAirReference should be NodeConnectionType_OutsideAir because this node is a real outdoor air node with flow, not just a sensor.

mjwitte · 2020-10-28T12:49:17Z

src/EnergyPlus/HybridUnitaryAirConditioners.cc

+                    ZoneHybridUnitaryAirConditioner(UnitLoop).TsaMax_schedule_pointer = GetScheduleIndex(state, Alphas(5));
+                    if (ZoneHybridUnitaryAirConditioner(UnitLoop).TsaMax_schedule_pointer == 0) {
+                        ShowSevereError("Invalid " + cAlphaFields(5) + '=' + Alphas(5));
+                        ShowContinueError("Entered in " + cCurrentModuleObject + '=' + cAlphaArgs(1));


Made a test file with bad inputs in a bunch of fields, and some of the error messages are still broken:

** Severe ** Invalid Minimum Supply Air Temperature Schedule Name=XMINSUPPLYT ** ~~~ ** Entered in ZoneHVAC:HybridUnitaryHVAC=MAIN ZONE BASEBOARD

cAlphaArgs should be Alphas here and elsewhere. Here's the test file:
UnitaryHybridAC_bug-ForceErrors1.idf.txt

…ridunitary-erroutput

mjwitte · 2020-11-03T22:14:17Z

@matthew-larson Almost there, no more cAlphaArgs lurking. But there are still 2 instances of cAlphaFieldNames that need to be fixed.

…ridunitary-erroutput

mjwitte

All good now. Merging.

matthew-larson added 5 commits September 29, 2020 14:53

Fix current module object variable mismatch naming.

c299e9b

Clean up error message.

9ad786a

Merge branch 'develop' of https://github.com/nrel/energyplus into hyb…

9c6f064

…ridunitary-erroutput

Add unit test for error check.

fe821f6

Merge branch 'develop' of https://github.com/nrel/energyplus into hyb…

b688a55

…ridunitary-erroutput

matthew-larson added Defect Includes code to repair a defect in EnergyPlus NotIDDChange Code does not impact IDD (can be merged after IO freeze) labels Sep 30, 2020

matthew-larson requested review from Myoldmopar and nealkruis September 30, 2020 16:04

matthew-larson self-assigned this Sep 30, 2020

mjwitte requested changes Oct 16, 2020

View reviewed changes

matthew-larson added 3 commits October 19, 2020 09:15

Merge develop

d635741

Fix error message references.

3d8a04e

Re-enable unit tests and fix error string for error output unit test.

b8404b1

matthew-larson assigned matthew-larson and unassigned matthew-larson Oct 20, 2020

matthew-larson added 2 commits October 26, 2020 14:41

Add if statements for optional fields, update docs that outdoor air n…

6f9efec

…ode is required, and update unit test to test optional fields.

Merge branch 'develop' of https://github.com/nrel/energyplus into hyb…

b6c822d

…ridunitary-erroutput

matthew-larson commented Oct 27, 2020

View reviewed changes

matthew-larson requested a review from mjwitte October 27, 2020 15:00

mjwitte mentioned this pull request Oct 28, 2020

PsyRhFnTdbWPb divide by zero when called for an uninitialized node #8359

Open

3 tasks

mjwitte reviewed Oct 28, 2020

View reviewed changes

matthew-larson added 2 commits November 2, 2020 08:39

Merge branch 'develop' of https://github.com/nrel/energyplus into hyb…

9caf7d9

…ridunitary-erroutput

Fixes for field references and outside air reference.

7994f7b

matthew-larson requested a review from mjwitte November 2, 2020 19:09

matthew-larson added 3 commits November 3, 2020 15:58

Change cAlphaFieldNames to cAlphaFields.

0c13045

Merge branch 'develop' of https://github.com/nrel/energyplus into hyb…

9ee3bd3

…ridunitary-erroutput

Fix Availability Schedule Name blank input and unit test.

af97526

mjwitte approved these changes Nov 4, 2020

View reviewed changes

mjwitte merged commit fb8d228 into NREL:develop Nov 4, 2020

matthew-larson deleted the hybridunitary-erroutput branch November 4, 2020 19:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ZoneHVAC:HybridUnitaryHVAC ERR object type output #8326

ZoneHVAC:HybridUnitaryHVAC ERR object type output #8326

matthew-larson commented Sep 30, 2020 •

edited by mjwitte

Loading

mjwitte commented Oct 16, 2020

Myoldmopar commented Oct 16, 2020

mjwitte left a comment

mjwitte Oct 16, 2020

mjwitte Oct 16, 2020

nealkruis commented Oct 19, 2020 •

edited

Loading

mjwitte commented Oct 19, 2020

matthew-larson commented Oct 19, 2020

mjwitte commented Oct 19, 2020

matthew-larson commented Oct 20, 2020

mjwitte commented Oct 20, 2020

matthew-larson commented Oct 20, 2020

mjwitte commented Oct 20, 2020

matthew-larson Oct 27, 2020

mjwitte Oct 28, 2020 •

edited

Loading

matthew-larson commented Oct 27, 2020

mjwitte left a comment

mjwitte Oct 28, 2020 •

edited

Loading

mjwitte Oct 28, 2020

mjwitte Oct 28, 2020

mjwitte commented Nov 3, 2020

mjwitte left a comment

ZoneHVAC:HybridUnitaryHVAC ERR object type output #8326

ZoneHVAC:HybridUnitaryHVAC ERR object type output #8326

Conversation

matthew-larson commented Sep 30, 2020 • edited by mjwitte Loading

Pull request overview

Pull Request Author

Reviewer

mjwitte commented Oct 16, 2020

Myoldmopar commented Oct 16, 2020

mjwitte left a comment

Choose a reason for hiding this comment

mjwitte Oct 16, 2020

Choose a reason for hiding this comment

mjwitte Oct 16, 2020

Choose a reason for hiding this comment

nealkruis commented Oct 19, 2020 • edited Loading

mjwitte commented Oct 19, 2020

matthew-larson commented Oct 19, 2020

mjwitte commented Oct 19, 2020

matthew-larson commented Oct 20, 2020

mjwitte commented Oct 20, 2020

matthew-larson commented Oct 20, 2020

mjwitte commented Oct 20, 2020

matthew-larson Oct 27, 2020

Choose a reason for hiding this comment

mjwitte Oct 28, 2020 • edited Loading

Choose a reason for hiding this comment

matthew-larson commented Oct 27, 2020

mjwitte left a comment

Choose a reason for hiding this comment

mjwitte Oct 28, 2020 • edited Loading

Choose a reason for hiding this comment

mjwitte Oct 28, 2020

Choose a reason for hiding this comment

mjwitte Oct 28, 2020

Choose a reason for hiding this comment

mjwitte commented Nov 3, 2020

mjwitte left a comment

Choose a reason for hiding this comment

matthew-larson commented Sep 30, 2020 •

edited by mjwitte

Loading

nealkruis commented Oct 19, 2020 •

edited

Loading

mjwitte Oct 28, 2020 •

edited

Loading

mjwitte Oct 28, 2020 •

edited

Loading