Adjustment to View Factor Fix for Surfaces that are Nearly Half of Zone Surface Area #8809
Conversation
This commit corrects a variety of view factor issues that were noted in CR#8700. While the changes are not particularly massive, it did require quite a bit of time to investigate these issues.
Fixed some format errors and included a new unit test for this work. Also modified the check slightly to see how that impacts results.
A few more formatting changes were made in this commit. In addition, some Engineering Reference documentation was updated to reflect the changes made and also correct a slightly off description of the view factor calculation algorithm.
Tweaked the description of to add a bit more information.
Came up with one more thought to add to this. Should be ready for review now.
One of the requests for this CR was an additional warning message when there is an internal mass surface in the zone. Includes new bool function and unit test.
| } | ||
| } | ||
|
|
||
| bool DoesZoneHaveInternalMass(EnergyPlusData &state, int const numZoneSurfaces, const Array1D_int &surfPointer) |
There was a problem hiding this comment.
state is unused here.
Also, minor detail, but numZoneSurfaces isn't necessary. The loop statement could be for (int surfNum : surfPointer).
If I'm following correctly, this only happens once per enclosure? So, there's no concern about repeating this, correct?
There was a problem hiding this comment.
Yes, should happen once (at most twice--not exactly sure why it's coming through twice sometimes?) per enclosure. So, basically, get rid of state and numZoneSurfaces and adjust loop to the suggestion. Will do.
There was a problem hiding this comment.
You sure I don't need state? It's used in the line:
if (state.dataSurface->Surface(surfPointer(surfNum)).Class == SurfaceClass::IntMass)...
My compiler (after getting rid of state) is unhappy and I've tried a variety of variations. Of course, maybe I just don't understand "state"...
There was a problem hiding this comment.
The for (int surfNum : surfPointer) didn't work...this happens once per zone. The second time it comes into this routine (for the second zone), the counter picks up where it left off and then I get a crash. In the file that I was running, there were 9 surfaces in the first zone and six in the second. The first time through there weren't any problems. The second time through for zone 2 the counter starts at 10 which is more than the number of surfaces in the zone and I get a pointer issue. I think I need to go back to what I originally had?
There was a problem hiding this comment.
It's fine as-is, but for future reference, there's no counter with the suggested form, so this should work fine for multiple calls.
for (int surfNum : surfPointer){
if (state.dataSurface->Surface(surfNum).Class == SurfaceClass::IntMass) return true;
}
| // "unbalanced" in surface area distribution so other strange cases can take advantage of | ||
| // this correction. The use of 0.9 is simply to provide some reasonable boundary numerically | ||
| // and does not have some derived theoretical basis. | ||
| if (LargestArea > 0.9 * (sum(A) - LargestArea) && (N > 3)) { |
There was a problem hiding this comment.
In the diff comments, you suggest the the files with diffs have a surface that is triggering this rule, but I'm not seeing anything like that in 5ZoneSupRetPlenRAB. I see a couple of large surfaces (TOP-1 and BOTTOM-1) but these should be balanced by an equal area of floor/ceiling surfaces, plus the wall surfaces. Even in the plenum zones I'm puzzled why results would change. I haven't looked at any of the other files with diffs. Is it possible some other change is driving these diffs?
There was a problem hiding this comment.
Oh, wait, I see what's going on, I think. For a plenum zone, say there are 2 large surfaces of 100m2 each and 4 small ones that add up to 10m2. So this would be if (100 > 0.9*(210-100)=99 which trips the rule. But I don't see why that isn't a valid enclosure as-is.
There was a problem hiding this comment.
Yeah, there's some question as to what is the limit of "valid". The old rule was a comparison without the 0.9. Maybe 0.9 is not great either. What about 0.99? The problem is that if a user gets close but not exactly there, the file could run into problems like was seen in this defect. How do you feel about 0.99? It would probably make most of the diffs from develop go away.
There was a problem hiding this comment.
"When one surface has an area that exceeds the sum of all other surface areas in a zone".
I assume the surfaces in question here are the zone floor, walls and ceiling. If so, then the largest surface's area should not be greater than 1/2 the total surface area. I can't think of a geometry where that would be the case unless the walls aren't vertical but I assume someone put this here for a reason. Not even sure if a unit test could sort this out.
There was a problem hiding this comment.
So perhaps a better solution to the numerical problem in the users file would be to use a small absolute tolerance, like
if (LargestArea > (sum(A) - LargestArea - somesmallvalue)
There was a problem hiding this comment.
@RKStrand are the walls in the defect file vertical? What is the specific geometry that trips this limit?
There was a problem hiding this comment.
Even a pyramid wouldn't trip this limit. Are walls missing?
There was a problem hiding this comment.
The zone in question here is a simple and fairly normal box (4 vertical walls, roof, floor) with one exception--it has an internal mass surface with an area equal to just slightly below the total area of the remaining surfaces. As a result the "correction" in the current code gets skipped and the view factor calculations never iterate to completion. This resulted in weird trends in the results. I agree that it is not possible to have a valid geometry (i.e., a full enclosure) where one surface has an area larger than the remaining surface areas--it's no longer a full enclosure. That's why the correction is there to avoid numerical issues when you are trying to figure out view factors for an incomplete enclosure. In the user file that resulted in the defect, it just missed the cutoff.
There was a problem hiding this comment.
@mjwitte I can do the small absolute tolerance instead of the percentage. I think I was thinking that because everything is scaled to area that a fraction would be better. Of course in either case, we have to establish either what the small value or the small fraction is. I could go to 0.99 or 0.999 as a fraction. I could also go to say 1m2 as a small tolerance. Which do you prefer? I'm good with any of those.
There was a problem hiding this comment.
I don't have a strong opinion, just use something reasonable that fixes the defect file without tripping 5ZoneSupRetPlenRAB.
| ShowContinueError(state, | ||
| "For zones with internal mass like this one, this" | ||
| "can happen when the internal mass has an area that" | ||
| "is much larger than the other surfaces in the zone."); |
There was a problem hiding this comment.
Would it be appropriate to suggest splitting the internal mass into two or more surfaces?
There was a problem hiding this comment.
I will adjust the error message to suggest splitting the thermal mass into two or more surfaces.
Addressed the review comments. One request to change the passed variables to the new subroutine could not be implemented--ran into problems with hard crash because counter variable was keeping it's value from call to call which caused a pointer issue.
|
@mjwitte I addressed the internal mass error messaging issue and adjusted the fraction from 0.9 to 0.99. Hopefully this resolves everything and the ci tests come back better than before. Let me know if you still want me to adjust the passed variables to the new subroutine (see note above--I couldn't get your suggestion to work). Thanks! |
|
Not sure why it is failing on "AirTerminalSingleDuctMixer_SimFCU_ATMInletSideTest". It runs on my machine just fine. Good news is that the 0.99 gets rid of all the other differences. Somehow GitHub didn't push one minor change to a comment line up so I'm doing that now... |
Not sure why GitHub did not push this change in the last round. Anyway, here it is...
Pull request overview
Audit diffs are due to additional table entries for internal mass.
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.
If feature, test running new feature, try creative ways to break itVerify IDF naming conventions and styles, memos and notes and defaultsIf new idf included, locally check the err file and other outputs