Fix #3369 - Add missing fields to AvailabilityManager:HybridVentilation #4037

jmarrec · 2020-08-11T16:18:33Z

Pull request overview

Fix AvailabilityManager:HybridVentilation missing EnergyPlus fields and OpenStudio methods #3369
Add missing fields to AvailabilityManager:HybridVentilation

Bumped the version to 3.1.0-alpha since I needed VersionTranslation

Pull Request Author

Add to this list or remove from it as applicable. This is a simple templated set of guidelines.

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

…es/model/OpenStudio.idd

@kbenne

@kbenne ``` C++ exception with description "std::bad_cast" thrown in the test body. ```

jmarrec · 2020-08-11T16:21:29Z

resources/model/OpenStudio.idd

+  N8 , \field Minimum HVAC Operation Time
+       \note Minimum operation time when HVAC system is forced on.
+       \type real
+       \units minutes
+       \minimum 0.0
+       \required-field
+  N9 ; \field Minimum Ventilation Time
+       \note Minimum ventilation time when natural ventilation is forced on.
+       \type real
+       \units minutes
+       \minimum 0.0
+       \required-field


Two new fields. They have a default to 0.0 in E+.idd, but I'm making them required-field and setting it to 0.0 in the model CTor.

jmarrec · 2020-08-11T16:21:52Z

src/energyplus/ForwardTranslator/ForwardTranslateAvailabilityManagerHybridVentilation.cpp

+  // AirflowNetwork Control Type Schedule Name
+  if (auto oSch = modelObject.airflowNetworkControlTypeSchedule()) {
+    auto idf = translateAndMapModelObject(oSch.get());
+    OS_ASSERT(idf);
+    idfObject.setString(AvailabilityManager_HybridVentilationFields::AirflowNetworkControlTypeScheduleName, idf->nameString());
+  }
+
+  // Simple Airflow Control Type Schedule Name
+  if (auto oSch = modelObject.simpleAirflowControlTypeSchedule()) {
+    auto idf = translateAndMapModelObject(oSch.get());
+    OS_ASSERT(idf);
+    idfObject.setString(AvailabilityManager_HybridVentilationFields::SimpleAirflowControlTypeScheduleName, idf->nameString());
+  }
+
+  // ZoneVentilation Object Name
+  if (auto oMo = modelObject.zoneVentilationObject()) {
+    auto idf = translateAndMapModelObject(oMo.get());
+    // Model API shouldn't allow setting a wrong type (other than ZV:DesignFlowRate or ZV:WindAndStackOpenArea) anyways
+    // and E+ will throw a clear warning if that were the case, so no need to check here
+    OS_ASSERT(idf);
+    idfObject.setString(AvailabilityManager_HybridVentilationFields::ZoneVentilationObjectName, idf->nameString());
+  }
+
+  // Minimum HVAC Operation Time
+  {
+    auto value = modelObject.minimumHVACOperationTime();
+    idfObject.setDouble(AvailabilityManager_HybridVentilationFields::MinimumHVACOperationTime,value);
+  }
+
+  // Minimum Ventilation Time
+  {
+    auto value = modelObject.minimumVentilationTime();
+    idfObject.setDouble(AvailabilityManager_HybridVentilationFields::MinimumVentilationTime,value);
+  }
+


FT for the three fields withing model API and the two new fields.

jmarrec · 2020-08-11T16:22:23Z

src/energyplus/Test/AvailabilityManagerHybridVentilation_GTest.cpp

@@ -48,22 +56,149 @@ TEST_F(EnergyPlusFixture, ForwardTranslator_AvailabilityManagerHybridVentilation
  Model m;

  AvailabilityManagerHybridVentilation avm(m);
+  avm.setName("My AvailabilityManagerHybridVentilation");


Added a full FT test (there was a VERY minimal one before only)

jmarrec · 2020-08-11T16:22:45Z

src/model/AvailabilityManagerHybridVentilation.cpp

+  ModelObject AvailabilityManagerHybridVentilation_Impl::clone(Model model) const {
+    auto avmClone = AvailabilityManager_Impl::clone(model).cast<AvailabilityManagerHybridVentilation>();
+    // Makes little sense to me to keep these two which are AirLoopHVAC specific. If you're cloning this object, it's to set to to ANOTHER AirLoopHVAC
+    avmClone.resetControlledZone();
+    avmClone.resetZoneVentilationObject();
+  }
+


Clone override, cf inline comment for justification.

jmarrec · 2020-08-11T16:23:04Z

src/model/AvailabilityManagerHybridVentilation.cpp

+  setMinimumHVACOperationTime(0.0);
+  setMinimumVentilationTime(0.0);


Ctor setting the E+ default.

jmarrec · 2020-08-11T16:23:08Z

src/model/AvailabilityManagerHybridVentilation.cpp

+  setMinimumHVACOperationTime(0.0);
+  setMinimumVentilationTime(0.0);


Ctor setting the E+ default.

jmarrec · 2020-08-11T16:23:42Z

src/model/AvailabilityManagerHybridVentilation.cpp

+  bool AvailabilityManagerHybridVentilation_Impl::setZoneVentilationObject(const ModelObject& mo) {
+    bool result = false;
+
+    switch(mo.iddObject().type().value())
+    {
+    case openstudio::IddObjectType::OS_ZoneVentilation_WindandStackOpenArea :
+      {
+        result = true;
+        break;
+      }
+    case openstudio::IddObjectType::OS_ZoneVentilation_DesignFlowRate :
+      {
+        result = true;
+        break;
+      }
+    default:
+      {
+        LOG(Warn, "Unsupported or invalid IddObjectType, only ZoneVentilation:DesignFlowRate or ZoneVentilation:WindAndStackOpenArea are supported. "
+            << "Occurred in setZoneVentilationObject for " << this->briefDescription() << ", was passed " << mo.briefDescription());
+        result = false;
+      }
+    }
+
+    if( result )
+    {
+      result = this->setPointer(OS_AvailabilityManager_HybridVentilationFields::ZoneVentilationObject, mo.handle());
+    }
+
+    return result;
+  }


'Zone Ventilation Object' only accepts a ZV:DesignFlowRate or a ZV:WindandStackOpenArea, so check that.
(I checked the E+ source in SystemAvailabilityManager.cc to confirm this is the case)

jmarrec · 2020-08-11T16:48:51Z

src/osversion/VersionTranslator.cpp

+    if (iddname == "OS:AvailabilityManager:HybridVentilation") {
+      // Added 2 new fields at end only, and made them required-field by setting their default in the Ctor, so VT needed
+
+      auto iddObject = idd_3_1_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());
+        }
+      }
+
+      // Set new fields per IDD default, same as Model Ctor, since it was made required-field
+      // Minimum HVAC Operation Time
+      newObject.setDouble(17, 0.0);
+      // Minimum Ventilation Time
+      newObject.setDouble(18, 0.0);
+
+      m_refactored.push_back(RefactoredObjectData(object, newObject));
+      ss << newObject;
+
+    // No-op
+    } else {
+      ss << object;
+    }


Vt for new fields at end (required)

jmarrec · 2020-08-11T16:49:08Z

src/osversion/test/VersionTranslator_GTest.cpp

@@ -899,3 +899,48 @@ TEST_F(OSVersionFixture, update_3_0_0_to_3_0_1_CoilCoolingDXTwoSpeed_minOATCompr
  ASSERT_TRUE(c.getTarget(34));
  EXPECT_EQ("Basin Heater Operating Schedule Name", c.getTarget(34)->nameString());
 }
+
+TEST_F(OSVersionFixture, update_3_0_1_to_3_1_0_AvailabilityManagerHybridVentilation) {


jmarrec · 2020-08-11T16:50:05Z

src/model/test/AvailabilityManagerHybridVentilation_GTest.cpp

+
+  // Maximum Outdoor Temperature: Required Double
+  EXPECT_TRUE(avm.setMaximumOutdoorTemperature(0.0));
+  EXPECT_EQ(0.0, avm.maximumOutdoorTemperature());
+  // Bad Value
+  EXPECT_FALSE(avm.setMaximumOutdoorTemperature(-110.0));
+  EXPECT_EQ(0.0, avm.maximumOutdoorTemperature());
+
+  // Minimum Outdoor Enthalpy: Required Double
+  EXPECT_TRUE(avm.setMinimumOutdoorEnthalpy(150000.0));
+  EXPECT_EQ(150000.0, avm.minimumOutdoorEnthalpy());
+  // Bad Value
+  EXPECT_FALSE(avm.setMinimumOutdoorEnthalpy(-10.0));
+  EXPECT_EQ(150000.0, avm.minimumOutdoorEnthalpy());
+
+  // Maximum Outdoor Enthalpy: Required Double
+  EXPECT_TRUE(avm.setMaximumOutdoorEnthalpy(150000.0));
+  EXPECT_EQ(150000.0, avm.maximumOutdoorEnthalpy());
+  // Bad Value
+  EXPECT_FALSE(avm.setMaximumOutdoorEnthalpy(-10.0));
+  EXPECT_EQ(150000.0, avm.maximumOutdoorEnthalpy());
+
+  // Minimum Outdoor Dewpoint: Required Double
+  EXPECT_TRUE(avm.setMinimumOutdoorDewpoint(0.0));
+  EXPECT_EQ(0.0, avm.minimumOutdoorDewpoint());
+  // Bad Value
+  EXPECT_FALSE(avm.setMinimumOutdoorDewpoint(-110.0));
+  EXPECT_EQ(0.0, avm.minimumOutdoorDewpoint());
+
+  // Maximum Outdoor Dewpoint: Required Double
+  EXPECT_TRUE(avm.setMaximumOutdoorDewpoint(0.0));
+  EXPECT_EQ(0.0, avm.maximumOutdoorDewpoint());
+  // Bad Value
+  EXPECT_FALSE(avm.setMaximumOutdoorDewpoint(-110.0));
+  EXPECT_EQ(0.0, avm.maximumOutdoorDewpoint());
+
+  // Minimum Outdoor Ventilation Air Schedule: Optional Object but set in Ctor and non optional
+  ScheduleConstant min_oa_sch(m);
+  EXPECT_TRUE(avm.setMinimumOutdoorVentilationAirSchedule(min_oa_sch));
+  EXPECT_EQ(min_oa_sch, avm.minimumOutdoorVentilationAirSchedule());
+
+  // Opening Factor Function of Wind Speed Curve: Optional Object
+  CurveLinear opening_factor_curve(m);
+  EXPECT_FALSE(avm.openingFactorFunctionofWindSpeedCurve());
+  EXPECT_TRUE(avm.setOpeningFactorFunctionofWindSpeedCurve(opening_factor_curve));
+  ASSERT_TRUE(avm.openingFactorFunctionofWindSpeedCurve());
+  EXPECT_EQ(opening_factor_curve, avm.openingFactorFunctionofWindSpeedCurve().get());
+  avm.resetOpeningFactorFunctionofWindSpeedCurve();
+  EXPECT_FALSE(avm.openingFactorFunctionofWindSpeedCurve());
+
+  // AirflowNetwork Control Type Schedule: Optional Object
+  ScheduleConstant afn_control_sch(m);
+  EXPECT_FALSE(avm.airflowNetworkControlTypeSchedule());
+  EXPECT_TRUE(avm.setAirflowNetworkControlTypeSchedule(afn_control_sch));
+  ASSERT_TRUE(avm.airflowNetworkControlTypeSchedule());
+  EXPECT_EQ(afn_control_sch, avm.airflowNetworkControlTypeSchedule().get());
+  avm.resetAirflowNetworkControlTypeSchedule();
+  EXPECT_FALSE(avm.airflowNetworkControlTypeSchedule());
+
+  // Simple Airflow Control Type Schedule: Optional Object
+  ScheduleConstant simple_control_sch(m);
+  EXPECT_FALSE(avm.simpleAirflowControlTypeSchedule());
+  EXPECT_TRUE(avm.setSimpleAirflowControlTypeSchedule(simple_control_sch));
+  ASSERT_TRUE(avm.simpleAirflowControlTypeSchedule());
+  EXPECT_EQ(simple_control_sch, avm.simpleAirflowControlTypeSchedule().get());
+  avm.resetSimpleAirflowControlTypeSchedule();
+  EXPECT_FALSE(avm.simpleAirflowControlTypeSchedule());
+
+  // ZoneVentilation Object: Optional Object
+  EXPECT_FALSE(avm.zoneVentilationObject());
+  ZoneVentilationDesignFlowRate zvd(m);
+  EXPECT_TRUE(avm.setZoneVentilationObject(zvd));
+  ASSERT_TRUE(avm.zoneVentilationObject());
+  EXPECT_EQ(zvd, avm.zoneVentilationObject().get());
+  ZoneVentilationWindandStackOpenArea zvw(m);
+  EXPECT_TRUE(avm.setZoneVentilationObject(zvw));
+  ASSERT_TRUE(avm.zoneVentilationObject());
+  EXPECT_EQ(zvw, avm.zoneVentilationObject().get());
+  // Bad type
+  ZoneMixing zm(z);
+  EXPECT_FALSE(avm.setZoneVentilationObject(zm));
+  ASSERT_TRUE(avm.zoneVentilationObject());
+  EXPECT_EQ(zvw, avm.zoneVentilationObject().get());
+  avm.resetZoneVentilationObject();
+  EXPECT_FALSE(avm.zoneVentilationObject());
+
+  // Minimum HVAC Operation Time: Required Double
+  EXPECT_TRUE(avm.setMinimumHVACOperationTime(0.1));
+  EXPECT_EQ(0.1, avm.minimumHVACOperationTime());
+  // Bad Value
+  EXPECT_FALSE(avm.setMinimumHVACOperationTime(-10.0));
+  EXPECT_EQ(0.1, avm.minimumHVACOperationTime());
+
+  // Minimum Ventilation Time: Required Double
+  EXPECT_TRUE(avm.setMinimumVentilationTime(0.2));
+  EXPECT_EQ(0.2, avm.minimumVentilationTime());
+  // Bad Value
+  EXPECT_FALSE(avm.setMinimumVentilationTime(-10.0));
+  EXPECT_EQ(0.2, avm.minimumVentilationTime());
+}


Add a full new test for Model API, testing all getters/setters (before there was only a Ctor test)

kbenne · 2020-08-11T17:35:23Z

src/model/AvailabilityManagerHybridVentilation.cpp

+  ModelObject AvailabilityManagerHybridVentilation_Impl::clone(Model model) const {
+    auto avmClone = AvailabilityManager_Impl::clone(model).cast<AvailabilityManagerHybridVentilation>();
+
+    // Makes little sense to me to keep these two which are AirLoopHVAC specific. If you're cloning this object, it's to set to to ANOTHER AirLoopHVAC


Agreed. This seems like the correct thing to do.

kbenne · 2020-08-11T17:40:10Z

I'm good with this. The testing is impressively thorough. I would just like to make sure @tijcolem is ready to accept the version bump.

Is there an integration test @jmarrec?

jmarrec · 2020-08-11T17:45:10Z

@kbenne I did not add an integration test no.

That would need to be a fully new test for the three existing fields that didn't have an implementation, and these fields are 1) weird and not straightforward (I'm not 100% sure how to produce a valid test, so it's going to require time and effort), 2) mutually exclusive. 3) this object is not one of the most common ones. 4) the existing test for AVMs is very minimal https://github.com/NREL/OpenStudio-resources/blob/a729a7c85f55cc05d7ba6d50c45d21b8621611c9/model/simulationtests/availability_managers.rb#L48-L49.

Instead I tested a bunch here.

I can add one if you really want to though.

ci-commercialbuildings · 2020-08-12T11:23:48Z

CI Results for d0fc429:

Linux:
- OpenStudio-3.1.0-alpha+d0fc4296c5-Linux.deb
- OpenStudio-3.1.0-alpha+d0fc4296c5-Linux.tar.gz
Darwin:
- OpenStudio-3.1.0-alpha+d0fc4296c5-Darwin.dmg
- OpenStudio-3.1.0-alpha+d0fc4296c5-Darwin.tar.gz
Windows:

jmarrec · 2020-08-12T15:55:58Z

Linux and mac are happy, aside from the usual suspects:

The following tests FAILED:
	614 - BCLFixture.RemoteBCLTest (SIGTRAP)
	616 - BCLFixture.GetComponentByUID (SIGTRAP)

Ready to drop in. (I need the VT part for another PR, so better sooner than later)

@kbenne do I need to add an OpenStudio-resources test?

tijcolem · 2020-08-12T16:43:52Z

Good with version bump!

…ed to bump to 3.1.0

jmarrec · 2020-08-12T21:00:10Z

Windows seem to have failed with the same error as on #4029

D:\jenkins\openstudio\develop\build\src/cli/embedded_files/ruby/2.5.0/gems/openstudio-standards-0.2.11/data/weather/YUBA-CO_724838_CZ2010_epw.cxx(1): fatal error C1060: compiler is out of heap space (compiling source file D:\jenkins\openstudio\develop\build\src\cli\embedded_files.cxx) [D:\jenkins\openstudio\develop\build\src\cli\openstudio.vcxproj]

@tijcolem https://ci.commercialbuildings.dev/blue/organizations/jenkins/openstudio-incremental-windows/detail/PR-4037/5/pipeline#step-34-log-99

jmarrec added 10 commits August 11, 2020 15:16

Add missing fields at end of AVM:HybridVentilation

a70883d

Implement the new numeric fields, and make them required-field

869fc87

Bump to 3.1.0 and add VT for the new required-fields

3dfa833

Implement the missing methods for existing IDD fields.

27d07eb

Add a full model tests for all fields (including the new ones)

ca05ced

wget https://raw.githubusercontent.com/NREL/OpenStudio/v3.0.1/resourc…

bf70b84

…es/model/OpenStudio.idd

Fix an issue in the 3_0_1 IDD: the \default in Version wasn't good

a637a67

Bump IDD to 3.1.0

c96bb98

Add a VersionTranslator test for AVM:HybridVentilation's new fields

37f4b66

Add a full FT test for AVM:HybridVentilation

111ee08

jmarrec added component - HVAC component - Model component - IDF Translation Pull Request - Ready for CI This pull request if finalized and is ready for continuous integration verification prior to merge. IDDChange labels Aug 11, 2020

jmarrec self-assigned this Aug 11, 2020

jmarrec added 2 commits August 11, 2020 18:44

Add a **failing** clone test (no clue why it's failing)

7860c04

@kbenne ``` C++ exception with description "std::bad_cast" thrown in the test body. ```

I forgot to return in the clone method. Why is GCC not picking that up?!

9b82631

jmarrec commented Aug 11, 2020

View reviewed changes

kbenne reviewed Aug 11, 2020

View reviewed changes

jmarrec added this to the OpenStudio SDK 3.1.0 milestone Aug 11, 2020

jmarrec mentioned this pull request Aug 12, 2020

Fix #4038 - Re-enable -Wall on Unix #4040

Merged

20 tasks

jmarrec force-pushed the 3369_AVMHybridVentilation_MissingFields branch from 99aa952 to 9b82631 Compare August 12, 2020 10:56

jmarrec added a commit that referenced this pull request Aug 12, 2020

Prepare VT and VT Test, pending #4037 to avoid merge conflcits and ne…

31669bb

…ed to bump to 3.1.0

jmarrec mentioned this pull request Aug 12, 2020

Fix #4039 - Add field 'Design Return Air Flow Fraction of Supply Air Flow' to AirLoopHVAC #4044

Merged

20 tasks

kbenne merged commit a24495d into develop Aug 17, 2020

jmarrec deleted the 3369_AVMHybridVentilation_MissingFields branch September 24, 2020 10:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #3369 - Add missing fields to AvailabilityManager:HybridVentilation #4037

Fix #3369 - Add missing fields to AvailabilityManager:HybridVentilation #4037

jmarrec commented Aug 11, 2020 •

edited

Loading

jmarrec Aug 11, 2020

jmarrec Aug 11, 2020

jmarrec Aug 11, 2020

jmarrec Aug 11, 2020

jmarrec Aug 11, 2020

jmarrec Aug 11, 2020

jmarrec Aug 11, 2020

jmarrec Aug 11, 2020

jmarrec Aug 11, 2020

jmarrec Aug 11, 2020

kbenne Aug 11, 2020

kbenne commented Aug 11, 2020

jmarrec commented Aug 11, 2020

ci-commercialbuildings commented Aug 12, 2020 •

edited

Loading

jmarrec commented Aug 12, 2020 •

edited

Loading

tijcolem commented Aug 12, 2020

jmarrec commented Aug 12, 2020 •

edited

Loading

		setMinimumHVACOperationTime(0.0);
		setMinimumVentilationTime(0.0);

Fix #3369 - Add missing fields to AvailabilityManager:HybridVentilation #4037

Fix #3369 - Add missing fields to AvailabilityManager:HybridVentilation #4037

Conversation

jmarrec commented Aug 11, 2020 • edited Loading

Pull request overview

Pull Request Author

Review Checklist

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

kbenne commented Aug 11, 2020

jmarrec commented Aug 11, 2020

ci-commercialbuildings commented Aug 12, 2020 • edited Loading

jmarrec commented Aug 12, 2020 • edited Loading

tijcolem commented Aug 12, 2020

jmarrec commented Aug 12, 2020 • edited Loading

jmarrec commented Aug 11, 2020 •

edited

Loading

ci-commercialbuildings commented Aug 12, 2020 •

edited

Loading

jmarrec commented Aug 12, 2020 •

edited

Loading

jmarrec commented Aug 12, 2020 •

edited

Loading