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.

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

Variable flow condenser plant control #10511

Merged
merged 23 commits into from
Jul 5, 2024
Merged

Variable flow condenser plant control #10511

merged 23 commits into from
Jul 5, 2024

Conversation

lymereJ
Copy link
Collaborator

@lymereJ lymereJ commented May 17, 2024

Pull request overview

NOTE: ENHANCEMENTS MUST FOLLOW A SUBMISSION PROCESS INCLUDING A FEATURE PROPOSAL AND DESIGN DOCUMENT PRIOR TO SUBMITTING CODE

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 structural output changes, add to output rules file and add OutputChange label

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

@lymereJ lymereJ added NewFeature Includes code to add a new feature to EnergyPlus IDDChange Code changes impact the IDD file (cannot be merged after IO freeze) labels May 17, 2024
@Myoldmopar Myoldmopar self-assigned this May 22, 2024
@lymereJ lymereJ marked this pull request as draft May 31, 2024 23:04
this->CondMassFlowRate = this->CondMassFlowRateMax;
} break;
case DataPlant::CondenserFlowControl::ModulatedChillerPLR: {
this->CondMassFlowRate = this->CondMassFlowRateMax * PartLoadRat;
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be out of scope for you, but in my custom version I add in limit for what the variable speed pump can actually do for a minimum. Could be a loop pump or a branch pump on this chiller.

Here is how I have this bit coded in custom version

                if (this->VSBranchPumpFoundCond) {                                                                         // TRANE
                    if (MyLoad < 0.0 && RunFlag) {                                                                         // TRANE
                        this->CondMassFlowRate = this->CondMassFlowRateMax * std::max(PartLoadRat, this->MinPartLoadRat);  // TRANE
                        this->CondMassFlowRate = std::max(this->CondMassFlowRate, this->VSBranchPumpMinLimitMassFlowCond); // TRANE
                    } else {                                                                                               // TRANE
                        this->CondMassFlowRate = 0.0;                                                                      // TRANE
                    }                                                                                                      // TRANE
                } else if (this->VSLoopPumpFoundCond) {                                                                    // TRANE
                    if (MyLoad < 0.0 && RunFlag) {                                                                         // TRANE
                        this->CondMassFlowRate = this->CondMassFlowRateMax * std::max(PartLoadRat, this->MinPartLoadRat);  // TRANE
                    } else {                                                                                               // TRANE
                        this->CondMassFlowRate = 0.0;                                                                      // TRANE
                    }                                                                                                      // TRANE
                }                                                                                                          // TRANE

where branch pump limit and bools get filled like this

            this->VSBranchPumpMinLimitMassFlowCond =
                PlantUtilities::MinFlowIfBranchHasVSPump(state, this->CDPlantLoc, this->VSBranchPumpFoundCond, this->VSLoopPumpFoundCond, false);
            if (this->VSLoopPumpFoundCond) {
                this->VSLoopPumpMinLimitMassFlowCond = this->VSBranchPumpMinLimitMassFlowCond;
                this->VSBranchPumpMinLimitMassFlowCond = 0.0;
            }

I think these pump limits would apply to all three methods.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks! I have a Condenser Minimum Flow Fraction input as part of the proposed changes but I guess it couldn't hurt to make sure that the pump minimum flow rate isn't less than the flow rate calculated from the fraction, I'll add that.

@lymereJ lymereJ added the OutputChange Code changes output in such a way that it cannot be merged after IO freeze label Jun 13, 2024
@lymereJ
Copy link
Collaborator Author

lymereJ commented Jun 13, 2024

Annual simulation results with the new control options:

  • ConstantFlow:

image

  • ModulatedChillerPLR:

image

  • ModulatedLoopPLR (with a CWFR = 1 * PLR curve and a minimum flow fraction of 0.35):

image

  • ModulatedDeltaTemperature (vertical bar correspond to cycling chiller operation):

image

Copy link
Collaborator Author

@lymereJ lymereJ left a comment

Choose a reason for hiding this comment

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

Code changes walk-through!

Comment on lines +1351 to +1353
// get minimum condenser plant loop pump mass flow rate
this->VSBranchPumpMinLimitMassFlowCond =
PlantUtilities::MinFlowIfBranchHasVSPump(state, this->CDPlantLoc, this->VSBranchPumpFoundCond, this->VSLoopPumpFoundCond, false);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Get minimum pump flow rate as suggested by @EnergyArchmage.

Copy link
Member

Choose a reason for hiding this comment

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

👍

@@ -655,6 +655,57 @@ void GetElectricEIRChillerInput(EnergyPlusData &state)
}
}

if (NumAlphas > 16) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Getting the new inputs.

@@ -1470,7 +1524,7 @@ void ElectricEIRChillerSpecs::size(EnergyPlusData &state)

Real64 rho = FluidProperties::GetDensityGlycol(state,
state.dataPlnt->PlantLoop(this->CDPlantLoc.loopNum).FluidName,
Constant::CWInitConvTemp,
this->TempRefCondIn,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Using a chilled water seemed incorrect since we dealing with a condenser loop. Also, it was inconsistent with the Cp below.

Copy link
Member

Choose a reason for hiding this comment

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

👍

Comment on lines +2366 to +2372
Real64 chwLoopCap = state.dataSize->PlantSizData(PltSizNum).DesCapacity;
Real64 chwLoopDemand =
std::abs(state.dataPlnt->PlantLoop(this->CWPlantLoc.loopNum).LoopSide(this->CWPlantLoc.loopSideNum).UpdatedDemandToLoopSetPoint);
Real64 cwhLoopPLR = 0.0;
if (chwLoopDemand > 0) {
cwhLoopPLR = chwLoopDemand / chwLoopCap;
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Calculation of the CWH loop PLR using the Sizing:Plant objects, an error is thrown if these objects are not included.

Comment on lines 2409 to 2411
this->CondMassFlowRate = max(min(this->CondMassFlowRate, this->CondMassFlowRateMax),
this->MinCondFlowRatio * this->CondMassFlowRateMax,
this->VSBranchPumpMinLimitMassFlowCond);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Apply the user-specified minimum flow fraction and consider the pump minimum flow rate as well.

Copy link
Member

Choose a reason for hiding this comment

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

So this is the max of three things? I wasn't expecting that. The logic here is fine, but I wouldn't mind this broken into an intermediate variable or two. The combined min/max with multiplication in the middle makes it difficult to spot a bug. The MinCondFlowRatio * CondMassFlowRateMax could be a single variable. Then the min(.., ..) portion could be a single named variable. Then the max of all three would be clear. This would be a nice change.

@@ -628,6 +628,57 @@ void GetElecReformEIRChillerInput(EnergyPlusData &state)
} else {
thisChiller.EndUseSubcategory = "General";
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Pretty much the same changes are applied to this object.

Comment on lines +3266 to +3268
if (state.dataPlnt->PlantFinalSizesOkayToReport) {
BaseSizer::reportSizerOutput(state, "PlantLoop", state.dataPlnt->PlantLoop(LoopNum).Name, "Design Capacity [W]", DesignPlantCapacity);
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

New EIO output which is useful to check the chilled water plant loop PLR used in the ModulatedLoopPLR control option.

foundLoopPump = true;
if (component.CompNum > 0) branchPumpMinFlowLimit = state.dataPumps->PumpEquip(component.CompNum).MassFlowRateMin;
break;
if (state.dataPlnt->PlantLoop(plantLoc.loopNum).LoopSide(DataPlant::LoopSideLocation::Supply).TotalBranches > 1) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Catches when there are no supply branches. I run into that issue for the unit test, I don't think this should be an issue in actual simulations...

Copy link
Member

Choose a reason for hiding this comment

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

Yeah...probably not. :)

@@ -371,3 +371,166 @@ TEST_F(EnergyPlusFixture, ChillerElectricEIR_EvaporativelyCooled_Calculate)
EXPECT_NEAR(6.22019725E-06, EvapCondWaterVolFlowRate, 0.000000001);
EXPECT_NEAR(EvapCondWaterVolFlowRate, thisEIRChiller.EvapWaterConsumpRate, 0.000000001);
}

TEST_F(EnergyPlusFixture, ChillerElectricEIR_WaterCooledChillerVariableSpeedCondenser)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

New tests for both chiller models that tests all three new control option as well as the minimum condenser flow fraction.

@lymereJ
Copy link
Collaborator Author

lymereJ commented Jun 13, 2024

Diff review:

  • 26a11b9 shows only AUD/EIO diffs which are due to the new inputs (with their defaults) and the new EIO output (PlantLoop Design Capacity [W])
  • c34186e includes the same diffs and others (ERR, MTR, etc...) which are all due to the simple change in that commit which I think is justified.

So all diffs are, I think, expected.

@lymereJ lymereJ marked this pull request as ready for review June 13, 2024 20:46
@lymereJ lymereJ added this to the EnergyPlus 24.2 IOFreeze milestone Jun 13, 2024
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.

Code changes are fine. I made some comments that you can act on or not, they aren't major concerns. The one thing that does need fixed is the LaTeX errors. Let me know if you have trouble identifying them.

@@ -2006,3 +2006,60 @@ \subsubsection{Chiller Basin Heater}\label{chiller-basin-heater-3}
\emph{Schedule\(_{heater,basin}\)} is the basin heater schedule, user input (schedule value \textgreater{} 0 means ON)

\emph{ChillerIsOFF} is the logical variable denoting that the chiller is not operating for the current simulation time step (e.g.,~ there is no cooling load to be met by the chiller, or if there is no water flow through the chiller due to a chiller or pump availability schedule, etc.).

\subsection{Variable Flow Condenser}\label{hot-water-heat-recovery-from-chillers}
Copy link
Member

Choose a reason for hiding this comment

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

This is a duplicate label causing a LaTeX warning. Give it a more meaningful label, or if you aren't going to reference it anywhere, just take off the label specification.

\({CAP_{CHWL}}\) is the design chilled water loop capacity determined using inputs from the Sizing:Plant object for the loop


The condenser plant loop water flow fraction (\({CWFR}\)) corresponding to the \({PLR_{CWHL}}\) is obtained by feeding it through a user-specified curve. A linear curve is expected: \(CWFR = C * {PLR_{CHWL}} + D\) where C and D are coefficients. A reference for these coefficients is provided in ``Optimizing Design & Control Of Chilled Water Plants, Part 5'', S. Taylor, ASHRAE Journal June 2012.
Copy link
Member

Choose a reason for hiding this comment

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

The & symbol needs to be escaped in this sentence, it is causing a LaTeX error.

@@ -2131,6 +2148,22 @@ \subsubsection{Inputs}\label{inputs-5-021}

This optional field allows you to specify a user-defined end-use subcategory, e.g., ``Process''. A new meter for reporting is created for each unique subcategory (ref: \hyperref[outputmeter-and-outputmetermeterfileonly]{Output:Meter} objects). Any text may be used here to further subcategorize the end-uses in the ABUPS End Uses by Subcategory table and in the LEED Summary EAp2-4/5 Performance Rating Method Compliance table. If this field is omitted or blank, the chiller will be assigned to the ``General'' end-use subcategory.

\paragraph{Field: Condenser Flow Control}\label{field-condenser-flow-control}
Copy link
Member

Choose a reason for hiding this comment

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

This label is multiply defined, give it a variant name or just remove the label spec if you aren't going to reference it anywhere.


This field is optional.~ It describes the chiller condenser flow request mode to be used in a simulation. With ``ConstantFlow'' a chiller will always request its maximum condenser flow rate. With ``ModulatedChillerPLR'' the condenser flow request corresponds to the chiller part load ratio multiplied by the chiller maximum condenser flow rate. With ``ModulatedLoopPLR'' the chiller will request a flow rate that is function of the chilled water loop's part load ratio, see the \hyperref[field-condenser-loop-flow-rate-function-of-loop-part-load-ratio-curve-name]{Condenser Loop Flow Rate Fraction Function of Loop Part Load Ratio Curve Name} input. With ``ModulatedDeltaTemperature'' the chiller will request the flow rate required to meet a condenser loop delta temperature, see the \hyperref[field-temperature-difference-across-condenser-schedule-name]{Temperature Difference Across Condenser Schedule Name} input. Use ``ConstantFlow'' when modeling a constant flow condenser plant loop, choose one of the other inputs when modeling a variable flow condenser plant loop. The default is ``ConstantFlow''.

\paragraph{Field: Condenser Loop Flow Rate Fraction Function of Loop Part Load Ratio Curve Name}\label{field-condenser-loop-flow-rate-function-of-loop-part-load-ratio-curve-name}
Copy link
Member

Choose a reason for hiding this comment

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

This label is multiply defined, give it a variant name or just remove the label spec if you aren't going to reference it anywhere.


This field describes the condenser loop flow rate fraction as a function of the chiller water loop part load ratio. A linear curve is expected: CWFR = C * PLR + D where CWFR is the condenser water flow fraction (actual/design), PLR is the chilled water plant loop part load ratio (actual/design), and C and D are coefficients. A reference for these coefficients is provided in ``Optimizing Design & Control Of Chilled Water Plants, Part 5'', S. Taylor, ASHRAE Journal June 2012. This input is only used when the ``ModulatedLoopPLR'' condenser flow control option is used.

\paragraph{Field: Temperature Difference Across Condenser Schedule Name}\label{field-temperature-difference-across-condenser-schedule-name}
Copy link
Member

Choose a reason for hiding this comment

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

This label is multiply defined, give it a variant name or just remove the label spec if you aren't going to reference it anywhere.

thisChiller.MinCondFlowRatio = state.dataIPShortCut->rNumericArgs(19);
} else {
thisChiller.MinCondFlowRatio = 0.2;
}
Copy link
Member

Choose a reason for hiding this comment

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

Several more of these could be simplified if the default value was just assigned on the variable at the class definition. Not necessary, but you could end up with 10-15 lines removed just from that change.

@@ -1470,7 +1524,7 @@ void ElectricEIRChillerSpecs::size(EnergyPlusData &state)

Real64 rho = FluidProperties::GetDensityGlycol(state,
state.dataPlnt->PlantLoop(this->CDPlantLoc.loopNum).FluidName,
Constant::CWInitConvTemp,
this->TempRefCondIn,
Copy link
Member

Choose a reason for hiding this comment

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

👍

Comment on lines +1351 to +1353
// get minimum condenser plant loop pump mass flow rate
this->VSBranchPumpMinLimitMassFlowCond =
PlantUtilities::MinFlowIfBranchHasVSPump(state, this->CDPlantLoc, this->VSBranchPumpFoundCond, this->VSLoopPumpFoundCond, false);
Copy link
Member

Choose a reason for hiding this comment

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

👍

Comment on lines 2409 to 2411
this->CondMassFlowRate = max(min(this->CondMassFlowRate, this->CondMassFlowRateMax),
this->MinCondFlowRatio * this->CondMassFlowRateMax,
this->VSBranchPumpMinLimitMassFlowCond);
Copy link
Member

Choose a reason for hiding this comment

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

So this is the max of three things? I wasn't expecting that. The logic here is fine, but I wouldn't mind this broken into an intermediate variable or two. The combined min/max with multiplication in the middle makes it difficult to spot a bug. The MinCondFlowRatio * CondMassFlowRateMax could be a single variable. Then the min(.., ..) portion could be a single named variable. Then the max of all three would be clear. This would be a nice change.

foundLoopPump = true;
if (component.CompNum > 0) branchPumpMinFlowLimit = state.dataPumps->PumpEquip(component.CompNum).MassFlowRateMin;
break;
if (state.dataPlnt->PlantLoop(plantLoc.loopNum).LoopSide(DataPlant::LoopSideLocation::Supply).TotalBranches > 1) {
Copy link
Member

Choose a reason for hiding this comment

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

Yeah...probably not. :)

@lymereJ
Copy link
Collaborator Author

lymereJ commented Jul 3, 2024

@Myoldmopar - I think that I've address all your comments, please let me know if I missed something, thanks! I ran tests locally and they all passed, hopefully it'll be the same here.

@Myoldmopar
Copy link
Member

@lymereJ there were still a few doc issues I had to clean up. While I was in there I added a small totally unrelated commit to quiet up a few advanced cmake vars, but that won't affect anything. I will plan on merging this shortly once the doc build finishes. Thanks.

@Myoldmopar
Copy link
Member

Docs are happy now, merging.

@Myoldmopar Myoldmopar merged commit 1033bdc into develop Jul 5, 2024
10 checks passed
@Myoldmopar Myoldmopar deleted the var_speed_cond_ctrl branch July 5, 2024 14:52
Copy link
Contributor

@jmarrec jmarrec left a comment

Choose a reason for hiding this comment

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

Again super late to the party (sorry about that), but I'm bother by the Min Cond Flow field defaulting in the C++ code and IDD having neither default nor required field.

not a big deal, I'm just saying.

int ChillerCondLoopFlowFLoopPLRIndex = 0; // Condenser loop flow rate fraction function of loop PLR
int CondDT = 0; // Temperature difference across condenser
int CondDTScheduleNum = 0; // Temperature difference across condenser schedule index
Real64 MinCondFlowRatio = 0.2; // Minimum condenser flow fraction
Copy link
Contributor

Choose a reason for hiding this comment

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

This defaults to 0.2

Copy link
Contributor

Choose a reason for hiding this comment

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

In the good old days when one could edit the IDD and change the default this sort thing made sense to me. Now that the IDD is hard coded in the JSON stuff what is the difference if it is hard coded in C++ or JSON? Just saying

RTFM?
image

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 documentation and clarity to declare it in the IDD ('cause who reads the documentation for that?). If a blank numeric doesn't default to zero, it should have a default declared in the IDD. IDF Editor, transition, and possibly other tools may fill in that default value.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Addressed in #10779.

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, IDD as documentation makes good sense. But I am curious if it is okay to hardcode the default as initialization in C++ variable declaration or if the preferred practice is to handle the default value coming from input processor.

Copy link
Contributor

Choose a reason for hiding this comment

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

Totally agree with @mjwitte here.

Also, I've made that point several times over the years, but in my opinion, every field should be either \required-field or \default unless there's a very good reason (examples: no default value is appropriate, and it's a field that's only used based on some other choice field; or an optional loop inlet/outlet nodes). This is very helpful for anyone consuming the IDD (such as OpenStudio SDK, and IDF / EPJSON editors).

\mininimum and \maximum and the \type attributes should be used as much as possible on the fields too.

And \extensible should be used on the object itself everywhere it's appropriate.

The IDD is a schema (and the fact it's translated into the epJSON schema makes that indisputable), so it should be as good as possible, without having to RTFM or even worse go through the C++ code


Note: In the OpenStudio SDK:

  • We have an our own IDD for model, and we have a modified Energy+.idd for workspace (IDF), but when a new E+ version comes out, we take the official E+ idd as a basis to bring in changes. Everytime the IDD changes are lacking proper flags, we have to read the documentation AND go through the C++ code.
    • We know that the IDD is lacking proper flags because we're more stringent with them:
      • The primary reason is that we have C++ code generators scripts that will read the IDD and implement getters/setters differently based on whether a field is required, defaulted, or nothing.
      • The second reason is that we try hard to avoid people shooting themselves in the foot
  • We differentiate between a defaulted value and a user-set value (the getter returns either the defaulted or user-set value, but you have a bool isValueDefaulted() and a resetValue() method as well, and interfaces such as the OpenStudioApp will display that field differently (green if defaulted for eg))
  • More often than not (for HVAC objects especially), we turn a \default xxx field in a \required-field and hardcode the value in the object's ctor, as this simplifies the API (saves writing and maintaining a getter and a resetter)

Comment on lines +73588 to +73592
N19; \field Condenser Minimum Flow Fraction
\note This input corresponds to the minimum flow fraction to be simulated. The minimum condenser flow
\note corresponds to this fraction multiplied by the maximum condenser flow rate. This input is only used
\note when the "Condenser Flow Control" input is set to "ModulatedChillerPLR", "ModulatedLoopPLR" or
\note "ModulatedDeltaTemperature".
Copy link
Contributor

Choose a reason for hiding this comment

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

It bothers me that this field is neither \required-field nor \default 0.2

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 OutputChange Code changes output in such a way that it cannot be merged after IO freeze
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Chiller always requests full condenser flow regardless of flow mode or pump type