Implement new swimming pool simulation model

[RKStrand]

Changes and additions needed to model indoor swimming pools; this work is ready for review

[RKStrand]

This work is ready for review--someone please take on this task and change the "assignee" to yourself. If you have any problems or questions, please let me know. Thanks!

[Myoldmopar]

@RKStrand Tried to merge develop into this branch, but I'm on my home computer and I had an issue. The IDD appears to be fully re-written, maybe somehow the line endings got messed up. I'll fix that, and also add the example file to the testfiles/CMakeLists.txt file so it gets included in testing. I'll keep you posted.

[Myoldmopar]

OK @RKStrand, I was able to resolve the IDD issue I was seeing. Somehow it was line-ending related. Anyway, I've pushed a couple commits up, and am doing a build right now. I'll go over the code changes and make sure testing goes well and then it should be ready to merge in.

[Myoldmopar]

@RKStrand I built the source and ran the new test file. Everything seems fine. Of course I didn't do a rigorous validation of the results, but didn't see anything out of the ordinary. The code looks good in terms of format; no surprises there. The IDD definition looks in order, my one question would be whether we actually want a new IDD group called \group Zone HVAC Swimming Pools. Seems like this will be the only object in there, so why have a separate group for it? Maybe that was decided during the review time, I don't recall. If you can comment on that, I think we should be ready to wrap this up. I'll wait on the CI machines to catch up with these latest commits before doing any merging anyway.

[mjwitte]

@RKStrand Is this a surface name or construction name? \reference does not apply here, only \object-list. There may be multiple \object-list if needed. If this is a construction name, then that's what the field name should be. If it's a surface name, then it should be \object-list

I agree with @Myoldmopar that this should not make a new IDD group. I would put it in the Internal Gains group, perhaps right after zonebaseboard:outdoortemperaturecontrolled?

[mjwitte]

@RKStrand Would this be correct? \object-list HeatTranBaseSurfNames
This reference exists but is never used anywhere.

[RKStrand]

Yes, it has to be a base surface…a floor to be specific. So, the list should probably only be HeatTranBaseSurfNames. Do you mean that it is never used anywhere in the IDD?

[mjwitte]

@RKStrand Correct, there are several \reference HeatTranBaseSurfNames to establish which objects are part of this group, but I did not see any \object-list HeatTranBaseSurfNames. If you want to make it exclusive to floors, the perhaps you want to make a new reference name just for objects which could be a floor (that's just two or three classes, I think).

[Myoldmopar]

@mjwitte @RKStrand I updated the IDD by moving the object into \group internal gains, and I adjusted the water maximum -> maximum water field name. I did not mess with the N7 field since it may require code changes.

@RKStrand Also, just to alert you, it appears there are lots of diffs showing up with this branch. This is with develop pulled into this branch, so the only differences are those introduced for this feature (i.e. no other develop changes are interfering). Any thoughts on where these diffs may have been introduced? I was expected absolutely no diffs since no other file has a swimming pool. Your changes do touch several parts of the heat balance, so I am a little concerned a typo or something has been introduced.

[Myoldmopar]

@RKStrand Just a reminder. If you make any changes to this branch, make sure you do a Git Pull first to bring down my changes. Otherwise if you make a commit locally and try to push, Git will complain that you need to do a Git Pull first. Then you'll have an extra merge commit to bring the two branches back together. Not a big problem, but a possibility for conflicts.

[RKStrand]

@Myoldmopar Thanks for the reminder and for fixing the field name issue. It seems that at a minimum I will need to fix the input issue that Mike brought up regarding non-standard input units on one parameter. That is an easy fix. Not sure what is wrong to cause the diffs in output.

[mjwitte]

@RKStrand Are you sure this should be in the upper subzone?

[RKStrand]

@mjwitte I'm just following what was done for the high temperature radiant system which has a similar convective gain (see variable SumConvHTRadSys). I felt that if I followed that example that I should be able to account for the convective gain (radiation terms to pool are reduced by the pool cover which is assumed to convert the blocked radiation into convection).

[EnergyArchmage]

But this is for room air modeling, and one expects high temperature radiant heaters to be located up high, shining down. One expects the pool to be on the floor. While the (slight) convective gains off the HT heater should go into the upper subzone, the convective gains off a pool should go into a lower subzone for the roomair model.

[RKStrand]

Got it. I will change the code to get this out of the upper sub zone and put it in the lower as soon as I can. Thanks for the comment.

[RKStrand]

Change made, committed and synched--thanks for the comment, Brent!

[mjwitte]

@RKStrand @Myoldmopar The diffs are troubling. They should not be there. May require some debugging to figure this out. Curious why you added new terms to the heat balance equations rather than just adding these to the generic internal gain structure?

[RKStrand]

@Myoldmopar @mjwitte Well, I agree that the diffs should not be there. I'm not sure why they are there but I'll do my best to figure it out and still get the zone as generic HVAC project debugged.
I added them to the heat balance because the swimming pool directly impacts one of the surfaces and so it followed much more closely to something akin to a radiant system. So, that is why I followed the established examples of the low and high temperature radiant systems. The swimming pool is in fact a surface heat balance that also is impacted by the plant loop and runs at the system time step. The radiant systems are the closest thing to that.

[RKStrand]

@Myoldmopar When I look at the "failed" files, I see a lot of audit and eio files but I do not see the csv or eso files. Why is that? I thought that was indicative of the csv files being the same, but perhaps that is not the case? Confused...

[Myoldmopar]

The diffs are almost certainly there for csv files, but within tolerance. Numeric diffs such as the csv, ssz, zsz, and mtr are handled so that it doesn't show a failure unless it is a big diff. However, the eio is not treated as numerics, so any binary difference throws a failure. We could discuss modifying this at a different time, but for now that's how it is.

[Myoldmopar]

Tests running now. I'll run design days with fine outputs first to get a feel for the diffs. Then I'll run annual with more coarse output to see the effect. I'd still like to hope we could resolve the problem to a no-diffs state, but we'll start with this anyway and see what we learn.

[Myoldmopar]

Diffs appear significant from the DDONLY run. I'll go ahead and kick off the annuals for tonight anyway, but I'm not super hopeful now.

[Myoldmopar]

@RKStrand Yeah, this looks much more promising now. I've pulled develop into this branch one last time and it should resolve all the diffs. We should be good to go once CI finishes (probably tomorrow).

[nrel-bot]

@RKStrand @lgentile it has been 7 days since this pull request was last updated.

[nrel-bot]

@RKStrand @lgentile it has been 7 days since this pull request was last updated.

[nrel-bot]

@RKStrand @lgentile it has been 7 days since this pull request was last updated.

[Myoldmopar]

@DeadParrot This diff and the next one had to be manually merged because @RKStrand added an IF block for if it isn't a swimming pool, or if it is, and you had tuned the math for performance.

In your optimization, it wasn't clear to me how you decided to pull out just part of the numerator, but leave a couple terms. It did make sense how you pulled out the divisor into a multiplication. Would you be able to comment here so I can tune the math similarly for the new else-side of this block? This pool model will not be hit very often, but might as well get it in fast to start with.

Also, the conditions on this IF block hint to me that it will be slow to check all those things every time in. Could you provide comment on this so I know whether we need to clean it up?

[DeadParrot]

TempTerm and TempDiv are pulled out into variables because those quantities were being computed twice each and they are not simple/cheap expressions. If you search for them you'll find the other uses a few lines down. While I was pulling out the divisor anyway I made it into a multiplication but that was not the primary purpose. The change saves 15 array lookups, 11 +/-, and one multiply every time you pass through that code. Sometimes I'm scrounging for small performance gains in hot spots and sometimes these silly changes pay off nicely.

If the new if block breaks the ability to reuse these quantities then there is no point to defining them for one-time use. If the surface is "almost never" a pool then computing them will still pay even though you occasionally don't use them -- yes that is hackish so decide whether you can live with it. You could get fancy and define them at the higher scope but with a ( surface.IsPool ? 0.0 : <formula> ) conditional so that you don't pay for the expensive computation unless it will get used.

This addition of features by conditionals in hot spot code is not conducive to performance, as we know.

If I haven't been clear enough let me know. I'm happy to rework this code segment if you tell me which version to start from.

[RKStrand]

Edwin:

All of the terms in this equation that are modified by the swimming pool model got bundled together. So, if it could be modified somehow (like because of the presence of a pool or something happening with the pool cover algorithm), then it was lumped together. Anything that could NOT be changed, got left alone. Hopefully that makes sense. The cover algorithm in particular ended up bring a lot of terms in because it alters what is happening at the surface and this affects various radiation terms.

Rick

On Nov 24, 2014, at 3:56 PM, Edwin Lee [email protected] wrote:

In src/EnergyPlus/HeatBalanceSurfaceManager.cc:

@@ -5141,10 +5154,14 @@ CalcHeatBalanceInsideSurf( Optional_int_const ZoneToResimulate ) // if passed in
if ( surface.HeatTransferAlgorithm == HeatTransferModel_EMPD ) {
CalcMoistureBalanceEMPD( SurfNum, TempSurfInTmp( SurfNum ), TH22, MAT( ZoneNum ), TempSurfInSat );
}

•               Real64 const TempTerm( CTFConstInPart( SurfNum ) + QRadThermInAbs( SurfNum ) + QRadSWInAbs( SurfNum ) + HConvIn( SurfNum ) \* RefAirTemp( SurfNum ) + QHTRadSysSurf( SurfNum ) + QHWBaseboardSurf( SurfNum ) + QSteamBaseboardSurf( SurfNum ) + QElecBaseboardSurf( SurfNum ) + NetLWRadToSurf( SurfNum ) );
  

  @DeadParrot This diff and the next one had to be manually merged because @RKStrand added an IF block for if it isn't a swimming pool, or if it is, and you had tuned the math for performance.

In your optimization, it wasn't clear to me how you decided to pull out just part of the numerator, but leave a couple terms. It did make sense how you pulled out the divisor into a multiplication. Would you be able to comment here so I can tune the math similarly for the new else-side of this block? This pool model will not be hit very often, but might as well get it in fast to start with.

Also, the conditions on this IF block hint to me that it will be slow to check all those things every time in. Could you provide comment on this so I know whether we need to clean it up?

—
Reply to this email directly or view it on GitHub.

[RKStrand]

I agree that the addition of the swimming pool at this “hot spot” (I like that—I’m now the creator of a simulation hot spot!) is not conducive to faster execution. From an algorithm standpoint, it was the clearest and most logical place to plug in though. If there are faster alternatives that are still essentially clear as to what is going on, that is fine with me.

Rick

On Nov 24, 2014, at 6:09 PM, Stuart Mentzer [email protected] wrote:

In src/EnergyPlus/HeatBalanceSurfaceManager.cc:

@@ -5141,10 +5154,14 @@ CalcHeatBalanceInsideSurf( Optional_int_const ZoneToResimulate ) // if passed in
if ( surface.HeatTransferAlgorithm == HeatTransferModel_EMPD ) {
CalcMoistureBalanceEMPD( SurfNum, TempSurfInTmp( SurfNum ), TH22, MAT( ZoneNum ), TempSurfInSat );
}

•               Real64 const TempTerm( CTFConstInPart( SurfNum ) + QRadThermInAbs( SurfNum ) + QRadSWInAbs( SurfNum ) + HConvIn( SurfNum ) \* RefAirTemp( SurfNum ) + QHTRadSysSurf( SurfNum ) + QHWBaseboardSurf( SurfNum ) + QSteamBaseboardSurf( SurfNum ) + QElecBaseboardSurf( SurfNum ) + NetLWRadToSurf( SurfNum ) );
  

  TempTerm and TempDiv are pulled out into variables because those quantities were being computed twice each and they are not simple/cheap expressions. If you search for them you'll find the other uses a few lines down. While I was pulling out the divisor anyway I made it into a multiplication but that was not the primary purpose. The change saves 15 array lookups, 11 +/-, and one multiply every time you pass through that code. Sometimes I'm scrounging for small performance gains in hot spots and sometimes these silly changes pay off nicely.

If the new if block breaks the ability to reuse these quantities then there is no point to defining them for one-time use. If the surface is "almost never" a pool then computing them will still pay even though you occasionally don't use them -- yes that is hackish so decide whether you can live with it. You could get fancy and define them at the higher scope but with a ( surface.IsPool ? 0.0 : ) conditional so that you don't pay for the expensive computation unless it will get used.

This addition of features by conditionals in hot spot code is not conducive to performance, as we know.

If I haven't been clear enough let me know. I'm happy to rework this code segment if you tell me which version to start from.

—
Reply to this email directly or view it on GitHub.

[RKStrand]

Rick,

  The way to do features (swimming pools, movie stars, ...) more
  cleanly will require some OO refactoring of the EnergyPlus design
  -- within the current design you probably had little choice so the
  critique is not about what you did but about what the design
  forces developers to do.

  Stuart
  --
      Stuart G. Mentzer
      Objexx Engineering, Inc.
      Office  1.781.455.1150 x11
      Mobile 1.781.708.3872


  On 11/26/2014 9:50 AM, Rick Strand wrote:



  I agree that the addition of the swimming pool at this “hot spot”
  (I like that—I’m now the creator of a simulation hot spot!) is not
  conducive to faster execution.  From an algorithm standpoint, it
  was the clearest and most logical place to plug in though.  If
  there are faster alternatives that are still essentially clear as
  to what is going on, that is fine with me.


  Rick




      On Nov 24, 2014, at 6:09 PM, Stuart Mentzer <[email protected]>
        wrote:



        In src/EnergyPlus/HeatBalanceSurfaceManager.cc:
        > @@ -5141,10 +5154,14 @@ CalcHeatBalanceInsideSurf( Optional_int_const ZoneToResimulate ) // if passed in

              if ( surface.HeatTransferAlgorithm == HeatTransferModel_EMPD ) {
                  CalcMoistureBalanceEMPD( SurfNum, TempSurfInTmp( SurfNum ), TH22, MAT( ZoneNum ), TempSurfInSat );
              }

•             Real64 const TempTerm( CTFConstInPart( SurfNum ) + QRadThermInAbs( SurfNum ) + QRadSWInAbs( SurfNum ) + HConvIn( SurfNum ) \* RefAirTemp( SurfNum ) + QHTRadSysSurf( SurfNum ) + QHWBaseboardSurf( SurfNum ) + QSteamBaseboardSurf( SurfNum ) + QElecBaseboardSurf( SurfNum ) + NetLWRadToSurf( SurfNum ) );
  

        TempTerm and TempDiv are pulled out into variables
          because those quantities were being computed twice each
          and they are not simple/cheap expressions. If you search
          for them you'll find the other uses a few lines down.
          While I was pulling out the divisor anyway I made it into
          a multiplication but that was not the primary purpose. The
          change saves 15 array lookups, 11 +/-, and one multiply
          every time you pass through that code. Sometimes I'm
          scrounging for small performance gains in hot spots and
          sometimes these silly changes pay off nicely.
        If the new if block breaks the ability to reuse these
          quantities then there is no point to defining them for
          one-time use. If the surface is "almost never" a pool then
          computing them will still pay even though you occasionally
          don't use them -- yes that is hackish so decide whether
          you can live with it. You could get fancy and define them
          at the higher scope but with a ( surface.IsPool ?
            0.0 : <formula> ) conditional so that you
          don't pay for the expensive computation unless it will get
          used.
        This addition of features by conditionals in hot spot
          code is not conducive to performance, as we know.
        If I haven't been clear enough let me know. I'm happy to
          rework this code segment if you tell me which version to
          start from.
        —
          Reply to this email directly or view
            it on GitHub.

[Myoldmopar]

@RKStrand Could/did you address @mjwitte's IDD comments about invalid units and other things? The code is now merged back up with develop so there are no conflicts, and I'd like to finish this up quickly before more conflicts come about. I'll go back over your code changes and I'll do a little more testing. Also I added your docs to a directory here. I may make a few minor changes to them, but overall they looked good.

[Myoldmopar]

@lefticus Any thoughts on the boto issue here? https://nrel.github.io/EnergyPlusBuildResults/EnergyPlus-beef00102fef21d093ba9a26a24eab1cc6127423-x86_64-Linux-Ubuntu-14.04-gcc-4.8.html

[lefticus]

Gee that's weird. I wonder if we'll see it again? Just the one file failed. Makes me wonder if we ran out of space or something?

[RKStrand]

Edwin:

It looks like I did not address those comments yet, but I understand your desire to get these taken care of ASAP. It should be an easy fix for the most part. It simply needs density calculated in the report subroutine and then the calculation modified to include density as a parameter. Then the docs and the IDD need refreshing. So, overall, this is a pretty simply fix. I have a feeling that I will not get to it today. At least not until much later today or maybe Friday. So, if you are worried about this causing any conflicts, then you have my okay to patch something in quick and go. Otherwise, I will take care of it as soon as I can.

Rick

On Nov 24, 2014, at 4:01 PM, Edwin Lee [email protected] wrote:

@RKStrand Could/did you address @mjwitte's IDD comments about invalid units and other things? The code is now merged back up with develop so there are no conflicts, and I'd like to finish this up quickly before more conflicts come about. I'll go back over your code changes and I'll do a little more testing. Also I added your docs to a directory here. I may make a few minor changes to them, but overall they looked good.

—
Reply to this email directly or view it on GitHub.

[Myoldmopar]

@RKStrand I won't patch it, I'll just anticipate continuing my review next week after you've had a chance to address the issues.

[EnergyArchmage]

@RKStrand, Did you consider putting SumLatentPool( ZoneNum ) and SumConvPool( ZoneNum ) terms, used in ZTPC, into the internal gains service using SetupZoneInternalGain? The idea with that is to organize those summations in a central way so they are easier to manage in the balances as the number of terms grows.

[RKStrand]

@EnergyArchmage @Myoldmopar I did not consider this because I was trying to keep the pool terms as contained as possible in the swimming pool "module". I felt that too many of the terms associated with the Sum___Pool variables were entrenched in the details of the pool simulation and was concerned that letting them out of the module boundary would lead to adding too much complexity outside the module. At least, that was what I was thinking when I wrote the code.

[EnergyArchmage]

The intent is if you use the service, then there should be less complexity outside the module. It registers a pointer to the gain terms located inside your module and you wouldn't have to touch anything outside the module, just make calls to SetupZoneInternalGain . The original design intent was that there would have to be something special about the gains, such as being from controlled equipment inside the system timestep, for them to really require their own terms outside of the (relatively new) central service for internal gains to zone air.

[RKStrand]

@EnergyArchmage @Myoldmopar Ah, well, that is also a bit of a "problem" because the swimming pool is somewhat similar to the radiant systems in that they are associated with a surface but they simulate at the system time step because of their connection to a plant loop. So, they are "controlled" at the system time step level. So, does that mean they are not candidates for the relatively new central service (sorry, wasn't aware of that).

[EnergyArchmage]

yes, if the zone gains are impacted by plant at the system timestep level, then that means they are special and shouldn't use the internal gains service. thanks.

[nrel-bot]

@RKStrand @lgentile it has been 7 days since this pull request was last updated.

[Myoldmopar]

@RKStrand I made a commit to the branch to fix the array bounds problem that was showing itself in the debug build. Hopefully once CI gets back in shape, we'll see everything is all clear and this can go in soon.

[mjwitte]

@RKStrand @Myoldmopar @kbenne These units are invalid. Needs to be "W/(m3/s)" then add a
\note Power per unit of pool water flow.
Noticed this when IDF Editor complained while working on a different issue. Should get this fixed before making the performance update build. This happens frequently (IDD errors) - we need an IDD validation step in the CI system. Noting this here and will post a fix shortly.