epJSON helper functions to get field values #8738

mjwitte · 2021-04-30T01:44:05Z

Pull request overview

Addresses Add helper functions to validate and get an epJSON object and field value #7632
Added three new input processor functions to simplify input processing using epJSON field_names instead of getObjectItem.
getObjectSchemaProps(state, CurrentModuleObject)
getAlphaFieldValue(state, CurrentModuleObject, objectFields, objectSchemaProps, "alpha_field_name")
getRealFieldValue(state, CurrentModuleObject, objectFields, objectSchemaProps, "real_field_name")
getIntFieldValue(state, CurrentModuleObject, objectFields, objectSchemaProps, "integer_field_name")

ToDos/Questions:

Add support for extensible fields - do we even have a pattern for that somewhere - grabbing extensible fields without using getObjectItem?
~~Look for more code re-use opportunities in InputProcessor (e.g. getDefaultValue also searches for the object and field schema)~~
Undo the Airflownetwork getinput changes to avoid conflicts with other AFN PRs.
Demonstrate the new functions in a getInput function somewhere
Add extensible fields to the unit tests

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.

rraustad · 2021-04-30T01:47:49Z

Your on a roll. Is getFieldName on your list? I'd love that for eio reporting.

rraustad · 2021-04-30T01:49:59Z

OK, so I guess that would just be a reformatting of the fieldname passed to these functions.

mjwitte · 2021-04-30T01:50:41Z

Is getFieldName on your list? I'd love that for eio reporting.

Do you mean pass in the epJSON field name "thermal_conductivity" and return the IDD field name "Thermal Conductivity"?

rraustad · 2021-04-30T01:51:58Z

Yes, I had an issue with getting field names during the sizer function refactor.

rraustad · 2021-04-30T02:02:43Z

And what happens when we change thermal_conductivity to material_thermal_conductivity? Are the field names frozen from here forward? I better get #8691 wrapped up quick.

rraustad · 2021-04-30T02:08:14Z

Seems it needs to be an index/vector, but then that index can't change in the future.

mjwitte · 2021-05-03T19:57:50Z

src/EnergyPlus/AirflowNetworkBalanceManager.cc

+                Real64 coeff = state.dataInputProcessing->inputProcessor->getRealFieldValue(
+                    state, CurrentModuleObject, fields, objectSchemaProps, "air_mass_flow_coefficient_at_reference_conditions");


@amirroth When trying to apply the new getFieldValue functions in an actual getInput function, it seemed silly to have patterns like

Real64 coeff = 0.0; state.dataInputProcessing->inputProcessor->getFieldValue(state, CurrentModuleObject, fields, objectSchemaProps, "air_mass_flow_coefficient_at_reference_conditions", coeff);

By moving to explicitly typed functions, this reduces to one line. Seems more readable.

Real64 coeff = state.dataInputProcessing->inputProcessor->getRealFieldValue(state, CurrentModuleObject, fields, objectSchemaProps, "air_mass_flow_coefficient_at_reference_conditions");

rraustad · 2021-05-03T20:00:36Z

src/EnergyPlus/InputProcessing/InputProcessor.cc

+            if (is_empty) {
+                isDefaulted = findDefault(value, schema_field_obj);
+            } else {
+                value = -99999; // autosize and autocalculate


I guess you assume here if !is_empty then the input is either blank or is set to Autosize.

I explicitly tested for that but maybe with this method you don't have to test. Anything other than the word autosize would be caught by the inputprocessor.

auto tempFieldVal = fields.at("heating_supply_air_flow_rate"); if (tempFieldVal == "Autosize") { loc_m_HeatingSAFMethod_SAFlow = DataSizing::AutoSize; } else { loc_m_HeatingSAFMethod_SAFlow = fields.at("heating_supply_air_flow_rate"); }

Do you need to check for?

\autosizable

I'm not 100% certain about that. In getRealFieldvalue I mimicked the code in findDefault for handling \autosize. It seems a bit fragile to assume if there's a string in a numeric field then it's autosize (or autocalculate). I'm not sure if illegal use of autosize or nonesense words get trapped during initial input processing (and that could be different if the incoming file is idf vs epJSON). Need to do some testing on that.

rraustad · 2021-05-19T12:05:36Z

tst/EnergyPlus/unit/InputProcessor.unit.cc

+    numericFieldValue =
+        state->dataInputProcessing->inputProcessor->getRealFieldValue(*state, obj_type1, wh1, objectSchemaProps, "heater_maximum_capacity");
+    EXPECT_EQ(numericFieldValue, -99999);
+    // Even a field that is not autoszable will return -99999 here (assuming that gets checked upon epJSON input processing)


I guess it is safe to assume this will always be caught. I wonder if this is actually unit tested somewhere.

** Severe ** <root>[Building][Building][loads_convergence_tolerance_value] - Value type "string" for input "autosize" not permitted by 'type' constraint.

Not sure why it says this input field type is "constraint"?

"loads_convergence_tolerance_value": { "type": "number",

mjwitte · 2021-05-25T18:39:32Z

This is ready for review. There is a design doc that shows prototype getInput blocks using these new functions. It still seems like a lot of lines of code are required to set things up before processing the individual fields. Suggestions are welecome.

amirroth · 2021-05-25T19:29:08Z

src/EnergyPlus/InputProcessing/InputProcessor.cc

+}
+
+Real64 InputProcessor::getRealFieldValue(
+    EnergyPlusData &state, std::string const &objectWord, json const &ep_object, json const &schema_obj_props, std::string const &fieldName)


Does it make sense to have separate functions for Real and Int?

Good question. I started to do that and then realized that the epJSON schema does not differentiate between real and integer: "type": "number"

getRealFieldVal could be changed to getNumericFieldVal if there is no differentiation between int and real. I wonder if returning a real will have any effect when reading into an int variable but since we do that now anyway with getObjectItem's Numbers(x) it shouldn't be a problem.

If you wanted to return int or Real, you would need to have separate functions since they'd likely have the same input parameter signature and only change the return type.

I will also say with the following, which I saw in an outdated comment in this PR:

Real64 coeff = 0.0; state.dataInputProcessing->inputProcessor->getFieldValue(state, CurrentModuleObject, fields, objectSchemaProps, "air_mass_flow_coefficient_at_reference_conditions", coeff);

It does seem odd to not directly return the fieldValue, however, this function could be overloaded to include int or string as input types with their own unique functions. Such as below:

int coeff = 0.0; state.dataInputProcessing->inputProcessor->getFieldValue(state, CurrentModuleObject, fields, objectSchemaProps, "air_mass_flow_coefficient_at_reference_conditions", coeff); std::string coeff = "look a coefficient!"; state.dataInputProcessing->inputProcessor->getFieldValue(state, CurrentModuleObject, fields, objectSchemaProps, "air_mass_flow_coefficient_at_reference_conditions", coeff);

This means there is only ever one function name to call.
Otherwise, you'll need to have getRealFieldValue, getIntFieldValue, and getAlphaFieldValue functions like below:

Real64 coeff = state.dataInputProcessing->inputProcessor->getRealFieldValue(state, CurrentModuleObject, fields, objectSchemaProps, "air_mass_flow_coefficient_at_reference_conditions");

amirroth · 2021-05-25T19:31:03Z

src/EnergyPlus/HeatBalanceManager.cc

+                // Load the material derived type from the input data.
+                state.dataMaterial->Material(MaterNum).Group = RegularMaterial;
+                state.dataMaterial->Material(MaterNum).Name = materialName;
+


Just a stylistic thing, but defining auto & Mat = state.dataMaterial->Material(MaterNum); and auto & ip = state.dataInputProcessing->inputProcessor; will make the rest of statements in this function into one liners.

amirroth · 2021-05-25T19:32:11Z

src/EnergyPlus/HeatBalanceManager.cc

+
+                std::string roughness = state.dataInputProcessing->inputProcessor->getAlphaFieldValue(
+                    state, state.dataHeatBalMgr->CurrentModuleObject, objectFields, objectSchemaProps, "roughness");
+                ValidateMaterialRoughness(state, MaterNum, roughness, ErrorsFound);


Not for this PR, but just noting that ValidateMaterialRoughness and functions of similar ilk will be going soon.

amirroth · 2021-05-25T19:36:38Z

src/EnergyPlus/InputProcessing/InputProcessor.cc

+        } else {
+            bool is_empty = field_value.get<std::string>().empty();
+            if (is_empty) {
+                isDefaulted = findDefault(value, schema_field_obj);


Need to do something if isDefaulted is false?

Another good question. This block of code is borrowed from elsewhere (somewhere in the getObjectItem chain. isDefaulted gets used elsewhere for some statistics on the number of defaulted fields in the input file. This could probably be simplified a bit here.

mbadams5 · 2021-06-09T19:24:35Z

src/EnergyPlus/InputProcessing/InputProcessor.cc

+}
+
+Real64 InputProcessor::getRealFieldValue(
+    EnergyPlusData &state, std::string const &objectWord, json const &ep_object, json const &schema_obj_props, std::string const &fieldName)


If you wanted to return int or Real, you would need to have separate functions since they'd likely have the same input parameter signature and only change the return type.

mbadams5 · 2021-06-09T19:31:35Z

src/EnergyPlus/InputProcessing/InputProcessor.cc

+}
+
+Real64 InputProcessor::getRealFieldValue(
+    EnergyPlusData &state, std::string const &objectWord, json const &ep_object, json const &schema_obj_props, std::string const &fieldName)


I will also say with the following, which I saw in an outdated comment in this PR:

Real64 coeff = 0.0; state.dataInputProcessing->inputProcessor->getFieldValue(state, CurrentModuleObject, fields, objectSchemaProps, "air_mass_flow_coefficient_at_reference_conditions", coeff);

It does seem odd to not directly return the fieldValue, however, this function could be overloaded to include int or string as input types with their own unique functions. Such as below:

int coeff = 0.0; state.dataInputProcessing->inputProcessor->getFieldValue(state, CurrentModuleObject, fields, objectSchemaProps, "air_mass_flow_coefficient_at_reference_conditions", coeff); std::string coeff = "look a coefficient!"; state.dataInputProcessing->inputProcessor->getFieldValue(state, CurrentModuleObject, fields, objectSchemaProps, "air_mass_flow_coefficient_at_reference_conditions", coeff);

This means there is only ever one function name to call.
Otherwise, you'll need to have getRealFieldValue, getIntFieldValue, and getAlphaFieldValue functions like below:

Real64 coeff = state.dataInputProcessing->inputProcessor->getRealFieldValue(state, CurrentModuleObject, fields, objectSchemaProps, "air_mass_flow_coefficient_at_reference_conditions");

mbadams5 · 2021-06-09T19:40:41Z

src/EnergyPlus/InputProcessing/InputProcessor.cc

+    return value;
+}
+
+json InputProcessor::getObjectSchemaProps(EnergyPlusData &state, std::string const &objectWord)


This will create a copy of the schema json everytime it is called. It should really return json const &, especially since getPatternProperties returns json const &:

json const &InputProcessor::getPatternProperties(EnergyPlusData &state, json const &schema_obj)

and you return from getPatternProperties

json schema_obj_props = getPatternProperties(state, object_schema); return schema_obj_props;

If this current function is called many times it will cause performance issues.

mjwitte · 2021-06-09T20:58:04Z

Otherwise, you'll need to have getRealFieldValue, getIntFieldValue, and getAlphaFieldValue functions like below:

@mbadams5 Is there anywhere in the current input processing that distinguishes between int and real? getObjectItem return an array of reals, regardless of the field type in the idd and the schema seems to just have alpha or numeric. So, I'm planning to change getRealFieldValue to getNumericFieldValue unless I'm missing something related to integer fields.

mbadams5 · 2021-06-10T12:41:07Z

@mjwitte No, but mostly that is an artifact of the old InputProcessor and IDD. The IDD does not distinguish between real and int, however, json schema can use integer type for fields that should be integer. So as long as we are auto-generating the epjson schema from the IDD, it will likely only ever be alpha or numeric. The same goes for internally within InputProcessor, but there could be a future where there are expanded types usable by epjson schema.

mjwitte · 2021-06-10T14:11:25Z

@mbadams5 Actually, there are some fields in the IDD that are tagged with \type integer so we could be using that information in the epJSON schema. I'll add an integer function here for now and then in the future the real and integer functions can validate against the numeric type.

I think I've addressed your const & comment in getObjectSchemaProps. Please confirm that I got it right.

mbadams5 · 2021-06-10T14:44:41Z

src/EnergyPlus/InputProcessing/InputProcessor.cc

+std::string InputProcessor::getAlphaFieldValue(json const &ep_object, json const &schema_obj_props, std::string const &fieldName)
+{
+    auto const &schema_field_obj = schema_obj_props[fieldName];
+    assert(!schema_field_obj.empty()); // Check that field name exists in the schema for this object type


Why did you convert this to an assert statement instead of the FatalErrorMessage? Asserts only work in debug builds, so is this something only developers will ever hit? If not, then it should probably be the FatalError.

I changed to assert because this is a developer error (same for the others). The function arguments for state and objectType were only being used for these error messages that should never happen unless the developer makes a mistake. So it seemed reasonable to use an assert and drop those.

@mjwitte can you give me an example of why schema_field_obj.empty() would ever be false? I get the misspelled field name issue but are there other instances? Inputs past min-fields?

I see this is merged, but just for the record, this line is looking at the schema (not the user input) in schema_obj_props. So empty will be true if the field name does not exist in schema_obj_props.

mbadams5 · 2021-06-10T14:46:05Z

src/EnergyPlus/InputProcessing/InputProcessor.cc

+            value = valuePair.first;
+            isDefaulted = valuePair.second;
+        } else {
+            assert(false); // String value requested but field type in numeric


Same comment about change to assert here. This assert statement provides little information except for the inline comment.

I can revert this, but I like the leaner function signature. If there's a better approach, I'm open to suggestions.

mbadams5 · 2021-06-10T14:46:21Z

src/EnergyPlus/InputProcessing/InputProcessor.cc

+Real64 InputProcessor::getRealFieldValue(json const &ep_object, json const &schema_obj_props, std::string const &fieldName)
+{
+    auto const &schema_field_obj = schema_obj_props[fieldName];
+    assert(!schema_field_obj.empty()); // Check that field name exists in the schema for this object type


Same question about change to assert.

mbadams5 · 2021-06-10T14:46:27Z

src/EnergyPlus/InputProcessing/InputProcessor.cc

+{
+    auto const &schema_properties = schema.at("properties");
+    const json &object_schema = schema_properties.at(objectWord);
+    assert(!object_schema.empty()); // If this fails, the object type does not exist in the schema


Same question about change to assert.

mbadams5 · 2021-06-10T14:49:18Z

@mjwitte I see in the IDD where it has \type integer and I also see in idd_parser.py script where it finds it but only uses number for both real and int. In the future it seems easy to break these apart to include a type integer for epjson schema.

The getObjectSchemaProps change looks correct.

rraustad · 2021-06-11T22:36:30Z

@mjwitte and @mbadams5 (thanks for the review) are there any remaining issues? I would like this merged as soon as possible. I am available to click merge at a moments notice.

rraustad · 2021-06-12T00:13:38Z

Unit tests run to completion on Win64. This seems ready and any remaining items can be addressed as needed with follow up issues. I am a little surprised there is an indication of a speed improvement albeit small. I will wait on CI for a bit.

rraustad

I see no significant issues. I am not sure int vs real functions is even an issue.

rraustad · 2021-06-12T00:22:34Z

Failed unit test PackagedTerminalHP_VSCoils_Sizing on Mac is likely unrelated.

mjwitte added 4 commits April 21, 2021 19:42

epJSON getFieldValue initial commit

de3cf57

Merge remote-tracking branch 'remotes/origin/develop' into epJSONhelpers

147ad9d

getFieldValue unit test

a6a34ed

epJSON getFieldSchema

4d5b8cb

mjwitte added the DoNotPublish Includes changes that shouldn't be reported in the changelog label Apr 30, 2021

mjwitte added 6 commits April 30, 2021 10:45

Merge remote-tracking branch 'remotes/origin/develop' into epJSONhelpers

9ddda8c

epJSON getObjectSchemaProps and other cleanup

4f35d70

Merge remote-tracking branch 'remotes/origin/develop' into epJSONhelpers

8c041ff

epJSON move to explicitly types getFieldValue functions

89ad8e5

epJSON move to explicitly types getFieldValue functions partB

6485bb4

epJSON getFieldValue small demonstration

b94cdf5

mjwitte commented May 3, 2021

View reviewed changes

rraustad reviewed May 3, 2021

View reviewed changes

mjwitte added 2 commits May 4, 2021 16:54

epJSON getObjectSchemaProps search the full schema

b9a7cc1

epJSON getFieldValue more unit tests

6f2a1ff

mjwitte added Refactoring Includes code changes that don't change the functionality of the program, just perform refactoring and removed DoNotPublish Includes changes that shouldn't be reported in the changelog labels May 5, 2021

mjwitte added 6 commits May 6, 2021 13:18

Merge remote-tracking branch 'remotes/origin/develop' into epJSONhelpers

eb465d4

Merge remote-tracking branch 'remotes/origin/develop' into epJSONhelpers

88916fe

Merge remote-tracking branch 'remotes/origin/develop' into epJSONhelpers

a186722

epJSON getObjectSchemaProps return json

d153e42

Merge remote-tracking branch 'remotes/origin/develop' into epJSONhelpers

c51b17a

epJSON GetSysAvailManagerListInputs

17244fb

rraustad reviewed May 19, 2021

View reviewed changes

mjwitte added 3 commits May 21, 2021 13:36

epJSON GetMaterialData skip if empty and preserve idf order

fca03ef

Merge remote-tracking branch 'remotes/origin/develop' into epJSONhelpers

4c3a045

epJSON revert AFN demonstration

855a779

mjwitte marked this pull request as ready for review May 25, 2021 18:35

mjwitte requested a review from Myoldmopar May 25, 2021 18:36

amirroth reviewed May 25, 2021

View reviewed changes

mjwitte requested a review from mbadams5 May 26, 2021 18:24

Merge remote-tracking branch 'remotes/origin/develop' into epJSONhelpers

9132be6

mbadams5 approved these changes Jun 9, 2021

View reviewed changes

epJSON address const reference and streamline for readability

c9328ff

mbadams5 reviewed Jun 10, 2021

View reviewed changes

mjwitte added 3 commits June 11, 2021 14:54

Merge remote-tracking branch 'remotes/origin/develop' into epJSONhelpers

97353a1

epJSON add getIntFieldValue

b7abee8

epJSON cleanup

d718ebd

rraustad approved these changes Jun 12, 2021

View reviewed changes

rraustad merged commit 5358d3e into develop Jun 12, 2021

rraustad deleted the epJSONhelpers branch June 12, 2021 00:43

mjwitte mentioned this pull request Jul 9, 2021

Add helper functions to validate and get an epJSON object and field value #7632

Closed

3 tasks

rraustad mentioned this pull request May 13, 2022

AirflowNetwork Refactor #9429

Merged

14 tasks

		Real64 coeff = state.dataInputProcessing->inputProcessor->getRealFieldValue(
		state, CurrentModuleObject, fields, objectSchemaProps, "air_mass_flow_coefficient_at_reference_conditions");

epJSON helper functions to get field values #8738

epJSON helper functions to get field values #8738

Conversation

mjwitte commented Apr 30, 2021 • edited

Pull request overview

Pull Request Author

Reviewer

rraustad commented Apr 30, 2021

rraustad commented Apr 30, 2021

mjwitte commented Apr 30, 2021

rraustad commented Apr 30, 2021

rraustad commented Apr 30, 2021

rraustad commented Apr 30, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjwitte commented May 25, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amirroth May 25, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjwitte commented Jun 9, 2021 • edited

mbadams5 commented Jun 10, 2021

mjwitte commented Jun 10, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbadams5 commented Jun 10, 2021

rraustad commented Jun 11, 2021

rraustad commented Jun 12, 2021

rraustad left a comment

Choose a reason for hiding this comment

rraustad commented Jun 12, 2021 • edited

mjwitte commented Apr 30, 2021 •

edited

amirroth May 25, 2021 •

edited

mjwitte commented Jun 9, 2021 •

edited

rraustad commented Jun 12, 2021 •

edited