-
Notifications
You must be signed in to change notification settings - Fork 421
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
fix VRF_FluidTCtrl heating round 2 #10331
Conversation
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 only issue I see is the use of PLR or RTF in the correct places. PLR is a capacity variable, RTF is an energy use variable.
src/EnergyPlus/DXCoils.cc
Outdated
@@ -17326,8 +17326,9 @@ void CalcVRFHeatingCoil_FluidTCtrl(EnergyPlusData &state, | |||
thisDXCoil.CompressorPartLoadRatio = PartLoadRatio; | |||
thisDXCoil.ActualSH = ActualSH; | |||
thisDXCoil.ActualSC = ActualSC; | |||
thisDXCoil.TotalHeatingEnergyRate = AirMassFlow * (OutletAirEnthalpy - InletAirEnthalpy); | |||
thisDXCoil.TotalHeatingEnergyRate = AirMassFlow * (OutletAirEnthalpy - InletAirEnthalpy) * thisDXCoil.HeatingCoilRuntimeFraction; |
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.
Energy transfer is multiplied by PLR, electricity consumption is multiplied by RTF.
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.
Thanks for the clarification. I'll change it here to PLR.
src/EnergyPlus/DXCoils.cc
Outdated
thisDXCoil.DefrostPower = thisDXCoil.DefrostPower * thisDXCoil.HeatingCoilRuntimeFraction; | ||
thisDXCoil.InletAirMassFlowRate = AirMassFlow * thisDXCoil.HeatingCoilRuntimeFraction; |
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.
use PLR here
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.
thisDXCoil.DefrostPower should also use PLR right?
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.
Correct.
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, power is multiplied by RTF.
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 changed it to use RTF in DefrostPower calculation
if ((Q_c_OU * C_cap_operation) > CompEvaporatingCAPSpdMaxCurrentTsuc + CompEvaporatingPWRSpdMaxCurrentTsuc) { | ||
CompSpdActual = this->CompressorSpeed(NumOfCompSpdInput); | ||
Ncomp = this->RatedCompPower * CurveValue(state, this->OUCoolingPWRFT(NumOfCompSpdInput), Tdischarge, this->EvaporatingTemp); | ||
} else if ((Q_c_OU * C_cap_operation) <= CompEvaporatingCAPSpdMin) { |
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 can't comment on the math here, but what if the curve result yields CompEvaporatingPWRSpdMaxCurrentTsuc that gives a value in the IF that is less than CompEvaporatingCAPSpdMin in the ELSE IF? Just pointing this out so you think about what that might mean.
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.
yeah, this part might also need some change as well. The intention was to match the calculation here (which computes the compressor speed) and previously in limitTUCapacity. Let me sort this through
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 added a error message to ensure that CompEvaporatingCAPSpdMin is less than CompEvaporatingCAPSpdMaxCurrentTsuc so the case when Q_c_OU * C_cap_operation <= CompEvaporatingCAPSpdMin
and Q_c_OU * C_cap_operation > CompEvaporatingCAPSpdMaxCurrentTsuc + CompEvaporatingPWRSpdMaxCurrentTsuc
won't occur.
@@ -13093,6 +13096,8 @@ void VRFTerminalUnitEquipment::CalcVRF_FluidTCtrl(EnergyPlusData &state, | |||
} | |||
} | |||
// calculate sensible load met using delta enthalpy | |||
AirMassFlow = state.dataDXCoils->DXCoil(this->HeatCoilIndex).InletAirMassFlowRate; |
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 assume this fixes one of the issues, since AirMassFlow gets set above at line 13079.
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.
yeah. Somehow the air mass flow used in the coil calculation didn't get passed here correctly, that's why the heating coil heating rate and the terminal heating rate didn't match.
@@ -14538,7 +14543,7 @@ void VRFCondenserEquipment::VRFOU_CalcCompH( | |||
NumOfCompSpdInput = this->CompressorSpeed.size(); | |||
CompEvaporatingPWRSpd.dimension(NumOfCompSpdInput); | |||
CompEvaporatingCAPSpd.dimension(NumOfCompSpdInput); | |||
Q_evap_req = TU_load + Pipe_Q - Ncomp; | |||
Q_evap_req = TU_load + Pipe_Q; |
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.
This also looks like it was a bug.
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'll double check
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.
reverted it back to Q_evap_req = TU_load + Pipe_Q
(this->CompressorSpeed(CompSpdUB) - this->CompressorSpeed(CompSpdLB)) / | ||
(CompEvaporatingCAPSpd(CompSpdUB) + CompEvaporatingPWRSpd(CompSpdUB) - | ||
(CompEvaporatingCAPSpd(CompSpdLB) + CompEvaporatingPWRSpd(CompSpdLB))) * | ||
(Q_evap_req * C_cap_operation - (CompEvaporatingCAPSpd(CompSpdLB) + CompEvaporatingPWRSpd(CompSpdLB))); |
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.
Not sure what this math represents. It looks like:
CompSpdActual = CompSpdLow + ((CompSpdHi - CompSpdLo) / {some sum of capacity + power)) * something
Do I have that right? That seems wierd for a compressor speed calculation but I'll leave that to you.
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.
it intends to linearly interpolate between the CompSpdLo and CompSpdHi.
Previously it tries to match the demand of TU_load + Pipe_Q - Ncomp
to a compressor output of CompEvaporatingCAPSpd(i)
where i is the correct speed level. There's some linear interpolation going on when the demand does not equal to the compressor output at discrete speed level.
Now it tries to match TU_load + Pipe_Q
to CompEvaporatingCAPSpd(i) + CompEvaporatingPWRSpd(i)
. Ncomp term is the compressor power. CompEvaporatingPWRSpd(i) is the compressor power curve.
The two approaches are kinda equivalent but the second approach matches the quantity used in the limitTUCapacity calculation, so I can reason about where the related supplemental coil calculation went wrong.
I might change it back to the old way after I finish all the modifications.
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.
reverted it back to Q_evap_req = TU_load + Pipe_Q
, also this change above to calculate CompSpdActual
@yujiex @Myoldmopar it has been 28 days since this pull request was last updated. |
The difference in inlet temperature should be anticipated as it reflects the outdoor air load
@yujiex there are several comments in here where it looks like you were going to make some changes or double check things. I see commits made after that, but I think it would be good to go back through the comments and close up all the conversation. I ran locally just for kicks and got a few ESO big diffs in VRF files, which is expected. |
@Myoldmopar I will go over the comments again and confirm if all issues are addressed. |
@rraustad @Myoldmopar I went over the comments again and addressed them. Please see if they look okay to you. Thanks |
@yujiex @Myoldmopar it has been 28 days since this pull request was last updated. |
@yujiex @Myoldmopar it has been 28 days since this pull request was last updated. |
"{:.3T} < {:.3T}", | ||
CompEvaporatingCAPSpdMaxCurrentTsuc, | ||
CompEvaporatingCAPSpdMin)); | ||
} |
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.
Should add the recurring error format so this warning, however unlikely, does not fill the err file if it ever does report. Also, if this warning does report shouldn't the model use one of these capacities (CAPSpdMin?) to set other variables so that subsequent calculations can proceed?
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.
add the recurring error format in commit a2a0441 "Fix recurring error"
@@ -13012,6 +13031,9 @@ void VRFTerminalUnitEquipment::CalcVRF_FluidTCtrl(EnergyPlusData &state, | |||
_, | |||
_, | |||
state.dataHVACVarRefFlow->MaxHeatingCapacity(VRFCond)); | |||
// yujie: update TU air mass flow | |||
state.dataLoopNodes->Node(state.dataHVACVarRefFlow->VRFTU(VRFTUNum).VRFTUInletNodeNum).MassFlowRate = | |||
state.dataDXCoils->DXCoil(this->HeatCoilIndex).InletAirMassFlowRate; |
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.
This is not standard practice to update another model's operating conditions. Why doesn't the VRF model know what air flow is being 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.
In the original code, the air mass flow of the coil is updated here through FanSpdRatio
.
As a result, the rate of air flowing into the coil should be updated as well. This is added in this PR, in the original code, the thisDXCoil.InletAirMassFlowRate
is not adjusted when FanSpdRatio
changes.
Maybe it's okay to just update the AirMassFlow
after the SimDXCoil
?
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 think I would delete the new lines at 13034- 13046 and then follow suggestion below.
// yujie: correct mismatching in the air mass flow rate | ||
if (thisDXCoil.TotalHeatingEnergyRate > 0.0) { | ||
AirMassFlow = state.dataDXCoils->DXCoil(this->HeatCoilIndex).InletAirMassFlowRate; | ||
} |
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.
Why is there a mismatch in AirMassFlow? AirMassFlow is used in the LoadMet calculation on line 13112. That calculation should include any supply side AT Mixer flow if used on the supply side of this TU which would be different than the AirMassFlow used by the coil. See line 13082 above.
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.
Replace lines 13107 - 13111 with:
AirMassFlow = state.dataLoopNodes->Node(VRFTUOutletNodeNum).MassFlowRate;
This is the code flow:
SetAverageAirFlow(state, VRFTUNum, PartLoadRatio, OnOffAirFlowRatio); <-- sets TU inlet node mass flow
call ATMixer on inlet side if one exists
call fan and coils in correct order
call ATMixer on supply side if one exists
So now here each child component has passed air mass flow from it's inlet to outlet all the way through the TU. And the outlet node of the TU should be the mass flow rate used to calculate LoadMet.
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 moved the AirMassFlow updates to after SimDXCoil
where the coil gets its updated air flow rate value.
@@ -14546,6 +14573,7 @@ void VRFCondenserEquipment::VRFOU_CalcCompH( | |||
RoutineName); | |||
C_cap_operation = this->VRFOU_CapModFactor( | |||
state, Pipe_h_comp_in, Pipe_h_out_ave, max(min(MinOutdoorUnitPe, RefPHigh), RefPLow), T_suction + this->SH, T_suction + 8, IUMaxCondTemp - 5); | |||
C_cap_operation = 1.0; |
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.
This line overwrites the calculation at line 14574, why?
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.
It was used for debugging things. I forgot to remove it afterward. Sorry for the confusion.
@@ -13012,6 +13034,7 @@ void VRFTerminalUnitEquipment::CalcVRF_FluidTCtrl(EnergyPlusData &state, | |||
_, | |||
_, | |||
state.dataHVACVarRefFlow->MaxHeatingCapacity(VRFCond)); | |||
AirMassFlow = state.dataDXCoils->DXCoil(this->HeatCoilIndex).InletAirMassFlowRate; |
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 variable AirMassFlow is not used until the LoadMet calculation below at new line 13108. I suggest moving this down to just after the supplemental heating coil call at new line 13078 using:
AirMassFlow = state.dataLoopNodes->Node(VRFTUOutletNodeNum).MassFlowRate
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.
But the coil air flow is updated in SimDXCoil
, should it be the updated coil air mass flow or TU air mass flow (probably need to be updated due to changes in coil air mass flow?) at new line 13078?
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.
No it is not. The coil inlet mass flow rate is passed to the outlet node. In the calc function there is a local variable that is updated to represent the maximum coil air flow rate and at the end of the calc function the outlet T/w/H are calculated based on the inlet mass flow rate, via the PLR variable.
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.
In InitDXCoil is this at line 7259:
thisDXCoil.InletAirMassFlowRate = state.dataLoopNodes->Node(AirInletNode).MassFlowRate;
and in UpdateDXCoil is this at line 14193:
state.dataLoopNodes->Node(AirOutletNode).MassFlowRate = thisDXCoil.InletAirMassFlowRate;
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.
Oh, I do see a place in CalcVRFHeatingCoil_FluidTCtrl that updates the inlet mass flow at line 17291:
thisDXCoil.InletAirMassFlowRate = AirMassFlow * PartLoadRatio;
That does not look right. The TU should have placed a mass flow rate at the coil inlet.
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 is what should happen with the DX coil model. The parent object (e.g., VRF TU, PTHP, WindowAC) will set a mass flow rate at the inlet of the parent (see SetAverageAirFlow). That inlet is also an inlet node to the first child object. Let's say the first child is the DX coil. The parent calls the DX coil with a PLR (the mass flow rate has already been set). The DX coil will use the inlet mass flow rate, the fan operating mode, and the PLR passed to the model to set a coil max air flow rate in a local variable at the beginning of the Calc function (e.g., AirMassFlow = InletMassFlow / PLR for cycling fan or AirMassFlow = InletMassFlow for constant fan). Then the DXCoil Calc function models the coil at full load. At the end of the Calc function the outlet conditions are calculated as full output for cycling fan (i.e., T/w at full load) or the average output for constant fan based on PLR (i.e., average of full output for PLR part of the time step and inlet condition for (1-PLR) part of the time step). And in the Update function the inlet mass flow rate is passed to the outlet. There is no child component that I can think of that set's it's own mass flow rate at the inlet node. The child can use a local variable for performance calculations (because the DX coil model calculates performance at full load), but not set the inlet node condition.
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.
So when the child uses its local variable (fan speed ratio and its air flow, which might be different from the one passed in from the parent VRF TU) to calculate its own performance, then the coil heating rate and TU heating rate will be different due to the air flow rate difference? But for a simple case of TU containing a heating coil and a supply fan with no outdoor air load, shouldn't the TU and coil heating rate be roughly equal?
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 think the point here is that the coil should not set a flow rate, the parent does that. The coil does have a rated flow rate but is not required to operate at the rated flow rate. The coil should provide the output at the flow rate specified by the parent. And at that flow rate the fan speed ratio should be valid. Why is the coil using a different fan speed ratio than the parent thinks it should use? I have not studied this model so some of my comments may not apply. I'm just trying to give enough guidance that you can make proper choices for a corrective action. Most importantly, why can't this model calculate a coil capacity given the air flow provided by the parent?
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.
Somehow the CalVRFTUAirFlowRate_FluidTCtrl and SimDXCoil have different results for the fan speed ratio. Maybe I need to resolve this difference. Currently the former almost always sets the air flow rate at the maximum value while the coil thinks it's smaller.
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 found the issue why the air mass flow doesn't match between the TU and the coil: both CalVRFTUAirFlowRate_FluidTCtrl
(computes TU air mass flow) and the SimDXCoil
(computes coil air mass flow) calls the ControlVRFIUCoil
, which calculates the fan speed ratio. The input variable QCoilReq
has the MaxHeatCap
limit imposed in SimDXCoil as is shown in the following code chunk, while the CalVRFTUAirFlowRate_FluidTCtrl
doesn't
if (present(MaxHeatCap)) {
TotCapAdj = min(MaxHeatCap, TotCap * HeatingCapacityMultiplier);
TotCap = min(MaxHeatCap, TotCap);
} else {
TotCapAdj = TotCap * HeatingCapacityMultiplier;
}
QCoilReq = PartLoadRatio * TotCap;
I reverted the previous modification that overwrites TU air flow with coil ones. I instead added the MaxHeatCap operational argument in the CalVRFTUAirFlowRate_FluidTCtrl
and used it to limit QCoilReq like in the SimDXCoil. See commit 444eee3. The non-matching air mass flow is resolved.
TU air flow rate and coil air flow rate did not matching as the TU airflow calc doesn't have capacity limit like in the coil calculation
This reverts commit a2a0441.
@yujiex In the plots above, is outdoor air load taken into account? (or is the contribution of outdoor air load relatively small?) |
Hi @dareumnam , the plots were produced with "Heating Outdoor Air Flow Rate" in TU1 set to 0. This makes the outdoor air load to be 0. |
Thanks @yujiex, It seems like all the comments have been addressed properly. The test diffs are expected. After @rraustad's double-check, I think this is ready to be merged in. @Myoldmopar |
Thanks @dareumnam |
@yujiex are the "after" plots in the PR description up to date? If not please update as a sanity check. I have no further comments. |
Thanks for double-checking, @rraustad. After the plot is updated, this is ready to be merged in. @Myoldmopar |
Thanks @rraustad and @dareumnam, I just updated the plots |
All super happy here on a local build with develop pulled in. Merging this. Thanks @yujiex and all the reviewers! |
Pull request overview
Fixes the following issues
Before-after comparison of the outputs
The results of the fixes are shown in the following comparison plots
result data for the plots above (outdoor air load is zero as "Heating Outdoor Air Flow Rate" in TU1 is set to 0, coil and VRF capacity all 10023W for the first two comparisons. The OU capacity is 5000W for the third comparison):
compare results vrfHeating3.xlsx
NOTE: ENHANCEMENTS MUST FOLLOW A SUBMISSION PROCESS INCLUDING A FEATURE PROPOSAL AND DESIGN DOCUMENT PRIOR TO SUBMITTING CODE
regression diffs
The regression diffs appear in the models involving the component-based (FluidCtrl) VRFs including
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.