V23.2.0-IOFreeze: IDD and transition fixes #10215

jmarrec · 2023-09-11T07:38:34Z

Pull request overview

Just upstreaming some stuff I found while updating the openstudio SDK to use v23.2.0-IOFreeze. Technically an IDDChange, but definitely not a breaking one.

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.

jmarrec · 2023-09-11T07:39:15Z

idd/Energy+.idd.in

+       \type object-list
+       \object-list UnivariateFunctions


Add object-list UnivariateFunctions for all Crankcase Heater Capacity curves

jmarrec · 2023-09-11T07:41:28Z

idd/Energy+.idd.in

@@ -57265,6 +57283,7 @@ Coil:Cooling:WaterToAirHeatPump:EquationFit,
        \note quadratic curve = a + b*PLR + c*PLR**2
        \note cubic curve = a + b*PLR + c*PLR**2 + d*PLR**3
        \note PLR = part load ratio (cooling load/steady state capacity)
+        \required-field


Add \requird-field for all new Part Load Fraction Correlation Curves. They are required. See #10043 (review)

Thanks @jmarrec, good catch.

jmarrec · 2023-09-11T07:42:13Z

idd/Energy+.idd.in

@@ -58175,7 +58194,7 @@ Coil:Heating:WaterToAirHeatPump:EquationFit,
        \memo Direct expansion (DX) heating coil for water-to-air heat pump (includes electric
        \memo compressor), single-speed, equation-fit model. Equation-fit model uses normalized
        \memo curves to describe the heat pump performance.
-        \min-fields 12
+        \min-fields 15


N7 + A8 with last one required => \min-fields 15

cf #10043 (review)

Also, thanks. I really look forward to tying the code and input schema together a little tighter as time marches on so that we don't have these accidental mismatches between the schema file and what the code is expecting.

jmarrec · 2023-09-11T07:42:24Z

src/Transition/InputRulesFiles/Rules23-1-0-to-23-2-0.md

+## Object Change: Coil:Heating:DX:VariableSpeed
+
+Field 1 to 13 remain the same.
+
+Field 14 is a new field named "Crankcase Heater Capacity Function of Temperature Curve Name" (labeled A6). It’s an optional field.
+
+Field 15 and onwards are the same.


Missing from the file.

Blah, good catch. Trying to blend together all the transition rules was an unusually intertwined mess, and this obviously just got missed in the process. Thanks

jmarrec · 2023-09-11T07:43:47Z

src/Transition/CreateNewIDFUsingRulesV23_2_0.f90

                  CoilLatentStuff(NumCoilLatentStuff)%CoolingCoilType = MakeUPPERCase(IDFRecords(Num)%Alphas(14))
-                  IF (CoilLatentStuff(NumCoilLatentStuff)%CoolingCoilType == 'COIL:COOLING:DX:VARIABLESPEED') CoilLatentStuff(NumCoilLatentStuff)%ActuallyCreateCurve = .true.


Do not create the PLF curve if it's not going to be used. The only types (4 in total) are

Coil:<Cooling/Heating>:WaterToAirHeatPump:EquationFit

Coil:<Cooling/Heating>:WaterToAirHeatPump:ParameterEstimation

@mjwitte this coil type should have a PLF curve?

Coil:Cooling:DX:VariableSpeed, A4, \field Energy Part Load Fraction Curve Name \required-field \type object-list \object-list UnivariateFunctions \note quadratic curve = a + b*PLR + c*PLR**2 \note cubic curve = a + b*PLR + c*PLR**2 + d*PLR**3 \note PLR = part load ratio (cooling load/steady state capacity)

jmarrec · 2023-09-11T07:44:29Z

src/Transition/CreateNewIDFUsingRulesV23_2_0.f90

                  CoilLatentStuff(NumCoilLatentStuff)%CoolingCoilType = MakeUPPERCase(IDFRecords(Num)%Alphas(11))
-                  IF (CoilLatentStuff(NumCoilLatentStuff)%CoolingCoilType == 'COIL:COOLING:WATERTOAIRHEATPUMP:PARAMETERESTIMATION') CoilLatentStuff(NumCoilLatentStuff)%ActuallyCreateCurve = .true.


ZoneHVAC:WaterToAirHeatPump only accepts Coil:<Cooling/Heating>:WaterToAirHeatPump:EquationFit (and VariableSpeed, but this one doesn't warrant a PLF curve).

@RKStrand

Note sure about the \note @RKStrand cf #10138 (review)

jmarrec · 2023-09-12T14:37:09Z

idd/Energy+.idd.in

+ N4,  \field Earth Tube Dimensionless Boundary Below
+      \note When set to 1.0, the below boundary is one earth tube radius below the earth tube.


@RKStrand Could you suggest the correct \note here please (I know what I wrote is wrong)
cf #10138 (review)

Suggested correction: When set to 1.0, the depth of the solution space is equal to the maximum vertical dimension above the earth tube. This maximum dimension above the earth tube is the depth of the earth tube minus three times the diameter of the earth tube itself. This sets the depth of the solution space below the earth tube node.

But in N3, the note says the maximum dimension above the earth tube is the maximum vertical dimension minus 1 earth tube radius? Should this note say "...minus two times the radius...". This stood out because it is so different from the note above. These 2 notes should look very similar?

\note When set to 1.0, the upper boundary is one earth tube radius below ground.

Yes, @rraustad, I was trying to write a quick response before having to head off to classes. I probably should have waited. Here is another attempt to clarify both of these notes:

For N3: When set to 1.0, the total thickness of the solution space above the earth tube node is equal to the maximum vertical dimension above the earth tube. This maximum dimension above the earth tube is the depth of the earth tube minus three times the radius of the earth tube itself. This sets the depth of the solution space above the earth tube node only. The earth tube node thickness is four times the earth tube radius so when this parameter is set to 1.0, the top node of the solution space is one earth tube radius below the ground surface. When this parameter is less than 1.0, the solution space thickness above the earth tube is simply this parameter times the maximum thickness defined by the earth tube depth and the earth tube radius.

For N4: When set to 1.0, the total thickness of the solution space below the earth tube node is equal to the maximum vertical dimension above the earth tube. This maximum dimension above the earth tube is the depth of the earth tube minus three times the radius of the earth tube itself. This sets the depth of the solution space below the earth tube node only. The earth tube node thickness is four times the earth tube radius so when this parameter is set to 1.0, the bottom node of the solution space is twice the earth tube depth minus the earth tube radius below the ground surface. When this parameter is less than 1.0, the solution space thickness below the earth tube is simply this parameter times the maximum thickness defined by the earth tube depth and the earth tube radius.

Hopefully this makes sense now? Sorry for all the confusion!

@jmarrec this still needs final polishing, right?

Yes, but I'm not sure what to put there. The IDD shouldn't replace the IO Ref guide and this seems like a ton of text.
Maybe just keep the first line?

Edit: I committed the one-line version and added suggestions for the full one below, apply them both or don't.

@jmarrec Yeah, I went a little too descriptive on my last suggestion. I think it could always be just the first line (sentence) as you suggested or it could be the first three lines (sentences). I don't have a strong preference either way, but agree that the note here get so long because that is why we have the IO Ref documentation.

…Parameters cf NREL/EnergyPlus#10215 (comment)

Myoldmopar

This is a good set of changes, and it's why we have the IO freeze in the first place. Thanks @jmarrec for finding them and dealing with the issues. I have a few comments before I mark it approved:

@jmarrec does the IDD still need a final tweak from the commentary between @RKStrand and @rraustad ?
@mjwitte could you scrutinize the IDD/transition changes to make sure this is still good to go? I am open to trying to transition our entire set of test files, running E+ on them, and comparing the outputs, but if it's not justified, I'll avoid it and spend my time elsewhere.
@jmarrec there was a Slack question about coils, does that relate to this work?

Myoldmopar · 2023-09-13T15:53:07Z

idd/Energy+.idd.in

+ N4,  \field Earth Tube Dimensionless Boundary Below
+      \note When set to 1.0, the below boundary is one earth tube radius below the earth tube.


@jmarrec this still needs final polishing, right?

Myoldmopar · 2023-09-13T15:53:57Z

idd/Energy+.idd.in

@@ -36467,7 +36467,7 @@ ZoneHVAC:WaterToAirHeatPump,
        \note Enter the type of performance specification object used to describe the multispeed coil or fan.
   A22; \field Design Specification Multispeed Object Name
        \type object-list
-        \object-list UnitarySystemPerformaceNames
+        \object-list UnitarySystemPerformanceNames


Yes, good catch

👀 typos need to watch out for @jmarrec 👀

Myoldmopar · 2023-09-13T15:55:26Z

idd/Energy+.idd.in

@@ -57265,6 +57283,7 @@ Coil:Cooling:WaterToAirHeatPump:EquationFit,
        \note quadratic curve = a + b*PLR + c*PLR**2
        \note cubic curve = a + b*PLR + c*PLR**2 + d*PLR**3
        \note PLR = part load ratio (cooling load/steady state capacity)
+        \required-field


Thanks @jmarrec, good catch.

Myoldmopar · 2023-09-13T15:56:57Z

idd/Energy+.idd.in

@@ -58175,7 +58194,7 @@ Coil:Heating:WaterToAirHeatPump:EquationFit,
        \memo Direct expansion (DX) heating coil for water-to-air heat pump (includes electric
        \memo compressor), single-speed, equation-fit model. Equation-fit model uses normalized
        \memo curves to describe the heat pump performance.
-        \min-fields 12
+        \min-fields 15


Also, thanks. I really look forward to tying the code and input schema together a little tighter as time marches on so that we don't have these accidental mismatches between the schema file and what the code is expecting.

Myoldmopar · 2023-09-13T15:58:45Z

src/Transition/InputRulesFiles/Rules23-1-0-to-23-2-0.md

+## Object Change: Coil:Heating:DX:VariableSpeed
+
+Field 1 to 13 remain the same.
+
+Field 14 is a new field named "Crankcase Heater Capacity Function of Temperature Curve Name" (labeled A6). It’s an optional field.
+
+Field 15 and onwards are the same.


Blah, good catch. Trying to blend together all the transition rules was an unusually intertwined mess, and this obviously just got missed in the process. Thanks

mjwitte · 2023-09-13T21:26:25Z

@mjwitte could you scrutinize the IDD/transition changes to make sure this is still good to go? I am open to trying to transition our entire set of test files, running E+ on them, and comparing the outputs, but if it's not justified, I'll avoid it and spend my time elsewhere.

@Myoldmopar The changes look correct. I can try to test some transitions tomorrow.

jmarrec · 2023-09-14T08:40:36Z

idd/Energy+.idd.in

@@ -24559,14 +24559,14 @@ ZoneEarthtube:Parameters,
      \maximum 10
      \default 3
 N3,  \field Earth Tube Dimensionless Boundary Above
-      \note When set to 1.0, the upper boundary is one earth tube radius below ground.
+      \note When set to 1.0, the total thickness of the solution space above the earth tube node is equal to the maximum vertical dimension above the earth tube.


Suggested change

\note When set to 1.0, the total thickness of the solution space above the earth tube node is equal to the maximum vertical dimension above the earth tube.

\note When set to 1.0, the total thickness of the solution space above the earth tube node is equal to the maximum vertical dimension above the earth tube.

\note This maximum dimension above the earth tube is the depth of the earth tube minus three times the radius of the earth tube itself.

\note This sets the depth of the solution space above the earth tube node only.

\note The earth tube node thickness is four times the earth tube radius so when this parameter is set to 1.0,

\note the top node of the solution space is one earth tube radius below the ground surface.

\note When this parameter is less than 1.0, the solution space thickness above the earth tube is simply

\note this parameter times the maximum thickness defined by the earth tube depth and the earth tube radius.

jmarrec · 2023-09-14T08:41:21Z

idd/Energy+.idd.in

-      \note When set to 1.0, the upper boundary is one earth tube radius below ground.
+      \default 1.0
+ N4,  \field Earth Tube Dimensionless Boundary Below
+      \note When set to 1.0, the total thickness of the solution space below the earth tube node is equal to the maximum vertical dimension above the earth tube.


Suggested change

\note When set to 1.0, the total thickness of the solution space below the earth tube node is equal to the maximum vertical dimension above the earth tube.

\note When set to 1.0, the total thickness of the solution space below the earth tube node is equal to the maximum vertical dimension above the earth tube.

\note This maximum dimension above the earth tube is the depth of the earth tube minus three times the radius of the earth tube itself.

\note This sets the depth of the solution space below the earth tube node only.

\note The earth tube node thickness is four times the earth tube radius so when this parameter is set to 1.0,

\note the bottom node of the solution space is twice the earth tube depth minus the earth tube radius below the ground surface.

\note When this parameter is less than 1.0, the solution space thickness below the earth tube is simply

\note this parameter times the maximum thickness defined by the earth tube depth and the earth tube radius.

jmarrec · 2023-09-14T08:46:04Z

3. @jmarrec there was a Slack question about coils, does that relate to this work?

Kindof. I did remove the superfluous curve in transition. Didn't tweak the IDD defaults though (This PR is just stuff I do on the fly while I implement the changes on the OS SDK side)

For reference the question I had on IDD defaults was:

I’m seeing different defaults for some of the Maximum Cycling Rate / Latent Time Capacity for one category of coils. I wonder why that is?

rraustad · 2023-09-14T10:34:28Z

@jmarrec there is no switch to turn off latent degradation (which is kinda silly) except for the model inputs themselves when 1 or more are 0. When latent degradation is used I think those non-zero defaults are fairly accurate. When the latent degradation model was added, 1 or more of these new inputs needed to have a default of 0 so as not to change the answer. How to resolve this across the coil models is another question.

Real64 CalcEffectiveSHR(EnergyPlusData &state, ... )

//  No moisture evaporation (latent degradation) occurs for runtime fraction of 1.0
//  All latent degradation model parameters cause divide by 0.0 if not greater than 0.0
//  Latent degradation model parameters initialize to 0.0 meaning no evaporation model used.
if (RTF >= 1.0 || Twet_Rated <= 0.0 || Gamma_Rated <= 0.0 || Nmax <= 0.0 || Tcl <= 0.0) {
    SHReff = SHRss;
    return SHReff;
}

…DD_fixes

Myoldmopar · 2023-09-20T13:50:28Z

I ran regressions locally and am not seeing any big table diffs at all. I suppose they were purely due to being behind develop. I fixed the tiny conflict and am ready to push my changes, wait a brief moment and merge this in. If anyone disagrees, speak up quick! Thanks @jmarrec.

Myoldmopar · 2023-09-20T14:21:10Z

I was hoping to catch this before Decent, but I missed, so I guess I'll just let Mac give a set of results and merge it if clean.

rraustad · 2023-09-20T15:45:17Z

src/Transition/CreateNewIDFUsingRulesV23_2_0.f90

                  IF (CoilLatentStuff(NumCoilLatentStuff)%CoolingCoilType == 'COIL:COOLING:WATERTOAIRHEATPUMP:PARAMETERESTIMATION') CoilLatentStuff(NumCoilLatentStuff)%ActuallyCreateCurve = .true.
                  IF (CoilLatentStuff(NumCoilLatentStuff)%CoolingCoilType == 'COIL:COOLING:WATERTOAIRHEATPUMP:EQUATIONFIT') CoilLatentStuff(NumCoilLatentStuff)%ActuallyCreateCurve = .true.
-                  IF (CoilLatentStuff(NumCoilLatentStuff)%CoolingCoilType == 'COIL:COOLING:WATERTOAIRHEATPUMP:VARIABLESPEEDEQUATIONFIT') CoilLatentStuff(NumCoilLatentStuff)%ActuallyCreateCurve = .true.


@mjwitte this coil type should have a PLF curve?

Coil:Cooling:WaterToAirHeatPump:VariableSpeedEquationFit, A6, \field Energy Part Load Fraction Curve Name \required-field \type object-list \object-list UnivariateFunctions \note quadratic curve = a + b*PLR + c*PLR**2 \note cubic curve = a + b*PLR + c*PLR**2 + d*PLR**3 \note PLR = part load ratio (cooling load/steady state capacity)

It already had one in v23.1, so it doesn't need a new one here. If I'm following this correctly.

Myoldmopar · 2023-09-20T16:04:53Z

This is coming out all clean on CI. I would be clicking merge pull request right now, but I will hold for a second after @rraustad 's comment. We are basically out of time, so unless that's a quick fix or an absolute red flag, we may just have to defer it.

mjwitte · 2023-09-20T16:16:33Z

@Myoldmopar This is good to go in as-is. I suppose for good measure, another round of running all test files through transition would be useful before packaging the release.

Myoldmopar · 2023-09-20T16:45:48Z

OK cool, let's merge then. Thanks all!

jmarrec added 3 commits August 30, 2023 16:53

Couple of IDD fixups: add object list and required-field for coil stuff

97fbfe8

Add missing "Coil:Heating:DX:VariableSpeed" to Rules23-1-0-to-23-2-0.md

b923a08

Transition: only create PLF curve for the coils that do need it

f3a1757

jmarrec added IDDChange Code changes impact the IDD file (cannot be merged after IO freeze) Developer Issue Related to cmake, packaging, installers, or developer tooling (CI, etc) labels Sep 11, 2023

jmarrec commented Sep 11, 2023

View reviewed changes

jmarrec requested review from Myoldmopar and nealkruis September 11, 2023 07:44

jmarrec added the Defect Includes code to repair a defect in EnergyPlus label Sep 11, 2023

jmarrec added 3 commits September 12, 2023 15:29

Typo in UnitarySystemPerformanceNames

b9c649c

Duplicated ZoneEarthTube:Parameters field name

344cf13

Note sure about the \note @RKStrand cf #10138 (review)

use a single document for the EarthTube

cdb9a0e

jmarrec commented Sep 12, 2023

View reviewed changes

english typo

ec88b1b

jmarrec added a commit to NREL/OpenStudio that referenced this pull request Sep 12, 2023

Unrelated: but there is a duplicated field name in the ZoneEarthTube:…

53a7202

…Parameters cf NREL/EnergyPlus#10215 (comment)

Myoldmopar reviewed Sep 13, 2023

View reviewed changes

Myoldmopar added this to the EnergyPlus 23.2 milestone Sep 13, 2023

Myoldmopar self-assigned this Sep 13, 2023

Use the one line version for ZoneEarthTube

b08f368

jmarrec commented Sep 14, 2023

View reviewed changes

Myoldmopar added 2 commits September 19, 2023 14:26

Pull in develop; resolve tiny conflict

67c4e7c

Merge remote-tracking branch 'origin/develop' into v23.2.0-IOFreeze-I…

1126f18

…DD_fixes

rraustad reviewed Sep 20, 2023

View reviewed changes

Myoldmopar merged commit 01f8719 into develop Sep 20, 2023
10 checks passed

Myoldmopar deleted the v23.2.0-IOFreeze-IDD_fixes branch September 20, 2023 16:45

		CoilLatentStuff(NumCoilLatentStuff)%CoolingCoilType = MakeUPPERCase(IDFRecords(Num)%Alphas(14))
		IF (CoilLatentStuff(NumCoilLatentStuff)%CoolingCoilType == 'COIL:COOLING:DX:VARIABLESPEED') CoilLatentStuff(NumCoilLatentStuff)%ActuallyCreateCurve = .true.

		N4, \field Earth Tube Dimensionless Boundary Below
		\note When set to 1.0, the below boundary is one earth tube radius below the earth tube.

-      \note When set to 1.0, the total thickness of the solution space above the earth tube node is equal to the maximum vertical dimension above the earth tube.
+      \note When set to 1.0, the total thickness of the solution space above the earth tube node is equal to the maximum vertical dimension above the earth tube.
+      \note This maximum dimension above the earth tube is the depth of the earth tube minus three times the radius of the earth tube itself.
+      \note This sets the depth of the solution space above the earth tube node only.
+      \note The earth tube node thickness is four times the earth tube radius so when this parameter is set to 1.0,
+      \note the top node of the solution space is one earth tube radius below the ground surface.
+      \note When this parameter is less than 1.0, the solution space thickness above the earth tube is simply
+      \note this parameter times the maximum thickness defined by the earth tube depth and the earth tube radius.

V23.2.0-IOFreeze: IDD and transition fixes #10215

V23.2.0-IOFreeze: IDD and transition fixes #10215

Conversation

jmarrec commented Sep 11, 2023

Pull request overview

Pull Request Author

Reviewer

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmarrec Sep 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

mjwitte commented Sep 13, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmarrec commented Sep 14, 2023

rraustad commented Sep 14, 2023 • edited Loading

Myoldmopar commented Sep 20, 2023

Myoldmopar commented Sep 20, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Myoldmopar commented Sep 20, 2023

mjwitte commented Sep 20, 2023

Myoldmopar commented Sep 20, 2023

jmarrec Sep 14, 2023 •

edited

Loading

rraustad commented Sep 14, 2023 •

edited

Loading