-
Notifications
You must be signed in to change notification settings - Fork 394
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 issue #8680 crash with multiple windows with blinds in the same zone #8681
Conversation
@@ -11946,7 +11946,7 @@ void CalcWinTransDifSolInitialDistribution(EnergyPlusData &state) | |||
construct_sh_BlAbsDiffBack(std::min(MaxSlatAngs, SurfWinSlatsAngIndex + 1), IGlass), | |||
SurfWinSlatsAngInterpFac); | |||
} else { | |||
BlAbsDiffBk = construct_sh_BlAbsDiffBack(1, Lay); | |||
BlAbsDiffBk = construct_sh_BlAbsDiffBack(1, IGlass); |
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.
Original regression from 57aa34f#diff-18fbb8a5b1334b62513de2441bdb4bb4ef1fbe5fbec24d5dd0effcd019dfc33dR10986
I think It should be IGlass indeed, fix looks good to me 👍
@jmarrec Thanks for the review. This branch is still hanging. Is this coverage issue blocking the merge? |
@xuanluo113 I agree that the fix is correct, but I'm concerned that this was not caught by the CI regression/integration tests. What feature in the defect file trips on this defect? We should consider adding a unit test or modifying an integration test file to cover this code. Also, does the integration and unit test code coverage show any other blocks of code in the solars hading functions that are not covered? |
@mjwitte It seems to need two windows in the same enclosure on different walls (with a view factor for each other), and at least one of them has blinds to trigger this branch. I can modify the current |
@xuanluo113 That would be good. Please make a separate PR for that testfile change, to confirm that CI reports a failure. |
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.
Tested this branch with the modified PurhAirWindowBlind (from #8702 ) and compared results with v9.4.0. No diffs (except in thermal comfort outputs which are expected from 9.4 to 9.5). This will merge once CI comes back green.
Clean and green. Merging. |
Pull request overview
Fixed a typo.
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.