Fix tdd calculations in CalcInteriorSolarDistribution

[xuanluo113]

Pull request overview

• Fixes Bug in TDD related calculations in CalcInteriorSolarDistribution #8356
• Fixes Incorrect local variable scope in CalcInteriorSolarDistribution #8369
• Code change explained in Issue descriptions.
• Diffs in DaylightingDevicesTubular.idf and DaylightingDeviceShelf.idf are expected with the fix.

These are the changes for DaylightingDeviceTubular:

The changes look reasonable, but the heat gain/loss is wrong based on the other temperatures. Will post a new issue for that.

Pull Request Author

Add to this list or remove from it as applicable. This is a simple templated set of guidelines.

• Title of PR should be user-synopsis style (clearly understandable in a standalone changelog context)
• Label the PR with at least one of: Defect, Refactoring, NewFeature, Performance, and/or DoNoPublish
• Pull requests that impact EnergyPlus code must also include unit tests to cover enhancement or defect repair
• Author should provide a "walkthrough" of relevant code changes using a GitHub code review comment process
• If any diffs are expected, author must demonstrate they are justified using plots and descriptions
• If changes fix a defect, the fix should be demonstrated in plots and descriptions
• If any defect files are updated to a more recent version, upload new versions here or on DevSupport
• If IDD requires transition, transition source, rules, ExpandObjects, and IDFs must be updated, and add IDDChange label
• If structural output changes, add to output rules file and add OutputChange label
• If adding/removing any LaTeX docs or figures, update that document's CMakeLists file dependencies

Reviewer

This will not be exhaustively relevant to every PR.

• Perform a Code Review on GitHub
• If branch is behind develop, merge develop and build locally to check for side effects of the merge
• If defect, verify by running develop branch and reproducing defect, then running PR and reproducing fix
• If feature, test running new feature, try creative ways to break it
• CI status: all green or justified
• Check that performance is not impacted (CI Linux results include performance check)
• Run Unit Test(s) locally
• Check any new function arguments for performance impacts
• Verify IDF naming conventions and styles, memos and notes and defaults
• If new idf included, locally check the err file and other outputs

[mjwitte]

@xuanluo113 The code changes look good.
Unit tests all pass when run serially.
I've added some plots showing the changes in the TDD.
But it seems like there should be diffs in the light shelf file. Stepping through the debugger, the code block is not getting used, but somehow the numbers aren't making it all the way to output.

[mjwitte]

@xuanluo113 Are you planning to look more closely at the lack of diff for the light shelf test file?

[xuanluo113]

@mjwitte I could do that by reporting related variables and add to the current regression test suite if necessary. However, in the meantime, I found some other similar bugs (non-tdd related) in this function (Issue #8369), which tested locally, may trigger more diffs (less in design day but more in year-round simulation reporting). Hope you can take another look at that issue to verify.

I plan to fix those together in this branch unless you suggest otherwise

[mjwitte]

@xuanariel Regarding the light shelf, I attempted to add output variables to that file and could not find any that were changing. If you have a test file that shows changes with this branch, please share it.

[mjwitte]

@xuanluo113 (correct name this time!) Regarding the light shelf, I attempted to add output variables to that file and could not find any that were changing. If you have a test file that shows changes with this branch, please share it.

[xuanluo113]

@mjwitte Thanks.

1. I fixed and linked Issue Incorrect local variable scope in CalcInteriorSolarDistribution #8369 here.

2. For the light shelf, I took another look and found there are no diffs because the whole branches are calculating things never got used. The problematic branch is calculating DGZoneWin and DSZoneWin. These are to add to DSZone // Factor for sky diffuse solar radiation into a zone and DGZone // Factor for ground diffuse solar radiation into a zone arrays.

I searched these arrays globally, they were used for calculating QDforDaylight (Diffuse solar radiation in a zone from sky and ground diffuse entering) back in 2007, but not anymore ever since. However, there still hang quite some blocks there to calculate these intermediate arrays.

Anyhow, I plan to leave this bug fixing branch as it is and switch back to window-struct2 (PR #8347) to fix the second issue. There are nested branches and variables related to those calculations and would be better to add the fix upon the refactoring branch. The branch window-struct2 is currently cleaned up to be reviewed next.

[mjwitte]

@xuanluo113 This looks good. A few comments that can be addressed here or in the next window-struct branch. Let me know. I've pulled in develop to let CI make another pass before merging.

[mjwitte]

Seems like CosInc should be declared here with local scope (and also up in line 6360?). Possibly the same for some of these other variables. If you are planning that already in the next windowstruct branch, then it's ok here as-is.

[mjwitte]

The full arrays for WinTransBmBmSolar and WinTransBmDifSolar are zeroed already in lines 6276:6277. Delete one of these?

[mjwitte]

And these lines probably are not necessary if these have been zeroed up at the top of the loop, but it's hard to know for sure if the path leads directly here or if the reset to zero is necessary.

[xuanluo113]

@mjwitte Thanks! These have been resolved or do not apply anymore in the window-struct2 branch. CosInc is declared locally. WinTransBmBmSolar and WinTransBmDifSolar are declared at the top without initialization, and reset to zero only when necessary in the loop. The redundant resets are removed.

[mjwitte]

@xuanluo113 Thanks for clarifying. This will merge once CI finishes.