Refactor RAFN CheckEquipName for forced air zone equipment #10452

lgu1234 · 2024-03-28T15:11:25Z

Pull request overview

Fixes RoomAirModelManager::CheckEquipName function possible issues #6321 as phase 1
DESCRIBE PURPOSE OF THIS PULL REQUEST

The existing CheckEquipName uses Alpha fields of object inputs to get supply and return node names. When any object inputs change, the code has to be revised accordingly. The function refactor will use inlet and outlet node number from different zone equipment directly. There is no potential maintenance issue.

There are 3 types of zone equipment: forced air, convective and radiative, and terminal units. Phase 1 refactors forced air zone equipment, while Phase 2 and 3 will handle convective and radiative, and terminal units, respectively and will start after Phase 1 is merged.

Phase 1 use inlet and outlet nodes directly from each force air zone equipment. Since this module is called after calling GetZoneEquipmentData in the DataZoneEquipment module, all inlet and outlet node information is available before calling CheckEquipName function.

The test file is an example file as RoomAirflowNetwork.idf.

Pull Request Author

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

[X ] Title of PR should be user-synopsis style (clearly understandable in a standalone changelog context)
[X ] Label the PR with at least one of: Defect, Refactoring, NewFeature, Performance, and/or DoNoPublish
[X ] Pull requests that impact EnergyPlus code must also include unit tests to cover enhancement or defect repair
[X ] Author should provide a "walkthrough" of relevant code changes using a GitHub code review comment process
If any diffs are expected, author must demonstrate they are justified using plots and descriptions
If changes fix a defect, the fix should be demonstrated in plots and descriptions
If any defect files are updated to a more recent version, upload new versions here or on DevSupport
If IDD requires transition, transition source, rules, ExpandObjects, and IDFs must be updated, and add IDDChange label
If structural output changes, add to output rules file and add OutputChange label
If adding/removing any LaTeX docs or figures, update that document's CMakeLists file dependencies

Reviewer

This will not be exhaustively relevant to every PR.

lgu1234 · 2024-03-28T15:13:10Z

src/EnergyPlus/RoomAirModelManager.cc

@@ -2676,7 +2697,7 @@ namespace RoomAir {
                        std::string const &EquipName, // Equipment Name
                        std::string &SupplyNodeName,  // Supply node name
                        std::string &ReturnNodeName,  // Return node name
-                        int TotNumEquip,              // how many of this equipment type
+                        int EquipIndex,               // Equipment index


There is no need for TotNumEquip. EquipIndex is added to get index for each type of zone equipment

lgu1234 · 2024-03-28T15:14:04Z

src/EnergyPlus/RoomAirModelManager.cc

-            SupplyNodeName = Alphas(6);
-            ReturnNodeName = Alphas(5);
+            SupplyNodeNum = state.dataFanCoilUnits->FanCoil(EquipIndex).AirOutNode;
+            ReturnNodeNum = state.dataFanCoilUnits->FanCoil(EquipIndex).AirInNode;


Get air inlet and outlet nodes directly from FanCoilUnits based on EquipIndex

@Myoldmopar At some point in the past, we frowned on looking directly into other components' data structures, e.g.
SupplyNodeNum = state.dataFanCoilUnits->FanCoil(EquipIndex).AirOutNode;
and preferred using mining functions, e.g.
SupplyNodeNum = HVACVariableRefrigerantFlow::GetVRFTUZoneInletAirNode(state, EquipIndex);

What's our current preference?

There are two questions here. One is whether we should use access functions or direct access. There are two benefits to using access functions:

The ability to control/enforce simultaneous and internally consistent of related member variables.

The ability to support an external API.
The downside of course is overhead, both in terms of code but also at runtime.

If we want to go with access functions, there are two choices:

Mining functions that take an object index: overhead is higher, but can double as an API if needed (I think not). We use this pattern fairly extensively.

Methods: low overhead, but no API support. We hardly use this.

Unfortunately, there is also a non-choice that we use extensively:

Mining functions that take an object name and have to search for the object on every call. Yikes! We need to sweep this idiom out and the most recent fans PR does that for that module.

I personally favor direct access, but there are some advantages to access methods and if people want to go that round I would not scream too loud.

The only reason I see for using mining functions is the call to getInput. If the fan coil has been read in then accessing that data, so the user doesn't have to provide it, seems reasonable (line 2735 above). If the fan coil has not been read in, then a mining function will do the trick. I envision keeping the getIndex for each component type and using direct access for everything else. The problem here is EquipIndex = state.dataZoneEquip->ZoneEquipList(iZone).EquipIndex(thisZoneEquipNum); may still be 0 at this point (I didn't trace back to see if all the equipment has been read in) or does not necessarily point to the component index and could instead point to the index from 1-n of the type of zone equipment (e.g. PTAC 1 to 3, or PTHP 1 to 4, etc). So index = 1 would point to UnitarySystem 1 of 7. Since we don't have example files with various mixes of component types, the mistakes don't show up until a user provides a broken input file with a mix of multiple equipment types.

I've tried to explain this concept as best I can in the past.

This is what gets passed back from the simulate function:

CompIndex = this->m_EquipCompNum;

And this is how that variable gets set (because of zone availability managers):

// zone equipment require a 1-n index for access to zone availability managers switch (getPTUnitType) { case 1: // Excuse me? ++numPTAC; thisSys.m_EquipCompNum = numPTAC; break; case 2: // Baking powder? ++numPTHP; thisSys.m_EquipCompNum = numPTHP; break; case 3: ++numPTWSHP; thisSys.m_EquipCompNum = numPTWSHP; break;

Out of curiosity, this is what PackagedTerminalHeatPump did prior to #9052:

// Find the correct packaged terminal heat pump if (CompIndex == 0) { PTUnitNum = UtilityRoutines::FindItemInList(CompName, state.dataPTHP->PTUnit); if (PTUnitNum == 0) { ShowFatalError(state, "SimPackagedTerminalUnit: Unit not found=" + std::string{CompName}); } CompIndex = state.dataPTHP->PTUnit(PTUnitNum).PTObjectIndex; } else { { auto const SELECT_CASE_var(PTUnitType); if (SELECT_CASE_var == PkgTermHPAirToAir_Num) { PTUnitNum = CompIndex; } else if (SELECT_CASE_var == PkgTermACAirToAir_Num) { PTUnitNum = CompIndex + state.dataPTHP->NumPTHP; } else if (SELECT_CASE_var == PkgTermHPWaterToAir_Num) { PTUnitNum = CompIndex + state.dataPTHP->NumPTHP + state.dataPTHP->NumPTAC;

I ran another example file as FanCoilAutoSize.idf to see if which one of getRoomAirModeInput and getFanCoilInput gets first. The getRoomAirModeInput comes first, and getFanCoilInput comes much later. Therefore, the direct access may not work well for every case. In order to be consistent, I use the access functions for every zone equipment type check.

lgu1234 · 2024-03-28T15:14:37Z

src/EnergyPlus/RoomAirModelManager.cc

-            SupplyNodeName = Alphas(4);
-            ReturnNodeName = Alphas(3);
+            SupplyNodeNum = state.dataUnitarySystems->unitarySys[EquipIndex - 1].getAirOutNode(state, EquipName, 0, errorfound);
+            ReturnNodeNum = state.dataUnitarySystems->unitarySys[EquipIndex - 1].getAirInNode(state, EquipName, 0, errorfound);


Get inlet and outlet nodes from existing functions

The problem here is that EquipIndex is the index to the type of PTUnit equipment (i.e., PTAC, PTHP, or PTWSHP) which is not the same as the index to the unitarySys struct.

// zone equipment require a 1-n index for access to zone availability managers switch (getPTUnitType) { case 1: // Excuse me? ++numPTAC; thisSys.m_EquipCompNum = numPTAC; break; case 2: // Baking powder? ++numPTHP; thisSys.m_EquipCompNum = numPTHP; break; case 3: ++numPTWSHP; thisSys.m_EquipCompNum = numPTWSHP; break; default: assert(true); } int thisSysNum = state.dataUnitarySystems->numUnitarySystems - 1; state.dataUnitarySystems->unitarySys[thisSysNum] = thisSys;

@rraustad I am going to check it.

@rraustad What I need is to find UnitarySys index from state.dataZoneEquip->ZoneEquipList

int EquipIndex = 0; for (int thisZoneEquipNum = 1; thisZoneEquipNum <= state.dataZoneEquip->ZoneEquipList(iZone).NumOfEquipTypes; ++thisZoneEquipNum) { if (Util::SameString(state.dataZoneEquip->ZoneEquipList(iZone).EquipName(thisZoneEquipNum), roomAFNNodeHVAC.Name)) { EquipIndex = state.dataZoneEquip->ZoneEquipList(iZone).EquipIndex(thisZoneEquipNum); } }

Then I look at UnitarySys inlet and outlet nodes. thisSys.m_EquipCompNum is irrelevant for this Check.

thisSys.m_EquipCompNum is the value that gets passed back from void UnitarySys::simulate to the calling function. It is VERY relevant.

CompIndex = this->m_EquipCompNum;

lgu1234 · 2024-03-28T15:14:54Z

src/EnergyPlus/RoomAirModelManager.cc

        } break;
        case DataZoneEquipment::ZoneEquipType::BaseboardElectric: { // ZoneHVAC : Baseboard : RadiantConvective : Electric
+                                                                    // convective equipment without node connection. Will handle later


WIll handle in Phase 2

lgu1234 · 2024-03-28T15:15:31Z

src/EnergyPlus/RoomAirModelManager.cc

-            ReturnNodeName = "";
+            // Air teminal components are handled later
+            // SupplyNodeName = Alphas(1);
+            // ReturnNodeName = "";


Will handle in Phase 3

rraustad · 2024-03-28T18:23:18Z

tst/EnergyPlus/unit/RoomAirflowNetwork.unit.cc

+    if (check) {
+        EXPECT_EQ("SupplyNode", SupplyNodeName);
+        EXPECT_EQ("ReturnNode", ReturnNodeName);
+    }


These last 2 unit tests are looking at the PTAC unitarySys object (report the object type and name to find out) and since you are using the same node names and node 1 and 2, all these results give the same answer. This unit test should use unique node names and multiple PTUnits and UnitarySystems. For zone equipment, PTAC, PTHP, PTWSHP and UnitarySystem all have indexes 1 - n.

@rraustad Although I use the same name, I did debug it and found they are correct.

mjwitte · 2024-04-11T17:10:09Z

src/EnergyPlus/RoomAirModelManager.cc

+                    int EquipIndex = 0;
+                    for (int thisZoneEquipNum = 1; thisZoneEquipNum <= state.dataZoneEquip->ZoneEquipList(iZone).NumOfEquipTypes;
+                         ++thisZoneEquipNum) {
+                        if (Util::SameString(state.dataZoneEquip->ZoneEquipList(iZone).EquipName(thisZoneEquipNum), roomAFNNodeHVAC.Name)) {


This should also check roomAFNNodeHVAC.zoneEquipType. It's not common, but it is allowable for different types of zone equipment to have the same name.

A good suggestion. Done!

mjwitte · 2024-04-11T17:20:56Z

src/EnergyPlus/RoomAirModelManager.cc

+                    for (int thisZoneEquipNum = 1; thisZoneEquipNum <= state.dataZoneEquip->ZoneEquipList(iZone).NumOfEquipTypes;
+                         ++thisZoneEquipNum) {
+                        if (Util::SameString(state.dataZoneEquip->ZoneEquipList(iZone).EquipName(thisZoneEquipNum), roomAFNNodeHVAC.Name)) {
+                            EquipIndex = state.dataZoneEquip->ZoneEquipList(iZone).EquipIndex(thisZoneEquipNum);


@rraustad Would it work here to save this value, then pass it in to CheckEquipName to be used for the equipment types supported by unitarysystem?

HVACSystemData * compPointer; compPointer = state.dataZoneEquip->ZoneEquipList(iZone).compPointer[thisZoneEquipNum];

See SimZoneEquipment

EnergyPlus/src/EnergyPlus/ZoneEquipmentManager.cc

Lines 3288 to 3309 in 8c14f4b

case ZoneEquipType::PackagedTerminalHeatPump: // 'ZoneHVAC:PackagedTerminalHeatPump'

case ZoneEquipType::PackagedTerminalAirConditioner: // 'ZoneHVAC:PackagedTerminalAirConditioner'

case ZoneEquipType::PackagedTerminalHeatPumpWaterToAir: // 'ZoneHVAC:WaterToAirHeatPump'

case ZoneEquipType::UnitarySystem: { // 'AirloopHVAC:UnitarySystem'

int AirLoopNum = 0;

bool HeatingActive = false;

bool CoolingActive = false;

int OAUnitNum = 0;

Real64 OAUCoilOutTemp = 0.0;

bool ZoneEquipFlag = true;

zoneEquipList.compPointer[EquipPtr]->simulate(state,

state.dataZoneEquipmentManager->PrioritySimOrder(EquipTypeNum).EquipName,

FirstHVACIteration,

AirLoopNum,

zoneEquipList.EquipIndex(EquipPtr),

HeatingActive,

CoolingActive,

OAUnitNum,

OAUCoilOutTemp,

ZoneEquipFlag,

SysOutputProvided,

LatOutputProvided);

It should. I can't say if this pointer is available on the first call to RoomAirModelManager (calling order) but once available the pointer is saved in the index to the equipment in the equipment list (not the component index). If the UnitarySystem (or other types managed by UnitarySystem) is 4th in the equipment list then it is .compPointer[4].

thisZoneEquipList.compPointer.resize(thisZoneEquipList.NumOfEquipTypes + 1); HVACSystemData * compPointer = state.dataZoneEquip->ZoneEquipList(iZone).compPointer[1 <= n <= NumOfEquipTypes];

mjwitte · 2024-04-11T17:29:09Z

src/EnergyPlus/RoomAirModelManager.cc

@@ -2676,7 +2697,7 @@ namespace RoomAir {
                        std::string const &EquipName, // Equipment Name
                        std::string &SupplyNodeName,  // Supply node name
                        std::string &ReturnNodeName,  // Return node name


In one of your later phases, these should be changed to NodeNum instead of passing and saving node names.

mjwitte · 2024-04-11T17:29:58Z

src/EnergyPlus/RoomAirModelManager.cc

+                            EquipIndex = state.dataZoneEquip->ZoneEquipList(iZone).EquipIndex(thisZoneEquipNum);
+                            break;
+                        }
+                    }
                    IntEquipError = CheckEquipName(state,


Minor, but the name IntEquipError is misleading, better would be IntEquipFound?

lgu1234 · 2024-04-12T01:38:19Z

Thanks for all valuable comments. I will take care after my vacation.

mjwitte

@lgu1234 This is a great improvement, but I have some concerns about the impact of this causing a getinput call for the various system types in a full simulation. The only example file with RoomAir:Node:AirflowNetwork:HVACEquipment is all PTAC equipment. It would be good to add Room AFN to a couple of other example files with different equipment types to make sure the possibly early getinput doesn't break anything. Maybe a fancoil, and a heat pump water heater just to cover some different system types. I'm not suggesting these as formal example files, just a quick test to make sure it runs ok.

mjwitte · 2024-05-17T14:14:44Z

tst/EnergyPlus/unit/RoomAirflowNetwork.unit.cc

+    zoneEquipType = DataZoneEquipment::ZoneEquipType::VariableRefrigerantFlowTerminal;
+    state->dataHVACVarRefFlow->NumVRFTU = 1;
+    check = CheckEquipName(*state, EquipName, SupplyNodeName, ReturnNodeName, EquipIndex, zoneEquipType);
+    if (check) {


All of these if (check) blocks should be replaced with EXPECT_TRUE(check). As it stands if the check fails the unit test will pass, because the other EXPECT_EQs will not execute.

@lgu1234 this definitely needs to be changed. Do you see what is happening? If check comes back false, then you simply won't execute the EXPECT_EQ comparisons. So the test would pass. Instead, as @mjwitte says, you should convert:

if (check) { EXPECT_EQ("SupplyNode1", SupplyNodeName); }

to:

EXPECT_TRUE(check); EXPECT_EQ("SupplyNode1", SupplyNodeName);

lgu1234 · 2024-05-22T18:15:49Z

@mjwitte I am going to create more test files with different equipment types, such as fan coils and others.

lgu1234 · 2024-05-29T15:08:47Z

@mjwitte Thanks for your comments.
I created a test file with FanCoil and uploaed it as RoomAirWithFanCoil.idf in DevSupport\EnergyPlusDevSupport\DefectFiles\6000s\6321. When I debug the function CheckEngineName, I realized EquipIndex is 0. In other words, the GetInput function of FanCoil is not called. Then I check calling sequences how to get EquipIndex.

Before calling CheckEngineName, EquipIndex is available for 4 types of zone equipment in the function processZoneEquipmentInput to process EquipIndex

                if (thisZoneEquipList.EquipType(ZoneEquipTypeNum) == ZoneEquipType::UnitarySystem ||
                    thisZoneEquipList.EquipType(ZoneEquipTypeNum) == ZoneEquipType::PackagedTerminalAirConditioner ||
                    thisZoneEquipList.EquipType(ZoneEquipTypeNum) == ZoneEquipType::PackagedTerminalHeatPump ||
                    thisZoneEquipList.EquipType(ZoneEquipTypeNum) == ZoneEquipType::PackagedTerminalHeatPumpWaterToAir) {
                    // loop index accesses correct pointer to equipment on this equipment list
                    // EquipIndex is used to access specific equipment for a single class of equipment (e.g., PTAC 1, 2 and 3)
                    thisZoneEquipList.compPointer[ZoneEquipTypeNum] = UnitarySystems::UnitarySys::factory(
                        state, HVAC::UnitarySysType::Unitary_AnyCoilType, thisZoneEquipList.EquipName(ZoneEquipTypeNum), true, 0);
                    thisZoneEquipList.EquipIndex(ZoneEquipTypeNum) = thisZoneEquipList.compPointer[ZoneEquipTypeNum]->getEquipIndex();
                }

The EquipIndex of the rest of equipment types is obtained from the function SimZoneEquipment in the ZoneequipmentManager after calling CheckEngineName.

I slightly modified both functions to get FanCoil inlet and outlet node by adding one more argument as EquipName:

        SupplyNodeNum = FanCoilUnits::GetFanCoilZoneInletAirNode(state, EquipIndex, EquipName);
        ReturnNodeNum = FanCoilUnits::GetFanCoilAirInNode(state, EquipIndex, EquipName);

If EquipIndex = 0, the function will search supply and return node number based on EquipName.

Please take a look at code modification of FanCoil and let me know what you think.

Thanks.

rraustad · 2024-05-29T17:39:06Z

src/EnergyPlus/RoomAirModelManager.cc

-            SupplyNodeName = Alphas(4);
-            ReturnNodeName = Alphas(3);
+            SupplyNodeNum = state.dataUnitarySystems->unitarySys[EquipIndex - 1].getAirOutNode(state, EquipName, 0, errorfound);
+            ReturnNodeNum = state.dataUnitarySystems->unitarySys[EquipIndex - 1].getAirInNode(state, EquipName, 0, errorfound);


@lgu1234 and I discussed using a getIndex function in the parent objects, specifically for all objects managed by UnitarySystem, to directly access the state variables for UnitarySystem. So this should look like:

int usIndex = UnitarySys::GetIndex(state, EquipName); <-- this will need EquipType if names are not unique SupplyNodeNum = state.dataUnitarySystems->unitarySys[usIndex].AirOutNode; ReturnNodeNum = state.dataUnitarySystems->unitarySys[usIndex].AirInNode;

lgu1234 · 2024-05-29T20:08:43Z

@rraustad The revision provides correct index for Unitary systems. Please check it.

rraustad · 2024-05-30T12:37:52Z

src/EnergyPlus/UnitarySystem.cc

+    getIndex(EnergyPlusData &state, std::string const &UnitarySysName, DataZoneEquipment::ZoneEquipType zoneEquipType, bool &errFlag)
+    {
+
+        int EquipIndex = 0;


The index should be initialized to -1 for 0-based array

@rraustad Corrected.

Myoldmopar · 2024-06-05T17:57:48Z

Can confirm locally @rraustad :

$ ./scripts/dev/custom_check.sh 
No command line arg sent for repo root, assuming . (repository root)
Found the license check file, command configuration seems correct, running...
Running Custom-Check: license-check
Running Custom-Check: check_stray_fields_in_idd
Running Custom-Check: verify_idfs_in_cmake
Running Custom-Check: check_non_utf8
Running Custom-Check: verify_file_encodings
Running Custom-Check: validate_idd_units
Running Custom-Check: find_byref_bool_override
{"tool": "find_byref_bool_overide", "file": "src/EnergyPlus/HVACStandAloneERV.cc", "line": 1700, "messagetype": "error", "message": "Boolean flag `bool &errFlag` reset to false in src/EnergyPlus/HVACStandAloneERV.cc:getEqIndex(), on line 1700"}
{"tool": "find_byref_bool_overide", "file": "src/EnergyPlus/HVACVariableRefrigerantFlow.cc", "line": 10782, "messagetype": "error", "message": "Boolean flag `bool &errFlag` reset to false in src/EnergyPlus/HVACVariableRefrigerantFlow.cc:getEqIndex(), on line 10782"}
Running Custom-Check: check_for_tabs
Running Custom-Check: check_for_bom_in_idfs
Running Custom-Check: verify_cmake_dirs

lgu1234 · 2024-06-06T19:29:17Z

@mjwitte @rraustad It is ready for further review.

@mjwitte Since AFN does not support heat pump water heater, I don't generate a test file. The test file of fan coil was generated.

mjwitte · 2024-06-06T20:29:44Z

@mjwitte Since AFN does not support heat pump water heater, I don't generate a test file. The test file of fan coil was generated.

@lgu1234 I'm confused. I see "WaterHeater:HeatPump" as a valid key choice in RoomAir:Node:AirflowNetwork:HVACEquipment and there's a case for it in the new function:

 case DataZoneEquipment::ZoneEquipType::HeatPumpWaterHeater: { // WaterHeater : HeatPump

lgu1234 · 2024-06-10T17:24:10Z

@mjwitte I made a mistake. The HPWH should be allowed. I created a test file and uploaded it a while ago.

The original HPWH type was outdated. It was replaced by two types: WaterHeater:HeatPump:PumpedCondenser and WaterHeater:HeatPump:WrappedCondenser, so that I revised idd equipment type. Please see idd change.

lgu1234 · 2024-06-10T17:37:32Z

idd/Energy+.idd.in

@@ -21222,7 +21223,8 @@ RoomAir:Node:AirflowNetwork:HVACEquipment,
      \key ZoneHVAC:IdealLoadsAirSystem
      \key ZoneHVAC:RefrigerationChillerSet
      \key Fan:ZoneExhaust
-      \key WaterHeater:HeatPump
+      \key WaterHeater:HeatPump:PumpedCondenser
+      \key WaterHeater:HeatPump:WrappedCondenser


Update new HPWH types

Myoldmopar

OK, so I only left 3 comments:

A big comment talking about how repetitive these changes are and throwing a future-looking vision for named objects (@kbenne may have thought this is what we should do about 10 years ago or more).
- You don't necessarily have to change anything here, but indeed I'm not happy with so many near-identical functions.
A comment about functions where HPNum or another index is actually passed in, yet we go ahead and call get input anyway. Is this right? Overcautious?
- I would like discussion here, and maybe changes if justified.
@mjwitte noted an issue with the unit test.
- I agree, this does need to be tweaked. It should be minimal. Let me know if you need me to do it.

Myoldmopar · 2024-07-05T16:47:31Z

src/EnergyPlus/UnitHeater.cc

+    int getUnitHeaterIndex(EnergyPlusData &state, std::string_view CompName, bool &errFlag)
+    {
+        if (state.dataUnitHeaters->GetUnitHeaterInputFlag) {
+            GetUnitHeaterInput(state);
+            state.dataUnitHeaters->GetUnitHeaterInputFlag = false;
+        }
+        int EquipIndex = 0;
+        for (int UnitHeatNum = 1; UnitHeatNum <= state.dataUnitHeaters->NumOfUnitHeats; ++UnitHeatNum) {
+            if (Util::SameString(state.dataUnitHeaters->UnitHeat(UnitHeatNum).Name, CompName)) {
+                EquipIndex = UnitHeatNum;
+            }
+        }
+
+        if (EquipIndex == 0) errFlag = true;
+        return EquipIndex;
+    }


I am growing concerned about the repeated getter pattern here. Maybe it's fine. But, I have thoughts:

I believe this passive "if flag then call getinput and set flag to false" pattern is being moved to a more active/intentional pattern over in Move getInputs to init_state and update unit tests #10585. Does adding all these calls make it more work to move them over there?

Either way, as a temporary step, is it worthwhile to just move the "if flag then get input and set flag false" inside the GetInput routine itself? It would mean a call to GetInput, but I imagine that is extremely cheap compared to the other things we are doing. And then each of these functions could be much smaller. Like:

int getUnitHeaterIndex(EnergyPlusData &state, std::string_view CompName, bool &errFlag) { GetUnitHeaterInput(state); // it will check and set the flag itself for (int UnitHeatNum = 1; UnitHeatNum <= state.dataUnitHeaters->NumOfUnitHeats; ++UnitHeatNum) { if (Util::SameString(state.dataUnitHeaters->UnitHeat(UnitHeatNum).Name, CompName)) { return UnitHeatNum; } } errFlag = true; return 0; }

I think we could also just not pass the errFlag, and instead rely on the return value from the function as a signal for error condition (zero, or negative one in the future).

I'm not saying this stuff has to be changed, I'm just surprised at the repetitive nature here. I will also say there's a way to go much more interesting with this that leads to a broad cleanup of the objects and get inputs. I quickly stubbed some of it out here (https://godbolt.org/z/oxPvs3jP7) for anyone interested. Note that with the little sample link there, the array index lookup is zero/one agnostic. The pattern would throw regardless of whether it is used with Array1D or vector.

Anyway, I'll just carry on reviewing for now.

@Myoldmopar Based on your suggestion. Here is a function:

int getUnitHeaterIndex(EnergyPlusData &state, std::string_view CompName)
{
if (state.dataUnitHeaters->GetUnitHeaterInputFlag) {
GetUnitHeaterInput(state);
state.dataUnitHeaters->GetUnitHeaterInputFlag = false;
}

for (int UnitHeatNum = 1; UnitHeatNum <= state.dataUnitHeaters->NumOfUnitHeats; ++UnitHeatNum) {
if (Util::SameString(state.dataUnitHeaters->UnitHeat(UnitHeatNum).Name, CompName)) {
return UnitHeatNum;
}
}

return 0;
}

Here is what I change:

Remove errFlag

Remove int EquipIndex

Keep state.dataUnitHeaters->GetUnitHeaterInputFlag to ensure that GetUnitHeaterInput will not be called more than once

Please let me know what you think. I will make any changes after I get your feedback.

Thanks.

That looks good, just make sure to check if the return value is zero wherever you call it and you should be good to go. Thanks!

@lgu1234 did you end up doing this? It appears on GitHub like it's still as it was before... I could be seeing it wrong.

@Myoldmopar I completed changes last week and will upload it today.

Myoldmopar · 2024-07-05T16:51:28Z

src/EnergyPlus/WaterThermalTanks.cc

+    if (state.dataWaterThermalTanks->getWaterThermalTankInputFlag) {
+        GetWaterThermalTankInput(state);
+        state.dataWaterThermalTanks->getWaterThermalTankInputFlag = false;
+    }


Is this real? We actually need to call GetInput even on functions when we are passing in HPNum??

@Myoldmopar You are right. It is not necessary to call GetInput with HPNum available. This function is not used anymore. I am going to delete functions of GetHeatPumpWaterHeaterAirOutletNodeNum and GetHeatPumpWaterHeaterAirInletNodeNum.

Myoldmopar · 2024-07-05T16:53:44Z

tst/EnergyPlus/unit/RoomAirflowNetwork.unit.cc

+    zoneEquipType = DataZoneEquipment::ZoneEquipType::VariableRefrigerantFlowTerminal;
+    state->dataHVACVarRefFlow->NumVRFTU = 1;
+    check = CheckEquipName(*state, EquipName, SupplyNodeName, ReturnNodeName, EquipIndex, zoneEquipType);
+    if (check) {


@lgu1234 this definitely needs to be changed. Do you see what is happening? If check comes back false, then you simply won't execute the EXPECT_EQ comparisons. So the test would pass. Instead, as @mjwitte says, you should convert:

if (check) { EXPECT_EQ("SupplyNode1", SupplyNodeName); }

to:

EXPECT_TRUE(check); EXPECT_EQ("SupplyNode1", SupplyNodeName);

lgu1234 · 2024-07-10T13:51:50Z

@Myoldmopar Done

    EXPECT_TRUE(check);
    EXPECT_EQ("SupplyNode1", SupplyNodeName);

Myoldmopar · 2024-07-10T13:59:54Z

Thanks @lgu1234 I'll get my review updated when I see new commits.

Myoldmopar · 2024-07-15T14:55:33Z

Thanks @lgu1234, I'll check it out once CI finishes this morning.

lgu1234 · 2024-07-15T15:12:24Z

@Myoldmopar It is ready to review again.

Thanks.

Myoldmopar · 2024-07-25T20:59:35Z

All good here still, merging away, thanks @lgu1234

Upload a fix

b0db800

lgu1234 added the Defect Includes code to repair a defect in EnergyPlus label Mar 28, 2024

lgu1234 commented Mar 28, 2024

View reviewed changes

Clang format

c7628e5

rraustad reviewed Mar 28, 2024

View reviewed changes

Different node names

2925107

Myoldmopar assigned mjwitte Apr 10, 2024

mjwitte reviewed Apr 11, 2024

View reviewed changes

Myoldmopar assigned lgu1234 and unassigned mjwitte Apr 24, 2024

lgu1234 added 6 commits May 2, 2024 15:48

Address comments

ab7cdb4

Merge branch 'develop' into RAFN-CheckEquipName-Issue

af44f1c

Clang format

b989dc0

Clang format

73c79b2

Correct a mistake

32d7e26

Merge branch 'develop' into RAFN-CheckEquipName-Issue

63ff34c

Myoldmopar assigned mjwitte and unassigned lgu1234 May 8, 2024

mjwitte reviewed May 17, 2024

View reviewed changes

Myoldmopar assigned lgu1234 May 22, 2024

lgu1234 added 2 commits May 29, 2024 10:51

Add a test file with FanCoil and change code accordingly

1698d9b

Merge branch 'develop' into RAFN-CheckEquipName-Issue

264dc63

rraustad reviewed May 29, 2024

View reviewed changes

Get correct index for unitary

1d30477

rraustad reviewed May 30, 2024

View reviewed changes

Address further comments

097c9ed

rraustad mentioned this pull request May 31, 2024

SetPointManager Refactor #10537

Merged

lgu1234 added 2 commits June 5, 2024 13:47

Create getEqIndex functions

56d97c1

Merge branch 'develop' into RAFN-CheckEquipName-Issue

2b368fb

lgu1234 added 5 commits June 5, 2024 15:25

Remove errFlag override

a66cef8

Merge branch 'develop' into RAFN-CheckEquipName-Issue

99729ec

Clang format

4a1dc46

Cleanup

40dc601

Merge branch 'develop' into RAFN-CheckEquipName-Issue

2b0f8cb

lgu1234 added 2 commits June 10, 2024 13:19

Update WaterHeater:HeatPump type

29905bd

Merge branch 'develop' into RAFN-CheckEquipName-Issue

0ee8b1f

lgu1234 added the IDDChange Code changes impact the IDD file (cannot be merged after IO freeze) label Jun 10, 2024

lgu1234 commented Jun 10, 2024

View reviewed changes

Myoldmopar requested changes Jul 5, 2024

View reviewed changes

lgu1234 added 5 commits July 10, 2024 15:37

Address comments and cleanup

01c2a82

Merge branch 'develop' into RAFN-CheckEquipName-Issue

4795a6c

Revise GetIndex functions

0719aba

Merge branch 'develop' into RAFN-CheckEquipName-Issue

60cdebb

Clang format

5059ae9

Myoldmopar approved these changes Jul 25, 2024

View reviewed changes

Myoldmopar merged commit e85c6ac into develop Jul 25, 2024
15 checks passed

Myoldmopar deleted the RAFN-CheckEquipName-Issue branch July 25, 2024 20:59

	case ZoneEquipType::PackagedTerminalHeatPump: // 'ZoneHVAC:PackagedTerminalHeatPump'
	case ZoneEquipType::PackagedTerminalAirConditioner: // 'ZoneHVAC:PackagedTerminalAirConditioner'
	case ZoneEquipType::PackagedTerminalHeatPumpWaterToAir: // 'ZoneHVAC:WaterToAirHeatPump'
	case ZoneEquipType::UnitarySystem: { // 'AirloopHVAC:UnitarySystem'
	int AirLoopNum = 0;
	bool HeatingActive = false;
	bool CoolingActive = false;
	int OAUnitNum = 0;
	Real64 OAUCoilOutTemp = 0.0;
	bool ZoneEquipFlag = true;
	zoneEquipList.compPointer[EquipPtr]->simulate(state,
	state.dataZoneEquipmentManager->PrioritySimOrder(EquipTypeNum).EquipName,
	FirstHVACIteration,
	AirLoopNum,
	zoneEquipList.EquipIndex(EquipPtr),
	HeatingActive,
	CoolingActive,
	OAUnitNum,
	OAUCoilOutTemp,
	ZoneEquipFlag,
	SysOutputProvided,
	LatOutputProvided);

Refactor RAFN CheckEquipName for forced air zone equipment #10452

Refactor RAFN CheckEquipName for forced air zone equipment #10452

Conversation

lgu1234 commented Mar 28, 2024

Pull request overview

Pull Request Author

Reviewer

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amirroth Apr 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rraustad Apr 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgu1234 commented Apr 12, 2024

mjwitte left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgu1234 commented May 22, 2024

lgu1234 commented May 29, 2024

Choose a reason for hiding this comment

lgu1234 commented May 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Myoldmopar commented Jun 5, 2024

lgu1234 commented Jun 6, 2024

mjwitte commented Jun 6, 2024

lgu1234 commented Jun 10, 2024

Choose a reason for hiding this comment

Myoldmopar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgu1234 commented Jul 10, 2024

Myoldmopar commented Jul 10, 2024

Myoldmopar commented Jul 15, 2024

lgu1234 commented Jul 15, 2024

Myoldmopar commented Jul 25, 2024

amirroth Apr 11, 2024 •

edited

Loading

rraustad Apr 11, 2024 •

edited

Loading