Skip to content

Conversation

Nigusse
Copy link
Contributor

@Nigusse Nigusse commented Mar 27, 2025

Pull request overview

Description of the purpose of this PR

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

@Nigusse Nigusse added the NewFeature Includes code to add a new feature to EnergyPlus label Mar 27, 2025
Copy link

⚠️ Regressions detected on macos-14 for commit b8abace

Regression Summary
  • BND: 12
  • MTD: 6
  • MTR Small Diffs: 3
  • Table Big Diffs: 2
  • Table String Diffs: 8
  • ERR: 6
  • ESO Small Diffs: 2

Copy link

⚠️ Regressions detected on macos-14 for commit 0b5e470

Regression Summary
  • BND: 12
  • MTD: 6
  • MTR Small Diffs: 3
  • Table Big Diffs: 2
  • Table String Diffs: 8
  • ERR: 6
  • ESO Small Diffs: 2

Copy link
Collaborator

@amirroth amirroth left a comment

Choose a reason for hiding this comment

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

Look at my suggested changes and try to propagate.

@@ -2979,895 +2979,978 @@ void GetDXCoils(EnergyPlusData &state)

// Loop over the Pumped DX Water Heater Coils and get & load the data
CurrentModuleObject = HVAC::cAllCoilTypes(HVAC::CoilDX_HeatPumpWaterHeaterPumped);
for (DXHPWaterHeaterCoilNum = 1; DXHPWaterHeaterCoilNum <= state.dataDXCoils->NumDXHeatPumpWaterHeaterPumpedCoils; ++DXHPWaterHeaterCoilNum) {
auto &s_ip = state.dataInputProcessing->inputProcessor;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I have a giant coil branch in the hopper and there are going to be a lot of conflicts. Just a matter of which one of us will have to fix them. 😄

thisDXCoil.RatedAirVolFlowRate(1) = Numbers(7);
if (thisDXCoil.RatedAirVolFlowRate(1) != Constant::AutoCalculate) {
if (thisDXCoil.RatedAirVolFlowRate(1) <= 0.0) {
std::string const cRatedHeatingCapFieldName = "Rated Heating Capacity";
Copy link
Collaborator

Choose a reason for hiding this comment

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

This can be a constexpr std::string_view. There is no need to construct a dynamic string object at runtime to represent a constant string.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

cFieldName = "Evaporator Fan Power Included in Rated COP";
std::string fieldValue = s_ip->getAlphaFieldValue(fields, schemaProps, "evaporator_fan_power_included_in_rated_cop"); // Alphas(2)
BooleanSwitch fanPowerIncluded = static_cast<BooleanSwitch>(getYesNoValue(Util::makeUPPER(fieldValue)));
switch (fanPowerIncluded) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

A cleaner way to do this. If you are just making a single comparison, if is better/faster than switch. Note the ShowSevereInvalidBool error wrapper.

if (fanPowerIncluded != BooleanSwitch::Invalid) {
    thisDXCoil.FanPowerIncludedInCOP = static_cast<bool>(fanPowerIncluded);
} else {
     ShowSevereInvalidBool(state, eoh, cFieldName, fieldValue);
     ErrorsFound = true;
}

Copy link
Contributor

@rraustad rraustad Mar 27, 2025

Choose a reason for hiding this comment

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

What makes you believe that fieldValue will be anything other than Yes or No? This just seems to be overhead that's not needed. It's a direct read. No warning message, no gymnastics, just read that input to be true or false.

A2 , \field Evaporator Fan Power Included in Rated COP
\type choice
\key Yes
\key No
\default Yes

So what would that 1 line be? I'll paste together what I see here but am probably wrong. 1 line and done.

thisDXCoil.FanPowerIncludedInCOP = static_cast<bool>(static_cast<BooleanSwitch>(getYesNoValue(Util::makeUPPER(fieldValue))));

Copy link
Collaborator

Choose a reason for hiding this comment

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

This seems to be your corner. That would work. You actually don't need the static_cast<BooleanSwitch>.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

if (!whPLFCurveName.empty()) {
thisDXCoil.CrankcaseHeaterCapacityCurveIndex = Curve::GetCurveIndex(state, whPLFCurveName);
if (thisDXCoil.CrankcaseHeaterCapacityCurveIndex == 0) { // can't find the curve
ShowSevereError(state, format("{} = {}: {} not found = {}", CurrentModuleObject, thisDXCoil.Name, cFieldName, whPLFCurveName));
Copy link
Collaborator

Choose a reason for hiding this comment

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

There is also an error wrapper for missing objects.

ShowSevereItemNotFound(state, eoh, cFieldName, whPLFCurveName);`

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

if (!fieldValue.empty()) {
thisDXCoil.HCapFAirFlow = GetCurveIndex(state, fieldValue);
if (thisDXCoil.HCapFAirFlow == 0) {
ShowSevereError(state, format("{}{}=\"{}\", invalid", RoutineName, CurrentModuleObject, thisDXCoil.Name));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Another ShowSevereItemNotFound. See UtilityRoutines for wrappers for the common errors, e.g., EmptyField, InvalidKey, DuplicateName, BadMin.

@@ -14988,31 +15071,29 @@ void GetFanIndexForTwoSpeedCoil(
int BranchNum;
int CompNum;

auto &thisDXCoil = state.dataDXCoils->DXCoil(CoolingCoilIndex);

FoundBranch = 0;
FoundAirSysNum = 0;
SupplyFanIndex = 0;
SupplyFanName = "n/a";
for (AirSysNum = 1; AirSysNum <= NumPrimaryAirSys; ++AirSysNum) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

While you are at it, may want to downscope the loop induction variables.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@@ -172,20 +172,22 @@ namespace VariableSpeedCoils {
SpeedCal = SpeedNum;
}

if ((state.dataVariableSpeedCoils->VarSpeedCoil(DXCoilNum).VSCoilType == HVAC::Coil_CoolingWaterToAirHPVSEquationFit) ||
(state.dataVariableSpeedCoils->VarSpeedCoil(DXCoilNum).VSCoilType == HVAC::Coil_CoolingAirToAirVariableSpeed)) {
auto &varSpeedCoil = state.dataVariableSpeedCoils->VarSpeedCoil(DXCoilNum);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Good lord, the conflicts are going to be massive.

ShowContinueError(state, format("...required {} is blank.", cAlphaFields(AlfaFieldIncre)));
std::string fieldName;
for (int I = 1; I <= varSpeedCoil.NumOfSpeeds; ++I) {
fieldName = "speed_" + std::to_string(I) + "_reference_unit_gross_rated_total_cooling_capacity";
Copy link
Collaborator

Choose a reason for hiding this comment

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

format instead of string concatenate.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

}
//"defrost_control",
cFieldName = "Defrost Control"; // cAlphaFields(8)
std::string defrostControl = s_ip->getAlphaFieldValue(fields, schemaProps, "defrost_control");
Copy link
Collaborator

Choose a reason for hiding this comment

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

getEnumValue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

// SUBROUTINE LOCAL VARIABLE DECLARATIONS:
int HPNum; // The Water to Air HP that you are currently loading input into
int NumCool;
int NumHeat;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wish I would have told you to hold off on this until I was done with what I was doing. Too late. Get this merged and I will deal with it. Lesson learned.

@Nigusse
Copy link
Contributor Author

Nigusse commented Mar 27, 2025

@amirroth Thanks for your feedback comments; I will address them. This can be merged early, as the new feature will be based on the work from this branch.

Copy link

⚠️ Regressions detected on ubuntu-24.04 for commit 57daecb

Regression Summary
  • BND: 12
  • MTD: 6
  • MTR Small Diffs: 4
  • ESO Small Diffs: 3
  • Table Small Diffs: 1
  • ERR: 6
  • IDF: 2

@mitchute
Copy link
Collaborator

mitchute commented Oct 8, 2025

@Nigusse are you still intending this to be included in this release?

@Nigusse
Copy link
Contributor Author

Nigusse commented Oct 8, 2025

@mitchute Yes, it has been ready for review. I will resolve the conflicts and push an updated branch by today.

@Nigusse
Copy link
Contributor Author

Nigusse commented Oct 8, 2025

@Nigusse can you explain these new warnings in the err file? This is for HeatPumpWaterHeater:

+   ************* There are 6 unused objects in input.
+   ************* Use Output:Diagnostics,DisplayUnusedObjects; to see them.

@rraustad I will check this.

Copy link

github-actions bot commented Oct 8, 2025

⚠️ Regressions detected on ubuntu-24.04 for commit 105387c

Regression Summary
  • BND: 12
  • MTD: 6
  • MTR Small Diffs: 4
  • EIO: 1
  • ESO Small Diffs: 3
  • Table Small Diffs: 1
  • ERR: 6
  • IDF: 2

@Nigusse
Copy link
Contributor Author

Nigusse commented Oct 8, 2025

Below is the unused objects error message running the example file locally:

   ** Warning ** The following lines are "Unused Objects".  These objects are in the input
   **   ~~~   **  file but are never obtained by the simulation and therefore are NOT used.
   **   ~~~   **  Each unused object is shown.
   **   ~~~   **  See InputOutputReference document for more details.
   ************* Object=Coil:WaterHeating:AirToWaterHeatPump:Pumped=HPWHDXCoil
   **   ~~~   ** Object=Coil:WaterHeating:AirToWaterHeatPump:Pumped=HPWHOutdoorDXCoil
   **   ~~~   ** Object=Coil:WaterHeating:AirToWaterHeatPump:Pumped=HPWHPlantDXCoil
   **   ~~~   ** Object=Coil:WaterHeating:AirToWaterHeatPump:Pumped=HPWHScheduledDXCoil
   **   ~~~   ** Object=Coil:WaterHeating:AirToWaterHeatPump:Pumped=Zone4HPWHDXCoil
   **   ~~~   ** Object=Coil:WaterHeating:AirToWaterHeatPump:Pumped=Zone4bHPWHDXCoil
   ** Warning ** The following schedule names are "Unused Schedules".  These schedules are in the idf
   **   ~~~   **  file but are never obtained by the simulation and therefore are NOT used.
   ************* Schedule:Year or Schedule:Compact or Schedule:File or Schedule:Constant=SSBWATERTEMPSCHEDULE

I am going to remove the unused schedule SSBWATERTEMPSCHEDULE object.

Copy link

github-actions bot commented Oct 8, 2025

⚠️ Regressions detected on ubuntu-24.04 for commit 8923c57

Regression Summary
  • BND: 12
  • MTD: 6
  • MTR Small Diffs: 4
  • EIO: 1
  • ESO Small Diffs: 3
  • Table Small Diffs: 1
  • Audit: 1
  • ERR: 6
  • IDF: 2

@mitchute
Copy link
Collaborator

mitchute commented Oct 8, 2025

@Nigusse I have the Mac build errors fixed locally. I can push them up if you'd like.

@mitchute
Copy link
Collaborator

mitchute commented Oct 8, 2025

@Nigusse I went ahead and cleaned up those Mac build issues, and ran unit tests locally. All passed.

Copy link

github-actions bot commented Oct 8, 2025

⚠️ Regressions detected on ubuntu-24.04 for commit e9ee899

Regression Summary
  • BND: 12
  • MTD: 6
  • MTR Small Diffs: 4
  • EIO: 1
  • ESO Small Diffs: 3
  • Table Small Diffs: 1
  • Audit: 1
  • ERR: 6
  • IDF: 2

Copy link

github-actions bot commented Oct 8, 2025

⚠️ Regressions detected on macos-14 for commit e9ee899

Regression Summary
  • BND: 12
  • MTD: 6
  • MTR Small Diffs: 3
  • Audit: 1
  • ERR: 6
  • IDF: 2
  • ESO Small Diffs: 2

@Nigusse
Copy link
Contributor Author

Nigusse commented Oct 9, 2025

@mitchute Thanks.

@mitchute
Copy link
Collaborator

mitchute commented Oct 9, 2025

This looks ready. Anything else here?

@rraustad
Copy link
Contributor

rraustad commented Oct 9, 2025

Doesn't look like this comment was addressed but it's an easy 1-line fix.

image

@Nigusse
Copy link
Contributor Author

Nigusse commented Oct 9, 2025

@mitchute @rraustad I will address it shortly.

@Nigusse
Copy link
Contributor Author

Nigusse commented Oct 9, 2025

I have found that the same fatal error message is missing in other DX Coils as well. I am addressing them all.

Copy link

github-actions bot commented Oct 9, 2025

⚠️ Regressions detected on ubuntu-24.04 for commit 966ff13

Regression Summary
  • BND: 12
  • MTD: 6
  • MTR Small Diffs: 4
  • EIO: 1
  • ESO Small Diffs: 3
  • Table Small Diffs: 1
  • Audit: 1
  • ERR: 6
  • IDF: 2

@rraustad
Copy link
Contributor

rraustad commented Oct 9, 2025

@mitchute if this comes back clean then it should be ready to merge. We've pounded on this one for some time now.

@mitchute
Copy link
Collaborator

mitchute commented Oct 9, 2025

@mitchute if this comes back clean then it should be ready to merge. We've pounded on this one for some time now.

Thanks @rraustad @Nigusse and all involved. Hope to merge this soon.

Copy link

github-actions bot commented Oct 9, 2025

⚠️ Regressions detected on macos-14 for commit 966ff13

Regression Summary
  • BND: 12
  • MTD: 6
  • MTR Small Diffs: 3
  • Audit: 1
  • ERR: 6
  • IDF: 2
  • ESO Small Diffs: 2

@mitchute
Copy link
Collaborator

mitchute commented Oct 9, 2025

Some regressions, as expected, but this is clean otherwise. Merging.

@mitchute mitchute merged commit 2ab91ec into develop Oct 9, 2025
11 checks passed
@mitchute mitchute deleted the avail_sch_missing_DX_coils_cleanup_getInput branch October 9, 2025 21:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

IDDChange Code changes impact the IDD file (cannot be merged after IO freeze) NewFeature Includes code to add a new feature to EnergyPlus

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Availability schedule in Coil:Cooling:DX object is not working

9 participants