Fix #4404 #4453 - Fix issues with ForwardTranslator options #4454

jmarrec · 2021-10-01T10:08:20Z

Pull request overview

I added a unit test for all options. #4404 is confirmed, but I also found out that Output:Table:SummaryReports was problematic also.

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

…to account Couple of performance improvements with std::any_of and emplace_back around the changes too

jmarrec · 2021-10-01T10:34:34Z

src/energyplus/ForwardTranslator.cpp

+      // If the user manually added an OutputTableSummaryReports, but he also opted-in to exclude it on the FT, which decision do we keep?
+      // Given that it's a much harder to set the option on the FT, I'll respect that one
+      if (!m_excludeHTMLOutputReport) {
+        auto optOutputTableSummaryReports = model.getOptionalUniqueModelObject<model::OutputTableSummaryReports>();
+        // Add default one if none explicitly specified
+        if (!optOutputTableSummaryReports) {
+          auto outputTableSummaryReports = model.getUniqueModelObject<model::OutputTableSummaryReports>();
+          outputTableSummaryReports.addSummaryReport("AllSummary");
+          translateAndMapModelObject(outputTableSummaryReports);
+        }


jmarrec · 2021-10-01T10:35:22Z

src/energyplus/ForwardTranslator.cpp

+    if (!m_excludeLCCObjects) {
+      bool hasAtLeastOneCost = std::any_of(m_idfObjects.cbegin(), m_idfObjects.cend(), [](const auto& obj) {
+        auto iddObjType = obj.iddObject().type();
+        return (iddObjType == openstudio::IddObjectType::LifeCycleCost_NonrecurringCost)
+               || (iddObjType == openstudio::IddObjectType::LifeCycleCost_RecurringCosts);
+      });
+
+      if (!hasAtLeastOneCost) {
+        // add default cost
+        auto& idfObject = m_idfObjects.emplace_back(openstudio::IddObjectType::LifeCycleCost_NonrecurringCost);
+        idfObject.setString(LifeCycleCost_NonrecurringCostFields::Name, "Default Cost");
+        idfObject.setString(LifeCycleCost_NonrecurringCostFields::Category, "Construction");
+        idfObject.setDouble(LifeCycleCost_NonrecurringCostFields::Cost, 0.0);
+        idfObject.setString(LifeCycleCost_NonrecurringCostFields::StartofCosts, "ServicePeriod");


Fix #4404 + use std::any_of instead of looping on all objects (and not even breaking as soon as one is found).
Also use emplace_back to construct in place for performance reasons

jmarrec · 2021-10-01T10:35:37Z

src/energyplus/Test/ForwardTranslator_GTest.cpp

+    EXPECT_EQ("HTML", obj.getString(0).get());
+    EXPECT_EQ("None", obj.getString(1, true).get());  // Return default
+    EXPECT_FALSE(obj.getString(1, false, true));      // not initialized
+  }
+
+  ft.setExcludeLCCObjects(true);
+  {
+    Workspace w = ft.translateModel(m);
+
+    EXPECT_EQ(0u, w.numObjectsOfType(IddObjectType::LifeCycleCost_Parameters));
+    EXPECT_EQ(0u, w.numObjectsOfType(IddObjectType::LifeCycleCost_UsePriceEscalation));
+    EXPECT_EQ(0u, w.numObjectsOfType(IddObjectType::LifeCycleCost_NonrecurringCost));
+
+    EXPECT_EQ(1u, w.numObjectsOfType(IddObjectType::Output_VariableDictionary));
+    EXPECT_EQ(1u, w.numObjectsOfType(IddObjectType::Output_SQLite));
+    EXPECT_EQ(1u, w.numObjectsOfType(IddObjectType::Output_Table_SummaryReports));
+
+    auto objs = w.getObjectsByType(IddObjectType::OutputControl_Table_Style);
+    ASSERT_EQ(1, objs.size());
+    auto obj = objs[0];
+    EXPECT_EQ("HTML", obj.getString(0).get());
+    EXPECT_EQ("None", obj.getString(1, true).get());  // Return default
+    EXPECT_FALSE(obj.getString(1, false, true));      // not initialized
+  }
+
+  ft.setExcludeVariableDictionary(true);
+  {
+    Workspace w = ft.translateModel(m);
+
+    EXPECT_EQ(0u, w.numObjectsOfType(IddObjectType::LifeCycleCost_Parameters));
+    EXPECT_EQ(0u, w.numObjectsOfType(IddObjectType::LifeCycleCost_UsePriceEscalation));
+    EXPECT_EQ(0u, w.numObjectsOfType(IddObjectType::LifeCycleCost_NonrecurringCost));
+
+    EXPECT_EQ(0u, w.numObjectsOfType(IddObjectType::Output_VariableDictionary));
+    EXPECT_EQ(1u, w.numObjectsOfType(IddObjectType::Output_SQLite));
+    EXPECT_EQ(1u, w.numObjectsOfType(IddObjectType::Output_Table_SummaryReports));
+
+    auto objs = w.getObjectsByType(IddObjectType::OutputControl_Table_Style);
+    ASSERT_EQ(1, objs.size());
+    auto obj = objs[0];
+    EXPECT_EQ("HTML", obj.getString(0).get());
+    EXPECT_EQ("None", obj.getString(1, true).get());  // Return default
+    EXPECT_FALSE(obj.getString(1, false, true));      // not initialized
+  }
+
+  ft.setExcludeSQliteOutputReport(true);
+  {
+    Workspace w = ft.translateModel(m);
+
+    EXPECT_EQ(0u, w.numObjectsOfType(IddObjectType::LifeCycleCost_Parameters));
+    EXPECT_EQ(0u, w.numObjectsOfType(IddObjectType::LifeCycleCost_UsePriceEscalation));
+    EXPECT_EQ(0u, w.numObjectsOfType(IddObjectType::LifeCycleCost_NonrecurringCost));
+
+    EXPECT_EQ(0u, w.numObjectsOfType(IddObjectType::Output_VariableDictionary));
+    EXPECT_EQ(0u, w.numObjectsOfType(IddObjectType::Output_SQLite));
+    EXPECT_EQ(1u, w.numObjectsOfType(IddObjectType::Output_Table_SummaryReports));
+
+    auto objs = w.getObjectsByType(IddObjectType::OutputControl_Table_Style);
+    ASSERT_EQ(1, objs.size());
+    auto obj = objs[0];
+    EXPECT_EQ("HTML", obj.getString(0).get());
+    EXPECT_EQ("None", obj.getString(1, true).get());  // Return default
+    EXPECT_FALSE(obj.getString(1, false, true));      // not initialized
+  }
+
+  ft.setIPTabularOutput(true);
+  {
+    Workspace w = ft.translateModel(m);
+
+    EXPECT_EQ(0u, w.numObjectsOfType(IddObjectType::LifeCycleCost_Parameters));
+    EXPECT_EQ(0u, w.numObjectsOfType(IddObjectType::LifeCycleCost_UsePriceEscalation));
+    EXPECT_EQ(0u, w.numObjectsOfType(IddObjectType::LifeCycleCost_NonrecurringCost));
+
+    EXPECT_EQ(0u, w.numObjectsOfType(IddObjectType::Output_VariableDictionary));
+    EXPECT_EQ(0u, w.numObjectsOfType(IddObjectType::Output_SQLite));
+    EXPECT_EQ(1u, w.numObjectsOfType(IddObjectType::Output_Table_SummaryReports));
+
+    auto objs = w.getObjectsByType(IddObjectType::OutputControl_Table_Style);
+    ASSERT_EQ(1, objs.size());
+    auto obj = objs[0];
+    EXPECT_EQ("HTML", obj.getString(0).get());
+    EXPECT_EQ("InchPound", obj.getString(1, false, true).get());
+  }
+
+  ft.setExcludeHTMLOutputReport(true);
+  {
+    Workspace w = ft.translateModel(m);
+
+    EXPECT_EQ(0u, w.numObjectsOfType(IddObjectType::LifeCycleCost_Parameters));
+    EXPECT_EQ(0u, w.numObjectsOfType(IddObjectType::LifeCycleCost_UsePriceEscalation));
+    EXPECT_EQ(0u, w.numObjectsOfType(IddObjectType::LifeCycleCost_RecurringCosts));
+
+    EXPECT_EQ(0u, w.numObjectsOfType(IddObjectType::Output_VariableDictionary));
+    EXPECT_EQ(0u, w.numObjectsOfType(IddObjectType::Output_SQLite));
+    EXPECT_EQ(0u, w.numObjectsOfType(IddObjectType::Output_Table_SummaryReports));
+
+    // This also turns off the OutputControl:Table:Style
+    EXPECT_EQ(0u, w.numObjectsOfType(IddObjectType::OutputControl_Table_Style));
+  }
+}


Add a unit test for all options

ci-commercialbuildings · 2021-10-01T10:54:08Z

CI Results for 1dff48b:

shorowit · 2021-10-04T14:56:11Z

I'd be happy to test this but I don't see any Ubuntu or Windows builds above?

jmarrec · 2021-10-04T18:34:44Z

@shorowit installers are up now

shorowit · 2021-10-04T20:06:08Z

Verified that this fixes the issue for me. Thanks, @jmarrec!

tijcolem

@jmarrec Looks great and thanks @shorowit for confirming!

jmarrec added severity - Minor Bug component - IDF Translation Pull Request - Ready for CI This pull request if finalized and is ready for continuous integration verification prior to merge. labels Oct 1, 2021

jmarrec self-assigned this Oct 1, 2021

Add a unit test for all ForwardTranslator Options (Failing for two)

fbf771c

jmarrec force-pushed the 4404_excludeLCC branch from fdbb166 to fbf771c Compare October 1, 2021 10:30

Fix #4404 and Fix #4453 - Correctly take ForwardTranslator options in…

1dff48b

…to account Couple of performance improvements with std::any_of and emplace_back around the changes too

jmarrec force-pushed the 4404_excludeLCC branch from e76ab8a to 1dff48b Compare October 1, 2021 10:34

jmarrec commented Oct 1, 2021

View reviewed changes

jmarrec requested review from tijcolem and shorowit October 1, 2021 10:35

tijcolem approved these changes Oct 5, 2021

View reviewed changes

tijcolem merged commit 65607ae into develop Oct 5, 2021

tijcolem deleted the 4404_excludeLCC branch October 5, 2021 02:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #4404 #4453 - Fix issues with ForwardTranslator options #4454

Fix #4404 #4453 - Fix issues with ForwardTranslator options #4454

jmarrec commented Oct 1, 2021

jmarrec Oct 1, 2021

jmarrec Oct 1, 2021

jmarrec Oct 1, 2021

ci-commercialbuildings commented Oct 1, 2021 •

edited

Loading

shorowit commented Oct 4, 2021

jmarrec commented Oct 4, 2021

shorowit commented Oct 4, 2021

tijcolem left a comment

Fix #4404 #4453 - Fix issues with ForwardTranslator options #4454

Fix #4404 #4453 - Fix issues with ForwardTranslator options #4454

Conversation

jmarrec commented Oct 1, 2021

Pull request overview

Pull Request Author

Review Checklist

jmarrec Oct 1, 2021

Choose a reason for hiding this comment

jmarrec Oct 1, 2021

Choose a reason for hiding this comment

jmarrec Oct 1, 2021

Choose a reason for hiding this comment

ci-commercialbuildings commented Oct 1, 2021 • edited Loading

shorowit commented Oct 4, 2021

jmarrec commented Oct 4, 2021

shorowit commented Oct 4, 2021

tijcolem left a comment

Choose a reason for hiding this comment

ci-commercialbuildings commented Oct 1, 2021 •

edited

Loading