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

Fix #10830 - incorrect curve unit type warning #10853

Open
wants to merge 6 commits into
base: develop
Choose a base branch
from
Open

Fix #10830 - incorrect curve unit type warning #10853

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
9521548
Add tests for #10830
jmarrec Dec 12, 2024
fbec2d2
Fix #10830 - Call IsCurveInputTypeValid when it's input, not IsCurveO…
jmarrec Dec 12, 2024
9ba5186
Caught another copy paste error
jmarrec Dec 12, 2024
04375ea
Add a dynamic test to ensure that ALL possible Curves Input/Output Un…
jmarrec Dec 12, 2024
35964ba
Add Missing Input Type VolumetricFlowPerPower: used by Curve:QuadLine…
jmarrec Dec 12, 2024
9bdb2b7
Unrelated but replace a C-array of std::string with an array
jmarrec Dec 12, 2024
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
25 changes: 19 additions & 6 deletions src/EnergyPlus/CurveManager.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,9 +47,11 @@

// C++ Headers
#include <algorithm>
#include <array>
#include <cmath>
#include <limits>
#include <string>
#include <string_view>

// ObjexxFCL Headers
#include <ObjexxFCL/Array.functions.hh>
Expand Down Expand Up @@ -798,8 +800,8 @@ namespace Curve {
}
}
if (NumAlphas >= 4) {
if (!IsCurveOutputTypeValid(Alphas(4))) {
ShowWarningError(state, format("In {} named {} the OInput Unit Type for Z is invalid.", CurrentModuleObject, Alphas(1)));
if (!IsCurveInputTypeValid(Alphas(4))) {
ShowWarningError(state, format("In {} named {} the Input Unit Type for Z is invalid.", CurrentModuleObject, Alphas(1)));
Comment on lines 802 to +804
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Noticed another mistake here in Curve:ChillerPartLoadWithLift

}
}
if (NumAlphas >= 5) {
Expand Down Expand Up @@ -1480,7 +1482,7 @@ namespace Curve {
}

constexpr int NumVar = 4;
std::string VarNames[NumVar] = {"w", "x", "y", "z"};
constexpr std::array<std::string_view, NumVar> VarNames{"w", "x", "y", "z"};
for (int i = 1; i <= NumVar; ++i) {
int MinIndex = 2 * i + 4;
int MaxIndex = MinIndex + 1;
Expand Down Expand Up @@ -1560,7 +1562,7 @@ namespace Curve {
}

constexpr int NumVar = 5;
std::string VarNames[NumVar] = {"v", "w", "x", "y", "z"};
constexpr std::array<std::string_view, NumVar> VarNames{"v", "w", "x", "y", "z"};
for (int i = 1; i <= NumVar; ++i) {
int MinIndex = 2 * i + 5;
int MaxIndex = MinIndex + 1;
Expand Down Expand Up @@ -2297,7 +2299,7 @@ namespace Curve {
// TODO: Actually use this to define output variable units
if (indVarInstance.count("unit_type")) {
std::string unitType = indVarInstance.at("unit_type").get<std::string>();
if (!IsCurveOutputTypeValid(unitType)) {
if (!IsCurveInputTypeValid(unitType)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Specific fix for #10830 is here

ShowSevereError(state, format("{}: Unit Type [{}] is invalid", contextString, unitType));
}
}
Expand Down Expand Up @@ -2953,10 +2955,21 @@ namespace Curve {
Distance,
Wavelength,
Angle,
VolumetricFlowPerPower,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Add Missing Input Type VolumetricFlowPerPower: used by Curve:QuadLinear & Curve:QuintLinear

Num
};
constexpr std::array<std::string_view, static_cast<int>(CurveInputType::Num)> inputTypes = {
"DIMENSIONLESS", "TEMPERATURE", "PRESSURE", "VOLUMETRICFLOW", "MASSFLOW", "POWER", "DISTANCE", "WAVELENGTH", "ANGLE"};
"DIMENSIONLESS",
"TEMPERATURE",
"PRESSURE",
"VOLUMETRICFLOW",
"MASSFLOW",
"POWER",
"DISTANCE",
"WAVELENGTH",
"ANGLE",
"VOLUMETRICFLOWPERPOWER",
};

if (InInputType.empty()) {
return true; // if not used it is valid
Expand Down
232 changes: 232 additions & 0 deletions tst/EnergyPlus/unit/CurveManager.unit.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,8 @@
#include <EnergyPlus/DataGlobals.hh>
#include <EnergyPlus/DataIPShortCuts.hh>
#include <EnergyPlus/FileSystem.hh>
#include <embedded/EmbeddedEpJSONSchema.hh>
#include <nlohmann/json.hpp>

#include "Fixtures/EnergyPlusFixture.hh"

Expand Down Expand Up @@ -742,3 +744,233 @@ TEST_F(EnergyPlusFixture, CSV_CarriageReturns_Handling)
EXPECT_FALSE(std::isnan(TestArray[i]));
}
}

nlohmann::json const &getPatternProperties(nlohmann::json const &schema_obj)
{
auto const &pattern_properties = schema_obj["patternProperties"];
int dot_star_present = pattern_properties.count(".*");
std::string pattern_property;
if (dot_star_present > 0) {
pattern_property = ".*";
} else {
int no_whitespace_present = pattern_properties.count(R"(^.*\S.*$)");
if (no_whitespace_present > 0) {
pattern_property = R"(^.*\S.*$)";
} else {
throw std::runtime_error(R"(The patternProperties value is not a valid choice (".*", "^.*\S.*$"))");
}
}
auto const &schema_obj_props = pattern_properties[pattern_property]["properties"];
return schema_obj_props;
}

std::vector<std::string> getPossibleChoicesFromSchema(const std::string &objectType, const std::string &fieldName)
{
// Should consider making this public, at least to the EnergyPlusFixture, but maybe in the InputProcessor directly
// At which point, should handle the "anyOf" case, here I don't need it, so not bothering
Comment on lines +769 to +770
Copy link
Contributor Author

Choose a reason for hiding this comment

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

note

static const auto json_schema = nlohmann::json::from_cbor(EmbeddedEpJSONSchema::embeddedEpJSONSchema());
auto const &schema_properties = json_schema.at("properties");
const auto &object_schema = schema_properties.at(objectType);
auto const &schema_obj_props = getPatternProperties(object_schema);
auto const &schema_field_obj = schema_obj_props.at(fieldName);
std::vector<std::string> choices;
for (const auto &e : schema_field_obj.at("enum")) {
choices.push_back(e);
}

return choices;
}
Comment on lines +748 to +782
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mine the json schema for the choices


TEST_F(EnergyPlusFixture, TableIndependentVariableUnitType_IsValid)
{
std::vector<std::string> unit_type_choices = getPossibleChoicesFromSchema("Table:IndependentVariable", "unit_type");
for (const auto &input_unit_type : unit_type_choices) {
EXPECT_TRUE(Curve::IsCurveInputTypeValid(input_unit_type)) << input_unit_type << " is rejected by IsCurveInputTypeValid";
}
EXPECT_EQ(8, unit_type_choices.size());
}
Comment on lines +784 to +791
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ensure it's valid


TEST_F(EnergyPlusFixture, TableLookupUnitType_IsValid)
{
std::vector<std::string> unit_type_choices = getPossibleChoicesFromSchema("Table:Lookup", "output_unit_type");
for (const auto &output_unit_type : unit_type_choices) {
if (output_unit_type.empty()) {
continue;
}
EXPECT_TRUE(Curve::IsCurveOutputTypeValid(output_unit_type)) << output_unit_type << " is rejected by IsCurveOutputTypeValid";
}
EXPECT_EQ(6, unit_type_choices.size());
}
Comment on lines +793 to +803
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same for TableLookup output.


class InputUnitTypeIsValid : public EnergyPlusFixture, public ::testing::WithParamInterface<std::string_view>
{
};
TEST_P(InputUnitTypeIsValid, IndepentVariable)
{
const auto &unit_type = GetParam();

std::string const idf_objects = delimited_string({
"Table:IndependentVariable,",
" SAFlow, !- Name",
" Cubic, !- Interpolation Method",
" Constant, !- Extrapolation Method",
" 0.714, !- Minimum Value",
" 1.2857, !- Maximum Value",
" , !- Normalization Reference Value",
fmt::format(" {}, !- Unit Type", unit_type),
" , !- External File Name",
" , !- External File Column Number",
" , !- External File Starting Row Number",
" 0.714286, !- Value 1",
" 1.0,",
" 1.2857;",
Comment on lines +805 to +826
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Parametrized test


"Table:IndependentVariableList,",
" SAFlow_Variables, !- Name",
" SAFlow; !- Independent Variable 1 Name",

"Table:Lookup,",
" CoolCapModFuncOfSAFlow, !- Name",
" SAFlow_Variables, !- Independent Variable List Name",
" , !- Normalization Method",
" , !- Normalization Divisor",
" 0.8234, !- Minimum Output",
" 1.1256, !- Maximum Output",
" Dimensionless, !- Output Unit Type",
" , !- External File Name",
" , !- External File Column Number",
" , !- External File Starting Row Number",
" 0.823403, !- Output Value 1",
" 1.0,",
" 1.1256;",
});

ASSERT_TRUE(process_idf(idf_objects));
EXPECT_EQ(0, state->dataCurveManager->NumCurves);

Curve::GetCurveInput(*state);
state->dataCurveManager->GetCurvesInputFlag = false;
EXPECT_TRUE(compare_err_stream("", true));
Comment on lines +851 to +853
Copy link
Contributor Author

Choose a reason for hiding this comment

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

That calls GetCurveInput specifically. That exhibits the issue where IsValidCurveInput is true, but it's IsValidCurveOUTPUT that's called by mistake

}

INSTANTIATE_TEST_SUITE_P(CurveManager,
InputUnitTypeIsValid,
testing::Values("", "Angle", "Dimensionless", "Distance", "MassFlow", "Power", "Temperature", "VolumetricFlow"),
[](const testing::TestParamInfo<InputUnitTypeIsValid::ParamType> &info) -> std::string {
if (info.param.empty()) {
return "Blank";
}
return std::string{info.param};
});
Comment on lines +856 to +864
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Instantiate the param tests... I could have used testing::ValuesIn(container) instead to make it fully dynamic but didn't


class OutputUnitTypeIsValid : public EnergyPlusFixture, public ::testing::WithParamInterface<std::string_view>
{
};
TEST_P(OutputUnitTypeIsValid, TableLookup)
Comment on lines +866 to +869
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same stuff with TableLookup output unit type

{
const auto &unit_type = GetParam();

std::string const idf_objects = delimited_string({
"Table:IndependentVariable,",
" SAFlow, !- Name",
" Cubic, !- Interpolation Method",
" Constant, !- Extrapolation Method",
" 0.714, !- Minimum Value",
" 1.2857, !- Maximum Value",
" , !- Normalization Reference Value",
" Dimensionless, !- Unit Type",
" , !- External File Name",
" , !- External File Column Number",
" , !- External File Starting Row Number",
" 0.714286, !- Value 1",
" 1.0,",
" 1.2857;",

"Table:IndependentVariableList,",
" SAFlow_Variables, !- Name",
" SAFlow; !- Independent Variable 1 Name",

"Table:Lookup,",
" CoolCapModFuncOfSAFlow, !- Name",
" SAFlow_Variables, !- Independent Variable List Name",
" , !- Normalization Method",
" , !- Normalization Divisor",
" 0.8234, !- Minimum Output",
" 1.1256, !- Maximum Output",
fmt::format(" {}, !- Output Unit Type", unit_type),
" , !- External File Name",
" , !- External File Column Number",
" , !- External File Starting Row Number",
" 0.823403, !- Output Value 1",
" 1.0,",
" 1.1256;",
});

ASSERT_TRUE(process_idf(idf_objects));
EXPECT_EQ(0, state->dataCurveManager->NumCurves);

Curve::GetCurveInput(*state);
state->dataCurveManager->GetCurvesInputFlag = false;
EXPECT_TRUE(compare_err_stream("", true));
}

INSTANTIATE_TEST_SUITE_P(CurveManager,
OutputUnitTypeIsValid,
testing::Values("", "Capacity", "Dimensionless", "Power", "Pressure", "Temperature"),
[](const testing::TestParamInfo<InputUnitTypeIsValid::ParamType> &info) -> std::string {
if (info.param.empty()) {
return "Blank";
}
return std::string{info.param};
});

std::pair<std::set<std::string>, std::set<std::string>> getAllPossibleInputOutputTypesForCurves()
{
const auto json_schema = nlohmann::json::from_cbor(EmbeddedEpJSONSchema::embeddedEpJSONSchema());
auto const &schema_properties = json_schema.at("properties");
std::set<std::string> all_input_choices;
std::set<std::string> all_output_choices;

for (const auto &[objectType, object_schema] : schema_properties.items()) {
const bool is_curve = (objectType.rfind("Curve:", 0) == 0) || (objectType == "Table:Lookup") || (objectType == "Table:IndependentVariable");
if (!is_curve) {
continue;
}
auto const &schema_obj_props = getPatternProperties(object_schema);
for (const auto &[fieldName, schema_field_obj] : schema_obj_props.items()) {
if (std::string(fieldName) == "output_unit_type") {
for (const auto &e : schema_field_obj.at("enum")) {
all_output_choices.insert(std::string{e});
}
} else if (fieldName.find("unit_type") != std::string::npos) {
for (const auto &e : schema_field_obj.at("enum")) {
all_input_choices.insert(std::string{e});
}
}
}
}

return {all_input_choices, all_output_choices};
}

TEST_F(EnergyPlusFixture, AllPossibleUnitTypeValid)
{
auto const [all_input_choices, all_output_choices] = getAllPossibleInputOutputTypesForCurves();

// As of 2024-12-18
// in = ["", "Angle", "Dimensionless", "Distance", "MassFlow", "Power", "Pressure", "Temperature", "VolumetricFlow","VolumetricFlowPerPower"]
// out = ["", "Capacity", "Dimensionless", "Power", "Pressure", "Temperature"]
EXPECT_FALSE(all_input_choices.empty()) << fmt::format("{}", all_input_choices);
EXPECT_FALSE(all_output_choices.empty()) << fmt::format("{}", all_output_choices);

for (const auto &input_unit_type : all_input_choices) {
EXPECT_TRUE(Curve::IsCurveInputTypeValid(input_unit_type)) << input_unit_type << " is rejected by IsCurveOutputTypeValid";
}

for (const auto &output_unit_type : all_output_choices) {
if (output_unit_type.empty()) {
continue;
}
EXPECT_TRUE(Curve::IsCurveOutputTypeValid(output_unit_type)) << output_unit_type << " is rejected by IsCurveOutputTypeValid";
}
}
Comment on lines +927 to +976
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Dynamic test that collates ALL possible input/output unit types by mining the schema for all curve objects and tests that IsCurve<In/Out>putTypeValid is ok. It wasn't for VolumetricFlowPerPower

Loading