Fix #10830 - incorrect curve unit type warning #10853
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
| if (indVarInstance.count("unit_type")) { | ||
| std::string unitType = indVarInstance.at("unit_type").get<std::string>(); | ||
| if (!IsCurveOutputTypeValid(unitType)) { | ||
| if (!IsCurveInputTypeValid(unitType)) { |
| 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
| Distance, | ||
| Wavelength, | ||
| Angle, | ||
| VolumetricFlowPerPower, |
There was a problem hiding this comment.
Add Missing Input Type VolumetricFlowPerPower: used by Curve:QuadLinear & Curve:QuintLinear
| 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
| // 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 |
| 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;", |
| 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
| 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
| 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
| 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. |
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. |
|
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. |
|
Oh, and anyway, your tests do pass. So this is good. I'll push the resolved commit and get this merged. Thanks @jmarrec |
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.