Change EquationFit coils to reference curves #8464

yzhou601 · 2021-01-12T00:42:07Z

Pull request overview

Closes Change EquationFit coils to reference curve objects #8352
Change Equationfit coils to reference curves
Create/modify curves if needed
Fix a defect in CurveManager where variables >3 are ignored

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.

…elop

Myoldmopar

Overall I think this is a great plan. I prefer to have quint-linear be added as a new object, but am curious what others think.

I noted what looks like an ugly defect (hope I'm wrong) in the curve manager but it's not anything wrong with your proposal here.

Myoldmopar · 2021-01-12T16:14:41Z

design/FY2021/EquationFitCoilCurveReference.md

+### Add quint-linear curve object? ###
+As described above, both quad-linear and quint-linear curves are needed for  Coil:Cooling:WaterToAirHeatPump:EquationFit, while other coils only require quad-linear curves. In order to satisfy Coil:Cooling:WaterToAirHeatPump:EquationFit curve specifications, we can either:
+ - Add quint-linear curves so that the curve type of every curve referred can be explicitly specified. This is a more straightforward path to move forward to.
+ - Change quad-linear curves to be more general as "multi-linear" or simply replace it by quint-linear curves, regarding to the similarity between quad-linear and quint linear curves. Users can simply add 0 coefficients to whatever variables that are not important or E+ can only read specific number of coefficients for specific input. Notice that there's no tri-linear or bi-linear curve types in Energyplus which might imply some preference to this approach in its first place (Is that true? Any example of using quad-linear curve type to read in tri-linear curves?).


My preference would be to have targeted curve types for each set of coefficients.

Right now the CurveValue function takes 1 required independent variable and 5 Objexx::Optional arguments. Then inside the function the values are passed into a curve worker function that then does operations to check if the arguments are present or not and override them with zeroes, then finally hit a big IF block on the curve type for evaluating the proper output.

The CurveValue function could eventually be refactored into individual functions that would reduce the amount of logic in each one and hopefully increase performance, especially considering how often curves are evaluated. With that in mind, I think it would be better then to keep each curve type separate.

Now here's an interesting aside.

The CurveValue function has 6 total possible independent variable arguments. 1 is required and 5 are optional. Inside that function, it switches depending on whether you are looking the value up in a table or evaluating a curve. If you are using a table, it passes all 6 arguments. However, if you are evaluating a curve, it only passes the first 3! Am I missing something?!

The condenser setpoint manager definitely uses a quad linear curve and calls CurveValue with 4 independent variables expecting them to be used....is the 4th just ignored?

Really though, am I missing something here @mjwitte @mitchute ?

Yeah, this looks like a bug to me. However, even if it were passed, it looks like the only thing the fourth variable is used for in PerformanceCurveObject is to do bounds checking.

EnergyPlus/src/EnergyPlus/CurveManager.cc

Line 2286 in 6163e5b

Real64 const V4(Var4.present() ? max(min(Var4, Curve.Var4Max), Curve.Var4Min) : 0.0); // 4th independent variable after limits imposed

V4 doesn't appear in any of the CurveType equations.

Nevermind, it does appear in the QuadLinear curve. So, yes, this is a bug.

@Myoldmopar Um, no, it appears that's been broken since Curve:Quadlinear was first added - long, long, ago. A quick series of unit tests of curve types should verify that.

There are a couple of rudimentary curve unit tests that don't even check the returned curve value, and this one has zero for the last two coefficients.

Fantastic 🤦

OK, so @yzhou601, for this feature to work, you'll need to modify the CurveValue logic to pass in more arguments. There could be unintended consequences (performance) to this, so it's worth discussing how you want to handle this. Do you want to open a separate issue and look at it yourself? Open a separate issue and request someone else on the team address it? Just address it as you work on this feature?

Oops, a stray click marked this all as resolved for a bit. Sorry about that. The last time I was in there, I think I fixed one of the other less commonly used curves and it caused diffs. Maybe that won't happen this time, but it is a real possibility.

@Myoldmopar Thanks, Edwin. As far as I know from the conversation, it doesn't sound like something complicated to address. I can try resolving it when I work on this feature, if I find any extra complication, I can try reaching out to one of you for some help. Does it sound good?

I'm happy to take a look at this separately if needed.

My suggestion is to hold off from doing a deep dive @mitchute, at least for now. @yzhou601 go ahead and work on getting this patched and let's just hope for the best that it doesn't cause any unintended consequences.

…rence curves

…ationfitcoil-curve

mjwitte · 2021-01-28T20:37:10Z

src/EnergyPlus/HeatPumpWaterToWaterSimple.cc

-            if (NumNums > 14) {
-                if (!DataIPShortCuts::lNumericFieldBlanks(15)) {
-                    GSHP(GSHPNum).refCOP = DataIPShortCuts::rNumericArgs(15);
+            GSHP(GSHPNum).CoolCapCurveIndex = GetCurveIndex(state, DataIPShortCuts::cAlphaArgs(6));


@yzhou601 The selected curves should be validated using CurveManager::CheckCurveDims. Here's an example in VRF. Also, I'm not sure if these curves are supposed to equal 1.0 with a certain set of inputs or not. If so, also use checkCurveIsNormalizedToOne to check and throw a warnings if it's not.

…ationfitcoil-curve

yzhou601

Some code walkthrough to help review.
No numerical diffs to justify on CI. CI only shows diffs on AUD, RDD and Table, table are different on object counts (curve objects added), RDD shows more input variables to look at, AUD shows more NumOfRVariable and numEMSActuatorsAvailable (not quite sure whether they're expected, can these be caused by new curve type that is actuable?).

yzhou601 · 2021-02-16T03:13:53Z

tst/EnergyPlus/unit/CurveManager.unit.cc

+    EXPECT_EQ(expected_value, CurveManager::CurveValue(*state, 1, var1, var2, var3, var4));
+}
+
+TEST_F(EnergyPlusFixture, QuintLinearCurve)


Unit test for new curve type

yzhou601 · 2021-02-16T03:14:47Z

tst/EnergyPlus/unit/CurveManager.unit.cc

+
+    double var1 = 1, var2 = 0.1, var3 = 20, var4 = 10;
+    double expected_value = -3.3333 + (0.1*1) + (38.9*0.1) + (0.1*20) + (0.5 * 10);
+    EXPECT_EQ(expected_value, CurveManager::CurveValue(*state, 1, var1, var2, var3, var4));


add curve value check for quad-linear curve bugfix (where 4th variable is ignored in calculations)

yzhou601 · 2021-02-16T03:19:54Z

tst/EnergyPlus/unit/SetPointManager.unit.cc

@@ -306,7 +306,7 @@ TEST_F(EnergyPlusFixture, SetPointManager_DefineCondEntSetPointManager)
                                                      "Dimensionless;           !- Input Unit Type for z",
                                                      "Curve:QuadLinear,",
                                                      "OptCondEntCurveName,     !- Name",
-                                                      "12.2711,                 !- Coefficient1 Constant",
+                                                      "13.2711,                 !- Coefficient1 Constant",


Unit test broke after the curve bugfix. Adjusted the constant to make it passing.

yzhou601 · 2021-02-16T03:24:09Z

src/EnergyPlus/CurveManager.cc

@@ -200,7 +201,7 @@ namespace CurveManager {
        {
            auto const SELECT_CASE_var(state.dataCurveManager->PerfCurve(CurveIndex).InterpolationType);
            if (SELECT_CASE_var == InterpTypeEnum::EvaluateCurveToLimits) {
-                CurveValue = PerformanceCurveObject(state, CurveIndex, Var1, Var2, Var3);
+                CurveValue = PerformanceCurveObject(state, CurveIndex, Var1, Var2, Var3, Var4, Var5, Var6);


Bugfix here.

Looks good.

yzhou601 · 2021-02-16T03:24:46Z

src/EnergyPlus/CurveManager.cc

@@ -269,6 +270,7 @@ namespace CurveManager {
        int NumQuadLinear;               // Number of quadratic linear curve objects in the input data file
        int NumCubicLinear;              // Number of cubic linear curve objects in the input file
        int NumQLinear;                  // Number of quad linear curve objects in the input data file
+        int NumQuintLinear;               // Number of quint linear curve objects in the input data file


Add quint-linear new curve type

yzhou601 · 2021-02-16T03:25:40Z

src/EnergyPlus/CurveManager.cc

-                ErrorsFound = true;
+            const int NumVar = 4;
+            std::string VarNames[NumVar] = {"w", "x", "y", "z"};
+            for (int i = 1; i <= NumVar; ++i) {


Refactor to simplify input checks when number of variables are large.

yzhou601 · 2021-02-16T03:30:31Z

src/EnergyPlus/HeatPumpWaterToWaterSimple.cc

@@ -291,20 +293,30 @@ namespace EnergyPlus::HeatPumpWaterToWaterSimple {
            if (state.dataHPWaterToWaterSimple->GSHP(GSHPNum).RatedPowerCool == DataSizing::AutoSize) {
                state.dataHPWaterToWaterSimple->GSHP(GSHPNum).ratedPowerCoolWasAutoSized = true;
            }
-            state.dataHPWaterToWaterSimple->GSHP(GSHPNum).CoolCap1 = DataIPShortCuts::rNumericArgs(5);


Change watertowater equationfit coils to reference curves.

yzhou601 · 2021-02-16T03:31:00Z

src/EnergyPlus/WaterToAirHeatPumpSimple.cc

@@ -363,24 +365,39 @@ namespace WaterToAirHeatPumpSimple {
            state.dataWaterToAirHeatPumpSimple->SimpleWatertoAirHP(HPNum).RatedCapCoolTotal = NumArray(3);
            state.dataWaterToAirHeatPumpSimple->SimpleWatertoAirHP(HPNum).RatedCapCoolSens = NumArray(4);
            state.dataWaterToAirHeatPumpSimple->SimpleWatertoAirHP(HPNum).RatedCOPCool = NumArray(5);
-            state.dataWaterToAirHeatPumpSimple->SimpleWatertoAirHP(HPNum).TotalCoolCap1 = NumArray(6);


Change watertoair equationfit coils to reference curves.

yzhou601 · 2021-02-16T03:32:12Z

src/ExpandObjects/epfilter.f90

@@ -17061,24 +17061,62 @@ SUBROUTINE CreateNewUnitary
         StringToReal(FldVal(base + ussCoolCoilSHROff)))
      ENDIF
      CALL AddToObjFld('Rated Cooling Coefficient of Performance',base + ussCoolCoilCOPOff,'')
-      CALL AddToObjStr('Total Cooling Capacity Coefficient 1','-9.149069561')


Change expand object structures of water-to-air equationfit coils

yzhou601 · 2021-02-16T03:40:47Z

src/Transition/CreateNewIDFUsingRulesV9_5_0.f90

@@ -398,6 +402,97 @@ SUBROUTINE CreateNewIDFUsingRules(EndOfFile,DiffOnly,InLfn,AskForInput,InputFile
                                                ' Radiant Exchange Method option IRTSurface is no longer valid. '// &
                                                'The air boundary will be modeled using the GroupedZones method.')
                 END IF
+
+             CASE('COIL:COOLING:WATERTOAIRHEATPUMP:EQUATIONFIT')


Transition of equationfit coil structure change from v9.4 to v9.5.

Myoldmopar · 2021-02-18T14:26:28Z

Well, from a first glance, this has a new build warning. It's super minor, but also annoying and won't be merged with a warning in it. I'm looking over the code changes again now.

Myoldmopar · 2021-02-18T14:27:55Z

The other CI issues are all fine, minor diffs about output variables.

Myoldmopar

Code looks good, I'll build and test the transition rules.

Myoldmopar · 2021-02-18T14:32:30Z

idd/Energy+.idd.in

-   N21, \field Cooling Power Consumption Coefficient 5
+        \type object-list
+        \object-list QuintvariateFunctions
+   A8, \field Cooling Power Consumption Curve Name


Yes, yes. Lots better here.

Myoldmopar · 2021-02-18T14:32:55Z

src/EnergyPlus/CurveManager.hh

-                                  Optional<Real64 const> Var4 = _  // 4th independent variable
+                                  Optional<Real64 const> Var4 = _,  // 4th independent variable
+                                  Optional<Real64 const> Var5 = _,  // 5th independent variable
+                                  Optional<Real64 const> Var6 = _  // 6th independent variable


Myoldmopar · 2021-02-18T14:33:14Z

src/EnergyPlus/CurveManager.cc

@@ -200,7 +201,7 @@ namespace CurveManager {
        {
            auto const SELECT_CASE_var(state.dataCurveManager->PerfCurve(CurveIndex).InterpolationType);
            if (SELECT_CASE_var == InterpTypeEnum::EvaluateCurveToLimits) {
-                CurveValue = PerformanceCurveObject(state, CurveIndex, Var1, Var2, Var3);
+                CurveValue = PerformanceCurveObject(state, CurveIndex, Var1, Var2, Var3, Var4, Var5, Var6);


Looks good.

Myoldmopar · 2021-02-18T14:33:50Z

src/EnergyPlus/HeatPumpWaterToWaterSimple.cc


-        Power = CoolPowerRated *
-                (CoolPowerCoeff1 + (func1 * CoolPowerCoeff2) + (func2 * CoolPowerCoeff3) + (func3 * CoolPowerCoeff4) + (func4 * CoolPowerCoeff5));
+        Power = CoolPowerRated * CurveValue(state, this->CoolPowCurveIndex, func1, func2, func3, func4);


Myoldmopar · 2021-02-18T14:34:36Z

tst/EnergyPlus/unit/SetPointManager.unit.cc

@@ -306,7 +306,7 @@ TEST_F(EnergyPlusFixture, SetPointManager_DefineCondEntSetPointManager)
                                                      "Dimensionless;           !- Input Unit Type for z",
                                                      "Curve:QuadLinear,",
                                                      "OptCondEntCurveName,     !- Name",
-                                                      "12.2711,                 !- Coefficient1 Constant",
+                                                      "13.2711,                 !- Coefficient1 Constant",


Myoldmopar · 2021-02-18T15:19:59Z

OK, built this locally no problem. I cleaned up the unused variable warning, which led to the Var6 argument being unused, so I marked it as maybe_unused for now. Someone should investigate that further. I transitioned a 9.4 file with a heat pump object and compared the old and new, everything looked good. I then ran the transitioned file and it ran fine with a clean error file. I resolved the conflict in the input rules file and have that committed locally. I'm going to go ahead and push it up and merge this now to avoid wasting our CI cycles just for an unused variable and minor conflict resolution.

@yzhou601 please check out the change I made to the PerformanceCurveObject function, if Var6 really isn't ever used then it shouldn't be passed in as an argument. If Var6 is supposed to be used, then something is wrong in the function logic. Open a follow-up issue as needed and address it in a separate PR.

Myoldmopar · 2021-02-18T15:20:54Z

(I pushed, merged and deleted very quickly so it is certainly possible that some of the github action CI tests will show weird failures)

yzhou601 · 2021-02-18T17:48:46Z

OK, built this locally no problem. I cleaned up the unused variable warning, which led to the Var6 argument being unused, so I marked it as maybe_unused for now. Someone should investigate that further. I transitioned a 9.4 file with a heat pump object and compared the old and new, everything looked good. I then ran the transitioned file and it ran fine with a clean error file. I resolved the conflict in the input rules file and have that committed locally. I'm going to go ahead and push it up and merge this now to avoid wasting our CI cycles just for an unused variable and minor conflict resolution.

@yzhou601 please check out the change I made to the PerformanceCurveObject function, if Var6 really isn't ever used then it shouldn't be passed in as an argument. If Var6 is supposed to be used, then something is wrong in the function logic. Open a follow-up issue as needed and address it in a separate PR.

@Myoldmopar Currently, there's no type of curve that utilizes 6 variables, so Var6 can only be possible to be used in the future. Didn't seem any issue for now.

Myoldmopar · 2021-02-18T17:59:35Z

In that case, can you open a followup issue to remove that extra argument for now? We don't want any runtime performance penalty from having that extra Optional argument in there if it really isn't ever used. Thanks!

yzhou601 · 2021-02-18T18:05:30Z

@Myoldmopar #8547

yzhou601 added 4 commits July 13, 2020 14:08

Merge branch 'develop' of https://github.com/NREL/EnergyPlus into dev…

40290e7

…elop

Merge branch 'develop' of https://github.com/NREL/EnergyPlus into dev…

11e5913

…elop

Merge branch 'develop' of https://github.com/NREL/EnergyPlus into dev…

9184238

…elop

Add NFP

228d782

yzhou601 added the NewFeature Includes code to add a new feature to EnergyPlus label Jan 12, 2021

yzhou601 self-assigned this Jan 12, 2021

yzhou601 added the IDDChange Code changes impact the IDD file (cannot be merged after IO freeze) label Jan 12, 2021

yzhou601 added this to the EnergyPlus 9.5.0 IOFreeze milestone Jan 12, 2021

move NFP to FY21

294d7f3

Myoldmopar reviewed Jan 12, 2021

View reviewed changes

yzhou601 and others added 15 commits January 12, 2021 19:23

bugfix on curve variable passing

25d9573

fix unit test

2bf07f5

Add quint linear curve, change water-to-air equationfit coils to refe…

97a1447

…rence curves

fix tests

bd36829

wwhp

051bb99

Merge branch 'develop' of https://github.com/NREL/EnergyPlus into equ…

37901b4

…ationfitcoil-curve

expand object and minor changes on testfiles

fd55faf

wwhp test files

db7ad68

expandobject bugfix

05352bf

bugfix2, added comments to generated idf

ce01eda

updated expandobject for unitary system template

773e151

updated docs

cc07e75

performance test

6a9ca3d

transition

3f13d21

Transition rule doc

74f1dea

mjwitte reviewed Jan 28, 2021

View reviewed changes

yzhou601 added 2 commits February 1, 2021 19:36

bugfix of transition, add curve dimension check

3ab4a08

Merge branch 'develop' of https://github.com/NREL/EnergyPlus into equ…

b805c78

…ationfitcoil-curve

yzhou601 marked this pull request as ready for review February 3, 2021 19:07

doc unique label

9c0a754

Merge branch 'develop' of https://github.com/NREL/EnergyPlus into equ…

390f984

…ationfitcoil-curve

yzhou601 requested a review from Myoldmopar February 9, 2021 17:14

yzhou601 added 2 commits February 15, 2021 19:16

Merge branch 'develop' of https://github.com/NREL/EnergyPlus into equ…

cc2308e

…ationfitcoil-curve

minor stuffs

f4c1e5e

yzhou601 commented Feb 16, 2021

View reviewed changes

Myoldmopar approved these changes Feb 18, 2021

View reviewed changes

Merge develop; resolve conflcit; fix unused variable

3ba2e34

Myoldmopar merged commit e92218e into develop Feb 18, 2021

Myoldmopar deleted the equationfitcoil-curve branch February 18, 2021 15:20

yzhou601 mentioned this pull request Feb 18, 2021

Remove unused Var6 in curve function #8547

Closed

3 tasks

shorowit mentioned this pull request Feb 22, 2021

Update to EnergyPlus 9.5.0 NREL/OpenStudio#4210

Closed

5 tasks

Change EquationFit coils to reference curves #8464

Change EquationFit coils to reference curves #8464

Conversation

yzhou601 commented Jan 12, 2021 • edited Loading

Pull request overview

Pull Request Author

Reviewer

Myoldmopar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yzhou601 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Myoldmopar commented Feb 18, 2021

Myoldmopar commented Feb 18, 2021

Myoldmopar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Myoldmopar commented Feb 18, 2021

Myoldmopar commented Feb 18, 2021

yzhou601 commented Feb 18, 2021

Myoldmopar commented Feb 18, 2021

yzhou601 commented Feb 18, 2021

yzhou601 commented Jan 12, 2021 •

edited

Loading

yzhou601 left a comment •

edited

Loading