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 speed fan powered induction boxes #10368

Open
wants to merge 37 commits into
base: develop
Choose a base branch
from
Open

Conversation

lymereJ
Copy link
Collaborator

@lymereJ lymereJ commented Jan 13, 2024

Pull request overview

Implement variable speed fan powered induction boxes. Follows the approach of #10336 and supersedes it.

Simulation results for the different controls are shown here (see the .ipnyb file).

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
  • 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

@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 Jan 13, 2024
@lymereJ lymereJ added this to the EnergyPlus 24.2 IOFreeze milestone Jan 13, 2024
@nrel-bot-2
Copy link

@lymereJ @Myoldmopar it has been 28 days since this pull request was last updated.

@lymereJ lymereJ added the OutputChange Code changes output in such a way that it cannot be merged after IO freeze label Mar 8, 2024
Comment on lines -3053 to -3061

} else {
// fill PlantSizFac from data structure
// for (BranchNum = 1;
// BranchNum <= PlantLoop(LoopNum).LoopSide(LoopSideLocation::Supply).TotalBranches; ++BranchNum) {
// if (PlantLoop(LoopNum).LoopSide(LoopSideLocation::Supply).NodeNumIn ==
// PlantLoop(LoopNum).LoopSide(LoopSideLocation::Supply).Branch(BranchNum).NodeNumIn) {
// break;
// }
// }
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Unused.

Comment on lines +117 to +120
${PROJECT_SOURCE_DIR}/input-output-reference/media/VSFanStagedHeatControlDiag.png
${PROJECT_SOURCE_DIR}/input-output-reference/media/SeriesVSFanModulatedHeatControlDiag.png
${PROJECT_SOURCE_DIR}/input-output-reference/media/vav_parallel_fpb_trane_staged.png
${PROJECT_SOURCE_DIR}/input-output-reference/media/vav_parallel_fpb_trane_modulated.png
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Add new figures/pictures.

@@ -1964,3 +1966,1069 @@ TEST_F(EnergyPlusFixture, PIU_InducedAir_Plenums)
});
EXPECT_TRUE(compare_err_stream(expectedError, true));
}

TEST_F(EnergyPlusFixture, VSParallelPIUStagedHeat)
Copy link
Collaborator Author

@lymereJ lymereJ Mar 8, 2024

Choose a reason for hiding this comment

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

New tests were added for both the VS series and parallel PIUs for each stage of the staged and modulated heating operations. A test was also added for the VS cooling operation of the series PIU.

@@ -1496,7 +1376,8 @@ void SizePIU(EnergyPlusData &state, int const PIUNum)
}

if (CurTermUnitSizingNum > 0) {
TermUnitSizing(CurTermUnitSizingNum).MinFlowFrac = thisPIU.MinPriAirFlowFrac;
TermUnitSizing(CurTermUnitSizingNum).MinPriFlowFrac = thisPIU.MinPriAirFlowFrac;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

MinFlowFrac is actually the minimum primary air flow fraction. This was changed throughout.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I see that MinFlowFrac (now MinPriAirFlowFrac) is used in the autosizing routines as such:

if (this->termUnitSizing(this->curTermUnitSizingNum).InducesPlenumAir) {
CoilInTemp = (this->termUnitFinalZoneSizing(this->curTermUnitSizingNum).DesHeatCoilInTempTU * MinPriFlowFrac) +
(this->termUnitFinalZoneSizing(this->curTermUnitSizingNum).ZoneRetTempAtHeatPeak * (1.0 - MinPriFlowFrac));
} else {
CoilInTemp = (this->termUnitFinalZoneSizing(this->curTermUnitSizingNum).DesHeatCoilInTempTU * MinPriFlowFrac) +
(this->termUnitFinalZoneSizing(this->curTermUnitSizingNum).ZoneTempAtHeatPeak * (1.0 - MinPriFlowFrac));

MinPriAirFlowFrac is the ratio of minimum primary air to maximum primary air. I think that perhaps for the sizing routine (as shown above) we would need to use the ratio of minimum primary air to maximum total air flow (primary and secondary air flow). This is probably beyond the scope of this PR.

@lymereJ
Copy link
Collaborator Author

lymereJ commented Mar 14, 2024

Plots showing the expected results using the new example file for each new control option:

image

image

image

image

@lymereJ
Copy link
Collaborator Author

lymereJ commented Mar 14, 2024

The only diffs (AUD, RDD, Tables) are due to the new output variables and the new default values which are all expected.

Comment on lines 616 to 650
SetupOutputVariable(state,
"Zone Air Terminal Total Air Mass Flow Rate",
Constant::Units::kg_s,
thisPIU.TotMassFlowRate,
OutputProcessor::SOVTimeStepType::System,
OutputProcessor::SOVStoreType::Average,
state.dataPowerInductionUnits->PIU(PIUNum).Name);
SetupOutputVariable(state,
"Zone Air Terminal Primary Air Mass Flow Rate",
Constant::Units::kg_s,
thisPIU.PriMassFlowRate,
OutputProcessor::SOVTimeStepType::System,
OutputProcessor::SOVStoreType::Average,
state.dataPowerInductionUnits->PIU(PIUNum).Name);
SetupOutputVariable(state,
"Zone Air Terminal Secondary Air Mass Flow Rate",
Constant::Units::kg_s,
thisPIU.SecMassFlowRate,
OutputProcessor::SOVTimeStepType::System,
OutputProcessor::SOVStoreType::Average,
state.dataPowerInductionUnits->PIU(PIUNum).Name);
SetupOutputVariable(state,
"Zone Air Terminal Outlet Discharge Air Temperature",
Constant::Units::C,
thisPIU.DischargeAirTemp,
OutputProcessor::SOVTimeStepType::System,
OutputProcessor::SOVStoreType::Average,
state.dataPowerInductionUnits->PIU(PIUNum).Name);
SetupOutputVariable(state,
"Zone Air Terminal Current Operation Control Stage",
Constant::Units::unknown,
thisPIU.CurOperationControlStage,
OutputProcessor::SOVTimeStepType::System,
OutputProcessor::SOVStoreType::Average,
state.dataPowerInductionUnits->PIU(PIUNum).Name);
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 output variables.

Comment on lines 1608 to 1617
if (thisPIU.fanControlType == FanCntrlType::VariableSpeedFan) {
// calculate fan speed ratio
Real64 fanFlowRatio(1.0);
if (thisPIU.MaxTotAirMassFlow > 0.0) {
fanFlowRatio = (thisPIU.PriAirMassFlow + thisPIU.SecAirMassFlow) / thisPIU.MaxTotAirMassFlow;
}
state.dataHVACFan->fanObjs[thisPIU.Fan_Index]->simulate(state, fanFlowRatio, _);
} else {
state.dataHVACFan->fanObjs[thisPIU.Fan_Index]->simulate(state, _, _);
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This might seem redundant but it is setup this way to avoid regression diffs due to #10438.

Copy link
Contributor

Choose a reason for hiding this comment

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

Since your math looks correct, which fan output report changed in the existing example files? FYI, I just fixed a fan sizing issue in PIU in a forked repo.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Here's what I see in the EIO file for PlantLoopHeatPump_EIR_Large-Office-2-AWHP-DedHR-AuxBoiler-Pri-Sec-HW.idf when running it from this branch but also from develop:

 Component Sizing Information, AirTerminal:SingleDuct:SeriesPIU:Reheat, CORE_BOTTOM PIUHEATING, Design Size Maximum Primary Air Flow Rate [m3/s], 4.18760
 Component Sizing Information, Fan:SystemModel, CORE_BOTTOM PIUHEATING FAN, Design Size Design Maximum Air Flow Rate [m3/s], 4.51031

The file is intended to be run for Chicago but I'm running it for Golden, CO.

Copy link
Contributor

@rraustad rraustad Mar 15, 2024

Choose a reason for hiding this comment

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

If you look at SystemAirFlowSizing.cc you will see that the TU fan sizes using FinalZoneSizing data while the TU sizes using TermUnitFinalZoneSizing. So the TU needs to set a flag to use the correct data for sizing a fan. This is what I set over in PoweredInductionUnits.cc to get fan sizing correct for:

SeriesPIU:

state.dataSize->ZoneEqSizing(state.dataSize->CurZoneEqNum).SystemAirFlow = true;
state.dataSize->ZoneEqSizing(state.dataSize->CurZoneEqNum).AirVolFlow = 
    state.dataPowerInductionUnits->PIU(PIUNum).MaxTotAirVolFlow; 

Parallel PIU:

state.dataSize->ZoneEqSizing(state.dataSize->CurZoneEqNum).SystemAirFlow = true;
state.dataSize->ZoneEqSizing(state.dataSize->CurZoneEqNum).AirVolFlow =
    state.dataPowerInductionUnits->PIU(PIUNum).MaxSecAirVolFlow;

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'll take a look at it. Is that okay if I include these changes in a dedicated PR for #10438? There's already a lot of changes (in my opinion) in this PR and the CI comes out relatively clean, at least with a manageable number of (expected) diffs.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, OK to include this fan sizing change in #10438.

@rraustad
Copy link
Contributor

Configuration reminder:

image

@lymereJ another good test for these changes to TU controls is to plot the air flow and outlet temperature versus the zone predicted load. Like these plots in the Eng. Ref. Probably want to also look at primary and secondary flow versus the expected performance.

image

@lymereJ
Copy link
Collaborator Author

lymereJ commented Mar 19, 2024

@lymereJ another good test for these changes to TU controls is to plot the air flow and outlet temperature versus the zone predicted load. Like these plots in the Eng. Ref. Probably want to also look at primary and secondary flow versus the expected performance.

Because this requires lots of plots (which might need to get updated based on review comments), I thought that perhaps it would be best to host them somewhere else so I put them a separate repository, here (see the .ipnyb file).

@lymereJ
Copy link
Collaborator Author

lymereJ commented Mar 19, 2024

The only diffs are AUD, RDD, and Table diffs and are due to the new outputs.

@rraustad
Copy link
Contributor

Those ipnyb plots look pretty good. The controls are in good shape.

@lymereJ lymereJ marked this pull request as ready for review March 21, 2024 15:43
@nrel-bot-3
Copy link

@lymereJ @Myoldmopar it has been 28 days since this pull request was last updated.

@Myoldmopar
Copy link
Member

@lymereJ so this got conflicted with some of the other refactors. The GitHub web interface is offering to let me resolve them, so they aren't very complex, but I'm not able to see exactly what changed. Could you take a pass and see what needs to be done? I am assuming it is purely the fan hierarchy refactor that @amirroth contributed. Let me know if you need assistance.

@lymereJ
Copy link
Collaborator Author

lymereJ commented May 8, 2024

Thanks for letting me know, I'll take a stab at it and try not to break things.

@lymereJ
Copy link
Collaborator Author

lymereJ commented May 9, 2024

@Myoldmopar - Conflicts resolved!

@nrel-bot
Copy link

nrel-bot commented Jun 7, 2024

@lymereJ @Myoldmopar 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
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.

None yet

8 participants