Fix ASHRAE simple method for vertical surfaces #7890
Conversation
… simple algorithm and to ensure that HCovIn is being set correctly for non-kiva managed surfaces.
nealkruis
added
Defect
nealkruis
changed the title
…ro delta temperature.
… and add ascii table comment explaining the outcomes of each condition of the ashrae simple algorithm.
…ctly, which reveals a bug in the original algorithm.
…e decision tree style algorithm which now catches more horizontal surface scenarios.
|
@ajscimone feel free to pull in develop again here. There's one minor merge conflict that isn't really a conflict. I merged in develop and ran tests locally, but I can't push to your fork. I'll work on reviewing it once CI takes another pass here. |
…shrae simple method is skipped.
|
@mitchute I re-merged develop and saw the white space merge conflict you were referring to. Everything should be fixed and up to date now, just waiting for the CI. |
| return 3.076; | ||
| } | ||
| else { | ||
| Real64 DeltaTempCosTilt = (Tamb - Tsurf)*cosTilt; |
There was a problem hiding this comment.
A little whitespace around that asteriscus would've been nice but not a big deal.
| // | Tilted Surface | Negative | -0.3827 to -0.9239 (157.5 to 157.5 degrees) | Positive | Enhanced | 3.870 | | ||
| // | Tilted Surface | Negative | 0.3827 to 0.9239 (22.5 to 67.5 degrees) | Negative | Reduced | 2.281 | | ||
| // | Tilted Surface | Positive | -0.3827 to -0.9239 (157.5 to 157.5 degrees) | Negative | Reduced | 2.281 | | ||
| // +---------------------+-----------+---------------------------------------------+------------------+-----------------+-------------+ |
There was a problem hiding this comment.
In a function like this with a bunch of code paths, this is a great addition to help a developer understand what should be happening, and also the limitations of the function.
| ConvectionCoefficient = DataHeatBalance::HConvIn(1); | ||
|
|
||
| EXPECT_EQ(ConvectionCoefficient, ExpectedCoefficient); | ||
| } |
There was a problem hiding this comment.
Great set of unit tests. Kind of a shame that you missed one code path in all the possible configurations (line 3874 here), but again, not going to hold this up for that, this is good work.
| return CalcASHRAESimpleIntConvCoeff(Tsurf, Tamb, cosTilt); | ||
| }; | ||
| } | ||
| else { |
There was a problem hiding this comment.
This else style is not the preferred form. } else { is preferred.
| @@ -49,7 +49,6 @@ | |||
| #include <algorithm> | |||
| #include <cassert> | |||
| #include <cmath> | |||
| #include <limits> | |||
|
CI is generally really happy. I'm not worried about the Windows bot, it looks to be due to the branch being behind develop. I'm running it locally to verify the build and fix. |
|
No defect file, but built and ran unit test suite locally. Good to go. Merging. |
Pull request overview
NaNfor surfaced tilted above 67.5 degrees. This essentially covers the "vertical surface" cases fed into the ASHRAE Simple algorithm. After this pull request, HConvIn is set correctly for all cases, and unit tests have been added to ensure that surfaces which don't already have an HConvIn set are assigned a convection coefficient correctly, and that the CalcASHRAESimpleIntConvCoeff function returns the correct coefficient for each case defined by the algorithm.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.