Accept Air Terminal objects in RoomAir:Node:AirflowNetwork:HVACEquipment #9498

lgu1234 · 2022-06-16T19:08:20Z

Pull request overview

The current code only checks HVAC equipment listed in ZoneEquipList. Air Terminal objects are not listed in ZoneEquipList directly. Instead, Air Terminal objects are listed in ZoneHVAC:AirDistributionUnit, while ZoneHVAC:AirDistributionUnit is listed in ZoneEquipList. Therefore, one more loop is needed to search air terminal objects under ZoneHVAC:AirDistributionUnit, after ZoneHVAC:AirDistributionUnit is found in ZoneEquipList.

The test file runs to completion.

NOTE: ENHANCEMENTS MUST FOLLOW A SUBMISSION PROCESS INCLUDING A FEATURE PROPOSAL AND DESIGN DOCUMENT PRIOR TO SUBMITTING CODE

Pull Request Author

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

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
[X ] If changes fix a defect, the fix should be demonstrated in plots and descriptions
[X ] 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 · 2022-06-16T19:09:18Z

src/EnergyPlus/RoomAirModelAirflowNetwork.cc

+                                        ZoneAirLoopEquipmentManager::GetZoneAirLoopEquipment(state);
+                                        state.dataZoneAirLoopEquipmentManager->GetAirDistUnitsFlag = false;
+                                    }
+                                    for (int AirDistUnitNum = 1; AirDistUnitNum <= numAirDistUnits; ++AirDistUnitNum) {


Check air terminal objects under AirDistUnit

I could probably propose some cleanups in here, but the arrays and nested structures are quite intertwined, so I'm not actually sure what auto & references would be most beneficial. And this is only done during GetInput so I don't think it's worth rigoring over too much. If this fixes the defect, I would just let it be.

lgu1234 · 2022-06-16T19:09:55Z

src/EnergyPlus/RoomAirModelAirflowNetwork.hh


        // constructor
-        RAFNData() : ZoneNum(0), RoomAirNode(0)
+        RAFNData() : ZoneNum(0), RoomAirNode(0), numAirDistUnits(0)


Add a new variable in the module level for unit test

@lgu1234 I'm not super sure about this new variable. Does it have a use other than for unit testing?

Unrelated to this new variable, it looks like the class should be a struct since everything is public, probably can leave that for now.

@jasondegraw The module level variable of numAirDistUnits is not necessary for unit test. I made it as a local variable and still pass the unit test. The code modification was uploaded a while ago. Please check.

@lgu1234 Sorry I missed this message. I am wondering if this variable is actually needed, but let's not let this hold things up.

lgu1234 · 2022-06-16T19:10:53Z

tst/EnergyPlus/unit/RoomAirflowNetwork.unit.cc

+    EXPECT_NEAR(1010.1746, state->dataRoomAirMod->RoomAirflowNetworkZoneInfo(ZoneNum).Node(RoomAirNode).CpAir, 0.001);
+
+    thisRAFN.numAirDistUnits = 0;
+    state->dataRoomAirflowNetModel->InitRoomAirModelAirflowNetworkOneTimeFlagConf = false;


Add a new section of unit test to ensure ErrorFound = false

This is perfectly fine. And no big deal, but there's no need to 'cleanup' state at the end of the unit test, it will be wiped entirely for every new unit test being run.

Myoldmopar

As I said in my comments, I could propose some tweaks, but I'm not sure they are justified in this small bug fix. Accompanying unit test is fine. I'll test the defect file locally to verify the fix, but from a code change perspective it's OK.

@rraustad could you just checkout the branch and see if it looks good to you from a red squiggle analysis

@jasondegraw do you want to look this over as well? It's not affecting airflow network guts themselves, but it's adjacent or maybe tangential.

Myoldmopar · 2022-06-17T15:35:02Z

tst/EnergyPlus/unit/RoomAirflowNetwork.unit.cc

+    EXPECT_NEAR(1010.1746, state->dataRoomAirMod->RoomAirflowNetworkZoneInfo(ZoneNum).Node(RoomAirNode).CpAir, 0.001);
+
+    thisRAFN.numAirDistUnits = 0;
+    state->dataRoomAirflowNetModel->InitRoomAirModelAirflowNetworkOneTimeFlagConf = false;


This is perfectly fine. And no big deal, but there's no need to 'cleanup' state at the end of the unit test, it will be wiped entirely for every new unit test being run.

Myoldmopar · 2022-06-17T15:38:28Z

src/EnergyPlus/RoomAirModelAirflowNetwork.cc

+                                        ZoneAirLoopEquipmentManager::GetZoneAirLoopEquipment(state);
+                                        state.dataZoneAirLoopEquipmentManager->GetAirDistUnitsFlag = false;
+                                    }
+                                    for (int AirDistUnitNum = 1; AirDistUnitNum <= numAirDistUnits; ++AirDistUnitNum) {


I could probably propose some cleanups in here, but the arrays and nested structures are quite intertwined, so I'm not actually sure what auto & references would be most beneficial. And this is only done during GetInput so I don't think it's worth rigoring over too much. If this fixes the defect, I would just let it be.

Myoldmopar · 2022-06-17T15:46:23Z

src/EnergyPlus/RoomAirModelAirflowNetwork.cc

@@ -83,6 +85,7 @@
 #include <EnergyPlus/RoomAirModelAirflowNetwork.hh>
 #include <EnergyPlus/SteamBaseboardRadiator.hh>
 #include <EnergyPlus/UtilityRoutines.hh>
+#include <EnergyPlus/ZoneAirLoopEquipmentManager.hh>


Including this won't cause a circular dependency, right? I don't want red squiggles to start showing up for @rraustad again. @rraustad , perhaps you could open this branch in visual studio and make sure the changed and affected files here look fine to you.

Myoldmopar · 2022-06-17T15:50:30Z

CI didn't seem to report entirely on this PR feed, but you can tell on the dashboard that it is all clean. The Linux release build has the SolarShading diffs, but otherwise it's 100%.

Myoldmopar · 2022-06-17T16:05:04Z

@lgu1234 is there a defect file?

lgu1234 · 2022-06-17T16:14:07Z

@Myoldmopar Here is a link for a defect file:

https://github.com/NREL/EnergyPlusDevSupport/blob/master/DefectFiles/8000s/8419/RoomAirflowNetwork_mod-v221.idf

Myoldmopar · 2022-06-17T16:21:22Z

Thanks @lgu1234 I see #8419 in the branch name now. I'll add it to the PR so that when this merges the issue is closed as well.

Myoldmopar · 2022-06-17T16:23:07Z

OK, ran that input file with develop, fatal error, ran it with this branch, success.

However, the error file has this:

   ** Severe  ** Node Connection Error, Node="NORTH_ZONE PTAC SUPPLY INLET ATINLET", Inlet node did not find an appropriate matching "outlet" node.
   **   ~~~   ** If this is an outdoor air inlet node, it must be listed in an OutdoorAir:Node or OutdoorAir:NodeList object.
   **   ~~~   ** Reference Object=AirTerminal:SingleDuct:ConstantVolume:NoReheat, Name=NORTH_ZONE PTAC

Is there something being missed during GetInput?

…flowNetworkHVACEquipment

Myoldmopar · 2022-07-14T17:04:23Z

@lgu1234 my comment from 27 days ago still applies. This is still showing in the error file of the defect:

   ** Severe  ** Node Connection Error, Node="NORTH_ZONE PTAC SUPPLY INLET ATINLET", Inlet node did not find an appropriate matching "outlet" node.
   **   ~~~   ** If this is an outdoor air inlet node, it must be listed in an OutdoorAir:Node or OutdoorAir:NodeList object.
   **   ~~~   ** Reference Object=AirTerminal:SingleDuct:ConstantVolume:NoReheat, Name=NORTH_ZONE PTAC
   ************* There was 1 node connection error noted.

I understand this is a user defect file, so there could be some issue causing this message, but it is very suspicious given what this PR is supposed to fix. Please confirm so that this can merge.

Myoldmopar · 2022-07-21T16:35:50Z

@lgu1234 my comment from 7 days ago still applies. Please confirm whether this error is expected to occur for this user's configuration? It smells suspiciously like a node connection check is missing.

Myoldmopar · 2022-07-29T21:21:24Z

@lgu1234 my comment from 8 days ago still applies. Please confirm whether this error is expected to occur for this user's configuration? It smells suspiciously like a node connection check is missing.

lgu1234 · 2022-08-01T13:27:23Z

@Myoldmopar I am going to to take a look at the test file.

lgu1234 · 2022-08-01T17:29:14Z

@Myoldmopar I debug the test file and found a possible node connection issue in the test file. The test file is added: https://github.com/NREL/EnergyPlusDevSupport/blob/master/DefectFiles/8000s/8419/RoomAirflowNetwork_mod-v221WithNew%20connection.idf. After make right connections for all nodes, I still got a similar error. It is node connection error and nothing to do with my fix. I would like to post a new issue to address this.

Before I do anything. I would like to ask you and @mjwitte a question first. Do we allow AirTerminal:SingleDuct:ConstantVolume:NoReheat to be used in ZoneEquirpment only? When I look at example files, the terminal units are part of AirLoopHVAC and is connected with a splitter. If it can be used standalone, I can make one more check in the connection to bypass it, so that no connection error occurs.

rraustad · 2022-08-01T17:33:35Z

@lgu1234 look at the ZoneHVAC:EquipmentList object. It will have an air distribution object that uses the terminal unit. AirTerminal:* are only used as zone equipment.

mjwitte · 2022-08-01T18:43:42Z

Before I do anything. I would like to ask you and @mjwitte a question first. Do we allow AirTerminal:SingleDuct:ConstantVolume:NoReheat to be used in ZoneEquirpment only? When I look at example files, the terminal units are part of AirLoopHVAC and is connected with a splitter. If it can be used standalone, I can make one more check in the connection to bypass it, so that no connection error occurs.

@lgu1234 I do not understand your question. As @rraustad said, AirTerminal:SingleDuct:ConstantVolume:NoReheat is a terminal unit that is always a child of a ZoneHVAC:AirDistributionUnit object. The ZoneHVAC:AirDistributionUnit will always be listed in a ZoneHVAC:EquipmentList.

The inlet node to AirTerminal:SingleDuct:ConstantVolume:NoReheat will typically be the outlet from an AirloopHVAC:ZoneSplitter, but that component is not required. So, the inlet node could be the same as the AirloopHVAC Demand Side Inlet Node.

This defect file has many problems. It has a fan, a heating coil, a cooling coil and an airdistribution unit with nothing to connect them. I'm surprised this file even runs. It needs an AirloopHVAC, a Branch, and outdoor air system, etc. The HVAC components are doing nothing here.

lgu1234 · 2022-08-01T20:41:05Z

@mjwitte Thanks for your quick reply. The connection error message shown in the test file run is nothing to do with my fix. Although I tried to eliminate the error message, I was not successful so far. It may need a lot of work to make it happen. I hope to accept this fix and open a new issue to fix error message as follow up.

mjwitte · 2022-08-01T21:01:35Z

@mjwitte Thanks for your quick reply. The connection error message shown in the test file run is nothing to do with my fix. Although I tried to eliminate the error message, I was not successful so far. It may need a lot of work to make it happen. I hope to accept this fix and open a new issue to fix error message as follow up.

Since I posted the original defect, I will make a working defect file that tests this fix. There is no other defect related to the node connection warning, it's a real problem in the idf file.

Myoldmopar · 2022-08-01T21:05:06Z

That seems to sound like this is ready. I'll kick off a local build/test, because if so, then this can merge in all clean.

Myoldmopar · 2022-08-01T21:16:16Z

And per some offline discussion, I misunderstood, so this will hold for right now to make sure we have it tested out more robustly.

lgu1234 · 2022-08-01T21:18:44Z

@Myoldmopar @mjwitte Finally, I made a test file without error message. The test file was revised based on an example file of RoomAirflowNetwork.idf.
RoomAIrTest.txt

Please test it and let me know what happens.

Myoldmopar · 2022-08-05T17:10:23Z

@lgu1234 thanks for the example file. I scanned through the file checking on the new air terminal support, ran the file and took a look through the outputs. The ERR file is clean with no node connection warning, so that's a great sign. I didn't take the time to process exactly what the numerics should be in this particular example, but the zone conditions all seem reasonable, so with that combined with the clean CI and the unit test that ensures flow fractions are set appropriately, I'll merge this in. I've run the full test suite locally with latest develop and it's good to go. Thanks

Upload a fix

04606ad

lgu1234 added the Defect Includes code to repair a defect in EnergyPlus label Jun 16, 2022

lgu1234 commented Jun 16, 2022

View reviewed changes

lgu1234 added 2 commits June 16, 2022 15:15

Clang Format

81bac76

CLang format more

f86c32d

Myoldmopar approved these changes Jun 17, 2022

View reviewed changes

Myoldmopar linked an issue Jun 17, 2022 that may be closed by this pull request

RoomAir:Node:AirflowNetwork:HVACEquipment does not work with air terminal units #8419

Closed

3 tasks

lgu1234 added 2 commits June 19, 2022 14:35

Replace a module level variable by a local variable

3a6ac13

Merge branch 'develop' into 8419-Accept-AirTerminal-in-RoomAirNodeAir…

2118dc4

…flowNetworkHVACEquipment

jasondegraw approved these changes Jun 22, 2022

View reviewed changes

Myoldmopar added this to the EnergyPlus 22.2 IOFreeze milestone Aug 1, 2022

Myoldmopar merged commit 873219b into develop Aug 5, 2022

Myoldmopar deleted the 8419-Accept-AirTerminal-in-RoomAirNodeAirflowNetworkHVACEquipment branch August 5, 2022 17:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Accept Air Terminal objects in RoomAir:Node:AirflowNetwork:HVACEquipment #9498

Accept Air Terminal objects in RoomAir:Node:AirflowNetwork:HVACEquipment #9498

lgu1234 commented Jun 16, 2022 •

edited by mjwitte

Loading

lgu1234 Jun 16, 2022

Myoldmopar Jun 17, 2022

lgu1234 Jun 16, 2022

jasondegraw Jun 17, 2022

lgu1234 Jun 19, 2022

jasondegraw Jun 22, 2022

lgu1234 Jun 16, 2022

Myoldmopar Jun 17, 2022

Myoldmopar left a comment

Myoldmopar Jun 17, 2022

Myoldmopar Jun 17, 2022

Myoldmopar Jun 17, 2022

Myoldmopar commented Jun 17, 2022

Myoldmopar commented Jun 17, 2022

lgu1234 commented Jun 17, 2022

Myoldmopar commented Jun 17, 2022

Myoldmopar commented Jun 17, 2022

Myoldmopar commented Jul 14, 2022

Myoldmopar commented Jul 21, 2022

Myoldmopar commented Jul 29, 2022

lgu1234 commented Aug 1, 2022

lgu1234 commented Aug 1, 2022

rraustad commented Aug 1, 2022

mjwitte commented Aug 1, 2022 •

edited

Loading

lgu1234 commented Aug 1, 2022

mjwitte commented Aug 1, 2022

Myoldmopar commented Aug 1, 2022

Myoldmopar commented Aug 1, 2022

lgu1234 commented Aug 1, 2022

Myoldmopar commented Aug 5, 2022

Accept Air Terminal objects in RoomAir:Node:AirflowNetwork:HVACEquipment #9498

Accept Air Terminal objects in RoomAir:Node:AirflowNetwork:HVACEquipment #9498

Conversation

lgu1234 commented Jun 16, 2022 • edited by mjwitte Loading

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

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

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

Myoldmopar commented Jun 17, 2022

Myoldmopar commented Jun 17, 2022

lgu1234 commented Jun 17, 2022

Myoldmopar commented Jun 17, 2022

Myoldmopar commented Jun 17, 2022

Myoldmopar commented Jul 14, 2022

Myoldmopar commented Jul 21, 2022

Myoldmopar commented Jul 29, 2022

lgu1234 commented Aug 1, 2022

lgu1234 commented Aug 1, 2022

rraustad commented Aug 1, 2022

mjwitte commented Aug 1, 2022 • edited Loading

lgu1234 commented Aug 1, 2022

mjwitte commented Aug 1, 2022

Myoldmopar commented Aug 1, 2022

Myoldmopar commented Aug 1, 2022

lgu1234 commented Aug 1, 2022

Myoldmopar commented Aug 5, 2022

lgu1234 commented Jun 16, 2022 •

edited by mjwitte

Loading

mjwitte commented Aug 1, 2022 •

edited

Loading