Round2 init state use #10662

rraustad · 2024-08-15T19:30:46Z

Pull request overview

Moves getInputs for schedules and curves to init_state
Reverted the previous method of calling init_state from process_idf because some of the unit tests read the idf snippet and act on that without calling the corresponding getInput. I figured it was not a good idea to automatically call getInputs when a developer might want to do something different. This should make it easier to move things to init_state because it won't affect existing unit tests.
Reverted a lot of the unit test changes from the previous init-state effort.
This may also mean I have to go back and add back in all the if (state.dataFluidProperties->GetInputFlag) call getFluidProperitesData occurrences that I removed on the previous effort. I didn't do that here but it may be necessary for unit testing, not sure yet.
Add a new warning counter for warning or severe errors encountered during input processing. Also include this count in ResultsFramework.
Report Curve objects that are not accessed for simulation purposes. If any curve is used in the simulation all curves are marked as used. This was improved to actually report curves that are not used.

NOTE: ENHANCEMENTS MUST FOLLOW A SUBMISSION PROCESS INCLUDING A FEATURE PROPOSAL AND DESIGN DOCUMENT PRIOR TO SUBMITTING CODE

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.

…nd2_init_state_use

rraustad · 2024-08-15T19:32:00Z

src/EnergyPlus/CurveManager.cc

@@ -653,6 +652,7 @@ namespace Curve {
        // initialize the array

        int CurveNum = 0; // keep track of the current curve index in the main curve array
+        state.dataCurveManager->UniqueCurveNames.clear();


The only change needed to allow curve manager getInput to be called more than once. It will likely not be called more than once but if it does it will give the same answer.

Probably better to set a flag and make sure the body is not called twice.

Well, the outer getInput call does protect from the second call. If a unit test calls the root function multiple times it would need to give the same answer. I think I did this before removing init_state from the unit test fixture (last thing I did) so maybe I can fix this and not have to change any unit tests. I'll just move that flag down into GetCurveInputData.

if (state.dataCurveManager->GetCurvesInputFlag) { GetCurveInput(state); GetPressureSystemInput(state); state.dataCurveManager->GetCurvesInputFlag = false; } void GetCurveInput(EnergyPlusData &state) { // wrapper for GetInput to allow unit testing when fatal inputs are detected - follow pattern from GetSetPointManagerInputs() bool GetInputErrorsFound = false; GetCurveInputData(state, GetInputErrorsFound); state.dataCurveManager->GetCurvesInputFlag = false; if (GetInputErrorsFound) { ShowFatalError(state, "GetCurveInput: Errors found in getting Curve Objects. Preceding condition(s) cause termination."); } } void GetCurveInputData(EnergyPlusData &state, bool &ErrorsFound)

There is a general practice in EnergyPlus of "protecting functions from bad inputs or bad calling contexts". This is actually counter-productive because it hides bad uses of the function. It's better programming to assert good inputs and good context (to the extent possible) within a function and then use those to progressively clean up the calling contexts.

rraustad · 2024-08-15T19:32:34Z

src/EnergyPlus/Data/EnergyPlusData.cc

    this->dataFluidProps->init_state(state);        // GetFluidPropertiesData
    this->dataPsychrometrics->init_state(state);    // InitializePsychRoutines
+    this->dataScheduleMgr->init_state(state);       // ProcessScheduleInput - requires NumOfTimeStepInHour and AnyEnergyManagementSystemInModel
+    this->dataCurveManager->init_state(state);      // GetCurveInput, GetPressureSystemInput


Added schedules and curves to init_state

rraustad · 2024-08-15T19:42:55Z

tst/EnergyPlus/unit/Furnaces.unit.cc

@@ -87,7 +87,7 @@ namespace EnergyPlus {
 TEST_F(EnergyPlusFixture, SetVSHPAirFlowTest_VSFurnaceFlowTest)
 {

-    state->init_state(*state);
+    state->init_state(*state); // get FluidProperties


hmm, why did I have to leave this here when FluidProperties is called in the fixture? Maybe this unit test needed something else now in init_state?

Oh, that's right. If the unit test does not have an idf snippet then process_idf will not be called and default fluids will not be set up yet. So this is correct.

rraustad · 2024-08-15T19:44:50Z

tst/EnergyPlus/unit/MixedAir.unit.cc

@@ -6563,10 +6563,10 @@ TEST_F(EnergyPlusFixture, CO2ControlDesignOARateTest)
        "   **   ~~~   ** This may be overriding desired ventilation controls. Check inputs for Minimum Outdoor Air Flow Rate, Minimum Outdoor Air "
        "Schedule Name and Controller:MechanicalVentilation",
        "   **   ~~~   ** Minimum OA fraction = 2.9412E-003, Mech Vent OA fraction = 1.5603E-003",
-        "   **   ~~~   **  Environment=, at Simulation time= 00:00 - 00:00",
+        "   **   ~~~   **  Environment=, at Simulation time= 00:15 - 00:15",


Now that Timestep is processed in process_idf the time stamp is the first time step of the simulation. Before, NumTimeStepInHour = 0, now it is non-zero and Timestep defaults to 4.

rraustad · 2024-08-15T19:45:18Z

tst/EnergyPlus/unit/OutputProcessor.unit.cc

@@ -2990,8 +2990,7 @@ namespace OutputProcessor {
        auto reportExtendedDataResults = queryResult("SELECT * FROM ReportExtendedData;", "ReportExtendedData");

        compare_eso_stream(
-            delimited_string({"Program Version,",
-                              "1,11,Boiler1,Boiler NaturalGas Rate [W] !RunPeriod [Value,Min,Month,Day,Hour,Minute,Max,Month,Day,Hour,Minute]",
+            delimited_string({"1,11,Boiler1,Boiler NaturalGas Rate [W] !RunPeriod [Value,Min,Month,Day,Hour,Minute,Max,Month,Day,Hour,Minute]",


These are all reversions of last effort changes.

rraustad · 2024-08-15T19:46:48Z

tst/EnergyPlus/unit/SimulationManager.unit.cc

@@ -112,6 +112,8 @@ TEST_F(EnergyPlusFixture, Test_PerformancePrecisionTradeoffs_DirectSolution_Mess

    EXPECT_TRUE(process_idf(idf_objects, false));

+    SimulationManager::GetProjectData(*state);
+


Another revert, unit tests are now responsible for setting up the inputs. Either this way or calling init_state directly.

rraustad · 2024-08-15T19:50:07Z

tst/EnergyPlus/unit/SimulationManager.unit.cc

+            "   ** Warning ** Output:Diagnostics: More than 1 occurrence of this object found, only first will be used.",
+        });
+        EXPECT_TRUE(compare_err_stream(expectedError, true));
+    }


Adding back in what was here before last effort.

rraustad · 2024-08-15T19:51:52Z

tst/EnergyPlus/unit/WeatherManager.unit.cc

@@ -688,7 +688,6 @@ TEST_F(EnergyPlusFixture, ASHRAE_Tau2017ModelTest)

    bool ErrorsFound(false);
    state->dataEnvrn->TotDesDays = 2;
-    state->dataGlobal->NumOfTimeStepInHour = 0;


NumOfTimeStepInHour is now 0 again because GetProjectData has not been called (i.e.,, I removed init_state from process_idf on this pass).

rraustad · 2024-08-15T21:11:42Z

Unanticipated diff. The schedules are now read very early, so early that these 3 warnings are no longer included in the summary report.

-   ************* Beginning Zone Sizing Calculations
    ** Warning ** ProcessScheduleInput: Schedule:Constant="VAV_BOT WITH REHEAT MAX OA FRACTION", Blank Schedule Type Limits Name input -- will not be validated.
    ** Warning ** ProcessScheduleInput: Schedule:Constant="VAV_MID WITH REHEAT MAX OA FRACTION", Blank Schedule Type Limits Name input -- will not be validated.
    ** Warning ** ProcessScheduleInput: Schedule:Constant="VAV_TOP WITH REHEAT MAX OA FRACTION", Blank Schedule Type Limits Name input -- will not be validated.
+   ************* Beginning Zone Sizing Calculations
    ** Warning ** CheckUsedConstructions: There are 19 nominally unused constructions in input.
    **   ~~~   ** For explicit details on each unused construction, use Output:Diagnostics,DisplayExtraWarnings;
    ** Warning ** GetDaylightingControls: Fraction of zone or space controlled by the Daylighting reference points is < 1.0.

    ************* EnergyPlus Warmup Error Summary. During Warmup: 0 Warning; 0 Severe Errors.
-   ************* EnergyPlus Sizing Error Summary. During Sizing: 22 Warning; 0 Severe Errors.
+   ************* EnergyPlus Sizing Error Summary. During Sizing: 19 Warning; 0 Severe Errors.

rraustad · 2024-08-16T02:27:19Z

Here's a thought at how to fix this.

************* EnergyPlus Inputs Error Summary. During Inputs: 3 Warning; 0 Severe Errors.
************* EnergyPlus Warmup Error Summary. During Warmup: 0 Warning; 0 Severe Errors.
************* EnergyPlus Sizing Error Summary. During Sizing: 19 Warning; 0 Severe Errors.

…nd2_init_state_use

rraustad · 2024-08-16T03:10:00Z

I decided to just restore the error summary information since only a fraction of getInputs have been moved to init_state.

rraustad · 2024-08-16T03:34:49Z

src/EnergyPlus/CurveManager.cc

@@ -610,6 +609,8 @@ namespace Curve {
        int IOStatus;                    // Used in GetObjectItem
        std::string CurrentModuleObject; // for ease in renaming.

+        if (!state.dataCurveManager->GetCurvesInputFlag) return;
+        state.dataCurveManager->GetCurvesInputFlag = false;


😆 🤣 😄 This is excellent @rraustad !

…nd2_init_state_use

rraustad · 2024-08-16T18:32:34Z

This diff in IndoorLivingWall is disturbing. There is in fact 1 curve in this input file that is unused. But "unused" here means it was never read in because there are no other objects in this example file that have a curve object name input and therefore the curves are never read in. In this branch, moving GetCurveInput to init_state means the curves are read in and there is no unused object message in the error file. More disturbing is that if any curve name was an object input then all curves would be read in, marked as used, and even if a dangling curve object was in the input file it would not show as an unused input object. This branch shows that a different method for determining an unused object is necessary. Debugging this I noticed that if an object is "read" using getObjectItem it is marked as used, when in fact my interpretation of "used" is if it is actually accessed by another object. If all object's getInput are moved to init_state then all objects will be marked as "used". Moving markObjectAsUsed from getObjectItem to getObjectIndex (e.g., GetFanIndex) might be one way to go (probably some effort needed to make sure all parent objects call for an index of child objects).

-   ************* There are 1 unused objects in input.
-   ************* Use Output:Diagnostics,DisplayUnusedObjects; to see them.

ScheduleManager uses the "actually accessed" method in GetScheduleIndex and accurately reports unused schedules:

int GetScheduleIndex(EnergyPlusData &state, std::string const &ScheduleName)

    if (state.dataScheduleMgr->NumSchedules > 0) {
        GetScheduleIndex = Util::FindItemInList(ScheduleName, state.dataScheduleMgr->Schedule({1, state.dataScheduleMgr->NumSchedules}));
        if (GetScheduleIndex > 0) {
            if (!state.dataScheduleMgr->Schedule(GetScheduleIndex).Used) {
                state.dataScheduleMgr->Schedule(GetScheduleIndex).Used = true; <------- marks it as actually used

and has it's own ReportOrphanedSchedule function:

void ReportOrphanSchedules(EnergyPlusData &state)
{

for (Item = 1; Item <= state.dataScheduleMgr->NumSchedules; ++Item) {
    if (state.dataScheduleMgr->Schedule(Item).Used) continue;
    if (NeedOrphanMessage && state.dataGlobal->DisplayUnusedSchedules) {
        ShowWarningError(state, "The following schedule names are \"Unused Schedules\".  These schedules are in the idf");
        ShowContinueError(state, " file but are never obtained by the simulation and therefore are NOT used.");

rraustad · 2024-08-19T13:19:18Z

MultiSpeedHeatPump_DirectSolution shows unused curve objects and that the PerformancePrecisionTradeoffs was processed during GetProjectData.

    ** Warning ** PerformancePrecisionTradeoffs: Coil Direct Solution simulation is selected.
+   ** Warning ** The following lines are "Unused Objects".  These objects are in the input
+   **   ~~~   **  file but are never obtained by the simulation and therefore are NOT used.
+   **   ~~~   **  Each unused object is shown.
+   **   ~~~   **  See InputOutputReference document for more details.
+   ************* Object=Curve:Quadratic=HPACCOOLPLFFPLR SPEED 2
+   **   ~~~   ** Object=Curve:Quadratic=HPACCOOLPLFFPLR SPEED 3
+   **   ~~~   ** Object=Curve:Quadratic=HPACCOOLPLFFPLR SPEED 4
+   ************* EnergyPlus Inputs Error Summary. During Inputs: 1 Warning; 0 Severe Errors.
    ************* EnergyPlus Sizing Error Summary. During Sizing: 0 Warning; 0 Severe Errors.
    ************* EnergyPlus Completed Successfully-- 5 Warning; 0 Severe Errors; Elapsed Time=00hr 00min  0.63sec

rraustad · 2024-08-19T19:12:59Z

src/EnergyPlus/CurveManager.cc

@@ -675,6 +675,7 @@ namespace Curve {
                                                     CurrentModuleObject,
                                                     state.dataIPShortCut->cAlphaFieldNames(1),
                                                     ErrorsFound);
+            state.dataInputProcessing->inputProcessor->unusedInputs.emplace(CurrentModuleObject, Alphas(1));


getObjectItem removes an object from the unusedInputs list just because it was read in. This change puts that curve back in the list and waits for a call to getCurveIndex before removing the curve from the unusedInputs list. This change was prompted by IndoorLivingWall not showing the unused curve warning in this branch. Now curves shown as unused are actually not used in the simulation (i.e., if I have 10 curves in the input and only use 1 then 9 curves are unused).

We should probably change GetObjectItem to not do this at some point.

rraustad · 2024-08-19T19:14:00Z

src/EnergyPlus/UtilityRoutines.cc

    state.dataResultsFramework->resultsFramework->SimulationInformation.setNumErrorsWarmup(NumWarningsDuringWarmup, NumSevereDuringWarmup);
    state.dataResultsFramework->resultsFramework->SimulationInformation.setNumErrorsSizing(NumWarningsDuringSizing, NumSevereDuringSizing);
    state.dataResultsFramework->resultsFramework->SimulationInformation.setNumErrorsSummary(NumWarnings, NumSevere);

+    ShowMessage(
+        state,
+        format("EnergyPlus Inputs Error Summary. During Inputs: {} Warning; {} Severe Errors.", NumWarningsDuringInputs, NumSevereDuringInputs));


I could not get the number of warning/severe errors to match what was reported previously so went ahead and added a summary for errors during getInput.

rraustad · 2024-08-19T19:49:47Z

src/EnergyPlus/InputProcessing/InputProcessor.cc

@@ -340,7 +340,8 @@ bool InputProcessor::checkVersionMatch(EnergyPlusData &state)
                    Which = static_cast<int>(index(v, MatchVersion));
                }
                if (Which != 0) {
-                    ShowWarningError(state, "Version: in IDF=\"" + v + "\" not the same as expected=\"" + MatchVersion + "\"");
+                    // this is reported in GetProjectData
+                    // ShowWarningError(state, "Version: in IDF=\"" + v + "\" not the same as expected=\"" + MatchVersion + "\"");


I am seeing duplicate messages so either the version issue is reported here or where the version object is read. I chose to comment this out here but maybe there is a reason to do the opposite (e.g., API calls).

** Warning ** Version: in IDF="24.1" not the same as expected="24.2" ** Warning ** Version: in IDF="24.1" not the same as expected="24.2"

amirroth · 2024-08-30T21:02:02Z

src/EnergyPlus/CurveManager.cc

@@ -610,6 +609,8 @@ namespace Curve {
        int IOStatus;                    // Used in GetObjectItem
        std::string CurrentModuleObject; // for ease in renaming.

+        if (!state.dataCurveManager->GetCurvesInputFlag) return;
+        state.dataCurveManager->GetCurvesInputFlag = false;


amirroth · 2024-08-30T21:05:59Z

src/EnergyPlus/CurveManager.cc

@@ -653,6 +652,7 @@ namespace Curve {
        // initialize the array

        int CurveNum = 0; // keep track of the current curve index in the main curve array
+        state.dataCurveManager->UniqueCurveNames.clear();


There is a general practice in EnergyPlus of "protecting functions from bad inputs or bad calling contexts". This is actually counter-productive because it hides bad uses of the function. It's better programming to assert good inputs and good context (to the extent possible) within a function and then use those to progressively clean up the calling contexts.

amirroth · 2024-08-30T21:08:02Z

src/EnergyPlus/CurveManager.cc

@@ -675,6 +675,7 @@ namespace Curve {
                                                     CurrentModuleObject,
                                                     state.dataIPShortCut->cAlphaFieldNames(1),
                                                     ErrorsFound);
+            state.dataInputProcessing->inputProcessor->unusedInputs.emplace(CurrentModuleObject, Alphas(1));


We should probably change GetObjectItem to not do this at some point.

amirroth · 2024-08-30T21:23:28Z

src/EnergyPlus/CurveManager.cc

@@ -824,6 +826,7 @@ namespace Curve {
                                                                     _,
                                                                     state.dataIPShortCut->cAlphaFieldNames,
                                                                     state.dataIPShortCut->cNumericFieldNames);
+            state.dataInputProcessing->inputProcessor->unusedInputs.emplace(CurrentModuleObject, Alphas(1));
            ++CurveNum;
            GlobalNames::VerifyUniqueInterObjectName(state,


This doesn't have to be part of this PR, but look at how I am using std::map to implement both VerifyUniqueObjectName and GetObjectIndex in one fast structure. There are examples in Fans, OutputProcessor and SetPointManager.

amirroth · 2024-08-30T21:26:50Z

src/EnergyPlus/CurveManager.hh

@@ -207,6 +210,17 @@ namespace Curve {
                                       const Real64 Var4, // 4th independent variable
                                       const Real64 Var5, // 5th independent variable
                                       const Real64 Var6);
+
+        void markUsed(EnergyPlusData &state)


I don't like this. Leave it for now but I'm going to look at how InputProcessor is doing this.

amirroth · 2024-08-30T21:30:41Z

src/EnergyPlus/UtilityRoutines.cc

@@ -548,6 +550,8 @@ int AbortEnergyPlus(EnergyPlusData &state)
    CloseMiscOpenFiles(state);
    NumWarnings = fmt::to_string(state.dataErrTracking->TotalWarningErrors);
    NumSevere = fmt::to_string(state.dataErrTracking->TotalSevereErrors);
+    NumWarningsDuringInputs = fmt::to_string(state.dataErrTracking->TotalWarningErrorsDuringInputs);


Tracking numerical values using strings is just nutty. Why are we doing this? For results framework? There has to be a better way.

Myoldmopar · 2024-09-11T17:54:32Z

Given the timing, I am going to suggest we just push this until after release. No reason to rush this in.

rraustad · 2024-09-11T17:56:16Z

I agree.

nrel-bot-2c · 2024-10-10T00:19:41Z

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

nrel-bot-2c · 2024-11-07T00:19:12Z

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

nrel-bot-2 · 2024-11-22T00:16:55Z

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

rraustad added 3 commits July 11, 2024 21:10

Move SetPreConstructionInputParameters setup function to init_state

f6e0e8a

Add schedules and curve getInputs to init_state

758b5e1

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

4dbd4c4

…nd2_init_state_use

rraustad added the DoNotPublish Includes changes that shouldn't be reported in the changelog label Aug 15, 2024

rraustad commented Aug 15, 2024

View reviewed changes

rraustad added 2 commits August 15, 2024 23:08

Repair error summary report and protect GetCurveInputData

5aefd65

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

167a8bb

…nd2_init_state_use

rraustad commented Aug 16, 2024

View reviewed changes

rraustad added 3 commits August 16, 2024 00:56

Revise and update another function

339fe3b

Revise to get warning summary to line up and correct unit tests FPE

f7ba56c

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

5774851

…nd2_init_state_use

Curves used, input warnings, ResultsFramework

85a2ef2

rraustad added 3 commits August 19, 2024 09:53

Fix unit tests

de455d7

Another unit test repair

0dafbab

Table objects don't set curveType

730845a

rraustad commented Aug 19, 2024

View reviewed changes

amirroth reviewed Aug 30, 2024

View reviewed changes

Myoldmopar added this to the EnergyPlus 25.1 milestone Sep 11, 2024

		@@ -112,6 +112,8 @@ TEST_F(EnergyPlusFixture, Test_PerformancePrecisionTradeoffs_DirectSolution_Mess

		EXPECT_TRUE(process_idf(idf_objects, false));

		SimulationManager::GetProjectData(*state);

Round2 init state use #10662

Are you sure you want to change the base?

Round2 init state use #10662

Conversation

rraustad commented Aug 15, 2024 • edited Loading

Pull request overview

Pull Request Author

Reviewer

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

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

rraustad commented Aug 15, 2024 • edited Loading

rraustad commented Aug 16, 2024

rraustad commented Aug 16, 2024

rraustad Aug 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rraustad commented Aug 16, 2024 • edited Loading

rraustad commented Aug 19, 2024 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Myoldmopar commented Sep 11, 2024

rraustad commented Sep 11, 2024

nrel-bot-2c commented Oct 10, 2024

nrel-bot-2c commented Nov 7, 2024

nrel-bot-2 commented Nov 22, 2024

rraustad commented Aug 15, 2024 •

edited

Loading

rraustad commented Aug 15, 2024 •

edited

Loading

rraustad Aug 16, 2024 •

edited

Loading

rraustad commented Aug 16, 2024 •

edited

Loading

rraustad commented Aug 19, 2024 •

edited

Loading