Skip to content

Conversation

@kevin-moos
Copy link
Contributor

Pull request overview

Description of the purpose of this PR

Fix a defect in which zone multipliers are incorrectly applied to water heater tank losses. Add unit test to show correctly adjusted space gain fraction.

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

@mitchute
Copy link
Collaborator

@mjwitte this looks OK to me, but I'd defer to you on it.

if (state.dataHeatBal->Zone(ZoneNum).numSpaces > 1) {
gainFrac = state.dataHeatBal->space(spaceNum).FloorArea / state.dataHeatBal->Zone(ZoneNum).FloorArea;
}
if (std::find(AdjustTankLossMultipliers.begin(), AdjustTankLossMultipliers.end(), IntGainCompType) != AdjustTankLossMultipliers.end()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The method here is sound, but a comment line to explain would be helpful here.
But should this be done at the space level instead of here? At this point, only zone-level internal gain types call SetupSpaceInternalGain directly, so no multiplier adjustment is necessary. However, if at some point in the future the tank loss allows specifying a Zone Name or Space Name, then this adjustment will not get applied.
I think it would be better to move this into SetupSpaceInternalGain.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a comment and moved to SetupSpaceInternalGain

gainFrac = state.dataHeatBal->space(spaceNum).FloorArea / state.dataHeatBal->Zone(ZoneNum).FloorArea;
}
if (std::find(AdjustTankLossMultipliers.begin(), AdjustTankLossMultipliers.end(), IntGainCompType) != AdjustTankLossMultipliers.end()) {
int multiplier = state.dataHeatBal->Zone(ZoneNum).Multiplier * state.dataHeatBal->Zone(ZoneNum).ListMultiplier;
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit picking, this can be const

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

int RetNodeNum = 0 // for return air heat gains
);

// Multipliers are double counted for tank losses - these are the internal gains that need adjust for this
Copy link
Contributor

Choose a reason for hiding this comment

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

It's not that the multipliers are double counted, but rather the loss should be distributed across the multiplied zones/spaces. Not sure how to say that more simply.

Comment on lines +87 to +91
static constexpr std::array<DataHeatBalance::IntGainType, 4> AdjustTankLossMultipliers = {
DataHeatBalance::IntGainType::WaterHeaterMixed,
DataHeatBalance::IntGainType::WaterHeaterStratified,
DataHeatBalance::IntGainType::ThermalStorageChilledWaterMixed,
DataHeatBalance::IntGainType::ThermalStorageChilledWaterStratified};
Copy link
Contributor

Choose a reason for hiding this comment

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

If I'm following this correctly, any gain that comes from HVAC, electrical, refrigeration, etc. (anything other than the zone gains like lights, people, etc) should be included in this list. @shorowit ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did it this way so we could easily add/remove other gains as needed, but beyond water heaters I wasn't sure what to add.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess this is because the tanks have an ambient zone specified, right?

Doing a quick search for "Ambient Temperature Zone Name" in the IDD shows the following components:

Chiller:Electric:ASHRAE205
WaterHeater:Mixed
WaterHeater:Stratified
ThermalStorage:ChilledWater:Mixed
ThermalStorage:ChilledWater:Stratified
ThermalStorage:HotWater:Stratified
Pipe:Indoor

Searching "Ambient Zone Name" shows these:

ZoneHVAC:ForcedAir:UserDefined
AirTerminal:SingleDuct:UserDefined
Coil:UserDefined
PlantComponent:UserDefined
Duct:Loss:Conduction

Do these get handled here too?

Also, we can put a 📌 in this, but we should probably unify these.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll try to test these out and see if any should be added to the list.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Opened #11365 to address other components with ambient temperature zones

Copy link
Collaborator

Choose a reason for hiding this comment

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

@mitchute , we'd like to prevent this PR from expanding in complexity more than the Issue it was created to resolve, so there's a new issue documenting your concerns so we don't lose track of them. In the meantime, this particular set of changes is ready to finalize.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@tanaya-mankad that works. Thanks.

@mitchute mitchute self-requested a review October 22, 2025 18:16
@mitchute mitchute added the Defect Includes code to repair a defect in EnergyPlus label Oct 23, 2025
@mitchute mitchute added this to the EnergyPlus 25.2 milestone Oct 23, 2025
…lies-to-water-heater-tank-losses

# Conflicts:
#	tst/EnergyPlus/unit/WaterThermalTanks.unit.cc
@nrel-bot-2
Copy link

@kevin-moos it has been 28 days since this pull request was last updated.

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.

Zone multiplier incorrectly applies to water heater tank losses

6 participants