Exclude air boundaries and non-used constructions in conduction finite difference

[LipingWang]

Pull request overview

• Fixes Conduction finite difference failed when thin or highly conductive constructions not in use or air boundary are in the model #7819
• Exclude air boundaries and non-used constructions to be processed in conduction finite difference. EnergyPlus models crashed when there are air boundaries in the model or stopped with a fatal error for thin or highly conductive constructions created in constructions (not being used by building surfaces).
• This modification excludes air boundaries and non-used constructions in the initialization and calculation using the conduction finite difference method.
• With this fix, both defect files run to completion.

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 - no diffs expected
• 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]

@LipingWang Please merge in develop to make this branch current.
@mitchute or @Myoldmopar I've tested this with the defect files and it fixes the problems. This is ready for final review.

[mitchute]

Looks OK to me.

[Myoldmopar]

The code changes seem fine, but the unit test needs work. It looks like the logic is copied from inside EnergyPlus into the unit test, rather than the unit test calling into EnergyPlus. This would defeat the purpose of the unit test because the code inside EnergyPlus could be broken at any time and the new unit test would never reveal that.

If I'm mistaken, please clarify how the unit test works.

[Myoldmopar]

What is happening here? Where are you calling the E+ code to exercise your changes? It almost looks like you copied the code from inside E+ and put it in here. If that were the case, you'd need to modify this to just call the E+ code instead. If you have trouble calling the E+ code because it requires too much data to be set up prior to calling the function, then just put some of the modified logic into a smaller function that is easier to call.

[LipingWang]

Thank you for your comments. I agree that directly calling the function for unit test is a better choice. The function under modification is ReportFiniteDiffInits(EnergyPlusData &state) for initialization of the finite difference calculation of each construction. I cannot figure out a way to directly call this function. So I put part of the code from the original function to run the test. Please feel free to suggest methods of calling this function or modification.

[Myoldmopar]

Take these four lines that are used to determine if a layer should be included or not:

• Create a small method on the construction object named "shouldSkipForFiniteDifference". It performs the same series of checks and returns true if the construction should be skipped.
• In the finite difference manager, replace all those lines with a call to that method on the construction object:

if (state.dataConstruction->Construct(ThisNum).shouldSkipForFiniteDifference()) continue;

• Then your unit test can really just set up a construction object, no input objects required, and call that tiny little worker function under a series of cases and see if it returns true or not.

Done.

[LipingWang]

Edwin, after further looking into this, I was able to call and test the original function InitialInitHeatBalFiniteDiff. Please see the updated unit test. Thank you for your comments.

[Myoldmopar]

The logic changes seem fine.

[Myoldmopar]

Much better! OK, I'll build locally but this is now on track to go in. Thanks for the quick fix up.

[Myoldmopar]

OK, CI is showing a couple of unused variables. I'll clean those up locally, build and then get this merged in.

[Myoldmopar]

@LipingWang OK, I fixed a couple tiny things in this branch, built it with latest develop, and ran the unit test suite. This one is good to go in. Thanks!

[LipingWang]

Thanks a lot!