Skip to content

Conversation

RKStrand
Copy link
Contributor

@RKStrand RKStrand commented Jun 20, 2025

Pull request overview

Description of the purpose of this PR

In an example file in our test suite, there exists a water heater that does not have any nodes defined in its own object but it is part of a branch where nodes for this object are defined. This conflict is noted with a warning message, but the simulation is allowed to continue with unknown consequences. With this correction, the error message has been bumped up to a severe/fatal so that the user has to fix the problem/inconsistency in the input file. In addition, the input file (ASHRAE901_OfficeSmall_STD2019_Denver.idf) has been corrected so that the branch and the water heater object both have the inlet and outlet node names. A unit test that verifies the error occurrence and messages is also included.

In general, any connection error where the parent (in this case the Branch) has registered node connections that are not matched by a child object (in this case the water heater) leaving the node types unefined will now result in a severe/fatal error.

Previous error:

** Warning ** Potential Node Connection Error for object WATERHEATER:MIXED, name=SHWSYS1 WATER HEATER
** ~~~ ** Node Types are still UNDEFINED -- See Branch/Node Details file for further information
** ~~~ ** Inlet Node : SHWSYS1 PUMP-SHWSYS1 WATER HEATERNODE
** ~~~ ** Outlet Node: SHWSYS1 SUPPLY EQUIPMENT OUTLET NODE

Revised error:

   ** Severe  ** Potential Node Connection Error for object WATERHEATER:MIXED, name=SHWSYS1 WATER HEATER
   **   ~~~   **   Node Types are still UNDEFINED -- See Branch/Node Details file for further information
   **   ~~~   **   Inlet Node : SHWSYS1 PUMP-SHWSYS1 WATER HEATERNODE
   **   ~~~   **   Outlet Node: SHWSYS1 SUPPLY EQUIPMENT OUTLET NODE
   ************* There was 1 node connection error noted.
   **  Fatal  ** Please correct either the branch nodes or the component nodes so that they match.

Pull Request Author

  • 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

  • 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

RKStrand added 3 commits June 12, 2025 15:59
Just a little experiment with our test suite to see if there are any other failures of this kind...
This commit corrects the temporary text of the fatal error.  It also corrects the input file that now causes a severe/fatal error.  This input file will have different results than develop because the node issue has been fixed in this input file.
This unit test causes the error message that was changed and tests to make sure that it is being correctly reported at the correct level.
@RKStrand RKStrand added this to the EnergyPlus 25.2 IO Freeze milestone Jun 20, 2025
@RKStrand RKStrand requested review from Myoldmopar and mjwitte June 20, 2025 17:52
@RKStrand RKStrand self-assigned this Jun 20, 2025
@RKStrand RKStrand added the Defect Includes code to repair a defect in EnergyPlus label Jun 20, 2025
Copy link

⚠️ Regressions detected on macos-14 for commit 5552b65

Regression Summary
  • Audit: 1
  • BND: 1
  • EIO: 1
  • ERR: 1
  • MTD: 1
  • MTR Big Diffs: 1
  • Table Big Diffs: 1
  • Table String Diffs: 1

@RKStrand
Copy link
Contributor Author

I should have added that the changes will result in differences in the input files. The unrevised IDF will fatal out now rather than just have a warning message. Also when comparing the old input file run with develop to the new input file run in the revised code, there will be differences in the output. All other files should show no differences in the output.

@mjwitte mjwitte changed the title Correct Error Message Level for WaterHeater:Mixed When Nodes are Missing Correct Error Message Level when Node Types are Undefined Jul 3, 2025
ShowContinueError(state, " Node Types are still UNDEFINED -- See Branch/Node Details file for further information");
ShowContinueError(state, format(" Inlet Node : {}", state.dataBranchNodeConnections->CompSets(Count).InletNodeName));
ShowContinueError(state, format(" Outlet Node: {}", state.dataBranchNodeConnections->CompSets(Count).OutletNodeName));
ShowFatalError(state, "Please correct either the branch nodes or the component nodes so that they match.");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@RKStrand When I test the original file (with missing nodes) the new code crashes. I'm not sure why/how, but following this in the debugger, ShowFatalError is being called multiple times. Adding a local error flag and moving the ShowFata to the end of ReportLoopConnections seems to fix this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that this change will require some changes to your unit test.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Weird. Not sure what happened there. I'll implement the recommended change and update the unit test. Look for the mods after the holiday weekend (hopefully). Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mjwitte Latest commit fixes this problem. Avoids a hard crash as per your suggestion. Unit test also updated.

Corrected a problem where the original IDF that revealed the problem was crashing after the fix was made.  This commit corrects the problem and updates the unit test to reflect this change.
Copy link

github-actions bot commented Jul 7, 2025

⚠️ Regressions detected on macos-14 for commit a3f7f91

Regression Summary
  • Audit: 1
  • BND: 1
  • EIO: 1
  • ERR: 1
  • MTD: 1
  • MTR Big Diffs: 1
  • Table Big Diffs: 1
  • Table String Diffs: 1

Copy link
Contributor

@mjwitte mjwitte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Original defect file now produces this error:

   ** Severe  ** Potential Node Connection Error for object WATERHEATER:MIXED, name=SHWSYS1 WATER HEATER
   **   ~~~   **   Node Types are still UNDEFINED -- See Branch/Node Details file for further information
   **   ~~~   **   Inlet Node : SHWSYS1 PUMP-SHWSYS1 WATER HEATERNODE
   **   ~~~   **   Outlet Node: SHWSYS1 SUPPLY EQUIPMENT OUTLET NODE
   ************* There was 1 node connection error noted.
   **  Fatal  ** Please see severe error(s) and correct either the branch nodes or the component nodes so that they match.

I'll merge in develop and let CI run one more time, then merge.

Copy link

⚠️ Regressions detected on macos-14 for commit 8bcb374

Regression Summary
  • Audit: 1
  • BND: 1
  • EIO: 1
  • ERR: 1
  • MTD: 1
  • MTR Big Diffs: 1
  • Table Big Diffs: 1
  • Table String Diffs: 1

@mjwitte
Copy link
Contributor

mjwitte commented Jul 16, 2025

CI is all green. Setting this to merge queue since @Myoldmopar is busy merging at the moment.

@mjwitte mjwitte assigned Myoldmopar and unassigned RKStrand Jul 16, 2025
Copy link
Member

@Myoldmopar Myoldmopar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, will check locally now.


if (nodeConnectionErrorFlag) {
ShowFatalError(state, "Please see severe error(s) and correct either the branch nodes or the component nodes so that they match.");
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a fine fix, bumping it to a severe makes sense. And adding the separate flag to provide extra error context is a good change.

BLDG_SWH_SCH, !- Use Flow Rate Fraction Schedule Name
, !- Cold Water Supply Temperature Schedule Name
SHWSys1 Pump-SHWSys1 Water HeaterNode, !- Use Side Inlet Node Name
SHWSys1 Supply Equipment Outlet Node; !- Use Side Outlet Node Name
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And yeah, good stuff getting the test file fixed up.

" ..... Last severe error=Potential Node Connection Error for object WATERHEATER:MIXED, name=WaterHeaterMixed1",
});

EXPECT_TRUE(compare_err_stream(error_string, true));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a quick aside, there is a separate unit test function that you can call that is a bit easier. In this case, we don't care what the exact error output looks like. We just really want to make sure it is reporting the node connection error. Instead of compare_err_stream(), try compare_err_stream_substring(). In that function you could call:

EXPECT_TRUE(compare_err_stream_substring("Potential Node Connection Error", true));

and that should be sufficient.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Myoldmopar That is very interesting and not something that I realized was an option. I will definitely try to remember that the next time I need to do something like this in a unit test. Thanks!

@Myoldmopar
Copy link
Member

Passing happily here, merging. Thanks @RKStrand

@Myoldmopar Myoldmopar merged commit 75c69bb into develop Jul 16, 2025
9 checks passed
@Myoldmopar Myoldmopar deleted the 11061MissingNodesinWaterHeaterMixedErrorMessageLevel branch July 16, 2025 15:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Defect Includes code to repair a defect in EnergyPlus

Projects

None yet

Development

Successfully merging this pull request may close these issues.

issue with example file: ASHRAE901_OfficeSmall_STD2019_Denver.idf

6 participants