Skip to content
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

Fixing cooling mode cooling coil cooling rate much higher than heat pump cooling rate and OU capacity #10557

Merged
merged 31 commits into from
Sep 12, 2024

Conversation

yujiex
Copy link
Collaborator

@yujiex yujiex commented Jun 11, 2024

Pull request overview

For both the original and the fluid-control VRF model, terminal unit capacity should have been constrained by condenser capacity, where a maximum allowable terminal unit capacity is calculated in LimitTUCapacity(.) for each VRF system. The maximum allowable terminal unit capacity is stored in state.dataHVACVarRefFlow->MaxCoolingCapacity(VRFCond) and passed into SimDXCoil(.) which is NOT passed into CalcVRFCoolingCoil_FluidTCtrl(.), as compared with the non-fluid-control VRF model, CalcVRFCoolingCoil(.)

This PR will add an optional argument MaxCoolCap to CalcVRFCoolingCoil_FluidTCtrl(.) as is in CalcVRFCoolingCoil(.) and use this quantity to constrain the cooling coil capacity.

The following shows the effect of the fix: before the fix, cooling coil cooling rate is much higher than heat pump cooling rate; after the fix, the two cooling rate are similar. The test file used here is the US+SF+CZ4A+hp+crawlspace+IECC_2006_VRF.idf example file, using NY weather epw input. The "Rated Evaporative Capacity" of the "AirConditioner:VariableRefrigerantFlow:FluidTemperatureControl" is adjusted to 500W.

image

Regression diff

The following files have regression diff

  • DOASDXCOIL_wADPBFMethod
  • DOASDXCOIL_wADPBFMethod_NoReturnPath
  • DOAToVRF
  • HVACTemplate-5ZoneVRF
  • US+SF+CZ4A+hp+crawlspace+IECC_2006_VRF
  • VRFMultispeedFan
  • VariableRefrigerantFlow_5Zone
  • VariableRefrigerantFlow_5Zone_wAirloop
  • VariableRefrigerantFlow_FluidTCtrl_5Zone
  • VariableRefrigerantFlow_FluidTCtrl_HR_5Zone
  • VariableRefrigerantFlow_FluidTCtrl_wSuppHeater_5Zone
  • VariableRefrigerantFlow_wSuppHeater_5Zone
  • _DOASDXCOIL_wUserSHRMethod

ESO difference

The following files have ESO diffs in Zone VRF Air Terminal Total(Latent) Cooling(Heating) Rate. These are expected as a result of the changes in the total and latent system output calculation which fixed the problem of total != sensible + latent output.

  • DOASDXCOIL_wADPBFMethod
  • DOASDXCOIL_wADPBFMethod_NoReturnPath
  • DOAToVRF
  • HVACTemplate-5ZoneVRF
  • VRFMultispeedFan
  • VariableRefrigerantFlow_5Zone
  • VariableRefrigerantFlow_5Zone_wAirloop
  • VariableRefrigerantFlow_wSuppHeater_5Zone
  • _DOASDXCOIL_wUserSHRMethod

The 4 files with fluidTCtrl VRF model (
US+SF+CZ4A+hp+crawlspace+IECC_2006_VRF, VariableRefrigerantFlow_FluidTCtrl_5Zone, VariableRefrigerantFlow_FluidTCtrl_HR_5Zone, VariableRefrigerantFlow_FluidTCtrl_wSuppHeater_5Zone
) have difference in many more fields as follows. The core difference is the coil cooling rate and the VRF air terminal cooling rate. The cooling coil cooling rate difference (due to a capacity limit added in this feature) will also cause difference in compressor side cooling output, such as cooling rate, cooling electricity rate, etc. Piping correction is also affected as it's calculated as (coil cooling rate) / (coil cooling rate + piping loss). The coil cooling rate difference will also cause the temperature and air flow difference in corresponding nodes. The coil cooling rate difference will affect zone temperature as well (In the extreme case, when OU capacity is very small, the zone might not be able to control to setpoint temperature any more. This won't happen if coil has unlimited capacity like before the fix). Zone temperature difference would result in slight difference in heating coil and supplemental heating coil heating rate as well.

  • Zone Air Temperature
  • Zone Air Relative Humidity
  • Zone Air Temperature
  • Zone Air Relative Humidity
  • Zone Air System Sensible Heating Rate
  • Zone Air System Sensible Cooling Rate
  • Zone Predicted Sensible Load to Setpoint Heat Transfer Rate
  • Zone Predicted Sensible Load to Heating Setpoint Heat Transfer Rate
  • Zone Predicted Sensible Load to Cooling Setpoint Heat Transfer Rate
  • Fan Electricity Rate
  • Cooling Coil Total Cooling Rate
  • Cooling Coil Sensible Cooling Rate
  • Cooling Coil Latent Cooling Rate
  • Cooling Coil Runtime Fraction
  • Zone VRF Air Terminal Cooling Electricity Rate
  • Zone VRF Air Terminal Cooling Electricity Energy
  • Zone VRF Air Terminal Total Cooling Rate
  • Zone VRF Air Terminal Sensible Cooling Rate
  • Zone VRF Air Terminal Latent Cooling Rate
  • Zone VRF Air Terminal Heating Electricity Rate
  • Zone VRF Air Terminal Heating Electricity Energy
  • Zone VRF Air Terminal Total Heating Rate
  • Zone VRF Air Terminal Sensible Heating Rate
  • Zone VRF Air Terminal Latent Heating Rate
  • VRF Heat Pump Total Cooling Rate
  • VRF Heat Pump Total Heating Rate
  • VRF Heat Pump Cooling Electricity Rate
  • VRF Heat Pump Cooling Electricity Energy
  • VRF Heat Pump Heating Electricity Rate
  • VRF Heat Pump Heating Electricity Energy
  • VRF Heat Pump Compressor Electricity Rate
  • VRF Heat Pump Compressor Rotating Speed
  • VRF Heat Pump Outdoor Unit Condensing Temperature
  • VRF Heat Pump Outdoor Unit Evaporating Temperature
  • System Node Standard Density Volume Flow Rate
  • System Node Wetbulb Temperature
  • System Node Standard Density Volume Flow Rate

EIO difference

EIO difference are in the four idfs with FluidTCtrl model. There are some warmup differences in each zone in the model. This is expected as the coil cooling rate difference will change the zone temperature, which affects the warmup convergence.

Meter diff

Meter diffs happen in two of the idfs with VRF FluidTCtrl object. It is expected as the cooling coil cooling rate difference will also affect its electricity rate, and corresponding HVAC, Facility electricity rate part of which is coil electricity rate.

Table string diffs

These table diffs happen in the four idfs with VRF FluidTCtrl objects. The diffs are in warmup statistics and electricity rate, which are explained in the eio diff and meter diff sections.

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)
  • Label the PR with at least one of: Defect, Refactoring, NewFeature, Performance, and/or DoNoPublish
  • Pull requests that impact EnergyPlus code must also include unit tests to cover enhancement or defect repair
  • 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
  • If changes fix a defect, the fix should be demonstrated in plots and descriptions
  • 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.

  • Perform a Code Review on GitHub
  • If branch is behind develop, merge develop and build locally to check for side effects of the merge
  • If defect, verify by running develop branch and reproducing defect, then running PR and reproducing fix
  • If feature, test running new feature, try creative ways to break it
  • CI status: all green or justified
  • Check that performance is not impacted (CI Linux results include performance check)
  • Run Unit Test(s) locally
  • Check any new function arguments for performance impacts
  • Verify IDF naming conventions and styles, memos and notes and defaults
  • If new idf included, locally check the err file and other outputs

yujiex and others added 5 commits August 30, 2023 15:51
Follow the heating mode case to use the air flow rate calculated from the
ControlVRFIUCoil, this makes coil cooling rate bounded by the OU capacity,
although the heat pump cooling rate and the coil cooling rate are still not
exactly the same.

Piping correction doesn't need to bound the load either, similar to the case in
heating mode
@yujiex yujiex added the Defect Includes code to repair a defect in EnergyPlus label Jun 11, 2024
@yujiex yujiex added this to the EnergyPlus 24.2 milestone Jun 11, 2024
@yujiex yujiex self-assigned this Jun 11, 2024
@yujiex yujiex changed the title Vrf fluid fix cooling tu cap Fixing cooling mode cooling coil cooling rate much higher than heat pump cooling rate and OU capacity Jun 11, 2024
@yujiex yujiex marked this pull request as ready for review June 27, 2024 21:44
@@ -231,7 +231,7 @@ void SimDXCoil(EnergyPlusData &state,
CalcDXHeatingCoil(state, DXCoilNum, PartLoadRatio, fanOp, AirFlowRatio, MaxCap);
} break;
case HVAC::CoilVRF_FluidTCtrl_Cooling: {
CalcVRFCoolingCoil_FluidTCtrl(state, DXCoilNum, HVAC::CompressorOp::On, FirstHVACIteration, PartLoadRatio, fanOp, CompCycRatio, _, _);
CalcVRFCoolingCoil_FluidTCtrl(state, DXCoilNum, HVAC::CompressorOp::On, FirstHVACIteration, PartLoadRatio, fanOp, CompCycRatio, _, _, MaxCap);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure why the max capacity is not always passed into the coil. It's either 1.0e+20 or it's a value. That's for another day.

Copy link
Collaborator Author

@yujiex yujiex Jun 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just changed MaxCoolCap to regular non-optional arguments. @rraustad

@@ -11374,9 +11374,6 @@ void VRFCondenserEquipment::CalcVRFCondenser_FluidTCtrl(EnergyPlusData &state)
if (Q_c_TU_PL > CompEvaporatingCAPSpdMax) {
// Required load is beyond the max system capacity

Q_c_TU_PL = CompEvaporatingCAPSpdMax;
TU_CoolingLoad = CompEvaporatingCAPSpdMax;
this->TUCoolingLoad = TU_CoolingLoad;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should TU_CoolingLoad be reset here or not? This var is used at line 11385 below. For a VRF system with 1 TU this might make sense but for multiple TUs this will likely have no impact.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It shouldn't be reset here. Similar to the situation in the heating side, which we fixed a while ago: #10154
I copied in the relative discussion here:
image

This upper bounding will cause the compressor to reduce its output at more extreme weather, which is bad. So this upper bounding needs to be removed.

state.dataHVACVarRefFlow->MaxHeatingCapacity(VRFCond) = 0.0;
this->HeatingCapacity = 0.0; // Include the piping loss
this->PipingCorrectionHeating = 1.0; // 1 means no piping loss
state.dataHVACVarRefFlow->MaxHeatingCapacity(VRFCond) = 0.0; // yujie: might be wrong here too, should be MaxCap = 1e+20
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please check where MaxHeatingCapacity and MaxCoolingCapacity get set to MaxCap(1.0e+20). That will answer your question. It should either be set to MaxCap or not set at all. Maybe this is the reason that MaxCap is an optional argument, because it's 0 at times. If this var were never set to 0, and only set to MaxCap when reset is needed, and only reduced when the system is overloaded, then MaxCap could always be passed to the coil model and NOT be an optional argument.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just changed MaxCoolCap to non-optional.
I think this initialization here (state.dataHVACVarRefFlow->MaxHeatingCapacity(VRFCond) = 0.0) might be just an error. The default for capacity limit should be a really large number, so that no limit is placed.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you checked where MaxHeatingCapacity and MaxCoolingCapacity are set to MaxCap(1.0e+20)? Or if you believe this initialization is an error, I think you can correct the code line and delete the comment in this code.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should be 1.0e+20. I'll change it and remove the comment.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dareumnam. I corrected the code line and removed the comment on the 4th. Sorry I forgot to ping you. Please see if it looks okay. Thanks

@rraustad
Copy link
Contributor

This change did improve the results based on the data shown in the description. This may be for a different issue, but how can coil capacity ever exceed HP capacity? Isn't it sum of coil capacity + piping losses = HP capacity? So coil capacity should always be less than or equal to HP capacity.

@yujiex
Copy link
Collaborator Author

yujiex commented Jun 27, 2024

This change did improve the results based on the data shown in the description. This may be for a different issue, but how can coil capacity ever exceed HP capacity? Isn't it sum of coil capacity + piping losses = HP capacity? So coil capacity should always be less than or equal to HP capacity.

Indeed, I'll look into this. There might still be something not quite right. Coil cooling rate should be no more than heat pump cooling rate

@yujiex
Copy link
Collaborator Author

yujiex commented Jun 28, 2024

This change did improve the results based on the data shown in the description. This may be for a different issue, but how can coil capacity ever exceed HP capacity? Isn't it sum of coil capacity + piping losses = HP capacity? So coil capacity should always be less than or equal to HP capacity.

I investigated this and found the issue might be caused by this 0.65 min fan speed ratio constraint.

    FanSpdRatioMin = min(max(OAMassFlow / Garate, 0.65), 1.0); // ensure that coil flow rate is higher than OA flow rate

After removing this constraint, the difference become smaller and the coil cooling rate is slightly smaller than HP cooling rate

image

@rraustad

Yujie Xu added 2 commits June 30, 2024 22:40
move the MaxCap(1e+20) to DataGlobalConstants, to avoid circular inclusion issue
@Myoldmopar Myoldmopar assigned Myoldmopar and unassigned yujiex Jul 3, 2024
@Myoldmopar
Copy link
Member

@yujiex in addition to the diffs in a handful of files, there are 3 unit tests that are now failing and must be dealt with before this goes much further. And at least one of the test failures is quite a significant change. We can discuss on technicalities if needed.

@yujiex
Copy link
Collaborator Author

yujiex commented Jul 10, 2024

@yujiex in addition to the diffs in a handful of files, there are 3 unit tests that are now failing and must be dealt with before this goes much further. And at least one of the test failures is quite a significant change. We can discuss on technicalities if needed.

Hi @Myoldmopar. The unit test failing is caused by this change (if you revert this part, the unit test can run through).

image

Previously the coil TotalCoolingEnergyRate and SensCoolingEnergyRate uses the InletAirMassFlowRate (shown in the following code chunk) instead of the AirMassFlow computed using the FanSpdRatio output from function ControlVRFIUCoil

        Real64 AirMassFlowRate = thisDXCoil.InletAirMassFlowRate;
        // Coil total/sensible/latent cooling rates
        CalcComponentSensibleLatentOutput(AirMassFlowRate,
                                          InletAirDryBulbTemp,
                                          InletAirHumRat,
                                          OutletAirTemp,
                                          OutletAirHumRat,
                                          thisDXCoil.SensCoolingEnergyRate,
                                          thisDXCoil.LatCoolingEnergyRate,
                                          thisDXCoil.TotalCoolingEnergyRate);

This might be wrong. The air flow should be changed to meet the cooling demand.

For example, in heating mode, the heating energy rate is actually computed using the AirMassFlow from function ControlVRFIUCoil

        ControlVRFIUCoil(state,
                         DXCoilNum,
                         QCoilReq,
                         thisDXCoil.InletAirTemp,
                         thisDXCoil.InletAirHumRat,
                         thisDXCoil.CondensingTemp,
                         AirMassFlowMin,
                         FanSpdRatio,
                         OutletAirHumRat,
                         OutletAirTemp,
                         OutletAirEnthalpy,
                         ActualSH,
                         ActualSC);
        AirMassFlow = FanSpdRatio * thisDXCoil.RatedAirMassFlowRate(Mode);
        ...
        thisDXCoil.TotalHeatingEnergyRate = AirMassFlow * (OutletAirEnthalpy - InletAirEnthalpy) * PartLoadRatio;

Copy link

github-actions bot commented Sep 4, 2024

⚠️ Regressions detected on macos-14 for commit ecfbb2d

Regression Summary
  • ESO Big Diffs: 11
  • ESO Small Diffs: 2
  • EIO: 4
  • Table Big Diffs: 4
  • Table String Diffs: 4
  • MTR Big Diffs: 2

Copy link

github-actions bot commented Sep 5, 2024

⚠️ Regressions detected on macos-14 for commit ae8bcbf

Regression Summary
  • ESO Big Diffs: 11
  • ESO Small Diffs: 2
  • EIO: 4
  • Table Big Diffs: 4
  • Table String Diffs: 4
  • MTR Big Diffs: 2

Copy link

github-actions bot commented Sep 7, 2024

⚠️ Regressions detected on macos-14 for commit 7787de8

Regression Summary
  • ESO Big Diffs: 11
  • ESO Small Diffs: 2
  • EIO: 4
  • Table Big Diffs: 4
  • Table String Diffs: 4
  • MTR Big Diffs: 2

@dareumnam
Copy link
Collaborator

@rraustad, Do you have any further comments on this? I think it's good to go after @Myoldmopar's final review, unless you have additional feedback.

@Myoldmopar
Copy link
Member

This has now got conflicts from all the other merges. @yujiex could you resolve them? If so this might be ready.

@yujiex
Copy link
Collaborator Author

yujiex commented Sep 11, 2024

@Myoldmopar I will resolve the conflict in a bit (on bus now)

@yujiex
Copy link
Collaborator Author

yujiex commented Sep 11, 2024

@Myoldmopar I just resolved the conflict.

@Myoldmopar
Copy link
Member

Thanks, let's see how CI responds in like 30 minutes.

Copy link

⚠️ Regressions detected on macos-14 for commit e392e07

Regression Summary
  • EIO: 6
  • ESO Big Diffs: 13
  • MTR Big Diffs: 4
  • Table Big Diffs: 6
  • Table String Diffs: 6
  • ESO Small Diffs: 2
  • ERR: 1

@Myoldmopar
Copy link
Member

image

OK OK 35 minutes not 30.

So there are 2 more files now showing big diffs. I wonder what's caused that. Do you have any idea?

@yujiex
Copy link
Collaborator Author

yujiex commented Sep 11, 2024

@Myoldmopar It's strange, as the additional diffs happen in "CentralChillerHeaterSystem_Cooling_Heating" and "CentralChillerHeaterSystem_Simultaneous_Cooling_Heating", which doesn't have the VRF objects. Could it be because I'm not up-to-date with the develop?

I just pulled develop and again and run regression on those two locally, they don't show diffs.

@Myoldmopar
Copy link
Member

Hmmm. Now that you mention it, maybe so. I was just bragging about how we wouldn't see diffs just from being behind develop. But maybe there's still a way. When the CI system is doing the build, it checks out the current develop as the baseline. Because CI is a bit bogged down right now, develop has definitely been moving fast. So yeah, it's possible. In most scenarios it won't be happening, just if develop moves really really fast.

@yujiex
Copy link
Collaborator Author

yujiex commented Sep 11, 2024

Hmmm. Now that you mention it, maybe so. I was just bragging about how we wouldn't see diffs just from being behind develop. But maybe there's still a way. When the CI system is doing the build, it checks out the current develop as the baseline. Because CI is a bit bogged down right now, develop has definitely been moving fast. So yeah, it's possible. In most scenarios it won't be happening, just if develop moves really really fast.

seems develop is getting many things merged in today

Copy link

⚠️ Regressions detected on macos-14 for commit f4ba8d2

Regression Summary
  • ESO Big Diffs: 11
  • ESO Small Diffs: 2
  • EIO: 4
  • ERR: 1
  • Table Big Diffs: 4
  • Table String Diffs: 4
  • MTR Big Diffs: 2

@Myoldmopar
Copy link
Member

Back to the 11 ESO big diffs like above. Seems like that took care of it. Thanks @yujiex

@yujiex
Copy link
Collaborator Author

yujiex commented Sep 12, 2024

Thanks @Myoldmopar

@Myoldmopar
Copy link
Member

This has a conflict in the unit test. I'm doing a final quick test on #10416 and then try to resolve the conflict. If it's minor I'll handle it, but if it's more complex, I'll toss it back over to you.

@Myoldmopar
Copy link
Member

I resolved the test conflicts and everything is passing locally. I'm going to stop for lunch and let CI catch up and get fresh results to confirm I've got this ready. If CI is clean this can go in.

Copy link

⚠️ Regressions detected on macos-14 for commit 268a433

Regression Summary
  • ESO Big Diffs: 11
  • ESO Small Diffs: 2
  • EIO: 4
  • ERR: 1
  • Table Big Diffs: 4
  • Table String Diffs: 4
  • MTR Big Diffs: 2

@yujiex
Copy link
Collaborator Author

yujiex commented Sep 12, 2024

Thanks for handling the conflict @Myoldmopar

@Myoldmopar
Copy link
Member

Looks like I didn't break anything with my conflict resolution, merging this. Thanks @yujiex

@Myoldmopar Myoldmopar merged commit 1b9d24d into develop Sep 12, 2024
7 checks passed
@Myoldmopar Myoldmopar deleted the VRFFluidFixCoolingTUCap branch September 12, 2024 20:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Defect Includes code to repair a defect in EnergyPlus
Projects
None yet
9 participants