Fix #10830 - incorrect curve unit type warning#10853
Merged
Myoldmopar merged 7 commits intodevelopfrom Feb 4, 2025
Merged
Conversation
…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
| if (indVarInstance.count("unit_type")) { | ||
| std::string unitType = indVarInstance.at("unit_type").get<std::string>(); | ||
| if (!IsCurveOutputTypeValid(unitType)) { | ||
| if (!IsCurveInputTypeValid(unitType)) { |
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))); |
Contributor
Author
There was a problem hiding this comment.
Noticed another mistake here in Curve:ChillerPartLoadWithLift
| Distance, | ||
| Wavelength, | ||
| Angle, | ||
| VolumetricFlowPerPower, |
Contributor
Author
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; | ||
| } |
Contributor
Author
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)); |
Contributor
Author
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}; | ||
| }); |
Contributor
Author
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) |
Contributor
Author
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"; | ||
| } | ||
| } |
Contributor
Author
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. |
5 similar comments
|
@jmarrec it has been 7 days since this pull request was last updated. |
|
@jmarrec it has been 7 days since this pull request was last updated. |
|
@jmarrec it has been 7 days since this pull request was last updated. |
|
@jmarrec it has been 7 days since this pull request was last updated. |
|
@jmarrec it has been 7 days since this pull request was last updated. |
Member
|
OK, something just struck me as I was looking through your new parameterized tests. I recalled our macro that actually parses through looking for unit tests to add as ctest entries with The code that sets up the ctest entries is here, with specifically this section in question: It is looking for I'm not inclined to hold this up to solve that, but I would like to solve it soon. Let me know if you have any thoughts. |
Member
|
Oh, and anyway, your tests do pass. So this is good. I'll push the resolved commit and get this merged. Thanks @jmarrec |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.