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

Correction of Inconsistent Flow Rates from Swimming Pools #10303

Merged
merged 6 commits into from
Nov 21, 2023
Merged

Correction of Inconsistent Flow Rates from Swimming Pools #10303

merged 6 commits into from
Nov 21, 2023

Conversation

RKStrand
Copy link
Contributor

Pull request overview

  • Fixes Inconsistent Water Flow Rates in SwimmingPool:Indoor #10102
  • This is the revised solution to the flow and temperature issues noted in the swimming pool when using constant flow pumps without any bypass branches. The problem was that the flow conditions were not updating properly and as a result there were inconsistencies in both temperature and flow rate in the same plant demand loop. Initially, attempts to resolve this issue through the existing solution strategy where the pool was in the zone equipment loop of the simulation were made. While a working solution was possible, it did make the swimming pool a special case that required coding at the upper level of the HVAC simulation that was deemed contrary to existing HVAC simulation protocol in EnergyPlus. Since the pool is not really a piece of zone equipment (it must trigger the zone to resimulate which is why it was put there initially), it was decided to move the pool to the plant simulation and make it a full-fledged plant component following the existing code. This was accomplished and tested with a variety of input files including: constant speed pump with no bypass, variable speed pump, constant speed pump with bypass, two pools with constant speed pump with no bypass, and two pools with constant speed pump and a bypass. All of these tests resulted in correct flows and temperatures at the pools as well as the remaining nodes on the local half loop. Two new unit tests are included in this pull request. No documentation changes needed. Only test idfs that show differences are the two swimming pool files and these differences were expected. The new results are what one would anticipate from the model.

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

@RKStrand
Initial Soluton, Take 2
1ffa003 
Includes code changes for making the pool a plant component rather than a zone equipment component.  Testing has been done for pools with constant speed pump with and without a bypass and a variable speed pump.  All of those seem to work.  Running into a snag with multiple pools.
@RKStrand
Merge branch 'develop' into 10102InconsistentPoolFlowRatesTake2
5712f10
@RKStrand
Swimming Pool Take 2 Fixes
9eb69e3 
Fixed a few issues with the change of the swimming pool to become a true plant component rather than zone equipment.
@RKStrand
Merge branch 'develop' into 10102InconsistentPoolFlowRatesTake2
be27610
@RKStrand
Addition of two unit tests
efd8ffd 
Two new unit tests for routines that underwent significant change.  No docs changes needed.  No new input files being added due to the fact that two swimming pool input files already exist.  Potential pull request candidate.
@RKStrand
Fix to accommodate compiler check issue
1834f50 
Description says it all...
@RKStrand RKStrand added the Defect Includes code to repair a defect in EnergyPlus label Nov 17, 2023
@RKStrand RKStrand requested a review from Myoldmopar November 17, 2023 14:38
@RKStrand RKStrand self-assigned this Nov 17, 2023
@RKStrand
Copy link
Contributor Author

@Myoldmopar
PoolAsPlantComponent.zip

Myoldmopar
Myoldmopar requested changes Nov 21, 2023
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.

I think this needs at least one change in plant to set the flow priority. I'll add that and test it out with latest develop locally.

src/EnergyPlus/Plant/PlantManager.cc
this_comp.CurOpSchemeType = OpScheme::Demand;
this_comp.compPtr = SwimmingPool::SwimmingPoolData::factory(state, CompNames(CompNum));
break;
}
Copy link
Member

Choose a reason for hiding this comment

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

This looks exactly right. I think there should be one more spot in PlantManager where we need to switch on equipment type. Down where we assign the flow request priority. If that's the only change needed here, I can add that while I'm doing final testing.

src/EnergyPlus/SwimmingPool.cc
ShowFatalError(state,
format("LocalSwimmingPoolFactory: Error getting inputs or index for swimming pool named: {}", objectName)); // LCOV_EXCL_LINE
// Shut up the compiler
return nullptr; // LCOV_EXCL_LINE
Copy link
Member

Choose a reason for hiding this comment

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

Yep, looks like a typical plant factory. I like that it returns a SwimmingPoolData* instead of a PlantComponent*. That makes things easier.

src/EnergyPlus/SwimmingPool.cc
// Now the power consumption of miscellaneous equipment
Real64 Density = FluidProperties::GetDensityGlycol(state, "WATER", this->PoolWaterTemp, this->GlycolIndex,
RoutineName); // density of water
if (Density > MinDensity) {
Copy link
Member

Choose a reason for hiding this comment

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

Hmm. I'm not sure GetDensityGlycol will ever give back zero density, but this check is fine I guess.

tst/EnergyPlus/unit/SwimmingPool.unit.cc
// Test 4: First Pool
factoryResult = SwimmingPoolData::factory(*state, poolData(4).Name);
EXPECT_NE(factoryResult, nullptr);
}
Copy link
Member

Choose a reason for hiding this comment

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

Auf allen steht „First Pool“, aber ansonsten sieht der test gut aus.

tst/EnergyPlus/unit/SwimmingPool.unit.cc
Real64 expectedMakeUpWaterMass = 6.0;
Real64 expectedEvapEnergyLoss = 0.96;
Real64 expectedMakeUpWaterVolFlowRate = 0.0001003;
Real64 expectedMakeUpWaterVol = 0.0060018;
Copy link
Member

Choose a reason for hiding this comment

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

Are these based on a specific controlled calculation?

@Myoldmopar
Copy link
Member

Ohh, that was already there, which makes sense (I was confused why it was passing happily). This is actually good to go from that standpoint. I'm marking my review as approved but still testing it out now.

@Myoldmopar
Copy link
Member

And it's all happy here, merging. Thanks for the extra work here @RKStrand, it's great to have the swimming pool as a first-class PlantComponent.

@Myoldmopar Myoldmopar merged commit 685c7f2 into develop Nov 21, 2023
12 checks passed
@Myoldmopar Myoldmopar deleted the 10102InconsistentPoolFlowRatesTake2 branch November 21, 2023 16:56
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

@RKStrand RKStrand

Labels
Defect Includes code to repair a defect in EnergyPlus
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Inconsistent Water Flow Rates in SwimmingPool:Indoor
7 participants
@RKStrand @Myoldmopar @nrel-bot @nrel-bot-2 @nrel-bot-3 @nrel-bot-2b @nrel-bot-2c