Fix #10933 - Unable to read Site:GroundDomain:Slab with Site:GroundTemperature:Undisturbed:FiniteDifference #10934
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
```
[ RUN ] EnergyPlusFixture.SiteGroundDomainSlab_FiniteDifference
/home/julien/Software/Others/EnergyPlus2/tst/EnergyPlus/unit/PlantPipingSystemsManager.unit.cc:2320: Failure
Expected: ReadZoneCoupledDomainInputs(*state, 1, 1, errorsFound) doesn't throw an exception.
Actual: it throws EnergyPlus::FatalError with description "Site:GroundTemperature:Undisturbed:FiniteDifference--Errors getting input for ground temperature model".
/home/julien/Software/Others/EnergyPlus2/tst/EnergyPlus/unit/Fixtures/EnergyPlusFixture.cc:211: Failure
Expected equality of these values:
expected_string
Which is: ""
stream_str
Which is: " ** Fatal ** Site:GroundTemperature:Undisturbed:FiniteDifference--Errors getting input for ground temperature model\n ...Summary of Errors that led to program termination:\n ..... Reference severe error count=0\n ..... Last severe error=\n"
With diff:
@@ -1,1 +1,4 @@
-""
+ ** Fatal ** Site:GroundTemperature:Undisturbed:FiniteDifference--Errors getting input for ground temperature model
+ ...Summary of Errors that led to program termination:
+ ..... Reference severe error count=0
+ ..... Last severe error=\n
/home/julien/Software/Others/EnergyPlus2/tst/EnergyPlus/unit/PlantPipingSystemsManager.unit.cc:2323: Failure
Value of: compare_err_stream("")
Actual: false
Expected: true
[ FAILED ] EnergyPlusFixture.SiteGroundDomainSlab_FiniteDifference (931 ms)
```
jmarrec
added
Defect
jmarrec
commented
Feb 10, 2025
Comment on lines
+2319
to
+2323
| state->dataPlantPipingSysMgr->domains.resize(1); | ||
| EXPECT_NO_THROW(ReadZoneCoupledDomainInputs(*state, 1, 1, errorsFound)); | ||
|
|
||
| EXPECT_FALSE(errorsFound); | ||
| EXPECT_TRUE(compare_err_stream("")); |
There was a problem hiding this comment.
Unit test. Would fail with a bad error messsage
[ RUN ] EnergyPlusFixture.SiteGroundDomainSlab_FiniteDifference
/home/julien/Software/Others/EnergyPlus2/tst/EnergyPlus/unit/PlantPipingSystemsManager.unit.cc:2320: Failure
Expected: ReadZoneCoupledDomainInputs(*state, 1, 1, errorsFound) doesn't throw an exception.
Actual: it throws EnergyPlus::FatalError with description "Site:GroundTemperature:Undisturbed:FiniteDifference--Errors getting input for ground temperature model".
/home/julien/Software/Others/EnergyPlus2/tst/EnergyPlus/unit/Fixtures/EnergyPlusFixture.cc:211: Failure
Expected equality of these values:
expected_string
Which is: ""
stream_str
Which is: " ** Fatal ** Site:GroundTemperature:Undisturbed:FiniteDifference--Errors getting input for ground temperature model\n ...Summary of Errors that led to program termination:\n ..... Reference severe error count=0\n ..... Last severe error=\n"
With diff:
@@ -1,1 +1,4 @@
-""
+ ** Fatal ** Site:GroundTemperature:Undisturbed:FiniteDifference--Errors getting input for ground temperature model
+ ...Summary of Errors that led to program termination:
+ ..... Reference severe error count=0
+ ..... Last severe error=\n
/home/julien/Software/Others/EnergyPlus2/tst/EnergyPlus/unit/PlantPipingSystemsManager.unit.cc:2323: Failure
Value of: compare_err_stream("")
Actual: false
Expected: true
[ FAILED ] EnergyPlusFixture.SiteGroundDomainSlab_FiniteDifference (931 ms)
this comparison couldn't work because objectName was poiting to dataIPShortcuts->cAlphaArgs(3) and that just got reassigned by getObjectItem
Comment on lines
+1096
to
+1100
| // Ok, this is a finicky bug, but I have to make a copy here. GetGroundTempModelAndInit takes name (last param) by const ref& | ||
| // It then calls FiniteDiffGroundTempsModel::FiniteDiffGTMFactory which also takes objectName by const ref& | ||
| // But it calls getObjectItem with s_ipsc->cAlphaArgs which overrides it, then the comparison fails | ||
| std::string const groundTempModelName = s_ipsc->cAlphaArgs(3); | ||
| thisDomain.groundTempModel = GroundTemp::GetGroundTempModelAndInit(state, gtmType, groundTempModelName); |
There was a problem hiding this comment.
The fix is here. Take a copy.
There was a problem hiding this comment.
Yep, these DataIPShortCuts arrays can definitely be abused. They were incredibly valuable in Fortran when we had to pre-size array arguments to pass to functions. With vectors, it's just unnecessary. Probably time to deprecate that stuff and just use local variables.
There was a problem hiding this comment.
Should we open an issue for this? I haven't looked if there's one (on a mobile device right now)
Myoldmopar
approved these changes
Feb 10, 2025
Comment on lines
+1096
to
+1100
| // Ok, this is a finicky bug, but I have to make a copy here. GetGroundTempModelAndInit takes name (last param) by const ref& | ||
| // It then calls FiniteDiffGroundTempsModel::FiniteDiffGTMFactory which also takes objectName by const ref& | ||
| // But it calls getObjectItem with s_ipsc->cAlphaArgs which overrides it, then the comparison fails | ||
| std::string const groundTempModelName = s_ipsc->cAlphaArgs(3); | ||
| thisDomain.groundTempModel = GroundTemp::GetGroundTempModelAndInit(state, gtmType, groundTempModelName); |
There was a problem hiding this comment.
Yep, these DataIPShortCuts arrays can definitely be abused. They were incredibly valuable in Fortran when we had to pre-size array arguments to pass to functions. With vectors, it's just unnecessary. Probably time to deprecate that stuff and just use local variables.
Comment on lines
+2319
to
+2323
| state->dataPlantPipingSysMgr->domains.resize(1); | ||
| EXPECT_NO_THROW(ReadZoneCoupledDomainInputs(*state, 1, 1, errorsFound)); | ||
|
|
||
| EXPECT_FALSE(errorsFound); | ||
| EXPECT_TRUE(compare_err_stream("")); |
|
All happy here, let's get this merged. Thanks @jmarrec |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Pull request overview
Description of the purpose of this PR
Unable to read Site:GroundDomain:Slab with Site:GroundTemperature:Undisturbed:FiniteDifference
Opening this PR, I just noticed @Myoldmopar seem to have caught a similar bug in KusudaAchenbach 3 months ago: 1980d22#diff-f58e3210edb785bf9d28bcc7801f467c941feb911b1981fdf562abd401bb8285R81
(I had intended to fix #10807 but after hours of sleuthing I found the root cause but don't understand how to fix it:
)
Pull Request Author
Reviewer