Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove allowedObjects check from energyPlusOutputRequests #5358

Merged
merged 6 commits into from
Mar 20, 2025
Merged

Remove allowedObjects check from energyPlusOutputRequests #5358

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
6f852b3
remove allowedObjects
eringold Mar 3, 2025
5c65ad0
Write a unit test for addEnergyplusOutputRequests and mergeOutputTabl…
jmarrec Mar 17, 2025
1c65ed0
Fix existing logic error in mergeOutputTableSummaryReports
jmarrec Mar 17, 2025
958020e
Fixup changes in addEnergyPlusOutputRequests: if unique, remove the e…
jmarrec Mar 17, 2025
1ea6309
Add a Warn message when prefix isn't marked safe
jmarrec Mar 19, 2025
5e9f22a
Update src/workflow/Util.hpp
jmarrec Mar 19, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
73 changes: 35 additions & 38 deletions src/workflow/Util.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
#include "../utilities/bcl/BCLMeasure.hpp"
#include "../utilities/time/DateTime.hpp"

#include <algorithm>
#include <boost/filesystem/operations.hpp>
#include <utilities/idd/IddEnums.hxx>

Expand Down Expand Up @@ -161,7 +162,7 @@ bool mergeOutputTableSummaryReports(IdfObject& existingObject, const IdfObject&
}

for (const auto& newReport : reportsToAdd) {
if (std::find(reports.cbegin(), reports.cend(), newReport) != reports.cend()) {
if (std::find(reports.cbegin(), reports.cend(), newReport) == reports.cend()) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was an existing logic error. Found it because I wrote a test.

existingObject.pushExtensibleGroup({newReport});
added = true;
}
Expand All @@ -170,28 +171,25 @@ bool mergeOutputTableSummaryReports(IdfObject& existingObject, const IdfObject&
return added;
}

bool isEnergyPlusOutputRequestPotentiallyUnsafe(const IddObjectType& iddObjectType) {
std::string const valueName = iddObjectType.valueName();
return std::none_of(util::safe_idd_prefixes.cbegin(), util::safe_idd_prefixes.cend(),
[&valueName](std::string_view s) { return valueName.starts_with(s); });
}

bool addEnergyPlusOutputRequest(Workspace& workspace, IdfObject& idfObject) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was super tempted to add a LOG (Warn or Info) in there, something along the lines of

if iddObjectType.valueName does not start with "Output" or "Meter", or "EnergyManagement" or "PythonPlugin", then issue a warning like "Requested to add an object of type XXX, ensure you aren't going to modify the energy usage of the model".

But I'm guessing @eringold and @shorowit wouldn't like that too much either, right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not against log messages. Though technically objects starting with 'EnergyManagement' or 'PythonPlugin' could modify energy use. Maybe better would be to check if object type names contain just 'Output' or 'Meter'.

Copy link
Collaborator

@jmarrec jmarrec Mar 19, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is true but PythonPlugin (and EnergyManagement is the same) were some of the objects that you specifically wanted in your OP, and they could have a valid application.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I decided to add the Warn after all, but not being restrictive and letting the request be processed anyways.

cf #5358 (review)

const auto iddObject = idfObject.iddObject();
const bool is_unique = iddObject.properties().unique;
const auto iddObjectType = iddObject.type();

// If not marked safe, we Warn, but we let you do it anyways
if (isEnergyPlusOutputRequestPotentiallyUnsafe(iddObjectType)) {
LOG_FREE(Warn, "openstudio.worklow.Util",
"Requested to add an EnergyPlus Output Request of type '"
<< iddObjectType.valueName() << "' which is not marked as safe: make sure you aren't going to modify the energy usage of the model.");
}
Comment on lines +185 to +190
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a Warn message when not safe, but still we allow it anyways.


static const std::vector<IddObjectType> allowedObjects{
IddObjectType::Output_Surfaces_List,
IddObjectType::Output_Surfaces_Drawing,
IddObjectType::Output_Schedules,
IddObjectType::Output_Constructions,
IddObjectType::Output_Table_TimeBins,
IddObjectType::Output_Table_Monthly,
IddObjectType::Output_Variable,
IddObjectType::Output_Meter,
IddObjectType::Output_Meter_MeterFileOnly,
IddObjectType::Output_Meter_Cumulative,
IddObjectType::Output_Meter_Cumulative_MeterFileOnly,
IddObjectType::Meter_Custom,
IddObjectType::Meter_CustomDecrement,
IddObjectType::EnergyManagementSystem_OutputVariable,
};

auto iddObjectType = idfObject.iddObject().type();

if (std::find(allowedObjects.cbegin(), allowedObjects.end(), iddObjectType) != allowedObjects.end()) {
if (!is_unique) {

// If already present, don't do it
for (const auto& wo : workspace.getObjectsByType(iddObjectType)) {
Expand All @@ -203,30 +201,29 @@ bool addEnergyPlusOutputRequest(Workspace& workspace, IdfObject& idfObject) {
workspace.addObject(idfObject);

return true;
}

// static const std::vector<IddObjectType> allowedUniqueObjects{
// // IddObjectType::Output_EnergyManagementSystem, // TODO: have to merge
// // IddObjectType::OutputControl_SurfaceColorScheme, // TODO: have to merge
// IddObjectType::Output_Table_SummaryReports, // TODO: have to merge
//
// // Not allowed
// // IddObjectType::OutputControl_Table_Style,
// // IddObjectType::OutputControl_ReportingTolerances,
// // IddObjectType::Output_SQLite,
// };

if (iddObjectType == IddObjectType::Output_Table_SummaryReports) {
} else if (iddObjectType == IddObjectType::Output_Table_SummaryReports) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it's summaryReports, merge.

auto summaryReports = workspace.getObjectsByType(iddObjectType);
if (summaryReports.empty()) {
workspace.addObject(idfObject);
return true;
} else {
mergeOutputTableSummaryReports(summaryReports.front(), idfObject);
return mergeOutputTableSummaryReports(summaryReports.front(), idfObject);
}
} else {
// It's a unique one, for which we didn't write a merge, so remove it first
auto existingUniqueObjects = workspace.getObjectsByType(iddObjectType);
if (!existingUniqueObjects.empty()) {
// Technically it should be size == 1
if (std::ranges::any_of(existingUniqueObjects, [&idfObject](const auto& wo) { return idfObject.dataFieldsEqual(wo); })) {
return false;
}
for (auto& wo : existingUniqueObjects) {
wo.remove();
}
}
workspace.addObject(idfObject);
return true;
Comment on lines +212 to +225
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it's a unique one, check if there's the same exact object in there already and return false if so.

Otherwise, remove it first before adding the one (=replace).

We shouldn't end up with 2 "Output:SQLite" objects for eg, or E+ will throw

}

return false;
}

/*****************************************************************************************************************************************************
Expand Down
13 changes: 13 additions & 0 deletions src/workflow/Util.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@
#define WORKFLOW_UTIL_HPP

#include "../utilities/core/Filesystem.hpp"
#include <array>
#include <string_view>

namespace openstudio {

Expand All @@ -17,6 +19,7 @@ class IdfObject;
class Workspace;
class WorkflowStepResult;
class BCLMeasure;
struct IddObjectType;

namespace workflow {

Expand All @@ -35,6 +38,16 @@ namespace workflow {
bool mergeOutputTableSummaryReports(IdfObject& existingObject, const IdfObject& newObject);
bool addEnergyPlusOutputRequest(Workspace& workspace, IdfObject& idfObject);

// Schedule can be used onto a meter/variable, the EMS/PythonPlugin **could** be used for custom reporting
// Also adding some cost/utility related objects, the goal is to be not too restrictive here
static constexpr std::array<std::string_view, 11> safe_idd_prefixes{
"Output", "Meter", "PythonPlugin", "EnergyManagementSystem", "Schedule", "LifeCycleCost", "UtilityCost", "ComponentCost",
"Compliance", "Currency", "FuelFactors",
};

// If the IddObjectType.valueName doesn't start with one of the allowed prefixes (see `safe_idd_prefixes`), it returns true.
bool isEnergyPlusOutputRequestPotentiallyUnsafe(const IddObjectType& iddObjectType);
Comment on lines +41 to +49
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • I aimed to strike the right balance here: the list is not restrictive on purpose
  • Remember that this is just resulting in a Warn message, it'll still process your request.
  • Also, energyplusOutputRequests is somewhat moot now that we have a modelOutputRequests method
  • Finally, I made the list of prefix really easy to change: just add to that std::array and you're good to go, which would allow anyone to propose a change even without knowing much C++


void zipResults(const openstudio::path& dirPath);

/** Remove any invalid characters in the measure attribute keys. Periods and Pipes are the most problematic
Expand Down
150 changes: 150 additions & 0 deletions src/workflow/test/Util_GTest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,20 @@
#include <gtest/gtest.h>

#include "../Util.hpp"
#include "../../utilities/idf/IdfFile.hpp"
#include "../../utilities/idf/IdfExtensibleGroup.hpp"
#include "../../utilities/idf/Workspace.hpp"
#include "../../utilities/idf/WorkspaceObject.hpp"
#include "../../utilities/idf/IdfObject.hpp"
#include "../../utilities/idd/IddObject.hpp"

#include <utilities/idd/IddEnums.hxx>

#include <map>
#include <string>

using namespace openstudio;

class WorkflowFixture : public testing::Test
{
};
Expand Down Expand Up @@ -68,3 +78,143 @@ TEST_F(WorkflowFixture, Util_sanitizeKey) {
EXPECT_EQ(sanitizedKey, expectedKey) << "Error: Key '" << originalKey << "' was not sanitized as expected.";
}
}

TEST_F(WorkflowFixture, Util_mergeOutputTableSummaryReports) {
IdfObject existingObject(IddObjectType::Output_Table_SummaryReports);
existingObject.pushExtensibleGroup({"InputVerificationandResultsSummary"});
existingObject.pushExtensibleGroup({"ClimaticDataSummary"});

IdfObject newObject(IddObjectType::Output_Table_SummaryReports);
newObject.pushExtensibleGroup({"ComponentSizingSummary"});
newObject.pushExtensibleGroup({"ClimaticDataSummary"});
newObject.pushExtensibleGroup({"EnvelopeSummary"});

EXPECT_TRUE(openstudio::workflow::util::mergeOutputTableSummaryReports(existingObject, newObject));
ASSERT_EQ(4, existingObject.numExtensibleGroups());
auto egs = existingObject.extensibleGroups();
EXPECT_EQ("InputVerificationandResultsSummary", egs.at(0).getString(0).get());
EXPECT_EQ("ClimaticDataSummary", egs.at(1).getString(0).get());
EXPECT_EQ("ComponentSizingSummary", egs.at(2).getString(0).get());
EXPECT_EQ("EnvelopeSummary", egs.at(3).getString(0).get());
}
Comment on lines +82 to +99
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New test for mergeOutputTableSummaryReports


TEST_F(WorkflowFixture, Util_addEnergyPlusOutputRequest) {

Workspace w(StrictnessLevel::Draft, IddFileType::EnergyPlus);
std::vector<IdfObject> energyPlusOutputRequests;
{
// Unique with explicit merge
WorkspaceObject wo = w.addObject(IdfObject(IddObjectType::Output_Table_SummaryReports)).get();
wo.pushExtensibleGroup({"InputVerificationandResultsSummary"});
wo.pushExtensibleGroup({"ClimaticDataSummary"});

auto& newObject = energyPlusOutputRequests.emplace_back(IddObjectType::Output_Table_SummaryReports);
newObject.pushExtensibleGroup({"ComponentSizingSummary"});
newObject.pushExtensibleGroup({"ClimaticDataSummary"});
newObject.pushExtensibleGroup({"EnvelopeSummary"});

EXPECT_TRUE(openstudio::workflow::util::addEnergyPlusOutputRequest(w, newObject));
{
auto wos = w.getObjectsByType(IddObjectType::Output_Table_SummaryReports);
ASSERT_EQ(1, wos.size());
auto egs = wos.front().extensibleGroups();
EXPECT_EQ("InputVerificationandResultsSummary", egs.at(0).getString(0).get());
EXPECT_EQ("ClimaticDataSummary", egs.at(1).getString(0).get());
EXPECT_EQ("ComponentSizingSummary", egs.at(2).getString(0).get());
EXPECT_EQ("EnvelopeSummary", egs.at(3).getString(0).get());
}
}
{
// Unique and present already
WorkspaceObject wo = w.addObject(IdfObject(IddObjectType::Output_SQLite)).get();
wo.setString(0, "Simple");

auto& newObject = energyPlusOutputRequests.emplace_back(IddObjectType::Output_SQLite);
newObject.setString(0, "SimpleAndTabular");
newObject.setString(1, "JtoKWH");

EXPECT_TRUE(openstudio::workflow::util::addEnergyPlusOutputRequest(w, newObject));
{
auto wos = w.getObjectsByType(IddObjectType::Output_SQLite);
ASSERT_EQ(1, wos.size());
const auto& new_wo = wos.front();
EXPECT_EQ("SimpleAndTabular", new_wo.getString(0).get());
EXPECT_EQ("JtoKWH", new_wo.getString(1).get());
}
}
{
// Unique and not present already
auto& newObject = energyPlusOutputRequests.emplace_back(IddObjectType::Output_Diagnostics);
newObject.setString(0, "DisplayExtraWarnings");
newObject.setString(1, "DisplayUnusedSchedules");

EXPECT_EQ(0, w.getObjectsByType(IddObjectType::Output_Diagnostics).size());
EXPECT_TRUE(openstudio::workflow::util::addEnergyPlusOutputRequest(w, newObject));
{
auto wos = w.getObjectsByType(IddObjectType::Output_Diagnostics);
ASSERT_EQ(1, wos.size());
const auto& new_wo = wos.front();
EXPECT_EQ("DisplayExtraWarnings", new_wo.getString(0).get());
EXPECT_EQ("DisplayUnusedSchedules", new_wo.getString(1).get());
}
}
{
// Non unique, and present already
WorkspaceObject wo = w.addObject(IdfObject(IddObjectType::Schedule_Constant)).get();
wo.setString(0, "AlwaysOn");
wo.setDouble(2, 1.0);

auto& newObject = energyPlusOutputRequests.emplace_back(IddObjectType::Schedule_Constant);
newObject.setString(0, "AlwaysOn");
newObject.setDouble(2, 1.0);

EXPECT_TRUE(w.getObjectByTypeAndName(IddObjectType::Schedule_Constant, "AlwaysOn"));
EXPECT_FALSE(openstudio::workflow::util::addEnergyPlusOutputRequest(w, newObject));
{
EXPECT_EQ(1, w.getObjectsByType(IddObjectType::Schedule_Constant).size());
EXPECT_TRUE(w.getObjectByTypeAndName(IddObjectType::Schedule_Constant, "AlwaysOn"));
}
}
{
// Non unique, and not present already
auto& newObject = energyPlusOutputRequests.emplace_back(IddObjectType::Schedule_Constant);
newObject.setString(0, "A New Schedule");
newObject.setDouble(2, 18.0);
EXPECT_EQ(1, w.getObjectsByType(IddObjectType::Schedule_Constant).size());
EXPECT_TRUE(w.getObjectByTypeAndName(IddObjectType::Schedule_Constant, "AlwaysOn"));
EXPECT_TRUE(openstudio::workflow::util::addEnergyPlusOutputRequest(w, newObject));
{
auto wos = w.getObjectsByType(IddObjectType::Schedule_Constant);
ASSERT_EQ(2, wos.size());
EXPECT_TRUE(w.getObjectByTypeAndName(IddObjectType::Schedule_Constant, "AlwaysOn"));
auto new_wo_ = w.getObjectByTypeAndName(IddObjectType::Schedule_Constant, "A New Schedule");
ASSERT_TRUE(new_wo_);
EXPECT_EQ("A New Schedule", new_wo_->getString(0).get());
EXPECT_TRUE(new_wo_->isEmpty(1));
EXPECT_EQ(18.0, new_wo_->getDouble(2).get());
}
}
}
Comment on lines +101 to +197
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New test for addEnergyPlusOutputRequest, specifically I'm testing what happens with unique objects. We should NOT end up with more than 1


TEST_F(WorkflowFixture, Util_isEnergyPlusOutputRequestPotentiallyUnsafe) {

std::vector<IddObjectType> should_be_allowed{
IddObjectType::Output_Table_Annual,
IddObjectType::Meter_Custom,
IddObjectType::Schedule_Compact,
IddObjectType::Output_Diagnostics,
IddObjectType::Output_Variable,
IddObjectType::EnergyManagementSystem_OutputVariable,
IddObjectType::PythonPlugin_OutputVariable,
IddObjectType::LifeCycleCost_Parameters,
IddObjectType::UtilityCost_Tariff,
IddObjectType::ComponentCost_Adjustments,
IddObjectType::Compliance_Building,
IddObjectType::CurrencyType,
};
for (const auto& iddObjectType : should_be_allowed) {
EXPECT_FALSE(openstudio::workflow::util::isEnergyPlusOutputRequestPotentiallyUnsafe(iddObjectType)) << "Failed for " << iddObjectType;
}

EXPECT_TRUE(openstudio::workflow::util::isEnergyPlusOutputRequestPotentiallyUnsafe(IddObjectType::People));
}
Comment on lines +199 to +220
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New test