Tank Source Side Flow Control Mode IndirectHeatAlternateSetpoint Code Not Hit

[yzhou601]

Pull request overview

The source side flow set by calling function PlantMassFlowRatesFunc didn't actually work for source side flow control mode: IndirectHeatAlternateSetpoint for indirect water heater system and tankless coils. Relevant question on Unmet Hour: Click Here
The issue and CR9176 changes are made under condition of MaybeRequestingFlow which is not stepped at final iteration in every time step.
So these codes are added to condition ThrottlingFlow to fix this problem.

Work Checklist

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)
• At least one of the following appropriate labels must be added to this PR to be consumed into the changelog:
  • Defect: This pull request repairs a github defect issue. The github issue should be referenced in the PR description
  • Refactoring: This pull request includes code changes that don't change the functionality of the program, just perform refactoring
  • NewFeature: This pull request includes code to add a new feature to EnergyPlus
  • Performance: This pull request includes code changes that are directed at improving the runtime performance of EnergyPlus
  • DoNoPublish: This pull request includes changes that shouldn't be included in the changelog

Review Checklist

This will not be exhaustively relevant to every PR.

• Functional code review (it has to work!)
• If defect, results of running current develop vs this branch should exhibit the fix
• CI status: all green or justified
• Performance: CI Linux results include performance check
• Unit Test(s)
• C++ checks:
  • Argument types
  • If any virtual classes, ensure virtual destructor included, other things
• IDD changes:
  • Verify naming conventions and styles, memos and notes and defaults
  • Open windows IDF Editor with modified IDD to check for errors
  • If transition, add to input rules file for interfaces
  • If transition, add transition source
  • If transition, update idfs
• If new idf included, locally check the err file and other outputs
• Required documentation updates?
• ExpandObjects changes?
• If output changes, including tabular output structure, add to output rules file for interfaces

[yzhou601]

This bug fix issue seems not have been covered in tests without any CI test fail. So it's necessary to show my local test results for indirect water heater system models (no backup water heater, 55C alternate setpoint schedule, 52.667C WH Setpoint, 1C deadband).

I expected the source side mass flow rate to be over 0 for time points when tank temperature is below 55C-1C.

Run in Develop

IndirectHeatAlternateSetpoint

IndirectHeatPrimarySetpoint

Run in this branch

IndirectHeatAlternateSetpoint

IndirectHeatPrimarySetpoint

It shows more insights of this bug, the alternate option is not really working.

[Myoldmopar]

The functional changes here look good, but I'd like to see some improvements made before this gets merged. Take a look at my comments and let me know what you think.

[yzhou601]

@Myoldmopar Do you know why these commits that happened in develop branch all appeared in this PR once I pulled develop into this branch?
(seemed to be normal again.)

[Myoldmopar]

Yeah I don't know what happened there. But your develop branch is 8 commits ahead of the NREL/EnergyPlus develop branch. So that's potentially problematic, but it seems not with the current issue.

[yzhou601]

Yeah I don't know what happened there. But your develop branch is 8 commits ahead of the NREL/EnergyPlus develop branch. So that's potentially problematic, but it seems not with the current issue.

Yeah, these 8 commits are all pulling the latest changes from NREL/EnergyPlus/develop, might be fine?

[Myoldmopar]

No, your develop branch should never be ahead of the NREL develop branch. Pulling in the latest changes from NREL develop should cause a git fast-forward, never a merge, so it should never be ahead. Something went wrong. But again, I don't think it caused an issue here, just something you need to get addressed before it accidentally does.

[mjwitte]

@Myoldmopar Um, there hasn't been a commit to this branch for 6 or 7 days, yet CI keeps making the rounds on this one. Working on it again now.

[Myoldmopar]

Yeah, CI has been having a bit of trouble with some external PRs lately. Nothing serious, just wastes some cycles running extra commits. I'm currently unclear as to the reason, but will dig into it soon after the release, if not before.

[Myoldmopar]

I'm not sure what editor you are using to write C++, but it shouldn't be leaving these trailing whitespaces in the files. In most editors there is an option to clean any trailing whitespace when you save a file, I'd look for that option. I'm not going to hold up this PR just for that though.

[Myoldmopar]

Code changes look much better now. I'm testing locally.

[Myoldmopar]

Merged up with develop, unit test suite passes, and ran a sampling of regression files and got no diffs. I'm merging this in.