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

fix Super high COP in VRFFluidTCtrl model #10752

Open
wants to merge 13 commits into
base: develop
Choose a base branch
from
Open

fix Super high COP in VRFFluidTCtrl model #10752

wants to merge 13 commits into from

Conversation

yujiex
Copy link
Collaborator

@yujiex yujiex commented Sep 19, 2024

Pull request overview

1. Fix of the super high COP problem

The following shows the distribution of heating and cooling COP before vs after the fix
image
eplusout_defect_output.csv
eplusout_fix.csv

COP is calculated as this->TotalHeatingCapacity divided by this->ElecHeatingPower and other auxiliary power.

this->OperatingHeatingCOP = (this->TotalHeatingCapacity) /
                            (this->ElecHeatingPower + this->CrankCaseHeaterPower + this->EvapCondPumpElecPower + this->DefrostPower);

this->ElecHeatingPower is calculated as compressor power and outdoor fan power this->ElecHeatingPower = this->Ncomp + this->OUFanPower;

Previously there were some problems of compressor power and fan electricity rate not considering cycling ratio, see #10427, #10105. Now that they're fixed, these terms have cycling ratios in them, while, the numerator term seems to have missed the multiplication of cycling ratio.

See the following comparison

// FluidTCtrl model
    this->TotalCoolingCapacity = TotalCondCoolingCapacity * CoolingPLR;
    this->TotalHeatingCapacity = TotalCondHeatingCapacity * HeatingPLR;
// curve-based model
    vrf.TotalCoolingCapacity = TotalCondCoolingCapacity * CoolingPLR * CyclingRatio;
    vrf.TotalHeatingCapacity = TotalCondHeatingCapacity * HeatingPLR * CyclingRatio;

The issue of super high COP is fixed by multiplying CyclingRatio in this->TotalCoolingCapacity and this->TotalHeatingCapacity.

2. Fix of the refrigerant warning

The issue is originated from the change of this line in PR#10416 the following line. The intention is to

image

The intention of the change is to impose a lower bound in solving for an evaporating temperature when the heating load is small. Such a lower bound is necessary as otherwise, in many time steps when the system is cycling, the OU evaporating temperature will be -72C, which is too low to be feasible. However, using the value of the input field "Variable Evaporating Temperature Minimum for Indoor Unit" as the lower bound will trigger a bunch of refrigerant warnings.

To fix the refrigerant flow problem, it is observed that the following line might have been a mistake CapMinPe = min(Pdischarge - this->CompMaxDeltaP, RefMinPe);. Pdischarge is the compressor discharge pressure,
this->CompMaxDeltaP is the maximum compressor pressure rise. Then Pdischarge - this->CompMaxDeltaP is minimum compressor suction pressure considering pressure rise bound. The RefMinPe is minimum refrigerant evaporating pressure, which is a pressure lower bound considering physical feasibility. CapMinPe should be no less than both of them, so it should be max of them, not min.

In the code base, there are three places where CapMinPe is calculated (shown in the following). Two of them are max, one is min, hinting that the min might be a bug.

image

After imposing the right pressure bound, the refrigerant warnings are mostly gone. The following are the eplusout.err files from the develop and the feature branch. A difference can be seen just from the file size.

fix_eplusout.err.zip
develop_eplusout.err.zip

3. Fix of the recurring warning issue

The warning message keep showing up is the non-recurring warning (which should only be shown once or a few times).

The excessive amount of warning is produced by this chunk of code. The condition this->errors[(int)RefrigError::SatSupDensity].count <= df->RefrigErrorLimitTest decides whether it prints this warning. The right hand side of the inequality is 1, while the left hand side overflows to negative at some point to -2147450880. From then on, the condition just keeps evaluates to true and the warning keeps showing up. It overflows as it is not a counter, but a sum of all previous counter values. This makes it get large very quickly. When it overflows, the counter value of df->SatErrCountGetSupHeatDensityRefrig is 65536. This this->errors[(int)RefrigError::SatSupDensity].count <= df->RefrigErrorLimitTest is 1 + 2 + ... + 65536, which overflowed.

++df->SatErrCountGetSupHeatDensityRefrig;
// send warning
this->errors[(int)RefrigError::SatSupDensity].count += df->SatErrCountGetSupHeatDensityRefrig;
// send warning
if (this->errors[(int)RefrigError::SatSupDensity].count <= df->RefrigErrorLimitTest) {
    ShowWarningMessage(
        state,
        format("{}: Refrigerant [{}] is saturated at the given conditions, saturated density at given temperature returned. **",
                routineName,
                this->Name));
    ShowContinueError(state, fmt::format("...Called From:{}", CalledFrom));
    ShowContinueError(state, format("Refrigerant temperature = {:.2R}", Temperature));
    ShowContinueError(state, format("Refrigerant pressure = {:.0R}", Pressure));
    ShowContinueError(state, format("Returned Density value = {:.3R}", saturated_density));
    ShowContinueErrorTimeStamp(state, "");
}

4. Cooling Coil Type [1] IP to SI unit conversion warning

The warning is produced because the code assumes there is a unit of measure inside the bracket and will attempt to perform unit conversion. Here this is a categorical variable and should not attempt to do unit conversion, so [] is not appropriate. The problem if fixed by changing the "[]" into "()" in this "Cooling Coil Type [1]".

Regression diffs

Table big diff and Table string diffs

There are 733 files have "ColumnHeadingDifference", leading to table big diffs and string diffs. This is caused by changing "Cooling Coil Type [1]" to "Cooling Coil Type (1)" in this feature branch. The diffs are expected.

meter diff

Meter diff happens in test idf "VariableRefrigerantFlow_FluidTCtrl_5Zone", in the following two fields.

  • Electricity:Facility
  • Electricity:HVAC
    The meter diff is caused by this change. The changes here will cause differences in compressor power, which will lead to changes in Electricity:HVAC and Electricity:Facility.
    image

eso diff

in US+SF+CZ4A+hp+crawlspace+IECC_2006_VRF, VariableRefrigerantFlow_FluidTCtrl_5Zone, and VariableRefrigerantFlow_FluidTCtrl_HR_5Zone, the following fields have eso diffs

  • VRF HEAT PUMP:VRF Heat Pump Total Cooling Rate
  • VRF HEAT PUMP:VRF Heat Pump Total Heating Rate
  • VRF HEAT PUMP:VRF Heat Pump Heating Electricity Rate
  • VRF HEAT PUMP:VRF Heat Pump Heating Electricity Energy
  • VRF HEAT PUMP:VRF Heat Pump Compressor Electricity Rate
  • VRF HEAT PUMP:VRF Heat Pump Outdoor Unit Evaporating Temperature

These diffs are expected as the numerator of the COP term (heating/cooling output) is changed in this feature: it is multiplied by the cycling ratio now. The changes in COP and heating cooling rate are both expected. Changes in Outdoor Unit Evaporating Temperature is due to the correction in refrigerant pressure lower bound, which is used in calculation of OU evaporating temperature. Heating electricity rate and energy difference happen in 1/21. Heating electricity rate is the sum of compressor power and outdoor unit fan power. As a result of the changing MinOutdoorUnitTe argument from max(this->IUEvapTempLow, CapMinTe) to CapMinTe in function VRFOU_CalcCompH, compressor power will change. So diffs in heating electricity rate is also expected.

err diffs

11 files have error diffs. This is because the following warning is removed as a result of the column header change of "Cooling Coil Type".

** Warning ** Unable to find a unit conversion from Cooling Coil Type [1] into IP units

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 yujiex added the Defect Includes code to repair a defect in EnergyPlus label Sep 19, 2024
@yujiex yujiex self-assigned this Sep 19, 2024
Copy link

⚠️ Regressions detected on macos-14 for commit f4c30c9

Regression Summary
  • ESO Big Diffs: 3

this->TotalCoolingCapacity = TotalCondCoolingCapacity * CoolingPLR;
this->TotalHeatingCapacity = TotalCondHeatingCapacity * HeatingPLR;
this->TotalCoolingCapacity = TotalCondCoolingCapacity * CoolingPLR * CyclingRatio;
this->TotalHeatingCapacity = TotalCondHeatingCapacity * HeatingPLR * CyclingRatio;
Copy link
Contributor

Choose a reason for hiding this comment

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

OK, I can see this one being missed because it's multiplied by PLR and looked correct at first glance. If COP came back in line then it's likely that all the electricity reports are correctly adjusting for cycling. Although it's not a given, because things like defrost, cranckcase heater and evap condenser pump power are small compared to compressor power, and are most likely 0 in the test files. I would check those. What else might have been missed? What happens to piping losses during cycling? What about something like compressor speed? should that also be adjusted by CyclingRatio?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Crankcase heater seems to have considered cycling

        VRFRTF = min(1.0, (CyclingRatio / PartLoadFraction));
...
        this->CrankCaseHeaterPower = this->CCHeaterPower * (1.0 - VRFRTF);

Defrost seems to have its own "runtime fraction", the FractionalDefrostTime:

                    this->DefrostPower = DefrostEIRTempModFac * (this->HeatingCapacity / 1.01667) * FractionalDefrostTime;

                } else { // Defrost strategy is resistive
                    this->DefrostPower = this->DefrostCapacity * FractionalDefrostTime;
                }

Copy link
Contributor

Choose a reason for hiding this comment

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

Crankcase heater power looks good. I think defrost power needs a CyclingRatio term. FractionalDefrostTime is the defrost time for a full time step. If the system doesn't run that time step (i.e., CyclingRatio = 0.000000001) then DefrostPower should be 0. You can check FractionalDefrostTime and see that it's a function of OA humidity ratio and has nothing to do with PLR or CyclingRatio. Of course you have to check my thinking before doing anything.

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 think you're right. FractionalDefrostTime is either from the user input or from OutdoorCoildw. It should probably multiply by cycling ratio.

For this->EvapCondPumpElecPower term, since the FluidTCtrl model doesn't have the "Evaporative Condenser Pump Rated Power Consumption" field like the curve-based VRF, this term is always 0.

Copy link

⚠️ Regressions detected on macos-14 for commit 2ef0177

Regression Summary
  • MTR Big Diffs: 10
  • Table Big Diffs: 13
  • Table String Diffs: 13
  • ESO Big Diffs: 8

Yujie Xu added 2 commits September 20, 2024 13:55
so that when the outdoor conditions cause a non-zero defrost power, AND the
system if off, defrost power would be set to zero.
Copy link

⚠️ Regressions detected on macos-14 for commit 038ba0e

Regression Summary
  • ESO Big Diffs: 3
  • Table Big Diffs: 4
  • Table String Diffs: 4
  • MTR Big Diffs: 2

Copy link

⚠️ Regressions detected on macos-14 for commit 2df905f

Regression Summary
  • ESO Big Diffs: 3
  • Table Big Diffs: 4
  • Table String Diffs: 4
  • MTR Big Diffs: 2

Copy link

⚠️ Regressions detected on macos-14 for commit 3dfc67e

Regression Summary
  • ESO Big Diffs: 3

Pdischarge is the compressor discharge pressure,
this->CompMaxDeltaP is the maximum compressor pressure rise
Pdischarge - this->CompMaxDeltaP is minimum compressor suction pressure

RefMinPe is minimum refrigerant evaporating pressure

These two terms are both lower bounds of the suction pressure

CapMinPe should be no less than both of them, so CapMinPe should be max of the
two LB, not min
Copy link

⚠️ Regressions detected on macos-14 for commit 1564bc2

Regression Summary
  • ESO Big Diffs: 3
  • MTR Big Diffs: 1
  • Table Big Diffs: 1

}
// adjust defrost power based on RTF
vrf.DefrostPower *= VRFRTF;
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks correct.

@@ -11592,7 +11591,8 @@ void VRFCondenserEquipment::CalcVRFCondenser_FluidTCtrl(EnergyPlusData &state, c
Tdischarge = this->refrig->getSatTemperature(state, max(min(Pdischarge, RefPHigh), RefPLow), RoutineName);

// Evaporative capacity ranges_Min
CapMinPe = min(Pdischarge - this->CompMaxDeltaP, RefMinPe);
// suction pressure lower bound need to be no less than both terms in the following
CapMinPe = max(Pdischarge - this->CompMaxDeltaP, RefMinPe);
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks correct. Not sure why this didn't show up during the original model develolpment.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, I'm surprised as well.

@@ -12237,6 +12236,7 @@ void VRFCondenserEquipment::CalcVRFCondenser_FluidTCtrl(EnergyPlusData &state, c
this->ElecHeatingPower = 0;
}
this->VRFCondRTF = VRFRTF;
this->DefrostPower *= VRFRTF;
Copy link
Contributor

@rraustad rraustad Sep 25, 2024

Choose a reason for hiding this comment

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

This looks correct. A plot of DefrostPower vs RTF or CyclingRatio would show it's working.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is a plot of heat pump defrost power vs cycling ratio

image

When the defrost power is non-zero, it is proportional to cycling ratio

The output file is here:
eplusout_defrost_vs_cyclingRatio.xlsx

Copy link
Contributor

Choose a reason for hiding this comment

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

Why is defrost electricity so small? at 6 E -9

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 used this test file. Its "Resistive Defrost Heater Capacity" is 0.0000001
US+SF+CZ5B+hp+slab+IECC_2021_VRFPhysics_v2_hrdsize_V2420.expidf.zip

@rraustad
Copy link
Contributor

This branch looks good now. I looked at fix_eplusout.err.zip and see this warning. Why is OutputReportTabular looking to convert a cooling coil type to IP units? This is probably a FluidTCtrl cooling coil. You could fix that here or file an issue.

** Warning ** Unable to find a unit conversion from Cooling Coil Type [1] into IP units

@rraustad
Copy link
Contributor

rraustad commented Sep 25, 2024

@yujiex these warnings (develop_eplusout.err.zip) are set up as recurring warnings so that the err file does not contain thousands of messages. However, this file does contain thousands of messages (300,000 warnings). Can you please investigate, either here or a different branch.

int RefrigErrorLimitTest = 1; // how many times error is printed with details before recurring called

++df->SatErrCountGetSupHeatDensityRefrig;
// send warning
this->errors[(int)RefrigError::SatSupDensity].count += df->SatErrCountGetSupHeatDensityRefrig;
// send warning
if (this->errors[(int)RefrigError::SatSupDensity].count <= df->RefrigErrorLimitTest) {
    ShowWarningMessage(

** Warning ** RefrigProps::getSupHeatDensity: Refrigerant [R410A] is saturated at the given conditions, saturated density at given temperature returned. **
**   ~~~   ** ...Called From:VRFOU_CapModFactor
**   ~~~   ** Refrigerant temperature = -3.55
**   ~~~   ** Refrigerant pressure = 962140
**   ~~~   ** Returned Density value = 27.216
**   ~~~   **  Environment=ANNUAL, at Simulation time=01/03 16:50 - 17:00

@yujiex
Copy link
Collaborator Author

yujiex commented Sep 25, 2024

Unable to find a unit conversion from Cooling Coil Type [1] into IP units

Thanks for noticing this. I will fix it here too

@yujiex
Copy link
Collaborator Author

yujiex commented Sep 25, 2024

@yujiex these warnings (develop_eplusout.err.zip) are set up as recurring warnings so that the err file does not contain thousands of messages. However, this file does contain thousands of messages (300,000 warnings). Can you please investigate, either here or a different branch.

int RefrigErrorLimitTest = 1; // how many times error is printed with details before recurring called

++df->SatErrCountGetSupHeatDensityRefrig;
// send warning
this->errors[(int)RefrigError::SatSupDensity].count += df->SatErrCountGetSupHeatDensityRefrig;
// send warning
if (this->errors[(int)RefrigError::SatSupDensity].count <= df->RefrigErrorLimitTest) {
    ShowWarningMessage(

** Warning ** RefrigProps::getSupHeatDensity: Refrigerant [R410A] is saturated at the given conditions, saturated density at given temperature returned. **
**   ~~~   ** ...Called From:VRFOU_CapModFactor
**   ~~~   ** Refrigerant temperature = -3.55
**   ~~~   ** Refrigerant pressure = 962140
**   ~~~   ** Returned Density value = 27.216
**   ~~~   **  Environment=ANNUAL, at Simulation time=01/03 16:50 - 17:00

Good point. I will make it recurring warnings

Copy link

⚠️ Regressions detected on macos-14 for commit 6bcb300

Regression Summary
  • Table Big Diffs: 733
  • Table String Diffs: 733
  • ERR: 11
  • ESO Big Diffs: 3
  • MTR Big Diffs: 1

the error message counter is a sum of all previous counter values
it overflows to negative
non-recurring warning keeps showing up after the overflow
use the counter, not the sum of counter in the predicate
@yujiex
Copy link
Collaborator Author

yujiex commented Sep 25, 2024

@rraustad I've investigated the two issues

SI to IP unit conversion warning

I fixed the warning of "unit conversion from Cooling Coil Type [1] into IP units", by changing the [] to () in the column header "Cooling Coil Type [1]". The warning is produced because the code assumes there is a unit of measure inside the bracket and will attempt to perform unit conversion. Here this is a categorical variable and should not attempt to do unit conversion, so [] is not appropriate. It is changed to () per suggestion from @JasonGlazer

recurring warning shows too many times

I also figured out the reason for the recurring warning message not showning as recurring. The messages keeps showing up is not produced by the ShowRecurringWarningErrorAtEnd, but by the following non-recurring part. The condition this->errors[(int)RefrigError::SatSupDensity].count <= df->RefrigErrorLimitTest decides wiether it will produce this warning. The right hand side of the inequality is 1, the left hand side overflows to negative at some point to -2147450880, and from then on, the condition just keeps evaluates to true and the warning keeps showing up. It overflows as it is not a counter, but a sum of all previous counter values. When it overflows, the counter value of df->SatErrCountGetSupHeatDensityRefrig is 65536. This this->errors[(int)RefrigError::SatSupDensity].count <= df->RefrigErrorLimitTest is 1 + 2 + ... + 65536, which overflowed.

I believe changing the condition to just if (df->SatErrCountGetSupHeatDensityRefrig <= df->RefrigErrorLimitTest) will solve the problem

++df->SatErrCountGetSupHeatDensityRefrig;
// send warning
this->errors[(int)RefrigError::SatSupDensity].count += df->SatErrCountGetSupHeatDensityRefrig;
// send warning
if (this->errors[(int)RefrigError::SatSupDensity].count <= df->RefrigErrorLimitTest) {
    ShowWarningMessage(
        state,
        format("{}: Refrigerant [{}] is saturated at the given conditions, saturated density at given temperature returned. **",
                routineName,
                this->Name));
    ShowContinueError(state, fmt::format("...Called From:{}", CalledFrom));
    ShowContinueError(state, format("Refrigerant temperature = {:.2R}", Temperature));
    ShowContinueError(state, format("Refrigerant pressure = {:.0R}", Pressure));
    ShowContinueError(state, format("Returned Density value = {:.3R}", saturated_density));
    ShowContinueErrorTimeStamp(state, "");
}

Copy link

⚠️ Regressions detected on macos-14 for commit 75dc8b3

Regression Summary
  • Table Big Diffs: 733
  • Table String Diffs: 733
  • ERR: 11
  • ESO Big Diffs: 3
  • MTR Big Diffs: 1

Copy link

⚠️ Regressions detected on macos-14 for commit 4943094

Regression Summary
  • Table Big Diffs: 733
  • Table String Diffs: 733
  • ERR: 11
  • ESO Big Diffs: 3
  • MTR Big Diffs: 1

@yujiex yujiex added this to the EnergyPlus 25.1 milestone Sep 30, 2024
Copy link

github-actions bot commented Oct 1, 2024

⚠️ Regressions detected on macos-14 for commit ff2a4ba

Regression Summary
  • Table Big Diffs: 733
  • Table String Diffs: 733
  • ERR: 11
  • ESO Big Diffs: 3
  • MTR Big Diffs: 1

Copy link

github-actions bot commented Oct 8, 2024

⚠️ Regressions detected on macos-14 for commit b617815

Regression Summary
  • Table Big Diffs: 735
  • Table String Diffs: 735
  • ERR: 11
  • ESO Big Diffs: 3
  • MTR Big Diffs: 1

@yujiex
Copy link
Collaborator Author

yujiex commented Oct 18, 2024

The fix for the high COP part (#10751) might have some problem. Let me investigate some more. Please hold off on reviewing. Thanks.

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
6 participants