Fix #10830 - incorrect curve unit type warning #10853
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…utputTypeValid
…it Types are accepted: they aren't ``` [ RUN ] EnergyPlusFixture.AllPossibleUnitTypeValid /home/julien/Software/Others/EnergyPlus2/tst/EnergyPlus/unit/CurveManager.unit.cc:967: Failure Value of: Curve::IsCurveInputTypeValid(input_unit_type) Actual: false Expected: true VolumetricFlowPerPower is rejected by IsCurveOutputTypeValid ```
…ar & Curve:QuintLinear
jmarrec
added
Defect
jmarrec
commented
Dec 12, 2024
| @@ -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)) { | |||
There was a problem hiding this comment.
Specific fix for #10830 is here
Comment on lines
802
to
+804
| 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))); |
There was a problem hiding this comment.
Noticed another mistake here in Curve:ChillerPartLoadWithLift
| @@ -2953,10 +2955,21 @@ namespace Curve { | |||
| Distance, | |||
| Wavelength, | |||
| Angle, | |||
| VolumetricFlowPerPower, | |||
There was a problem hiding this comment.
Add Missing Input Type VolumetricFlowPerPower: used by Curve:QuadLinear & Curve:QuintLinear
Comment on lines
+748
to
+782
| 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 | ||
| 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; | ||
| } |
There was a problem hiding this comment.
Mine the json schema for the choices
Comment on lines
+769
to
+770
| // 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
+805
to
+826
| 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
+851
to
+853
| Curve::GetCurveInput(*state); | ||
| state->dataCurveManager->GetCurvesInputFlag = false; | ||
| EXPECT_TRUE(compare_err_stream("", true)); |
There was a problem hiding this comment.
That calls GetCurveInput specifically. That exhibits the issue where IsValidCurveInput is true, but it's IsValidCurveOUTPUT that's called by mistake
Comment on lines
+856
to
+864
| 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}; | ||
| }); |
There was a problem hiding this comment.
Instantiate the param tests... I could have used testing::ValuesIn(container) instead to make it fully dynamic but didn't
Comment on lines
+866
to
+869
| class OutputUnitTypeIsValid : public EnergyPlusFixture, public ::testing::WithParamInterface<std::string_view> | ||
| { | ||
| }; | ||
| TEST_P(OutputUnitTypeIsValid, TableLookup) |
There was a problem hiding this comment.
Same stuff with TableLookup output unit type
Comment on lines
+927
to
+976
| 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"; | ||
| } | ||
| } |
There was a problem hiding this comment.
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
|
@jmarrec it has been 8 days since this pull request was last updated. |
|
@jmarrec it has been 7 days since this pull request was last updated. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Pull request overview
Pull Request Author
Add to this list or remove from it as applicable. This is a simple templated set of guidelines.
Reviewer
This will not be exhaustively relevant to every PR.