#4963 - AirLoopHVACUnitarySystem Default Supply Air Flow Rate Method When No Cooling or Heating is Required to None #4971

jmarrec · 2023-09-18T19:12:51Z

Pull request overview

Potential fix for AirLoopHVACUnitarySystem: Look at changes in Fan energy use between 3.6.1 and current develop #4963
When Method is None or empty, doesn't matte…r if the flow rate is Autosized, it takes precendence

cf https://github.com/NREL/EnergyPlus/blob/4502b0c6561281693f837217eb99339372b86abd/src/EnergyPlus/UnitarySystem.cc#L6080-L6081

Marking Developer Issue as this only affects develop (not released yet)

Follow up to

AirLoopHVACUnitarySystem set Method During XXX Operation #4873

Pull Request Author

Model API Changes / Additions
Any new or modified fields have been implemented in the EnergyPlus ForwardTranslator (and ReverseTranslator as appropriate)
Model API methods are tested (in src/model/test)
EnergyPlus ForwardTranslator Tests (in src/energyplus/Test)
If a new object or method, added a test in NREL/OpenStudio-resources: Add Link
If needed, added VersionTranslation rules for the objects (src/osversion/VersionTranslator.cpp)
Verified that C# bindings built fine on Windows, partial classes used as needed, etc.
All new and existing tests passes
If methods have been deprecated, update rest of code to use the new methods

Labels:

If change to an IDD file, add the label IDDChange
If breaking existing API, add the label APIChange
If deemed ready, add label Pull Request - Ready for CI so that CI builds your PR

Review Checklist

This will not be exhaustively relevant to every PR.

Perform a Code Review on GitHub
Code Style, strip trailing whitespace, etc.
All related changes have been implemented: model changes, model tests, FT changes, FT tests, VersionTranslation, OS App
Labeling is ok
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

jmarrec · 2023-09-19T14:24:25Z

@joseph-robertson can you remind me why we didn't apply the same treatment to "WhenNoHeatingOrCoolingIsRequired" as we did for DuringCooling / DuringHeating in #4873 ?

joseph-robertson · 2023-09-19T15:42:04Z

@joseph-robertson can you remind me why we didn't apply the same treatment to "WhenNoHeatingOrCoolingIsRequired" as we did for DuringCooling / DuringHeating in #4873 ?

I don't recall a specific reason, but certainly seems like an oversight.

mdahlhausen · 2023-09-20T18:11:26Z

Taking an example model
FullServiceRestaurant-90.1-2019

This is what OS 3.6.1 generates

OS:AirLoopHVAC:UnitarySystem,
  {ae7d7030-e043-4c80-9019-4ebb3cf8b4aa}, !- Handle
  Dining ZN PSZ-AC Unitary AC,            !- Name
  ,                                       !- Supply Air Flow Rate Method During Cooling Operation
  Autosize,                               !- Supply Air Flow Rate During Cooling Operation {m3/s}
  ,                                       !- Supply Air Flow Rate Per Floor Area During Cooling Operation {m3/s-m2}
  ,                                       !- Fraction of Autosized Design Cooling Supply Air Flow Rate
  ,                                       !- Design Supply Air Flow Rate Per Unit of Capacity During Cooling Operation {m3/s-W}
  ,                                       !- Supply Air Flow Rate Method During Heating Operation
  Autosize,                               !- Supply Air Flow Rate During Heating Operation {m3/s}
  ,                                       !- Supply Air Flow Rate Per Floor Area during Heating Operation {m3/s-m2}
  ,                                       !- Fraction of Autosized Design Heating Supply Air Flow Rate
  ,                                       !- Design Supply Air Flow Rate Per Unit of Capacity During Heating Operation {m3/s-W}
  ,                                       !- Supply Air Flow Rate Method When No Cooling or Heating is Required
  Autosize,                               !- Supply Air Flow Rate When No Cooling or Heating is Required {m3/s}
  ,                                       !- Supply Air Flow Rate Per Floor Area When No Cooling or Heating is Required {m3/s-m2}

This is what the OS 3.7.0-alpha build on this PR generates:

OS:AirLoopHVAC:UnitarySystem,
  {9a4152e4-2c35-4621-b22a-35d8bdde5b09}, !- Handle
  Dining ZN PSZ-AC Unitary AC,            !- Name
  SupplyAirFlowRate,                      !- Supply Air Flow Rate Method During Cooling Operation
  Autosize,                               !- Supply Air Flow Rate During Cooling Operation {m3/s}
  ,                                       !- Supply Air Flow Rate Per Floor Area During Cooling Operation {m3/s-m2}
  ,                                       !- Fraction of Autosized Design Cooling Supply Air Flow Rate
  ,                                       !- Design Supply Air Flow Rate Per Unit of Capacity During Cooling Operation {m3/s-W}
  SupplyAirFlowRate,                      !- Supply Air Flow Rate Method During Heating Operation
  Autosize,                               !- Supply Air Flow Rate During Heating Operation {m3/s}
  ,                                       !- Supply Air Flow Rate Per Floor Area during Heating Operation {m3/s-m2}
  ,                                       !- Fraction of Autosized Design Heating Supply Air Flow Rate
  ,                                       !- Design Supply Air Flow Rate Per Unit of Capacity During Heating Operation {m3/s-W}
  None,                                   !- Supply Air Flow Rate Method When No Cooling or Heating is Required
  ,                                       !- Supply Air Flow Rate When No Cooling or Heating is Required {m3/s}

The performance results appear to be the same. But I'm confused as to the default behavior in the OS 3.6.1 instance. It appears the Heating/Cooling modes defaulted to SupplyAirFlowRate with the flow rate set to Autosize, where the NoHeatingCooling mode defaulted to None.

jmarrec · 2023-09-21T05:05:13Z

The "good" news is that one or our regression test has differences (see #4977) so we have something to test with. I might reach out to get one or two OSMs from you.

I linked to the bit of E+ code that explains the difference (and in the commit messages), but that wasn't very clear.
I think the confusion is from E+ and would be fixed if the IDD made fields required, and Throw in the get input routine when you have incompatible flow/method fields combos

If I put autosize for the flow rate field, and leave the method field empty, it treats it as being like method=None and it takes precedence, so I get 0

jmarrec · 2023-09-22T13:22:10Z

resources/model/OpenStudio.idd

@@ -11902,7 +11902,7 @@ OS:AirLoopHVAC:UnitarySystem,
       \key FlowPerFloorArea
       \key FractionOfAutosizedCoolingValue
       \key FlowPerCoolingCapacity
-       \default None
+       \required-field


Make all Supply Air Flow Method fields required.

jmarrec · 2023-09-22T13:24:44Z

src/model/AirLoopHVACUnitarySystem.hpp

@@ -142,7 +142,7 @@ namespace model {
    boost::optional<double> designSupplyAirFlowRatePerUnitofCapacityDuringHeatingOperation() const;

    /** In EnergyPlus 8.3.0 and above this property maps to the EnergyPlus field "No Load Supply Air Flow Rate Method" **/
-    boost::optional<std::string> supplyAirFlowRateMethodWhenNoCoolingorHeatingisRequired() const;
+    std::string supplyAirFlowRateMethodWhenNoCoolingorHeatingisRequired() const;


jmarrec · 2023-09-22T13:24:53Z

src/model/AirLoopHVACUnitarySystem.hpp

+    /** Sets the Heating Coil. If the "Supply Air Flow Rate Method During Heating Operation" was "None", switch to "SupplyAirFlowRate"
+      * and autosize the "Supply Air Flow Rate During Heating Operation" field */
    bool setHeatingCoil(const HVACComponent& heatingCoil);


Docstring everywhere

jmarrec · 2023-09-22T13:25:46Z

src/model/AirLoopHVACUnitarySystem.hpp

+    /** Sets the field and switches "Supply Air Flow Rate Method During Cooling Operation" (coolingSAFMethod) to "SupplyAirFlowRate" */
    bool setSupplyAirFlowRateDuringCoolingOperation(double supplyAirFlowRateDuringCoolingOperation);
-    OS_DEPRECATED(3, 7, 0) void resetSupplyAirFlowRateDuringCoolingOperation();
+    /** Sets the field and switches the coolingSAFMethod to "SupplyAirFlowRate" */


@kbenne should we return false if the unitary doesn't have a cooling coil set already?

jmarrec · 2023-09-22T13:26:29Z

src/osversion/VersionTranslator.cpp

+      } else if (iddname == "OS:AirLoopHVAC:UnitarySystem") {
+
+        // NOTE: With 23.2.0-IOFreeze, we always have a change anyways cause some fields were removed
+
+        auto iddObject = idd_3_7_0.getObject(iddname);
+        IdfObject newObject(iddObject.get());
+
+        for (size_t i = 0; i < object.numFields(); ++i) {
+          if ((value = object.getString(i))) {
+            newObject.setString(i, value.get());
+          }
+        }


VT is complicated.... I'm going to break it down.

First, copy everything.

jmarrec · 2023-09-22T13:34:14Z

src/osversion/VersionTranslator.cpp

+        } else if (!noCoolHeatSAFMethod) {
+
+          // Blank is equivalent to None here, no question
+          newObject.setString(noCoolHeatSAFMethodIndex, "None");


If there isn't a method, E+ treats it the same as None, so just use that.

jmarrec · 2023-09-22T13:34:42Z

src/osversion/VersionTranslator.cpp

+        } else {
+          std::string noCoolHeatSAFMethodUC = ascii_to_upper_copy(*noCoolHeatSAFMethod);
+          if (noCoolHeatSAFMethodUC != "NONE") {
+            auto it = std::find_if(noCoolHeatSAFMethodChoicesUC.cbegin(), noCoolHeatSAFMethodChoicesUC.cend(),
+                                   [&noCoolHeatSAFMethodUC](auto& s) { return s == noCoolHeatSAFMethodUC; });
+            if (it == noCoolHeatSAFMethodChoicesUC.cend()) {
+              LOG(Error, "For AirLoopHVACUnitarySystem '"
+                           << object.nameString()
+                           << "', Unrecognized Supply Air Flow Rate Method When No Cooling or Heating is Required=" << *noCoolHeatSAFMethod);
+            } else {
+              auto dist = std::distance(noCoolHeatSAFMethodChoicesUC.cbegin(), it);
+              const size_t index = noCoolHeatSAFMethodIndex + 1 + dist;
+              if ((value = object.getString(index))) {
+                newObject.setString(index, *value);
+              } else {
+                LOG(Error, "For AirLoopHVACUnitarySystem '" << object.nameString()
+                                                            << "', Supply Air Flow Rate Method When No Cooling or Heating is Required is '"
+                                                            << *noCoolHeatSAFMethod << "' but associated field is empty");
+              }
+            }
+          }
+        }


If there is a method, do the same as the cooling and heating coil: try to get corresponding numeric field and use that, or use zero if not found.

jmarrec · 2023-09-22T13:35:19Z

src/model/test/AirLoopHVACUnitarySystem_GTest.cpp

+TEST_F(ModelFixture, AirLoopHVACUnitarySystem_SupplyAirFlowRateMethodDuringOperation_NoCoolHeat) {
+  // Test for #4695 - AirLoopHVACUnitarySystem: Supply Air Flow Rate Method During <XXX> Operation should be set via related setters/autosize


lotsa model testing to make sure we enforce a "Method/FlowField" combo.

jmarrec · 2023-09-22T13:36:29Z

src/osversion/test/3_7_0/test_vt_UnitarySystem_SAFMethods.py

(I used a python script to create the VT test models this time (I've been using ruby until now). Just because it was easier for me and because I could)

jmarrec · 2023-09-22T13:37:03Z

src/osversion/test/VersionTranslator_GTest.cpp

@@ -2548,3 +2549,200 @@ TEST_F(OSVersionFixture, update_3_6_1_to_3_7_0_GroundHeatExchangerVertical) {
  EXPECT_EQ(3.2, uka.getDouble(6).get());                                                       // Average Amplitude of Surface Temperature
  EXPECT_EQ(8.0, uka.getDouble(7).get());                                                       // Phase Shift of Minimum Surface Temperature
 }
+
+TEST_F(OSVersionFixture, update_3_6_1_to_3_7_0_UnitarySystem_SAFMethods_default) {


Lotsa VT testing, not going to comment line by line unless someone asks me to.

jmarrec · 2023-09-22T13:56:05Z

Running reg tests: 3.6.1, develop, this PR:

% EUI change compared to 3.6.1:

joseph-robertson

So the two main changes in this PR are to (1) setSupplyAirFlowRateMethodWhenNoCoolingorHeatingisRequired("None") in the ctor, and (2) apply the same treatment to "WhenNoHeatingOrCoolingIsRequired"?

joseph-robertson · 2023-09-25T17:26:19Z

src/model/AirLoopHVACUnitarySystem.cpp

@@ -1391,7 +1402,7 @@ namespace model {
    OS_ASSERT(ok);
    getImpl<detail::AirLoopHVACUnitarySystem_Impl>()->setSupplyAirFlowRateMethodDuringCoolingOperation("None");
    getImpl<detail::AirLoopHVACUnitarySystem_Impl>()->setSupplyAirFlowRateMethodDuringHeatingOperation("None");
-    autosizeSupplyAirFlowRateWhenNoCoolingorHeatingisRequired();
+    getImpl<detail::AirLoopHVACUnitarySystem_Impl>()->setSupplyAirFlowRateMethodWhenNoCoolingorHeatingisRequired("None");


Now consistent with "XXXMethodDuringCooling / Heating".

jmarrec · 2023-09-26T11:25:09Z

So the two main changes in this PR are to (1) setSupplyAirFlowRateMethodWhenNoCoolingorHeatingisRequired("None") in the ctor, and (2) apply the same treatment to "WhenNoHeatingOrCoolingIsRequired"?

Yes.

…r if the flow rate is Autosized, it takes precendence cf https://github.com/NREL/EnergyPlus/blob/4502b0c6561281693f837217eb99339372b86abd/src/EnergyPlus/UnitarySystem.cc#L6080-L6081

…lds required-field in the OpenStudio.idd

…h the IOFreeze branch)

…e" all the time

``` ZERO Which is: 0 unitary.getDouble(OS_AirLoopHVAC_UnitarySystemFields::FractionofAutosizedDesignCoolingSupplyAirFlowRate).get() Which is: 1.03613e-317 /Users/jenkins/git/OpenStudioIncr ```

jmarrec

Dont understand why mac fails, will try to get CI to spit it out for me

src/osversion/test/VersionTranslator_GTest.cpp

…E_EQ

ci-commercialbuildings · 2023-09-26T15:37:57Z

CI Results for ff08894:

Ubuntu 20.04:
Ubuntu 22.04:
- OpenStudio-3.7.0-alpha+ff08894284-Ubuntu-22.04-x86_64.deb
- OpenStudio-3.7.0-alpha+ff08894284-Ubuntu-22.04-x86_64.tar.gz
Darwin:
- OpenStudio-3.7.0-alpha+ff08894284-Darwin-x86_64.dmg
- OpenStudio-3.7.0-alpha+ff08894284-Darwin-x86_64.tar.gz
Windows:
- OpenStudio-3.7.0-alpha+ff08894284-Windows.exe
- OpenStudio-3.7.0-alpha+ff08894284-Windows.tar.gz
Code Coverage:

jmarrec · 2023-09-27T13:54:58Z

Pfff, I merged this one by mistake, I'm sorry. I'm going to have to fix the ctest on develop / another PR.

jmarrec · 2023-09-27T14:14:05Z

Done in 50fe58a

jmarrec added Developer Issue component - HVAC Pull Request - Ready for CI This pull request if finalized and is ready for continuous integration verification prior to merge. labels Sep 18, 2023

jmarrec self-assigned this Sep 18, 2023

jmarrec force-pushed the 4963_Unitary_DefaultWhenNoHeatingOrCoolingRequired branch from c855d5a to ccb000d Compare September 21, 2023 11:20

jmarrec requested a review from kbenne September 22, 2023 13:20

jmarrec commented Sep 22, 2023

View reviewed changes

jmarrec requested a review from joseph-robertson September 22, 2023 14:04

joseph-robertson approved these changes Sep 25, 2023

View reviewed changes

jmarrec added 13 commits September 26, 2023 13:26

Potential fix for #4963 - When Method is None or empty, doesn't matte…

7e8f062

…r if the flow rate is Autosized, it takes precendence cf https://github.com/NREL/EnergyPlus/blob/4502b0c6561281693f837217eb99339372b86abd/src/EnergyPlus/UnitarySystem.cc#L6080-L6081

AirLoopHVAC:UnitarySystem: Make all 3 Supply Air Flow Rate Method fie…

34001c3

…lds required-field in the OpenStudio.idd

Stub required-fields changes + add docstrings

ff0573a

Cleanup impl + changes

70ced91

Implement required-field changes

2bad678

Start VT for the UnitarySystem required-field (this will conflict wit…

717a91e

…h the IOFreeze branch)

Add Gtest and FT update

42e6512

Update VT: When "No Load Method" is blank, that's defnitely like "Non…

debaee4

…e" all the time

Write extensive VT tests (using a python script this time!)

2589657

Fix VT for tests to pass

8309dd9

Update release notes [ci skip]

c404add

Mac does not like the EXPECT_EQ(ZERO) witht he constexpr

50c9ea9

``` ZERO Which is: 0 unitary.getDouble(OS_AirLoopHVAC_UnitarySystemFields::FractionofAutosizedDesignCoolingSupplyAirFlowRate).get() Which is: 1.03613e-317 /Users/jenkins/git/OpenStudioIncr ```

Use EXPECT_DOUBLE_EQ. Mac still unhappy

ce0d9c6

jmarrec force-pushed the 4963_Unitary_DefaultWhenNoHeatingOrCoolingRequired branch from d29d060 to ce0d9c6 Compare September 26, 2023 11:26

jmarrec commented Sep 26, 2023

View reviewed changes

src/osversion/test/VersionTranslator_GTest.cpp Outdated Show resolved Hide resolved

src/osversion/test/VersionTranslator_GTest.cpp Outdated Show resolved Hide resolved

src/osversion/test/VersionTranslator_GTest.cpp Outdated Show resolved Hide resolved

Print the unitary system for the failing mac CI tests in EXPECT_DOUBL…

ff08894

…E_EQ

jmarrec merged commit e31381d into develop Sep 27, 2023

jmarrec deleted the 4963_Unitary_DefaultWhenNoHeatingOrCoolingRequired branch September 27, 2023 13:54

jmarrec added a commit that referenced this pull request Sep 27, 2023

Hotfix #4971 - fix VersionTranslator and test

50fe58a

jmarrec mentioned this pull request Oct 2, 2023

AirLoopHVACUnitarySystem: Look at changes in Fan energy use between 3.6.1 and current develop #4963

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#4963 - AirLoopHVACUnitarySystem Default Supply Air Flow Rate Method When No Cooling or Heating is Required to None #4971

#4963 - AirLoopHVACUnitarySystem Default Supply Air Flow Rate Method When No Cooling or Heating is Required to None #4971

jmarrec commented Sep 18, 2023

jmarrec commented Sep 19, 2023

joseph-robertson commented Sep 19, 2023

mdahlhausen commented Sep 20, 2023

jmarrec commented Sep 21, 2023 •

edited

Loading

jmarrec Sep 22, 2023

jmarrec Sep 22, 2023

jmarrec Sep 22, 2023

jmarrec Sep 22, 2023 •

edited

Loading

jmarrec Sep 22, 2023

jmarrec Sep 22, 2023

jmarrec Sep 22, 2023

jmarrec Sep 22, 2023

jmarrec Sep 22, 2023

jmarrec Sep 22, 2023

jmarrec commented Sep 22, 2023

joseph-robertson left a comment

joseph-robertson Sep 25, 2023

jmarrec commented Sep 26, 2023

jmarrec left a comment

ci-commercialbuildings commented Sep 26, 2023 •

edited

Loading

jmarrec commented Sep 27, 2023

jmarrec commented Sep 27, 2023

		TEST_F(ModelFixture, AirLoopHVACUnitarySystem_SupplyAirFlowRateMethodDuringOperation_NoCoolHeat) {
		// Test for #4695 - AirLoopHVACUnitarySystem: Supply Air Flow Rate Method During <XXX> Operation should be set via related setters/autosize

#4963 - AirLoopHVACUnitarySystem Default Supply Air Flow Rate Method When No Cooling or Heating is Required to None #4971

#4963 - AirLoopHVACUnitarySystem Default Supply Air Flow Rate Method When No Cooling or Heating is Required to None #4971

Conversation

jmarrec commented Sep 18, 2023

Pull request overview

Pull Request Author

Review Checklist

jmarrec commented Sep 19, 2023

joseph-robertson commented Sep 19, 2023

mdahlhausen commented Sep 20, 2023

jmarrec commented Sep 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmarrec Sep 22, 2023 • 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

jmarrec commented Sep 22, 2023

joseph-robertson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmarrec commented Sep 26, 2023

jmarrec left a comment

Choose a reason for hiding this comment

ci-commercialbuildings commented Sep 26, 2023 • edited Loading

jmarrec commented Sep 27, 2023

jmarrec commented Sep 27, 2023

jmarrec commented Sep 21, 2023 •

edited

Loading

jmarrec Sep 22, 2023 •

edited

Loading

ci-commercialbuildings commented Sep 26, 2023 •

edited

Loading