Catch EMS variables initialized after reference#11409
Conversation
…ter reference does not throw error.
|
As I understand it, Windows builds don't pick up uninitialized variables, which we treat as errors. /home/mitchute/Projects/EnergyPlus/tst/EnergyPlus/unit/EMSManager.unit.cc:1065:9: error: unused variable ‘wallSurfNum’ [-Werror=unused-variable]
1065 | int wallSurfNum = Util::FindItemInList("WALL", state->dataSurface->Surface);
| ^~~~~~~~~~~ |
|
This PR is about the EMS variable |
| " Set power_mult = site_temp_adj, !- Program Line 1", | ||
| " Set site_temp_adj = 0.1, !- Program Line 2", |
There was a problem hiding this comment.
Variable site_temp_adj is being initialized after it is referenced. We should expect an error, right?
| int wallSurfNum = Util::FindItemInList("WALL", state->dataSurface->Surface); | ||
| bool anyRan; | ||
| EMSManager::ManageEMS(*state, EMSManager::EMSCallFrom::BeginTimestepBeforePredictor, anyRan, ObjexxFCL::Optional_int_const()); | ||
| EXPECT_EQ(state->dataSurface->Surface(wallSurfNum).ViewFactorGround, 0.1); |
There was a problem hiding this comment.
I intentionally had this line commented out so that we could get to EXPECT_TRUE(seriousErrorFound); down below.
There was a problem hiding this comment.
Sure, feel free to do what you need. I won't interfere anymore until you need someone to look. I was just pointing out that the CI linux builds were failing due to to the unused variable, that we treat as an error.
There was a problem hiding this comment.
No problem, understood.
… just ManageSimulation.
|
Pretty sure I have this narrowed down. I think |
|
|
||
| internalVarNum = RuntimeLanguageProcessor::FindEMSVariable(*state, "site_temp_adj", 1); | ||
| ASSERT_GT(internalVarNum, 0); | ||
| EXPECT_TRUE(state->dataRuntimeLang->ErlVariable(internalVarNum).Value.initialized); |
There was a problem hiding this comment.
This is the main difference with the first test; why is this suddenly initialized?
It's not in I'm not totally sure what the fix is here. Perhaps |
testfiles/_ResidentialBase.idf
Outdated
| Output:EnergyManagementSystem, | ||
| Verbose, !- Actuator Availability Dictionary Reporting | ||
| Verbose, !- Internal Variable Availability Dictionary Reporting | ||
| Verbose; !- EMS Runtime Language Debug Output Level |
There was a problem hiding this comment.
This test file has a program (BeginZoneTimestepAfterInitHeatBalance) that uses variables which are initialized in another program/subroutine (EndOfSystemTimestepAfterHVACReporting). I.e., initialized after reference.
| Run CalcBouyancyTerms, !- <none> | ||
| SEt Numerator = WindMCT + NatBouyMCT + SumHATroof + QdotCond + SumReliefMCT, !- <none> | ||
| Set Numerator = WindMCT + NatBouyMCT + SumHATroof + QdotCond + SumReliefMCT, !- <none> | ||
| Set Denominator = WindMC + NatBouyMC + SumHAroof + SumIntakeMdotCp, !- <none> | ||
| Set Troof = Numerator / Denominator, !- <none> | ||
| IF Ta < 2.0, !- <none> | ||
| Set Troof = Ta, !- <none> | ||
| ENDIF, !- <none> | ||
| IF Troof < Twb, !- <none> | ||
| Set Twb = Troof - 0.2, !- <none> | ||
| ENDIF; !- <none> |
There was a problem hiding this comment.
Something weird going on in this program. Here Troof is a function of, e.g., NatBouyMCT. Subroutine CalcBouyancyTerms needs Troof, which is used to calculate NatBouyMCT... am I missing something here?
There was a problem hiding this comment.
Agreed. CalcBouyancyTerms should probably be moved above Numerator
| if ((!state.dataGlobal->DoingSizing && !state.dataGlobal->KickOffSimulation && !state.dataEMSMgr->FinishProcessingUserInput) || | ||
| (!(thisErlVar.SetByGlobalVariable || thisErlVar.SetByInternalVariable))) { |
There was a problem hiding this comment.
The basic idea here is to do what we were doing before, unless the uninitialized variable is either a global/internal variable. If it isn't global/internal, set seriousErrorFound = true. If it is, let it slide.
There was a problem hiding this comment.
Seems like a reasonable approach.
|
|
|
|
testfiles/_ResidentialBase.idf
Outdated
| EnergyManagementSystem:ProgramCallingManager, | ||
| central_ac_and_furnace_airloop_0_duct_program calling manager, !- Name | ||
| EndOfSystemTimestepAfterHVACReporting, !- EnergyPlus Model Calling Point | ||
| BeginZoneTimestepBeforeInitHeatBalance, !- EnergyPlus Model Calling Point |
There was a problem hiding this comment.
Without this IDF file change, and with the fix in this PR, you'd see ATTIC_UNVENTED_1_DUCT_LEAKAGE_IMBALANCE_INFIL_PROGRAM,Line 3,SET QDUCTS = QDUCTS + CENTRAL_AC_AND_FURNACE_AIRLOOP_0_DUCTIMBALLKEXHFANEQUIVDZ, *** Error: EvaluateExpression: Variable = 'QDUCTS' used in expression has not been initialized! *** , During Warmup, Occurrence info=DENVER CENTENNIAL GOLDEN N ANN HTG 99% CONDNS DB, 12/21 00:00 - 01:00 in the EDD file and the simulation fatals.
EnergyManagementSystem:Program,
attic_unvented_1_duct_leakage_imbalance_infil_program, !- Name
Set Qducts = 0, !- Program Line 1
Set Qducts = Qducts - central_ac_and_furnace_airloop_0_DuctImbalLkSupFanEquivDZ, !- Program Line 2
Set Qducts = Qducts + central_ac_and_furnace_airloop_0_DuctImbalLkExhFanEquivDZ, !- <none>
Set attic_unvented_1_duct_leakage_imbalance_infil_flow_act = (@Abs(Qducts)); !- <none>
Qducts is initialized on line 1. On line 2 the throw is skipped; central_ac_and_furnace_airloop_0_DuctImbalLkSupFanEquivDZ is uninitialized but doesn't fatal since central_ac_and_furnace_airloop_0_DuctImbalLkSupFanEquivDZ is a global variable. Then on line 3 Qducts is uninitialized (because of the preceding program line), and fatals since Qducts is not a global variable.
So basically the change in the above PCM from EndOfSystemTimestepAfterHVACReporting to BeginZoneTimestepBeforeInitHeatBalance causes both central_ac_and_furnace_airloop_0_DuctImbalLkSupFanEquivDZ and central_ac_and_furnace_airloop_0_DuctImbalLkExhFanEquivDZ to initialize so that program line 2 and 3 above don't leave Qducts uninitialized.
mitchute
left a comment
There was a problem hiding this comment.
Looks good. I'll let you decide on what to do regarding this: https://github.com/NatLabRockies/EnergyPlus/pull/11409/changes#r2790815597
…CalcBouyancyTerms.
With this change, we can have Edit: Actually this change leads to the following in the ERR file: I'll just revert to the original program, and initialize |
|
|
…tialize Troof along with the other global vars.
|
|
testfiles/_ResidentialBase.idf
Outdated
| EnergyManagementSystem:ProgramCallingManager, | ||
| central_ac_and_furnace_airloop_0_duct_program calling manager, !- Name | ||
| EndOfSystemTimestepAfterHVACReporting, !- EnergyPlus Model Calling Point | ||
| BeginZoneTimestepBeforeInitHeatBalance, !- EnergyPlus Model Calling Point |
There was a problem hiding this comment.
Perhaps the changes could instead be:
central_ac_and_furnace_airloop_0_duct_program calling manager: EndOfSystemTimestepAfterHVACReporting -> EndOfSystemTimestepBeforeHVACReportingattic_unvented_1_duct_leakage_imbalance_infil_program calling manager: BeginZoneTimestepAfterInitHeatBalance -> EndOfSystemTimestepAfterHVACReportinginfil_program calling manager: BeginZoneTimestepAfterInitHeatBalance -> EndOfSystemTimestepAfterHVACReporting
There was a problem hiding this comment.
After some local testing/diffing, it appears this approach causes a slightly larger regression than EndOfSystemTimestepAfterHVACReporting -> BeginZoneTimestepBeforeInitHeatBalance for central_ac_and_furnace_airloop_0_duct_program calling manager.
So I suppose the question is whether we want to avoid regression or whether we think there's actually an issue in how the programs are set up.
There was a problem hiding this comment.
We can handle regressions if they are justified.
There was a problem hiding this comment.
Sorry, I've been out of the office. I will take a look at this soon.
…tion marks for some error statements.
…rands global to avoid regressions.
| EnergyManagementSystem:GlobalVariable, | ||
| Qducts, !- Erl Variable 1 Name | ||
| Qsupply, !- Erl Variable 2 Name | ||
| Qexhaust; !- Erl Variable 3 Name |
There was a problem hiding this comment.
So we can avoid EvaluateExpression errors during setup.
| EnergyManagementSystem:GlobalVariable, | ||
| A, !- Erl Variable 1 Name | ||
| B, !- Erl Variable 2 Name | ||
| C, !- Erl Variable 3 Name | ||
| D, !- <none> | ||
| E, !- <none> | ||
| F, !- <none> | ||
| G, !- <none> | ||
| H, !- <none> | ||
| I, !- <none> | ||
| J, !- <none> | ||
| K, !- <none> | ||
| L, !- <none> | ||
| M, !- <none> | ||
| N, !- <none> | ||
| O, !- <none> | ||
| P, !- <none> | ||
| Q, !- <none> | ||
| R, !- <none> | ||
| S, !- <none> | ||
| T, !- <none> | ||
| U, !- <none> | ||
| V, !- <none> | ||
| W, !- <none> | ||
| X, !- <none> | ||
| Y, !- <none> | ||
| Z, !- <none> | ||
| AA, !- <none> | ||
| BB, !- <none> | ||
| CC, !- <none> | ||
| DD, !- <none> | ||
| EE, !- <none> | ||
| FF, !- <none> | ||
| GG, !- <none> | ||
| HH, !- <none> | ||
| II; !- <none> |
There was a problem hiding this comment.
So we can avoid EvaluateExpression errors during setup.
| DeltaT, !- Erl Variable 2 Name | ||
| ratioT, !- Erl Variable 3 Name |
There was a problem hiding this comment.
So we can avoid EvaluateExpression errors during setup.
|
|
|
…/EnergyPlus into fix-ems-var-init-after-ref
|
This looks like a good set of cleanups. We'll merge this next, assuming tests all are consistent with previous results. |
|
|
|
All good here. Merging. |

Pull request overview
Description of the purpose of this PR
Pull Request Author
Reviewer