-
Notifications
You must be signed in to change notification settings - Fork 394
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Correct physics based VRF model to align inlet and outlet air flow rates #7295
Correct physics based VRF model to align inlet and outlet air flow rates #7295
Conversation
@@ -8663,34 +8663,35 @@ namespace HVACVariableRefrigerantFlow { | |||
} | |||
|
|||
// Set inlet air mass flow rate based on PLR and compressor on/off air flow rates | |||
SetAverageAirFlow(VRFTUNum, 1.0, temp); | |||
//SetAverageAirFlow(VRFTUNum, 1.0, temp); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The TUs have already been simulated. At this point it appears the model want to know what the evap T and cond T are for use during the next time step. Anything that tried to resimulate the TUs is now commented out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note here that the original model did not set OA flow rate and therefore used the OA flow for the last TU simulated when SetAverageAirFlow was called. When I corrected this (lines 8671 and 8676), my new method matched the old method results.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also note that any calls to this and other functions updates the node flow rates. So data set when the TU was actually modelled is used here instead.
|
||
// Simulation the OAMixer if there is any | ||
if (this->OAMixerUsed) { | ||
SimOAMixer(this->OAMixerName, false, this->OAMixerIndex); | ||
//SimOAMixer(this->OAMixerName, false, this->OAMixerIndex); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mixer has already been simulated.
T_coil_in = Node(FanOutletNode).Temp; | ||
W_coil_in = Node(FanOutletNode).HumRat; | ||
} | ||
//if (this->FanPlace == BlowThru) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fan has already been simulated. If the intent here is to simulate at full flow then why weren't the coils simulated again?
@@ -8702,8 +8703,9 @@ namespace HVACVariableRefrigerantFlow { | |||
if ((Garate > 0.0) && ((!VRF(VRFNum).HeatRecoveryUsed && CoolingLoad(VRFNum)) || | |||
(VRF(VRFNum).HeatRecoveryUsed && TerminalUnitList(TUListIndex).HRCoolRequest(IndexToTUInTUList)))) { | |||
// 1.1) Cooling coil is running | |||
QZnReqSenCoolingLoad = max(0.0, -1.0 * ZoneSysEnergyDemand(ZoneIndex).OutputRequiredToCoolingSP); | |||
Tout = T_TU_in - QZnReqSenCoolingLoad * 1.2 / Garate / 1005; | |||
//QZnReqSenCoolingLoad = max(0.0, -1.0 * ZoneSysEnergyDemand(ZoneIndex).OutputRequiredToCoolingSP); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's the big question. What Tout is needed here? The Tout when the coil is operating I assume. Line 8706 is wrong since for that zone, the TU operated and probably met the load, so OutputRequiredToCoolingSP is probably 0. Instead I use the actual coil outlet temp, but this is premised on needing actual Tout.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess I should read the Eng. Ref. to get better acquainted.
@nealkruis @mbadams5 @mjwitte @Myoldmopar @jasondegraw @kbenne this will need some serious thought and review because I don't know this model. What the heck should it do? Does it need full output conditions while coils are operating? Yet the coils can have different PLR's so how to average correctly to get condenser performance? Anyone recognize old line 8706 above? Q*1.2/Mdot/1005 ? Figuring out what this function needs will probably correct this issue. If Tout at full air flow is really needed then the coils will need to be simulated again, or better yet that information needs to be saved when the TUs model full flow and then used here. Is Xiufeng Pang or RP Zhang or anyone at LBNL available for consult on this model?
…unit-flow-balance-problems
…1-VRF-Terminal-unit-flow-balance-problems
…//github.com/NREL/EnergyPlus into 6301-VRF-Terminal-unit-flow-balance-problems
Since the answers didn't line up well with develop I decided to mimic what the original model does. The answers don't line up perfectly because there was a mistake in the original code. I also don't understand why the model assumes full flow rate when determining suction/discharge temperature at the condenser when actual TU flow rates used are known and coil outlet temps are also known. Regardless, these changes mimic the original model. Air flow rate is now consistent and zone temperature is roughly unchanged. Develop is left column, this branch is right column. In this first figure since zone temperature is held to set point, the TU outlet node flow rate was correct so in the right column, the new flows should roughly match the blue line in the left figure. The compressor speed and cooling coil rate are also roughly the same. The evap/condenser temps used are also similar but won't match because of the problem in code. Where full flow was set, the developer forgot to set the OA flow rate and therefore when SetAverageAirFlow was called it used the OA flow rate from the last TU simulated and wound up with a different mixed air temp. |
VRFTU(VRFTUNum).ControlVRF_FluidTCtrl(VRFTUNum, QZnReq, FirstHVACIteration, PartLoadRatio, OnOffAirFlowRatio); | ||
VRFTU(VRFTUNum).CalcVRF_FluidTCtrl(VRFTUNum, FirstHVACIteration, PartLoadRatio, SysOutputProvided, OnOffAirFlowRatio, LatOutputProvided); | ||
if ( PartLoadRatio == 0.0 ) { // set coil inlet conditions when coil does not operate. Inlet conditions are set in ControlVRF_FluidTCtrl when PLR=1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pick up temps used to calculate evap/cond temps later on. Here only for PLR=0.
TotalTUHeatingCapacity, | ||
TerminalUnitList(TUListNum).TotalHeatLoad, | ||
MaxHeatingCapacity(VRFCond)); | ||
TerminalUnitList(TUListNum).TotalCoolLoad, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, will fix this on next commit.
@@ -10186,6 +10202,14 @@ namespace HVACVariableRefrigerantFlow { | |||
// Otherwise the coil needs to turn on. Get full load result | |||
PartLoadRatio = 1.0; | |||
this->CalcVRF_FluidTCtrl(VRFTUNum, FirstHVACIteration, PartLoadRatio, FullOutput, OnOffAirFlowRatio); | |||
this->fanOutletT = Node(this->fanOutletNode).Temp; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here, when the TU operates the conditions resulting from PLR=1 (full flow to mimic what was done before) are captured and used in calculations for evap/cond temps.
@@ -700,6 +700,10 @@ namespace HVACVariableRefrigerantFlow { | |||
int ATMixerSecNode; // secondary air inlet node number for the air terminal mixer | |||
int ATMixerOutNode; // outlet air node number for the air terminal mixer | |||
bool firstPass; // used to reset global sizing data | |||
Real64 coilInNodeT; // coil inlet node temp at full flow (C) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
new variables to hold data in order to eliminate the problem with flow mismatch from inlet to outlet.
@@ -3287,6 +3287,8 @@ namespace HVACVariableRefrigerantFlow { | |||
if (errFlag) { | |||
ShowContinueError("...occurs in " + cCurrentModuleObject + " = " + VRFTU(VRFTUNum).Name); | |||
ErrorsFound = true; | |||
} else { | |||
VRFTU(VRFTUNum).fanOutletNode = Fans::Fan(VRFTU(VRFTUNum).FanIndex).OutletNodeNum; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Save fan outlet node for use in TeTc calculations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems reasonable enough, if you are going to have to query what the fan is doing, you can check the outlet node.
I should probably show the marked difference between this branch and develop. Since the original model tried to use full flow in the TeTc calc's, and forgot to also set OACompOnMassFlow along with CompOnMassFlow, the resulting TeTc is now different. This impacts the air flow rate and will show diff's. Other than this, right or wrong the model still does what it originally did. |
W_coil_in = Node(FanOutletNode).HumRat; | ||
} | ||
T_coil_in = this->coilInNodeT; | ||
W_coil_in = this->coilInNodeW; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change here is that coil inlet temps are now used so it doesn't matter if blow thru fan or OA mixer is used. Gets rid of a lot of unnecessary code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 👍 👍 👍 👍 👍 👍 👍 👍 👍 👍
The 2 new diff's are from the merge of #7258 into develop. No need to spin off CI again just for that. |
But there are build warnings. It looks like just one unused "using" statement (which is interpreted as an unused variable in a function). The error message is difficult to parse, so the CI message doesn't always point you to the right spot easily, but I can track it down for you if you need help diagnosing where it is. |
@@ -895,7 +895,7 @@ \subsubsection{Overview}\label{VRF-FluidTCtrl-HP-overview} | |||
|
|||
Note that a number of calculation steps are coupled together in the VRF-FluidTCtrl model, for instance, the piping loss calculation and the system performance calculation. More specifically, the piping loss changes the operating conditions of the system, which may lead to a different control strategy and thus in reverse affect the amount of piping loss. This makes it difficult to obtain an analytical solution for a number of operational parameters (e.g., enthalpy of refrigerant entering the indoor unit), and therefore numerical iterations are employed to address this problem (refer to Figure VRF-FluidTCtrl-3 for more details). Therefore, the VRF-FluidTCtrl model can have a longer execution time to perform the simulation than the VRF-SysCurve model. | |||
|
|||
The object connections for VRF-FluidTCtrl model is similar to those for VRF-SysCurve model. The difference lies in the object used to describe a specific components. For example, VRF-SysCurve model uses \emph{AirConditioner:VariableRefrigerantFlow} object to describe the VRF outdoor unit performance, while in VRF-FluidTCtrl model the \emph{AirConditioner:VariableRefrigerantFlow} object is used. | |||
The object connections for VRF-FluidTCtrl model is similar to those for VRF-SysCurve model. The difference lies in the object used to describe a specific components. For example, VRF-SysCurve model uses \emph{AirConditioner:VariableRefrigerantFlow} object to describe the VRF outdoor unit performance, while in VRF-FluidTCtrl model the \emph{AirConditioner:VariableRefrigerantFlow:FluidTemperatureControl} object is used. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good find.
@@ -3287,6 +3287,8 @@ namespace HVACVariableRefrigerantFlow { | |||
if (errFlag) { | |||
ShowContinueError("...occurs in " + cCurrentModuleObject + " = " + VRFTU(VRFTUNum).Name); | |||
ErrorsFound = true; | |||
} else { | |||
VRFTU(VRFTUNum).fanOutletNode = Fans::Fan(VRFTU(VRFTUNum).FanIndex).OutletNodeNum; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems reasonable enough, if you are going to have to query what the fan is doing, you can check the outlet node.
@@ -8526,7 +8537,7 @@ namespace HVACVariableRefrigerantFlow { | |||
for (int i = 1; i <= TerminalUnitList(TUListNum).NumTUInList; i++) { | |||
int VRFTUNum = TerminalUnitList(TUListNum).ZoneTUPtr(i); | |||
// analyze the conditions of each IU | |||
VRFTU(VRFTUNum).CalcVRFIUVariableTeTc(VRFTUNum, EvapTemp(i), CondTemp(i)); | |||
VRFTU(VRFTUNum).CalcVRFIUVariableTeTc(EvapTemp(i), CondTemp(i)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 👍 👍 👍
W_coil_in = Node(FanOutletNode).HumRat; | ||
} | ||
T_coil_in = this->coilInNodeT; | ||
W_coil_in = this->coilInNodeW; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 👍 👍 👍 👍 👍 👍 👍 👍 👍 👍
EXPECT_EQ(VRF(IndexVRFCondenser).IUEvaporatingTemp, 15); | ||
// fan heat added to coil inlet temp, coil surface temp should also be lower to account for less heat tranfer | ||
EXPECT_LT(VRF(IndexVRFCondenser).IUCondensingTemp, saveHeatingCondTemp); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great unit test.
EXPECT_EQ(VRF(IndexVRFCondenser).IUEvaporatingTemp, 15); | ||
// fan heat added to coil inlet temp, coil surface temp should also be lower to account for less heat tranfer | ||
EXPECT_LT(VRF(IndexVRFCondenser).IUCondensingTemp, saveHeatingCondTemp); | ||
|
||
// Clean up | ||
VRF.deallocate(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally these are cleaned up as part of the clear_state after every single unit test is run, but I understand it doesn't hurt to have them here.
…unit-flow-balance-problems
Alright, I fixed that unused variable issue, merged up from develop, and will let this run over the next couple hours. I'm anticipating very clean CI except for the actual expected diff file. If so I'll merge in the morning. |
This:
should have been removed. I was so focused on testing this that I missed this. We should not allow deallocate in unit tests? New python script warranted? |
I was already half asleep and this phrase woke me right back up. I like your thinking but I would worry that in this particular case, it may hit too many false alarms. There are certainly times where you want to deallocate/reallocate within a unit test, and I feel like we may end up hitting too many of those cases to warrant a script. But your line of thinking here makes me smile. |
Yeah, the full warning message text is:
Which makes it pretty clear in the third or so line that the error occurs in |
Shoot, now I have to think about a valid deallocate in a unit test... |
Yeah, maybe there's not one, I may be concerned at that possibility for nothing. For better or worse (worse) there definitely are a ton of deallocate calls in tst/EnergyPlus/unit though. |
"definitely are a ton of deallocate calls in tst/EnergyPlus/unit" Of course there are, we are old and lazy!!! |
Everything looks great. Merging. |
Pull request overview
The physics based VRF model calculates the operating evaporator and condenser temperatures after all terminal units have been simulated. The method used adjusted the terminal unit simulated inlet node air flow rate such that it did not match the TU outlet node air flow rate. This change fixes #6301.
Work Checklist
Add to this list or remove from it as applicable. This is a simple templated set of guidelines.
Review Checklist
This will not be exhaustively relevant to every PR.