Fill object defaults for blank or missing fields and fix Site:GroundReflectance:SnowModifier defaults #10295

rraustad · 2023-11-05T15:24:56Z

Pull request overview

Fixes Supplemental heating coil default not filled in #10282. Supersedes Increase VRF terminal unit min-field to include supp heat coil turn-off temperature #10283?
Adds alternative method for ZoneHVAC:TerminalUnit:VariableRefrigerantFlow input for Maximum Outdoor Dry-Bulb Temperature for Supplemental Heater Operation to return default when input field is missing from inputs. Default = 21 C.
Fixes incorrect polygon method tag in dxf output. When dxf output is requested without specifying Report Specifications 1 the dxf file is written using the correct default method Triangulate3DFace but the tag in the dxf file was ThickPolyline
Fixes Site:GroundReflectance:SnowModifier may use incorrect default for daylighting #10360 .

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 · 2023-11-05T15:33:18Z

@mjwitte @yujiex this appears to fill numeric defaults past min-fields. The #10282 defect file reads in 21 C as the default. If this field is entered then that value is used instead of the defaut. I'm not saying all getInputs are set up to read this correctly but in the case of the TUs, the default value is read in correctly.

If the user enters a value for that field (in this case 17 C) that value will be used instead.

rraustad · 2023-11-05T16:06:27Z

For the case of the HeatBalanceAlgorithm, which has no min-fields and this defect file does not have any of the numeric fields, the defaults are filled (just not used since getInput for this object is not set up to read them in unless they are present).

HeatBalanceAlgorithm,ConductionTransferFunction;

In this case the defaults don't need to be read in since these data are initialized to the defaults in the constructor.

struct HeatBalanceData : BaseGlobalStruct
{
    Real64 LowHConvLimit = 0.1; // Lowest allowed convection coefficient for detailed model
    Real64 HighHConvLimit = 1000.0;         // upper limit for HConv, mostly used for user input limits in practice. !W/m2-K

struct HeatBalSurfData : BaseGlobalStruct
{
    Real64 MaxSurfaceTempLimit = 200.0;            // Highest inside surface temperature allowed in Celsius
    Real64 MaxSurfaceTempLimitBeforeFatal = 500.0; // 2.5 times MaxSurfaceTempLimit

rraustad · 2023-11-05T16:43:53Z

Diff for GroundTempOSCCompactSched:

SurfaceProperty:OtherSideCoefficients,
N8, \field Period of Sinusoidal Variation
  \note Use with sinusoidal variation to define the time period
  \type real
  \units hr
  \default 24
  \minimum> 0

        state.dataSurface->OSC(OSCNum).Name = state.dataIPShortCut->cAlphaArgs(1);
        state.dataSurface->OSC(OSCNum).SurfFilmCoef = state.dataIPShortCut->rNumericArgs(1);
        state.dataSurface->OSC(OSCNum).ConstTemp = state.dataIPShortCut->rNumericArgs(2); //  This will be replaced if  schedule is used
        state.dataSurface->OSC(OSCNum).ConstTempCoef =
            state.dataIPShortCut->rNumericArgs(3); //  This multiplier is used (even with schedule).  It should normally be 1.0
        state.dataSurface->OSC(OSCNum).ExtDryBulbCoef = state.dataIPShortCut->rNumericArgs(4);
        state.dataSurface->OSC(OSCNum).GroundTempCoef = state.dataIPShortCut->rNumericArgs(5);
        state.dataSurface->OSC(OSCNum).WindSpeedCoef = state.dataIPShortCut->rNumericArgs(6);
        state.dataSurface->OSC(OSCNum).ZoneAirTempCoef = state.dataIPShortCut->rNumericArgs(7);
        state.dataSurface->OSC(OSCNum).SinusoidPeriod = state.dataIPShortCut->rNumericArgs(8);

rraustad · 2023-11-05T17:17:57Z

Diffs for WindowTests:

Site:GroundReflectance:SnowModifier,
1.0;                     !- Ground Reflected Solar Modifier

There is no object for Site:GroundReflectance:Snow:Daylighting in this example file.

nrel-bot-2b · 2023-12-04T00:09:51Z

@rraustad @Myoldmopar it has been 28 days since this pull request was last updated.

mjwitte

And we may as well do this for alphas, too?

mjwitte · 2023-12-27T21:44:55Z

src/EnergyPlus/InputProcessing/InputProcessor.cc

-            } else {
-                Numbers(numeric_index) = 0;
-            }
+            findDefault(Numbers(numeric_index), schema_field_obj);


findDefault is a bool function. Should check here and set the value to 0 if it returns false.

No need to check, Numbers(numeric_index) is set to 0 at top of function and if default does not exist for that field will return false and a 0 for that field.

bool InputProcessor::findDefault(Real64 &default_value, json const &schema_field_obj) { auto const find_default = schema_field_obj.find("default"); default_value = 0; if (find_default != schema_field_obj.end()) { // get default for autosize, int or real } return false; }

I'm not always inclined to use the latest fancy C++, but I will just note that C++23 will have std::expected, and it has the potential to be deployed throughout our code some day in the future. We're years from anything like that, so no reason to discuss it, but feel free to look it up if you are an interested student.

rraustad · 2024-01-02T14:32:19Z

For alpha fields I don't think it's an issue, at least not the same issue we see with Real fields. It's either a required-field or it's not. And if it's not and is a key choice then there should be a default. If there is no default I think that's an issue with the idd not the alpha findDefault function. If it's not a key choice then a non-required field blank should return a blank string (and getInput needs to handle that case?). Worst case here is to add an else at the bottom to "set" default_value to a null string but I don't think that's necessary, because the string passed to this function should already be blank.

bool InputProcessor::findDefault(std::string &default_value, json const &schema_field_obj)
{
    auto const find_default = schema_field_obj.find("default");
    if (find_default != schema_field_obj.end()) {
        auto const &default_val = find_default.value();
        if (default_val.is_string()) {
            default_value = default_val.get<std::string>();
        } else {
            if (default_val.is_number_integer()) {
                i64toa(default_val.get<std::int64_t>(), s);
            } else {
                dtoa(default_val.get<double>(), s);
            }
            default_value = s;
        }
        if (schema_field_obj.find("retaincase") == schema_field_obj.end()) {
            default_value = Util::makeUPPER(default_value);
        }
        return true;
    }
    return false;
}

mjwitte · 2024-01-02T17:52:02Z

For alpha fields, it looks like the code won't fill in the default unless it's within_max_fields , see line 866

    if (field_type == "a") {
            if (!(within_max_fields && findDefault(Alphas(alpha_index), schema_field_obj))) {
                Alphas(alpha_index) = "";

rraustad · 2024-01-02T18:21:59Z

I see that. I wonder what to do with lines 876 and 882 to get NumAlphas and NumNumbers right? I think I'll just make the one suggestion now to see what happens.

…t-fill-numeric-defaults

rraustad · 2024-01-02T20:38:35Z

Given the changes to unit tests I'd say this is working as expected.

rraustad · 2024-01-02T20:41:12Z

tst/EnergyPlus/unit/InputProcessor.unit.cc

@@ -3416,7 +3421,7 @@ TEST_F(InputProcessorFixture, getObjectItem_fan_on_off)

    EXPECT_EQ(4, NumAlphas);
    EXPECT_TRUE(compare_containers(
-        std::vector<std::string>({"SUPPLY FAN 1", "FANANDCOILAVAILSCHED", "ZONE EXHAUST NODE", "DX COOLING COIL AIR INLET NODE", "", "", ""}),
+        std::vector<std::string>({"SUPPLY FAN 1", "FANANDCOILAVAILSCHED", "ZONE EXHAUST NODE", "DX COOLING COIL AIR INLET NODE", "", "", "General"}),


Retain case on this last alpha field:

Fan:OnOff, A7 ; \field End-Use Subcategory \note Any text may be used here to categorize the end-uses in the ABUPS End Uses by Subcategory table. \type alpha \retaincase \default General

mjwitte · 2024-01-03T15:06:02Z

These eio diffs are significant.

- Site:GroundReflectance:SnowModifier,   1.000,   0.000
+ Site:GroundReflectance:SnowModifier,   1.000,   1.000
 ! ,Jan{dimensionless},Feb{dimensionless},Mar{dimensionless},Apr{dimensionless},May{dimensionless},Jun{dimensionless},Jul{dimensionless},Aug{dimensionless},Sep{dimensionless},Oct{dimensionless},Nov{dimensionless},Dec{dimensionless}
  Site:GroundReflectance:Snow,  0.20,  0.20,  0.20,  0.20,  0.20,  0.20,  0.20,  0.20,  0.20,  0.20,  0.20,  0.20
 ! ,Jan{dimensionless},Feb{dimensionless},Mar{dimensionless},Apr{dimensionless},May{dimensionless},Jun{dimensionless},Jul{dimensionless},Aug{dimensionless},Sep{dimensionless},Oct{dimensionless},Nov{dimensionless},Dec{dimensionless}
- Site:GroundReflectance:Snow:Daylighting,  0.00,  0.00,  0.00,  0.00,  0.00,  0.00,  0.00,  0.00,  0.00,  0.00,  0.00,  0.00
+ Site:GroundReflectance:Snow:Daylighting,  0.20,  0.20,  0.20,  0.20,  0.20,  0.20,  0.20,  0.20,  0.20,  0.20,  0.20,  0.20

There are no results diffs on CI, because the weather file doesn't have any times with snow on the ground and the winter design day has no sun and no snow.

But if you change the winter design day to have snow and solar, then there's a difference in the daylighting illuminance.
Results for WindowTests-DDaysWithSunAndSnow, develop on the left, this branch on the right.

mjwitte · 2024-01-03T15:29:44Z

The dxf diffs also reveal a bug:

 DXF created from EnergyPlus
 999
 999
-Polygon Action,ThickPolyline
+Polygon Action,TRIANGULATE3DFACE

In this case, the drawing is correct, just the info line is wrong, because this line of code is wrong:

EnergyPlus/src/EnergyPlus/OutputReports.cc

Lines 532 to 533 in 6fafbb7

    
           if (PolygonAction.empty()) { 
        
               print(dxffile, Format_708, "Polygon Action", ",", "ThickPolyline");

The drawing action is correctly set to the default Triangulate3DFace, but the print statement is writing the wrong string. With the changes in this branch, PolygonAction will never be empty. So, this if can be removed, or at least change the offending line to say "Triangulate3DFace".

rraustad · 2024-01-03T16:03:45Z

Re: DXF diffs: maybe the default used to be ThickPolyline? I did wonder why there were DXF diffs, thanks for checking.

rraustad · 2024-01-03T16:24:43Z

The impact of this change will be on getInput syntax. json has a similar issue getting defaults for missing fields. For example:

ChillerAbsorption getInput:

    if (NumNums > 16) {
        thisChiller.GeneratorSubcool = state.dataIPShortCut->rNumericArgs(17);
    } else {
        thisChiller.GeneratorSubcool = 1.0;
    }

    if (NumNums > 17) {
        thisChiller.SizFac = state.dataIPShortCut->rNumericArgs(18);
    } else {
        thisChiller.SizFac = 1.0;
    }

should change to:

    thisChiller.GeneratorSubcool = state.dataIPShortCut->rNumericArgs(17);
    thisChiller.SizFac = state.dataIPShortCut->rNumericArgs(18);

because:

Chiller:Absorption,
  N17, \field Degree of Subcooling in Steam Generator
       \type real
       \units C
       \default 1.0
  N18; \field Sizing Factor
       \type real
       \minimum> 0.0
       \default 1.0

mjwitte · 2024-01-03T16:33:17Z

I wouldn't worry about removing code that sets hard-wired defaults like the absorption chiller. That can go away when getObjectItem goes away and it's all replaced with direct json input processing.

Myoldmopar · 2024-01-05T15:32:33Z

@rraustad @mjwitte this is a really exciting change. I am super happy to see the removal of default values duplicated in both the IDD and the get-input routines. From the comments above, it seems the EIO diffs are still an issue? Let me know if I can do anything to help spur this along, as I'd love to get this in.

mjwitte · 2024-01-05T17:31:08Z

@rraustad @mjwitte this is a really exciting change. I am super happy to see the removal of default values duplicated in both the IDD and the get-input routines. From the comments above, it seems the EIO diffs are still an issue? Let me know if I can do anything to help spur this along, as I'd love to get this in.

I posted a new issue regarding the eio diffs, #10360 . This PR fixes that issue, no further changes required here.

For the dxf diff, it's fixed here and has minimal impact, but it would be good to clean up the offending code per this comment.

Otherwise, this is good to go.

rraustad · 2024-01-05T17:54:14Z

I'm OK with the eio diffs, as expected. And @mjwitte has added to the description as necessary for unanticipated changes revealed by this branch. This looks ready to merge.

…numeric-defaults

mjwitte · 2024-01-26T22:05:27Z

@Myoldmopar Assuming CI comes back same as for d6cfeba#comments this is ready to merge. Do you want to review anymore?

Myoldmopar

All good, let's get this in.

Myoldmopar · 2024-02-08T15:10:24Z

src/EnergyPlus/InputProcessing/InputProcessor.cc

-            } else {
-                Numbers(numeric_index) = 0;
-            }
+            findDefault(Numbers(numeric_index), schema_field_obj);


I'm not always inclined to use the latest fancy C++, but I will just note that C++23 will have std::expected, and it has the potential to be deployed throughout our code some day in the future. We're years from anything like that, so no reason to discuss it, but feel free to look it up if you are an interested student.

Myoldmopar · 2024-02-08T15:11:59Z

One unrelated unit test failure on Mac, otherwise good to go. Thanks @rraustad and @mjwitte ! Merging this one.

Myoldmopar · 2024-02-08T15:12:27Z

Note (to self mostly) this could cause diffs in some of this morning's other merges. I'll run regressions locally as needed.

Testing fill numeric defaults past min-fields

a62b606

rraustad added the Defect Includes code to repair a defect in EnergyPlus label Nov 5, 2023

rraustad changed the title ~~Testing fill numeric defaults past min-fields~~ Fill numeric defaults for missing fields not entered by user Nov 5, 2023

Update unit tests

fab1713

Myoldmopar assigned mjwitte Dec 6, 2023

mjwitte reviewed Dec 27, 2023

View reviewed changes

rraustad added 2 commits January 2, 2024 15:37

Include Alpha field defaults

14f4cab

Merge branch 'develop' of https://github.com/NREL/EnergyPlus into Tes…

38de5b8

…t-fill-numeric-defaults

rraustad commented Jan 2, 2024

View reviewed changes

rraustad changed the title ~~Fill numeric defaults for missing fields not entered by user~~ Fill object defaults for missing fields not entered by user Jan 2, 2024

mjwitte mentioned this pull request Jan 5, 2024

Site:GroundReflectance:SnowModifier may use incorrect default for daylighting #10360

Closed

3 tasks

Merge remote-tracking branch 'remotes/origin/develop' into Test-fill-…

d6cfeba

…numeric-defaults

mjwitte changed the title ~~Fill object defaults for missing fields not entered by user~~ Fill object defaults for blank or missing fields and fix Site:GroundReflectance:SnowModifier defaults Jan 25, 2024

Fix default output for dxf polygon action

528c6c0

mjwitte assigned Myoldmopar and unassigned mjwitte Jan 29, 2024

rraustad added this to the EnergyPlus 24.1 milestone Jan 31, 2024

Myoldmopar approved these changes Feb 8, 2024

View reviewed changes

Myoldmopar merged commit ab1e876 into develop Feb 8, 2024
15 checks passed

Myoldmopar deleted the Test-fill-numeric-defaults branch February 8, 2024 15:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fill object defaults for blank or missing fields and fix Site:GroundReflectance:SnowModifier defaults #10295

Fill object defaults for blank or missing fields and fix Site:GroundReflectance:SnowModifier defaults #10295

rraustad commented Nov 5, 2023 •

edited by mjwitte

Loading

rraustad commented Nov 5, 2023 •

edited

Loading

rraustad commented Nov 5, 2023 •

edited

Loading

rraustad commented Nov 5, 2023 •

edited

Loading

rraustad commented Nov 5, 2023

nrel-bot-2b commented Dec 4, 2023

mjwitte left a comment

mjwitte Dec 27, 2023

rraustad Jan 2, 2024

Myoldmopar Feb 8, 2024

rraustad commented Jan 2, 2024

mjwitte commented Jan 2, 2024

rraustad commented Jan 2, 2024

rraustad commented Jan 2, 2024

rraustad Jan 2, 2024

mjwitte commented Jan 3, 2024

mjwitte commented Jan 3, 2024

rraustad commented Jan 3, 2024 •

edited

Loading

rraustad commented Jan 3, 2024

mjwitte commented Jan 3, 2024

Myoldmopar commented Jan 5, 2024

mjwitte commented Jan 5, 2024

rraustad commented Jan 5, 2024

mjwitte commented Jan 26, 2024 •

edited

Loading

Myoldmopar left a comment

Myoldmopar Feb 8, 2024

Myoldmopar commented Feb 8, 2024

Myoldmopar commented Feb 8, 2024

Fill object defaults for blank or missing fields and fix Site:GroundReflectance:SnowModifier defaults #10295

Fill object defaults for blank or missing fields and fix Site:GroundReflectance:SnowModifier defaults #10295

Conversation

rraustad commented Nov 5, 2023 • edited by mjwitte Loading

Pull request overview

Pull Request Author

Reviewer

rraustad commented Nov 5, 2023 • edited Loading

rraustad commented Nov 5, 2023 • edited Loading

rraustad commented Nov 5, 2023 • edited Loading

rraustad commented Nov 5, 2023

nrel-bot-2b commented Dec 4, 2023

mjwitte left a comment

Choose a reason for hiding this comment

mjwitte Dec 27, 2023

Choose a reason for hiding this comment

rraustad Jan 2, 2024

Choose a reason for hiding this comment

Myoldmopar Feb 8, 2024

Choose a reason for hiding this comment

rraustad commented Jan 2, 2024

mjwitte commented Jan 2, 2024

rraustad commented Jan 2, 2024

rraustad commented Jan 2, 2024

rraustad Jan 2, 2024

Choose a reason for hiding this comment

mjwitte commented Jan 3, 2024

mjwitte commented Jan 3, 2024

rraustad commented Jan 3, 2024 • edited Loading

rraustad commented Jan 3, 2024

mjwitte commented Jan 3, 2024

Myoldmopar commented Jan 5, 2024

mjwitte commented Jan 5, 2024

rraustad commented Jan 5, 2024

mjwitte commented Jan 26, 2024 • edited Loading

Myoldmopar left a comment

Choose a reason for hiding this comment

Myoldmopar Feb 8, 2024

Choose a reason for hiding this comment

Myoldmopar commented Feb 8, 2024

Myoldmopar commented Feb 8, 2024

rraustad commented Nov 5, 2023 •

edited by mjwitte

Loading

rraustad commented Nov 5, 2023 •

edited

Loading

rraustad commented Nov 5, 2023 •

edited

Loading

rraustad commented Nov 5, 2023 •

edited

Loading

rraustad commented Jan 3, 2024 •

edited

Loading

mjwitte commented Jan 26, 2024 •

edited

Loading