-
Notifications
You must be signed in to change notification settings - Fork 389
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix #10279 - Make sure that assigning the result of a TendVariable (eg @TrendValue) results in proper actuator behavior #10575
Conversation
…nnual simulation ``` /home/julien/Software/Others/EnergyPlus/tst/EnergyPlus/unit/EMSManager.unit.cc:2463: Failure Value of: actuatorDirectErlVar.Value.TrendVariable Actual: true Expected: false /home/julien/Software/Others/EnergyPlus/tst/EnergyPlus/unit/EMSManager.unit.cc:2464: Failure Expected equality of these values: 0 actuatorDirectErlVar.Value.TrendVarPointer Which is: 1 /home/julien/Software/Others/EnergyPlus/tst/EnergyPlus/unit/EMSManager.unit.cc:2475: Failure Value of: actuatorIndirectErlVar.Value.TrendVariable Actual: true Expected: false /home/julien/Software/Others/EnergyPlus/tst/EnergyPlus/unit/EMSManager.unit.cc:2476: Failure Expected equality of these values: 0 actuatorIndirectErlVar.Value.TrendVarPointer Which is: 1 /home/julien/Software/Others/EnergyPlus/tst/EnergyPlus/unit/EMSManager.unit.cc:2499: Failure Value of: resultValueErlVar.Value.TrendVariable Actual: true Expected: false /home/julien/Software/Others/EnergyPlus/tst/EnergyPlus/unit/EMSManager.unit.cc:2500: Failure Expected equality of these values: 0 resultValueErlVar.Value.TrendVarPointer Which is: 1 /home/julien/Software/Others/EnergyPlus/tst/EnergyPlus/unit/EMSManager.unit.cc:2536: Failure In comparing enums of type 'EnergyPlus::DataRuntimeLanguage::Value', Expected equality of these values: DataRuntimeLanguage::Value::Number Which is: 1 actuatorDirectErlVar.Value.Type Which is: 0 /home/julien/Software/Others/EnergyPlus/tst/EnergyPlus/unit/EMSManager.unit.cc:2540: Failure Value of: actuatorDirectErlVar.Value.TrendVariable Actual: true Expected: false /home/julien/Software/Others/EnergyPlus/tst/EnergyPlus/unit/EMSManager.unit.cc:2541: Failure Expected equality of these values: 0 actuatorDirectErlVar.Value.TrendVarPointer Which is: 1 /home/julien/Software/Others/EnergyPlus/tst/EnergyPlus/unit/EMSManager.unit.cc:2548: Failure In comparing enums of type 'EnergyPlus::DataRuntimeLanguage::Value', Expected equality of these values: DataRuntimeLanguage::Value::Number Which is: 1 actuatorIndirectErlVar.Value.Type Which is: 0 /home/julien/Software/Others/EnergyPlus/tst/EnergyPlus/unit/EMSManager.unit.cc:2552: Failure Value of: actuatorIndirectErlVar.Value.TrendVariable Actual: true Expected: false /home/julien/Software/Others/EnergyPlus/tst/EnergyPlus/unit/EMSManager.unit.cc:2553: Failure Expected equality of these values: 0 actuatorIndirectErlVar.Value.TrendVarPointer Which is: 1 /home/julien/Software/Others/EnergyPlus/tst/EnergyPlus/unit/EMSManager.unit.cc:2576: Failure Value of: resultValueErlVar.Value.TrendVariable Actual: true Expected: false /home/julien/Software/Others/EnergyPlus/tst/EnergyPlus/unit/EMSManager.unit.cc:2577: Failure Expected equality of these values: 0 resultValueErlVar.Value.TrendVarPointer Which is: 1 /home/julien/Software/Others/EnergyPlus/tst/EnergyPlus/unit/EMSManager.unit.cc:2582: Failure Expected equality of these values: 45.0 Which is: 45 schDirect.CurrentValue Which is: 18 /home/julien/Software/Others/EnergyPlus/tst/EnergyPlus/unit/EMSManager.unit.cc:2583: Failure Value of: schDirect.EMSActuatedOn Actual: false Expected: true /home/julien/Software/Others/EnergyPlus/tst/EnergyPlus/unit/EMSManager.unit.cc:2584: Failure Expected equality of these values: 45.0 Which is: 45 schDirect.EMSValue Which is: 0 /home/julien/Software/Others/EnergyPlus/tst/EnergyPlus/unit/EMSManager.unit.cc:2585: Failure Expected equality of these values: 45.0 Which is: 45 schIndirect.CurrentValue Which is: 18 /home/julien/Software/Others/EnergyPlus/tst/EnergyPlus/unit/EMSManager.unit.cc:2586: Failure Value of: schIndirect.EMSActuatedOn Actual: false Expected: true /home/julien/Software/Others/EnergyPlus/tst/EnergyPlus/unit/EMSManager.unit.cc:2587: Failure Expected equality of these values: 45.0 Which is: 45 schIndirect.EMSValue Which is: 0 ```
Before the simulation, the Actuators in EvaluateStack would be assigned a Value naively, which means if it was assigned the result of a @TrendValue, it would set the TrendVariable to true Once we get to the Simulation itself again, BeginEnvrnInitializeRuntimeLanguage is called, which resets the actuators to a Value with Type Null In EvaluateStack, we go into a different block, because it now thinks it is a trendVariable And the assignment for trend variable only sets Number, not Type, so Type stays Null, and the actuator is not actuating anything as a result.
EXPECT_EQ(45.0, actuatorDirectErlVar.Value.Number); | ||
EXPECT_TRUE(actuatorDirectErlVar.Value.String.empty()); | ||
EXPECT_EQ(0, actuatorDirectErlVar.Value.Expression); | ||
EXPECT_FALSE(actuatorDirectErlVar.Value.TrendVariable); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before fix, this is actually TRUE, because we assigned a Value to the actuator that is the result of evaluating a trend variable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense. Good fix and test.
// Now, here's the kicker. Once we get to the Simulation itself again, we go into a different block, because t thinks it's a trend variable | ||
// BeginEnvrnInitializeRuntimeLanguage is called, which resets the actuators to a Value with Type Null | ||
// And the assignment for trend variable only sets Number, not Type, so Type stays Null, and the actuator is not actuating | ||
EMSManager::ManageEMS(*state, EMSManager::EMSCallFrom::SetupSimulation, anyEMSRan, ObjexxFCL::Optional_int_const()); | ||
EXPECT_FALSE(anyEMSRan); | ||
|
||
trendVar.TrendValARR = trendValues; | ||
|
||
state->dataGlobal->CurrentTime = 24.00; | ||
EMSManager::ManageEMS(*state, EMSManager::EMSCallFrom::BeginTimestepBeforePredictor, anyEMSRan, ObjexxFCL::Optional_int_const()); | ||
EXPECT_TRUE(anyEMSRan); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This explains what was happening before.
ScheduleManager::UpdateScheduleValues(*state); | ||
EXPECT_EQ(45.0, schDirect.CurrentValue); | ||
EXPECT_TRUE(schDirect.EMSActuatedOn); | ||
EXPECT_EQ(45.0, schDirect.EMSValue); | ||
EXPECT_EQ(45.0, schIndirect.CurrentValue); | ||
EXPECT_TRUE(schIndirect.EMSActuatedOn); | ||
EXPECT_EQ(45.0, schIndirect.EMSValue); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before fix, schedule CurrentValue is 18, and the EMS is off.
// #10279 - We don't do `thisErlVar.Value = ReturnValue;` because we don't want to copy TrendVariable stuff | ||
thisErlVar.Value.Type = ReturnValue.Type; | ||
thisErlVar.Value.Number = ReturnValue.Number; | ||
// thisErlVar.Value.String = ReturnValue.String; | ||
thisErlVar.Value.Error = ReturnValue.Error; | ||
thisErlVar.Value.initialized = ReturnValue.initialized; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the actual fix...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahhh very nice. 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great!!
// #10279 - We don't do `thisErlVar.Value = ReturnValue;` because we don't want to copy TrendVariable stuff | ||
thisErlVar.Value.Type = ReturnValue.Type; | ||
thisErlVar.Value.Number = ReturnValue.Number; | ||
// thisErlVar.Value.String = ReturnValue.String; | ||
thisErlVar.Value.Error = ReturnValue.Error; | ||
thisErlVar.Value.initialized = ReturnValue.initialized; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahhh very nice. 👍
EXPECT_EQ(45.0, actuatorDirectErlVar.Value.Number); | ||
EXPECT_TRUE(actuatorDirectErlVar.Value.String.empty()); | ||
EXPECT_EQ(0, actuatorDirectErlVar.Value.Expression); | ||
EXPECT_FALSE(actuatorDirectErlVar.Value.TrendVariable); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense. Good fix and test.
Great stuff, thanks @jmarrec, merging this. |
Pull request overview
First commit only takes a few references so the code is more readable (and debuggable...). Exclude it from the diff when reviewing: https://github.com/NREL/EnergyPlus/pull/10575/files/84c142cf8edb62e90735ef23c0ddf2e37c0491db..dd1efa4e56c4090afaf769b063ed1c4014b53169
Comparing before and after, for mcve.idf in the EnergyPlusDevSupport:
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.