Skip to content

Conversation

Nigusse
Copy link
Contributor

@Nigusse Nigusse commented Jul 28, 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 Defect Includes code to repair a defect in EnergyPlus label Jul 28, 2025
@Nigusse Nigusse added this to the EnergyPlus 25.2 milestone Jul 28, 2025
Copy link
Contributor Author

@Nigusse Nigusse left a comment

Choose a reason for hiding this comment

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

Variable DX cooling coil is turned off when there is latent load but no sensible load. Need to modify a logic to avoid turning off the VS dx cooling when there is latent load only.

if ((PartLoadRatio > 0.0) || (PartLoadRatio == 0.0 && this->m_CoolingSpeedRatio > 0.0)) {
CompressorOn = HVAC::CompressorOp::On;
this->m_LastMode = CoolingMode;
}
Copy link
Contributor Author

@Nigusse Nigusse Jul 28, 2025

Choose a reason for hiding this comment

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

When there is latent load only (i.e., when there is no sensible load), then the above logic if (PartLoadRatio > 0.0) holds true for single speed DX coil only. For variable speed DX cooling coil, the PLR can be zero when the speed ratio is greater than zero. Thus, it needs to add a second logic to avoid turning off a variable DX cooling coil when there is latent load only.

Copy link
Contributor

Choose a reason for hiding this comment

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

At the bottom of controlCoolingSystemToSP is:

    this->m_CoolingPartLoadFrac = PartLoadFrac;
    this->m_CoolingSpeedRatio = SpeedRatio;
    this->m_CoolingCycRatio = CycRatio;
    this->m_DehumidificationMode = DehumidMode;

I wonder if it would be better to correct this issue here because you would want the UnitarySystem to know what the PartLoadRaio is for control and reporting of m_PartLoadFrac. There is also the calcUnitarySystemToLoad function which may need review to see if an update is needed there as well.

this->m_CoolingPartLoadFrac = std::max(PartLoadFrac, SpeedRatio);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this->m_CoolingPartLoadFrac = std::max(PartLoadFrac, SpeedRatio);

This logic may work but we will be mixing up two variables. The m_CoolingPartLoadFrac and SpeedRatio have a different meaning and resetting the m_CoolingPartLoadFrac that may have un-intended consequence elsewhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

May be use a local variable that does not affect the m_CoolingPartLoadFrac member variable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I understand correctly, for variable speed DX coil, the m_CoolingPartLoadFrac value is set 0.0 whenever the SpeedRatio is > 0.0. @rraustad Do you agree?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's CycRatio that gets set to 0 when SpeedRatio > 0. The VS coil is tricky. I suggest plotting Unitary System Part Load Ratio to see what that shows during latent only operation. Also, CycRatio > 0 at speed = 1 and SpeedRatio > 0 at speed > 1 so my suggestion was not quite right.

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 see on one place where the PLR is set to SpeedRatio this->m_CoolingPartLoadFrac = SpeedRatio; for VS DX cooling coil. Let me examine the report variables (PLR, CyclingRatio and SpeedRatio).

Copy link
Contributor

Choose a reason for hiding this comment

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

That could be a mistake. this->m_CoolingPartLoadFrac is similar to CycRatio and both should be 1 at speed > 1.

PartLoadRatio = this->m_CoolingPartLoadFrac;
CompressorOn = HVAC::CompressorOp::Off;
if (PartLoadRatio > 0.0) {
if ((PartLoadRatio > 0.0) || (PartLoadRatio == 0.0 && this->m_CoolingSpeedRatio > 0.0)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The same change as the above.

@Nigusse
Copy link
Contributor Author

Nigusse commented Jul 28, 2025

The plots below show a DX cooling coil outlet node humidity ratio for a single speed and a variable speed DX coil when the Run on Latent Load input field in CoilSystem:Cooling:DX object is set to Yes. For the variable speed coil, the results are before and after the fix. Before, the fix the VS DX cooling coil is Off when there is latent load only or it is On when there are sensible and latent loads. Whereas the after-fix results show that the VS DX cooling coil is On whenever there is latent load regardless of the presence of a sensible load.

image

@Nigusse
Copy link
Contributor Author

Nigusse commented Jul 28, 2025

The plots below show similar results trend for Total Cooling Rate report variable as well. The after the fix, the variable speed DX cooling coil is On whenever there is latent load regardless of the sensible load. Before the fix, the VS DX cooling coil is On only when there are sensible and latent loads.

image

@Nigusse
Copy link
Contributor Author

Nigusse commented Jul 28, 2025

See the PLR, CyclingRatio and SpeedNumber before and after the fix. In this test case, the SpeedNumber is always > 1 and the cycling ratio in both cases is 1. Note that the PLR is always 1.0 before and after the fix.

image

General::SolveRoot(state, HumRatAcc, MaxIte, SolFla, SpeedRatio, f, 0.0, 1.0);
CycRatio = 0.0;
}
PartLoadFrac = SpeedRatio;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now I reset the PRL in one place using the SpeedRatio and removed the previous code changes.

HVAC::CompressorOp::On);
};
General::SolveRoot(state, HumRatAcc, MaxIte, SolFla, SpeedRatio, f, 0.0, 1.0);
CycRatio = 0.0;
Copy link
Contributor Author

@Nigusse Nigusse Jul 28, 2025

Choose a reason for hiding this comment

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

The CycRatio is reset to 0.0 when the SpeedNumber is = 1.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The CycRatio should be set 1.0 when the SpeedNumber is >1. I will remove the CycRatio = 0.0; statement that added above.

this->controlCoolingSystemToSP(state, AirLoopNum, FirstHVACIteration, HXUnitOn, CompressorOn);
PartLoadRatio = this->m_CoolingPartLoadFrac;
CompressorOn = HVAC::CompressorOp::Off;
// if ((PartLoadRatio > 0.0) || (PartLoadRatio == 0.0 && this->m_CoolingSpeedRatio > 0.0)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This commented out line will be removed.

Copy link

⚠️ Regressions detected on macos-14 for commit 549dd90

Regression Summary
  • EIO: 1
  • ERR: 1
  • ESO Big Diffs: 1
  • Table Big Diffs: 1
  • Table String Diffs: 1

EXPECT_NEAR(0.112564, CoilSys.m_CoolingSpeedRatio, 0.000001);
EXPECT_NEAR(0.008917, CoilSys.m_DesiredOutletHumRat, 0.000001);
EXPECT_NEAR(0.008917, state->dataLoopNodes->Node(CoilSys.AirOutNode).HumRat, 0.000001);
}
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 unit test to demonstrate the fix.

};
General::SolveRoot(state, HumRatAcc, MaxIte, SolFla, SpeedRatio, f, 0.0, 1.0);
}
PartLoadFrac = SpeedRatio;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The SpeedRatio is saved in a local variable which is then assigned to this->m_CoolingPartLoadFrac.

Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure where this->m_CoolingPartLoadFrac = SpeedRatio is ever used. If the speedNum > 1 then the coil never turns off during the time step and this->m_CoolingPartLoadFrac = 1. So this line should be:

    PartLoadRatio = CycRatio;

At the bottom of this function:

    this->m_CoolingPartLoadFrac = PartLoadFrac;
    this->m_CoolingSpeedRatio = SpeedRatio;
    this->m_CoolingCycRatio = CycRatio;
    this->m_DehumidificationMode = DehumidMode;

Copy link
Contributor

@rraustad rraustad Jul 30, 2025

Choose a reason for hiding this comment

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

Oh, I see it now. It's used a lot in the MuiltStage electric heater. I think that's a mistake. Here speedNum > 1 so the heating coil never turns off during the time step.

case HVAC::Coil_HeatingElectric_MultiStage: {
    if (this->m_SuppHeatingSpeedNum > 1) {
        auto f = [&state, this, DesOutTemp, CycRatio](Real64 const SpeedRatio) {
            return UnitarySys::heatingCoilVarSpeedResidual(state,
                                                           SpeedRatio,
                                                           this->m_SuppHeatCoilIndex,
                                                           DesOutTemp,
                                                           this->m_UnitarySysNum,
                                                           CycRatio,
                                                           this->m_SuppHeatingSpeedNum,
                                                           this->m_FanOpMode,
                                                           HVAC::CompressorOp::On,
                                                           true);
        };
        General::SolveRoot(state, Acc, MaxIte, SolFla, SpeedRatio, f, 0.0, 1.0);
        PartLoadFrac = SpeedRatio;


this->m_SuppHeatPartLoadFrac = PartLoadFrac;
this->m_SuppHeatingCycRatio = CycRatio;
this->m_SuppHeatingSpeedRatio = SpeedRatio;

Copy link

⚠️ Regressions detected on macos-14 for commit 9d2cbf2

Regression Summary
  • EIO: 1
  • ERR: 1
  • ESO Big Diffs: 1
  • Table Big Diffs: 1
  • Table String Diffs: 1

@nrel-bot-2c
Copy link

@Nigusse @Myoldmopar it has been 30 days since this pull request was last updated.

@mitchute mitchute self-requested a review September 24, 2025 18:38
@mitchute
Copy link
Collaborator

This looks OK to me. @rraustad any final comments?

Copy link

⚠️ Regressions detected on macos-14 for commit 9946d93

Regression Summary
  • EIO: 1
  • ERR: 1
  • ESO Big Diffs: 1
  • Table Big Diffs: 1
  • Table String Diffs: 1

@rraustad
Copy link
Contributor

@mitchute the code change appears to correct the issue. CI failures are unrelated. OK to merge.

@mitchute mitchute merged commit 8fc445e into develop Sep 29, 2025
10 of 11 checks passed
@mitchute mitchute deleted the Issue10765_vs_dx_cooling_coil branch September 29, 2025 14:34
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.

Coil:Cooling:DX:VariableSpeed does not run on latent only load

5 participants