-
Notifications
You must be signed in to change notification settings - Fork 377
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 VRFFluidCtrl heating cycling issue by moving cycling ratio calc in compressor spd calc #10416
base: develop
Are you sure you want to change the base?
Conversation
if (CapDiff > (Tolerance * Cap_Eva0)) NumIteCcap = 999; | ||
|
||
Ncomp = this->RatedCompPower * CurveValue(state, this->OUCoolingPWRFT(CounterCompSpdTemp), T_discharge, T_suction); | ||
if (CapDiff > (Tolerance * Cap_Eva0)) { |
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.
cycling ratio calculation is moved to here
@@ -11856,80 +11856,60 @@ void VRFCondenserEquipment::CalcVRFCondenser_FluidTCtrl(EnergyPlusData &state) | |||
CapMinTe + 8, | |||
this->IUCondensingTemp - 5); | |||
|
|||
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.
The cycling ratio calculation is removed here
Tdischarge, | ||
h_IU_cond_out_ave, | ||
this->IUCondensingTemp, | ||
this->IUEvapTempLow, |
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.
enforces a lower bound to the outdoor unit evaporating temperature
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 know the goto Label20
was present in the original code but we should try to eliminate goto statements when possible. This does not need to change in this branch and is a suggestion for future code. I moved out of the loop any code that does not need to be calculated each iteration.
Counter = 1;
m_air = this->OUAirFlowRate * RhoAir;
Ncomp_new = Ncomp;
while (std::abs(Ncomp_new - Ncomp) > (Tolerance * Ncomp)) && (Counter < 30)) {
}
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 feels more like a do-while loop, where the code inside label20 is executed once regardless of the condition, and then continues with a while loop. Although it is not a clean do-while loop as there's some operation inside the if (condition) {goto label}
branch, especially for the label23 code, before goto, the h_IU_cond_in
is recomputed differently. Let me think about it a bit.
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.
Would it be better to have a separate PR fixing goto ones? There are a few more instances in the physics-based VRF code. @rraustad
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 existing code is like the following after simplification. Let A = stuff A, B = stuff B and counter increment, C being the condition check, then this code will be evaluated to something like the following regex: A(CBA)*C.
label:
stuff A
if (some condition) {
stuff B
goto label
}
A while loop like this will evaluate to C(BAC)*
while (condition) {
stuff B
stuff A
}
a do-while loop like this will evaluate to (BAC)+
do {
stuff B
stuff A
} while (condition)
Maybe there's not a uniform clean transition, we'll need to reorganize the original code a little bit (some might be straightforward others might not).
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.
That's fine to make goto changes in a different branch. I was just pointing that 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.
Definitely. I will have a separate branch for goto fixes so that they could be tested separately. I feel it might have been more involved as label20 is nested inside label23. They probably should be considered together while making changes.
@yujiex @Myoldmopar it has been 28 days since this pull request was last updated. |
1 similar comment
@yujiex @Myoldmopar it has been 28 days since this pull request was last updated. |
NumIteCcap = 999; | ||
CyclingRatio = min(1.0, (TU_load + Pipe_Q) * C_cap_operation / Cap_Eva1); | ||
} else { | ||
CyclingRatio = 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 looks odd. This appears to say if the calculation converged set CyclingRatio = 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.
when it converges, the capacity Cap_Eva1
and demand Cap_Eva0
are close to each other, the system should run full-time, thus the cycling ratio should be 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.
If (TU_load + Pipe_Q) * C_cap_operation = Cap_Eva1
when this converges then I agree. I just can't follow the math well enough to see that clearly.
Before it converges:
CyclingRatio = min(1.0, (TU_load + Pipe_Q) * C_cap_operation / Cap_Eva1);
and after it converges:
CyclingRatio = 1.;
Then why can't the more detailed calculation happen all the time?
if (CapDiff > (Tolerance * Cap_Eva0)) NumIteCcap = 999;
CyclingRatio = min(1.0, (TU_load + Pipe_Q) * C_cap_operation / Cap_Eva1);
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 might have missed a "-Ncomp", it should be TU_load + Pipe_Q - Ncomp. TU_load + Pipe_Q - Ncomp and Q_evap_req should be equal. Maybe it's better to just use Cap_Eva0 / Cap_Eva1
to compute cycling ratio like in the cooling mode branch #10353
Pull request overview
The PR fixes the cycling ratio calculation of the VRF model in heating mode.
It also enforces a lower bound on the outdoor unit's evaporating temperature. The older version uses the refrigerant lowest saturation temperature as the lower bound, which might not be realistically reachable.
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.
Reviewer
This will not be exhaustively relevant to every PR.