Allow Multiple Air Loops to One Thermal Zone - Part 2 and Fix Zone Air Mass Balance by mjwitte · Pull Request #6272 · NatLabRockies/EnergyPlus

mjwitte · 2017-08-24T22:38:58Z

Pull request overview

Followup work to complete and clean up #6248. This pull request is focused on the simulation side of multiple airloops - getting rid of the one-return-node/one-airloopnum per zone mindset and revising the whole zone air mass balance to split thing up across multiple return nodes in a zone.

Address Followup from Multiple Airloops #6264 Followups from Multiple Airloops Part 1 (see checklist there)
Address DOATToUnitVentilator node connection errors #6273 DOATToUnitVentilator node connection errors (never released this way, error introduced in Add air terminal unit sizing option, allow air loops with no return path, and partially allow multiple air loops serving the same zone #6248)
Address Zone air flow mass balance is broken #6219 Zone air mass balance is broken.
Finish implementing options to specify the return air node for heat gain from lights, airflow windows, and refrigeration cases.
Finish implementing allocation of return air flow to multiple airloops serving the same zone.
Engineering ref changes.

Diffs Comments

6 files with big diffs as of 2017-09-21

5ZoneAirCooledWithDOASAirLoop and DOASDXCOIL_wADPBFMethod_NoReturnPath - big table diffs, because the outdoor air summary table is correct now (previously zeros)

5ZoneAirCooled_ZoneAirMassFlowBalance and 5ZoneAirCooled_ZoneAirMassFlowBalance_Pressurized - big eso diffs etc because zone air mass conservation is fixed here

DOAToWaterToAirHPInlet - These diffs should go away once #6333 drops in.

VSHeatPumpWaterHeater - big table and eio diffs because there is one less warmup day now as of 09-25 there are now a few more warmup days - but no code changes here just merging in develop

3 files with err diffs as of 2017-09-21

DOAToUnitVentilator - Node connection errors are gone.

HospitalLowEnergy - One fewer SolveAirLoopControllers warning for DOAS1 and DOAS5.

VariableRefrigerantFlow_FluidTCtrl_HR_5Zone - New unbalanced air flow warnings - should go away when #6301 is fixed.

Work Checklist

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)
At least one of the following appropriate labels must be added to this PR to be consumed into the changelog:
- Defect: This pull request repairs a github defect issue. The github issue should be referenced in the PR description
- Refactoring: This pull request includes code changes that don't change the functionality of the program, just perform refactoring
- NewFeature: This pull request includes code to add a new feature to EnergyPlus
- Performance: This pull request includes code changes that are directed at improving the runtime performance of EnergyPlus
- DoNoPublish: This pull request includes changes that shouldn't be included in the changelog

Review Checklist

This will not be exhaustively relevant to every PR.

…loop-Part2

rraustad · 2017-08-30T03:00:21Z

What's the status here. I don't see anything in the diff's that stand out.

mjwitte · 2017-08-30T03:26:18Z

Still wrestling with this. And have some other parts to implement. The thing is, at this point it should be zero diffs. But finding interesting things that are contributing to diffs - like this which is likely responsible for ReverseDD failures in the two data center files.

…to Beyond1Airloop-Part2 # Conflicts: # src/EnergyPlus/SimulationManager.cc

rraustad · 2017-08-30T12:38:39Z

I thought ResetNodeData was already called at BeginEnvironment. In your "reset earlier" branch, I see you added another instead of moving the existing call. That seems wrong (should only call once), but of course your the one looking at this.

I also see that the plenum does it's own node reset. That seems redundant unless node data isn't read in first.

mjwitte · 2017-08-30T13:17:12Z

Probably correct to move ResetNodeData instead of calling it twice. The place it's called currently (in develop) is too late for the IT equipment which uses a supply node temp to calculate it's power consumption. It showed up because the results coming out of SetUpSimulation (which runs everything for a few time steps) was different in this branch (because the airloop nums aren't all intialized yet) but that shouldn't matter, because in theory everything gets reset on begin environment. Follow? Sorry, confusing.

The plenum wants to start at 20C like the zones do. But ResetNodeData sets everything to 0C. For the IT equipment, I could add a begin environment init there instead, but I wanted to see if it might change some other files - which it did not. Still seems easier just to move ResetNodeData. I should open a PR for that branch.

mjwitte · 2017-08-30T21:36:00Z

CMakeLists.txt

    endif()

-    set( REGRESSION_BASELINE_SHA "" CACHE STRING "SHA of baseline comparison" )
+    set( REGRESSION_BASELINE_SHA "73377e2" CACHE STRING "SHA of baseline comparison" )


@Myoldmopar Did I do this right? I want to compare this branch with branch ResetNodeDataEarlier

Nah, I'd revert that out. Change it here instead.

…loop-Part2

mjwitte · 2017-09-06T01:07:35Z

Diffs explanations (as of 0f8a85a )

5ZoneAirCooledWithCoupledInGradeSlab and UnitarySystem_WaterCoils_wMultiSpeedFan - eio warmup convergence diffs and tiny eso diffs

5Zone_Unitary_HXAssistedCoil - ** Warning ** CoilSystem:Cooling:DX:HeatExchangerAssisted "HEAT EXCHANGER ASSISTED COOLING COIL 1" -- Exceeded max iterations error continues - total error count is 68 now instead of 70, almost all during warmup. No other diffs.

DOAToUnitVentilator - Node connection warnings introduced earlier with this feature are now gone.

HospitalLowEnergy - Warning during warmup is gone In AirLoopHVAC DOAS2 there is unbalanced exhaust air flow. All other results have no diffs. This warning normally is allowed to print only once. Ran with a version of develop that allowed this warning to always print, and there were only six occurences, all during warmup.

rraustad · 2017-09-06T01:54:15Z

doc/engineering-reference/src/integrated-solution-manager/zone-air-mass-flow-conservation.tex

+
+\paragraph{Allocation to Multiple Return Nodes}\label{allocation-to-multiple-return-nodes}
+
+If there are multiple return air nodes in the zone, then each return air node is assumed to be paired with on supply air inlet node. The initial flow rate at a given return air node is based on the supply air inlet node for the same airloop and the AirloopHVAC Design Return Air Flow Fraction of Supply Air Flow:


... paired with ONE supply air inlet node.

rraustad · 2017-09-06T12:46:12Z

I went throught this again last night. It is admittedly hard to follow the intricate changes necessary for two air loops serving the same zone. Nothing stood out as blatently wrong. The unit test shows that return air flow can be properly managed. Diff's are minimal and explained. Question for HospitalLowEnergy results - if warning is now gone, why were there 6 occurrences during warmup for the special build?

mjwitte · 2017-09-06T13:55:51Z

Not sure about HospitalLowEnergy - that file runs terribly slow in debug. In fact, the time stamps on those are all 01/01 which isn't even one of the environments for this file. That says the warnings are happening during SetUpSimulation which really shouldn't be producing warnings at all. With the new multi-airloop world, the airloop number mapping to zone equipment isn't known yet during SetUpSimulation so that explains why these aren't present in this branch.

Regarding the changes, there are more to come.

…loop-Part2

…loop-Part2 # Conflicts: # src/EnergyPlus/OutputReportTabular.cc

Myoldmopar · 2017-09-23T03:45:52Z

My final adventure...I'm going to go ahead and kick off an RC package so it can build overnight and I can feel good about the release package situation. And I'll get this reviewed along with #6338.

Myoldmopar · 2017-09-24T12:34:59Z

So how coupled are #6338 and this? I saw the title change on the other one to include a reference to fixes for multiple air loops. Does that mean these two are basically coupled? If so, what do you think about pulling that branch into here, or vice versa, to make sure you are capturing the combined effects? If they aren't that coupled and I'm just reading it wrong, that's fine too. Things just still feel squishy, so I'm hesitating before investing much more review time.

mjwitte · 2017-09-24T18:25:14Z

This pull request is focused on the simulation side of multiple airloops - getting rid of the one-return-node/one-airloopnum per zone mindset. And also, revising the whole zone air mass balance to split thing up across multiple return nodes in a zone. #6338 fixes some issues with the new air terminal sizing inputs - cases that I had not tested before now. So, they are closely related, yet somewhat separate. If you'd prefer one combo pull request to review, I can do that too.

…loop-Part2

mjwitte · 2017-09-25T04:22:40Z

@Myoldmopar Air terminal sizing limitations added to I/O Ref. Ready for final review here.

Myoldmopar · 2017-09-25T12:54:24Z

Stray unit test failure is unrelated. Linux failures are the s3 problem.

Doing final code review now.

Myoldmopar

This is exactly what I was expecting to find. Tons and tons of bookkeeping. Changing single lines of code where nodes where looked up or assigned into loops. This is a huge amount of code changes, but they look good. It's great to see some tests added on to existing unit tests, and the new unit tests look like they cover core functionality. The doc changes look good, and they build locally for me. I'll check them out visually and do some testing, but I think this is good to go.

Myoldmopar · 2017-08-30T21:38:00Z

CMakeLists.txt

    endif()

-    set( REGRESSION_BASELINE_SHA "" CACHE STRING "SHA of baseline comparison" )
+    set( REGRESSION_BASELINE_SHA "73377e2" CACHE STRING "SHA of baseline comparison" )


Nah, I'd revert that out. Change it here instead.

Myoldmopar · 2017-09-25T13:30:28Z

doc/input-output-reference/src/overview/group-design-objects.tex

 \item  \emph{Maximum} means that the maximum flow derived from \emph{Outdoor Air Flow per Person,} \emph{Outdoor Air Flow per Area, Outdoor Air Flow per Zone,} and \emph{Air Changes per Hour} (using the associated conversions to m\(^{3}\)/s for each field) will be used as the zone outdoor air flow rate.

-\item  \emph{IndoorAirQualityProcedure} means that the program will use the other procedure defined in ASHRAE Standard 62.1-2007 to calculate the amount of outdoor air necessary in order to maintain the levels of indoor air carbon dioxide at or below the setpoint defined in the ZoneControl:ContaminantController object. Appendix A of the ASHRAE 62.1-2010 user’s manual discusses another method for implementing CO2-based DCV in a single zone system. The last two methods of Proportional Control calculate the required outdoor air flow rate which varies in proportion to the percentage of the CO2 signal range and has two choices to calculate occupancy-based outdoor air rate. 
+\item  \emph{IndoorAirQualityProcedure} means that the program will use the other procedure defined in ASHRAE Standard 62.1-2007 to calculate the amount of outdoor air necessary in order to maintain the levels of indoor air carbon dioxide at or below the setpoint defined in the ZoneControl:ContaminantController object. Appendix A of the ASHRAE 62.1-2010 userâ€™s manual discusses another method for implementing CO2-based DCV in a single zone system. The last two methods of Proportional Control calculate the required outdoor air flow rate which varies in proportion to the percentage of the CO2 signal range and has two choices to calculate occupancy-based outdoor air rate. 


There's still a funny character in here, but I'm assuming there's not enough need to push more commits solely for this.

Whacky - you know when I made the doc changes last night, the local commit diff showed this line and another even though I hadn't changed anything, so I thought maybe my editor had automatically done something it shouldn't. So, I undid those before I pushed it up.

Myoldmopar · 2017-09-25T13:30:50Z

doc/input-output-reference/src/overview/group-design-objects.tex

+\item It does not work with VRP sizing.
+\item It does not work with a specified system flow rate in Sizing:System.
+\item It does not work if the base minimum outdoor air flow exceeds the heating or cooling air flow.
+\end{itemize}


Myoldmopar · 2017-09-25T13:32:41Z

src/EnergyPlus/DataHeatBalFanSys.hh

-	extern Array1D< Real64 > OAMFL; // OUTDOOR AIR MASS FLOW (M**3/SEC) for infiltration
-	extern Array1D< Real64 > VAMFL; // OUTDOOR AIR MASS FLOW (M**3/SEC) for ventilation
+	extern Array1D< Real64 > OAMFL; // OUTDOOR AIR MASS FLOW (kg/s) for infiltration
+	extern Array1D< Real64 > VAMFL; // OUTDOOR AIR MASS FLOW (kg/s) for ventilation


Myoldmopar · 2017-09-25T13:35:01Z

src/EnergyPlus/DataZoneEquipment.cc

+				}
+			}}
+		}
+


So, at some point, when the entire codebase is setup to handle the possibility of multiple nodes per zone, this function should never need to warn anymore, right? The NodeName will be required and provided once the callers all understand the new paradigm. I hope so, but if I'm wrong then I just want to understand why.

Good question. I had wanted the node names to be optional in places like return air heat gain from lights so if you do have only one return node, you don't have to enter a node name. If we make the return node required, then it wouldn't work with a zonelist name anymore. So, not sure.

That is interesting. And tricky. I see your point, but it feels funny as optional. Nothing to worry about for today, that's for sure. We'll see how it plays out.

Myoldmopar · 2017-09-25T13:51:02Z

src/EnergyPlus/ZoneEquipmentManager.cc

+		int numRetNodes = thisZoneEquip.NumReturnNodes;
+		Real64 totReturnFlow = 0.0; // Total flow to all return nodes in the zone (kg/s)
+		Real64 totVarReturnFlow = 0.0; // Total variable return flow, for return nodes connected to an airloop with an OA system or not with specified flow (kg/s)
+		Real64 returnSchedFrac = ScheduleManager::GetCurrentScheduleValue( thisZoneEquip.ReturnFlowSchedPtrNum );


You feel OK calling this without validation of the index variable? I'm sure it's done elsewhere, so it's probably fine...

Myoldmopar · 2017-09-25T13:51:58Z

src/EnergyPlus/ZoneEquipmentManager.cc

+		} else {
+			FinalTotalReturnMassFlow = totReturnFlow;
+		}
+


Whew, quite a function 👍

Myoldmopar · 2017-09-25T13:53:11Z

tst/EnergyPlus/unit/HVACFourPipeBeam.unit.cc

 		DataGlobals::KickOffSimulation = true;

 		WeatherManager::ResetEnvironmentCounter();
+		TestAirPathIntegrity( ErrorsFound ); // Needed to initialize return node connections to airloops and inlet nodes


Myoldmopar · 2017-09-25T13:54:22Z

tst/EnergyPlus/unit/ThermalComfort.unit.cc

 		" 10.0,                     !- Watts per Zone Floor Area{ W / m2 }",
 		" ,                         !- Watts per Person{ W / person }",
-		" 0.1,                      !- Return Air Fraction",
+		" 0.0,                      !- Return Air Fraction",


Is this to avoid having to change all the values being checked by the unit test?

Return air doesn't impact thermal comfort calcs. This change was to avoid tripping on this error because the unit test doesn't have any conditioned zone info.

Myoldmopar · 2017-09-25T13:55:16Z

tst/EnergyPlus/unit/ZoneEquipmentManager.unit.cc

+	EXPECT_EQ( FinalTotalReturnMassFlow, 6.0 );
+	EXPECT_EQ( Node( returnNode1 ).MassFlowRate, 2.0 );
+	EXPECT_EQ( Node( returnNode2 ).MassFlowRate, 1.0 );
+	EXPECT_EQ( Node( returnNode3 ).MassFlowRate, 3.0 );


👍 👍 👍

Myoldmopar · 2017-09-25T14:00:05Z

@mjwitte Apparently I had started a review a while back and tried to comment to you about the REGRESSION_SHA in the CMakeLists file. I guess Github kept that comment in a cache, even though that diff is long gone. Anyway, sorry for any confusion on the CMakeLists.txt (outdated) comment above.

Myoldmopar · 2017-09-25T14:04:48Z

You don't think this bnd diff is going to cause a problem in HVACDiagram or other tools do you?

Myoldmopar · 2017-09-25T14:09:27Z

Interesting. Checking out the diffs on 5ZoneAirCooled_ZoneAirMassFlowBalance, and the EIO is showing better convergence at the end of the warmup days. Nothing significant, but to be honest, I kinda expected to see the system having a bit more freedom and taking a little longer to converge. Looks like this improved it. Or it was just luck?

(new EIO lines on the bottom in blue)

Myoldmopar · 2017-09-25T14:10:03Z

OK, if no one else has anything to say on this I'm merging it in very soon.

Myoldmopar · 2017-09-25T14:36:28Z

You don't think this bnd diff is going to cause a problem in HVACDiagram or other tools do you?

Oh I guess #6345 explains it 😆

mjwitte · 2017-09-25T14:36:44Z

The bnd change doesn't affect HVAC diagram, not sure if that's used by any other tools. I can revert that back and save it for post-release. But that reminds me, HVAC diagram chokes on zones with more than one airloop (e.g. 5ZoneAirCooledWithDOASAirLoop ). Posted #6345 for that.

Regarding 5ZoneAirCooled_ZoneAirMassFlowBalance, the mass balance was broken before. It works now - actually balances.

Myoldmopar · 2017-09-25T14:37:19Z

OK, thanks for all these comments.

mjwitte · 2017-09-25T14:38:47Z

Thanks for the review. I can clean up the funny doc chars and the whitespace stuff if you want. Or just take care of them in that other open PR.

Myoldmopar · 2017-09-25T14:39:24Z

"Documentation Issues October-November 2017" 👍

mjwitte added 2 commits August 24, 2017 17:32

Multiple Airloops - Initial engineering ref

22ab816

Merge remote-tracking branch 'remotes/origin/develop' into Beyond1Air…

dc2217b

…loop-Part2

mjwitte added the NewFeature Includes code to add a new feature to EnergyPlus label Aug 24, 2017

mjwitte added this to the EnergyPlus 8.8.0 milestone Aug 24, 2017

mjwitte added 5 commits August 25, 2017 11:14

Multiple Airloops - Allocate return flows part 1

9fbc4f2

Merge remote-tracking branch 'remotes/origin/develop' into Beyond1Air…

c077cae

…loop-Part2

Multiple Airloops - Fix unit ventilator with ATMixer

1905ddf

Multiple Airloops - Allocate return flows part 2

11d73d8

Multiple Airloops - Fix build warnings and test failures

5846a28

mjwitte added 2 commits August 29, 2017 22:51

Multiple Airloops - Resolve diffs

478a127

Merge remote-tracking branch 'remotes/origin/ResetNodeDataEarlier' in…

36d4c4c

…to Beyond1Airloop-Part2 # Conflicts: # src/EnergyPlus/SimulationManager.cc

Multiple Airloops - Resolve diffs part 2

45a2334

mjwitte commented Aug 30, 2017

View reviewed changes

mjwitte added 5 commits August 30, 2017 17:36

Merge remote-tracking branch 'remotes/origin/develop' into Beyond1Air…

f10471c

…loop-Part2

Undo alternate regression SHA

70fc8f3

Merge remote-tracking branch 'remotes/origin/develop' into Beyond1Air…

295d8e6

…loop-Part2

Multiple Airloops - Add initial unit tests

5e6df21

Merge remote-tracking branch 'remotes/origin/develop' into Beyond1Air…

0f8a85a

…loop-Part2

rraustad reviewed Sep 6, 2017

View reviewed changes

Multiple Airloops - Simplify zone unbalanced exhaust warning

95378d2

mjwitte added 3 commits September 6, 2017 12:04

Multiple Airloops - Fix doc typo

43157cb

Multiple Airloops - Simplify airloop flow balance calcs

4ef7908

Merge remote-tracking branch 'remotes/origin/develop' into Beyond1Air…

4b0e421

…loop-Part2

mjwitte added 4 commits September 21, 2017 11:12

Multiple Airloops - Fix yet another stray using ZoneEquipConfig

51bd2bf

Merge remote-tracking branch 'remotes/origin/develop' into Beyond1Air…

2563906

…loop-Part2

Merge remote-tracking branch 'remotes/origin/develop' into Beyond1Air…

652d5dd

…loop-Part2 # Conflicts: # src/EnergyPlus/OutputReportTabular.cc

Multiple Airloops - Merge cleanup

7f626d2

mjwitte mentioned this pull request Sep 24, 2017

Fix VAV min flow frac > 1.0 and a bunch of termunitsizing issues related to multiple airloops #6338

Merged

13 tasks

mjwitte added 2 commits September 24, 2017 22:34

Merge remote-tracking branch 'remotes/origin/develop' into Beyond1Air…

9b5eced

…loop-Part2

I/O ref - air terminal sizing limitations

fc30677

Myoldmopar approved these changes Sep 25, 2017

View reviewed changes

Myoldmopar merged commit 9148e5c into develop Sep 25, 2017

Myoldmopar deleted the Beyond1Airloop-Part2 branch September 25, 2017 14:39

This was referenced Sep 29, 2017

Followup from Multiple Airloops #6264

Closed

DOATToUnitVentilator node connection errors #6273

Closed

Zone air flow mass balance is broken #6219

Closed

mjwitte mentioned this pull request Oct 25, 2017

Improve error message when SetpointManager:SingleZone:Reheat does not find matching zone inlet node #6375

Closed

3 tasks

mjwitte mentioned this pull request Nov 13, 2017

Adaptive convection with lights and baseboard heat or high-temp radiant will crash in v8.8 #6391

Closed

3 tasks

mjwitte mentioned this pull request Sep 20, 2021

Fix HVACDiagram Error Messages #9076

Merged

20 tasks


		\paragraph{Allocation to Multiple Return Nodes}\label{allocation-to-multiple-return-nodes}

		If there are multiple return air nodes in the zone, then each return air node is assumed to be paired with on supply air inlet node. The initial flow rate at a given return air node is based on the supply air inlet node for the same airloop and the AirloopHVAC Design Return Air Flow Fraction of Supply Air Flow:

Conversation

mjwitte commented Aug 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull request overview

Diffs Comments

6 files with big diffs as of 2017-09-21

3 files with err diffs as of 2017-09-21

Work Checklist

Review Checklist

Uh oh!

rraustad commented Aug 30, 2017

Uh oh!

mjwitte commented Aug 30, 2017

Uh oh!

rraustad commented Aug 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mjwitte commented Aug 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mjwitte commented Sep 6, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rraustad commented Sep 6, 2017

Uh oh!

mjwitte commented Sep 6, 2017

Uh oh!

Myoldmopar commented Sep 23, 2017

Uh oh!

Myoldmopar commented Sep 24, 2017

Uh oh!

mjwitte commented Sep 24, 2017

Uh oh!

mjwitte commented Sep 25, 2017

Uh oh!

Myoldmopar commented Sep 25, 2017

Uh oh!

Myoldmopar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Myoldmopar commented Sep 25, 2017

Uh oh!

Myoldmopar commented Sep 25, 2017

Uh oh!

Myoldmopar commented Sep 25, 2017

Uh oh!

Myoldmopar commented Sep 25, 2017

mjwitte commented Aug 24, 2017 •

edited

Loading

rraustad commented Aug 30, 2017 •

edited

Loading

mjwitte commented Aug 30, 2017 •

edited

Loading