Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

More robust internal gain subtotals for UFAD and Displacement Ventilation #10419

Merged
merged 7 commits into from
Mar 15, 2024
Merged

More robust internal gain subtotals for UFAD and Displacement Ventilation #10419

merged 7 commits into from
Mar 15, 2024

Conversation

mjwitte
Copy link
Contributor

@mjwitte mjwitte commented Feb 29, 2024

Pull request overview

  • Adds some asserts and related arrays to catch when new internal gain types are added. Prompted by 5ac18d6 that was necessary to fix a unit test when the indoor green model added a new gain type.

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

@mjwitte mjwitte added the Defect Includes code to repair a defect in EnergyPlus label Feb 29, 2024
mjwitte
mjwitte commented Mar 6, 2024
View reviewed changes
src/EnergyPlus/DisplacementVentMgr.hh
Comment on lines +121 to +140
DataHeatBalance::IntGainType::RefrigerationTransSysAirCooledGasCooler,
DataHeatBalance::IntGainType::RefrigerationTransSysSuctionPipeMT,
DataHeatBalance::IntGainType::RefrigerationTransSysSuctionPipeLT,
DataHeatBalance::IntGainType::Pump_VarSpeed,
DataHeatBalance::IntGainType::Pump_ConSpeed,
DataHeatBalance::IntGainType::Pump_Cond,
DataHeatBalance::IntGainType::PumpBank_VarSpeed,
DataHeatBalance::IntGainType::PumpBank_ConSpeed,
DataHeatBalance::IntGainType::PlantComponentUserDefined,
DataHeatBalance::IntGainType::CoilUserDefined,
DataHeatBalance::IntGainType::ZoneHVACForcedAirUserDefined,
DataHeatBalance::IntGainType::AirTerminalUserDefined,
DataHeatBalance::IntGainType::PackagedTESCoilTank,
DataHeatBalance::IntGainType::SecCoolingDXCoilSingleSpeed,
DataHeatBalance::IntGainType::SecHeatingDXCoilSingleSpeed,
DataHeatBalance::IntGainType::SecCoolingDXCoilTwoSpeed,
DataHeatBalance::IntGainType::SecCoolingDXCoilMultiSpeed,
DataHeatBalance::IntGainType::SecHeatingDXCoilMultiSpeed,
DataHeatBalance::IntGainType::ElectricLoadCenterConverter,
DataHeatBalance::IntGainType::FanSystemModel};
Copy link
Contributor Author

@mjwitte mjwitte Mar 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Hmmm, seems like these end use types should be included in the occupied zone convective gains rather than be ignored? But maybe I don't understand the intent here.
@EnergyArchmage ?

Copy link
Member

Choose a reason for hiding this comment

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

?? I'll accept it as is if everything is running fine, but open to comments/follow-up changes.

@Myoldmopar Myoldmopar added this to the EnergyPlus 24.1 milestone Mar 12, 2024
Myoldmopar
Myoldmopar approved these changes Mar 15, 2024
View reviewed changes
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.

OK, this looks fine. I'll check CI, and test locally, but it's probably worth merging this purely from the string to enum cleanups alone :)

src/EnergyPlus/DataHeatBalance.hh
@@ -1304,7 +1304,6 @@ namespace DataHeatBalance {
struct GenericComponentZoneIntGainStruct
{
// Members
std::string CompObjectType; // device object class name
Copy link
Member

Choose a reason for hiding this comment

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

Nice, shouldn't need to hang on to these type strings.

src/EnergyPlus/DisplacementVentMgr.cc
@@ -603,27 +601,31 @@ namespace RoomAir {
}
}

ConvGainsOccupiedSubzone = SumInternalConvectionGainsByTypes(state, ZoneNum, IntGainTypesOccupied);
ConvGainsOccupiedSubzone = InternalHeatGains::SumInternalConvectionGainsByTypes(state, ZoneNum, IntGainTypesOccupied);
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the namespace fixing.

src/EnergyPlus/DisplacementVentMgr.cc
ConvGainsMixedSubzone += RetAirGain;
}

ConvGains = ConvGainsOccupiedSubzone + ConvGainsMixedSubzone;

// Make sure all types of internal gains have been gathered
assert((int)(size(IntGainTypesOccupied) + size(IntGainTypesMixedSubzone) + size(ExcludedIntGainTypes)) ==
(int)DataHeatBalance::IntGainType::Num);
Copy link
Member

Choose a reason for hiding this comment

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

OK, this should be fine.

src/EnergyPlus/DisplacementVentMgr.hh
Comment on lines +121 to +140
DataHeatBalance::IntGainType::RefrigerationTransSysAirCooledGasCooler,
DataHeatBalance::IntGainType::RefrigerationTransSysSuctionPipeMT,
DataHeatBalance::IntGainType::RefrigerationTransSysSuctionPipeLT,
DataHeatBalance::IntGainType::Pump_VarSpeed,
DataHeatBalance::IntGainType::Pump_ConSpeed,
DataHeatBalance::IntGainType::Pump_Cond,
DataHeatBalance::IntGainType::PumpBank_VarSpeed,
DataHeatBalance::IntGainType::PumpBank_ConSpeed,
DataHeatBalance::IntGainType::PlantComponentUserDefined,
DataHeatBalance::IntGainType::CoilUserDefined,
DataHeatBalance::IntGainType::ZoneHVACForcedAirUserDefined,
DataHeatBalance::IntGainType::AirTerminalUserDefined,
DataHeatBalance::IntGainType::PackagedTESCoilTank,
DataHeatBalance::IntGainType::SecCoolingDXCoilSingleSpeed,
DataHeatBalance::IntGainType::SecHeatingDXCoilSingleSpeed,
DataHeatBalance::IntGainType::SecCoolingDXCoilTwoSpeed,
DataHeatBalance::IntGainType::SecCoolingDXCoilMultiSpeed,
DataHeatBalance::IntGainType::SecHeatingDXCoilMultiSpeed,
DataHeatBalance::IntGainType::ElectricLoadCenterConverter,
DataHeatBalance::IntGainType::FanSystemModel};
Copy link
Member

Choose a reason for hiding this comment

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

?? I'll accept it as is if everything is running fine, but open to comments/follow-up changes.

src/EnergyPlus/HeatBalanceInternalHeatGains.cc
FoundDuplicate = true;
break;
}
if ((thisIntGain.device(IntGainsNum).CompType == IntGainCompType) &&
Copy link
Member

Choose a reason for hiding this comment

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

Yes!

@Myoldmopar
Copy link
Member

CI was all happy the last time this ran. Everything builds and tests OK with develop pulled in. I'll merge this PR in, and follow-on stuff can continue in a later PR.

@Myoldmopar Myoldmopar merged commit 2960f6b into develop Mar 15, 2024
15 checks passed
@Myoldmopar Myoldmopar deleted the moreRobustIntGainSubtotals branch March 15, 2024 15:50
@mjwitte mjwitte changed the title More robust internal gain subtotals More robust internal gain subtotals for UFAD and Displacement Ventilation Mar 18, 2024
@Myoldmopar Myoldmopar added the LowComplexityApproved Used for subcontractor defect complexity requests label Apr 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Myoldmopar Myoldmopar Myoldmopar approved these changes

Assignees
No one assigned
Labels
Defect Includes code to repair a defect in EnergyPlus LowComplexityApproved Used for subcontractor defect complexity requests
Projects
None yet
Milestone
EnergyPlus 24.1
Development

Successfully merging this pull request may close these issues.
7 participants
@mjwitte @Myoldmopar @nrel-bot @nrel-bot-2 @nrel-bot-3 @nrel-bot-2b @nrel-bot-2c