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
Change test tank height for heaters in stratified water heater #5881
Conversation
Fixes #5635 [#129956735]
@mjwitte I put you down to review, but if you are out of time, I can handle it. The defect file from the issue now runs successfully though. |
@Myoldmopar When running the unit tests locally, EnergyPlusFixture.HPWHWrappedDummyNodeConfig fails as well as EnergyPlusFixture.StratifiedTankUseEnergy |
Yeah, I saw the failures come through, I'll take a look in a bit. Can you post the failure output? |
[ RUN ] EnergyPlusFixture.HPWHEnergyBalance [ RUN ] EnergyPlusFixture.StratifiedTankUseEnergy |
@@ -2921,7 +2921,8 @@ namespace WaterThermalTanks { | |||
WaterThermalTank( WaterThermalTankNum ).HeaterHeight1 = rNumericArgs( 7 ); | |||
|
|||
//Test if Heater height is within range | |||
if ( ( !WaterThermalTank( WaterThermalTankNum ).HeightWasAutoSized ) && ( WaterThermalTank( WaterThermalTankNum ).HeaterHeight1 > WaterThermalTank( WaterThermalTankNum ).Height ) ) { | |||
Real64 tankHeightForTesting = 2.0 * sqrt( ( WaterThermalTank( WaterThermalTankNum ).Volume / WaterThermalTank( WaterThermalTankNum ).Height ) / DataGlobals::Pi ); |
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.
@Myoldmopar @JasonGlazer I think tankHeightForTesting needs to be sensitive to the Tank Shape type.
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.
That makes all the sense. #dumb
I'll make that change shortly.
Also, the code seems to be assuming a horizontal cylinder but looks like it would run for a vertical or horizontal orientation. |
… integration tests now pass locally
I think 222ee92 should take care of this now. Unless I'm missing something from @JasonGlazer 's last comment. |
@Myoldmopar Code changes look correct to me. I don't have time to test at the moment, but can tomorrow. |
@Myoldmopar and @mjwitte the code change looks good to me too and the CI tests are clean. |
This just changes which height to use in checking whether the heaters are out of range. I didn't do a deep inspection of the whole module, but the defect file now runs to completion with a clean error file, so I think this is good to go. Fixes: #5635