Fix Site:Precipitation divide-by-zero

[ajscimone]

Pull request overview

• Fixes Zero Site:Precipitation input results in very large outputs #8038
• This pull request fixes a divide by zero error which occurs if the Site:Precipitation idf object is entered with a zero total annual precipitation. EnergyPlus will attempt to calculate the Rainfall CurrentRate each time the WaterManager::UpdatePrecipitation function is called, and perform a divide by zero error. This will result in the CurrentRate being set as a very large int for EnergyPlus 9.3 and will result in a crash for some older versions of EnergyPlus.

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]

A couple very quick changes requested, and then a more broad question about the static variables inside the water manager. I realize that probably falls into the scope of a follow-on issue/PR.

[Myoldmopar]

If you could plop the else { up right after the } that would be great. This change makes great sense.

[Myoldmopar]

Another new unit test file, this is great. I went to see if there was a clear_state in the water manager, but there isn't -- because the water manager doesn't hold any global state....at least that's how it "appears". There are a number of static variables inside functions inside WaterManager that could cause the simulation state to go haywire. I would like to see those extracted into the global state and a new clear_state method that will call it to reset the water manager. It doesn't necessarily have to be in this PR, but if not, then in a quick follow-up PR. And if you aren't able to accommodate it right now, I'll open a PR and do it.

[Myoldmopar]

I'm not sure what this line is doing. The GetWaterManagerInput function calls GetScheduleIndex, which allocates and sets up the schedule array. This line is doing nothing I think. Please take it out, it should still work perfectly.

[ajscimone]

Ah I think you're right. I don't need to allocate a second time here. Ill remove this.

[Myoldmopar]

This deallocate isn't necessary. The schedule manager's state is cleared after each unit test, and the deallocate is happening over there.

[ajscimone]

Roger that. These will be removed.

[Myoldmopar]

Same comments about allocate/deallocate, but these are great unit tests!

[Myoldmopar]

Something went awry with the Mac unit test. A hybrid unit test failed. Any thoughts why? I will re-run Mac just to make sure, but that doesn't look to be one of the ones we tend to have issues with.

[Myoldmopar]

I just noticed you had noted my comments but not made the changes yet anyway. No problem at all, I was just going to be moving to final review if CI was all happy and the changes were made. Carry on...

[ajscimone]

@Myoldmopar Hey Edwin. Yeah sorry for the delay with pushing things up. The hybrid unit test passes on my Mac so im not quite sure whats going on with the CI there. Nothing we have done here should effect that code. All of the changes you mentioned have been made on my local branch except for the static variables/ clear_state function. I was waiting to get a chance to speak with Neal about what I was proposing to do there, but since that change is less related to this, I will push the other changes up and see if the failed unit tests are a false positive.

[ajscimone]

@Myoldmopar Looks like pushing your requested changes caused those failed tests to go away.

[Myoldmopar]

Everything looks great now. Thanks for addressing my comments, and CI is happy. Merging.