Fix identity matrix for diffuse solar exchange factors between zones

[xuanluo113]

Pull request overview

This is to fix the diagonal values of the matrix computing solar exchange factors between zones. The diagonal values of the matrix are set to ones at each time step and are used to calculate zone short-wave flux density QS, QSLights, and QSDifSol. However, during kickoff simulation and sizing, the values are set to zeros, which I suppose they should not, as the zone short-wave flux density values are set to zero during kicking off. And changing this line is causing major diffs in this branch.

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

[Myoldmopar]

This is a pretty significant amount of diffs. I understand the change, but I need some testing before we can just merge this. First, you should come up with a unit test to ensure that the matrix is always reset to the identity whenever it needs to be. Second, some plots showing the impact on simulation results. It could be that this change simply triggers HVAC to be on and off at different times, but overall energy use isn't affected. That should be demonstrated through some plots. There are 7 files with big ESO diffs, and while you don't need to show plots for every file, you should show enough to confirm the change.

[xuanluo113]

@Myoldmopar The unit test is added.

Comparing all the regression diffs in ASHRAE_LargeOffice during the design day simulations:

1. The eio diff is due to the different warmup days required (as the load change during Kickoff periods)
2. The err diff and html diff are due to a change of HVAC warning condition in Cooling tower convergence calculation.

**   ~~~   **  ...Valid range = 1.11 to 11.11. Occurrence info = DENVER-AURORA-BUCKLEY.AFB_CO_USA ANN CLG .4% CONDNS DB=>MWB, 07/21 02:30 - 02:38
-   **   ~~~   **  ...Tower range temperature passed to the model = 1.05610
+   **   ~~~   **  ...Tower range temperature passed to the model = 1.05611

Due to the 0.00001 diff in the invalid temp range, the plant loop errors occur more time than the dev branch during warmup and are causing different behavior during sizing as the figure below shows.

3. This is causing the data center zone temperature difference at the same hours.

4. This is not causing noticeable diffs in energy meters and zone temperatures other than the data center zone.

[Myoldmopar]

The code change here is great, and the unit test is as well. I am still nervous about how widespread the diffs are. I will need to run a little bit locally to convince myself but then I bet this can merge in.

[Myoldmopar]

This change is very succinct, and makes good sense.

[Myoldmopar]

This is a really great unit test. Just enough setup to demonstrate the change.

[Myoldmopar]

I ran a sample of 50 files locally for design days and found the incident rate of big diffs was actually pretty low. I then went back and ran some specific files that were showing big diffs on CI. The diffs seemed small. I re-ran those files with annual runs and found the MTR diffs were technically out of range, but actually small. I think this is ready to go. Anyone have any concerns before merging? (Timer set...)

[Myoldmopar]

Timer goes off

OK, merging.