New Constant Flow and Electric Radiant System Controls and 2-D Modeling Capabilities #8042

RKStrand · 2020-06-03T21:33:31Z

Pull request overview

Fixes Addition of New Controls for Constant Flow and Electric Radiant Systems and 2-D Control Enhancement #8021
The purpose of this pull request is the implementation of the new surface face temperature and surface interior temperature controls for constant flow and electric radiant systems. In addition, this also implements an additional input for the Construction:InternalSource that allows the user to control the location of the surface interior temperature location. So, this pull request includes code changes, new and modified input files, changes and additions to documentation in the Input Output Reference and the Engineering Reference.

Pull Request Author

Add to this list or remove from it as applicable. This is a simple templated set of guidelines.

Reviewer

This will not be exhaustively relevant to every PR.

Addition of various IDFs that test the new surface controls for constant flow and electric low temperature radiant systems. This includes changes to the IDD to get the controls to be allowed as options in the input file and to the CMake file for the CI run. The results for each of the new input files are expected though it points to the need for additional documentation to explain some of the switching that takes place in a constant flow system when it is very close to the "deadband" and why it turns off at certain times.

This commit includes a new feature that allows a user to specify where a temperature is to be calculated inside a radiant system. Previous code only allowed the user to select the depth. The new feature allows the user to also specify the position horizontally when performing a 2-D calculation for the radiant system. This commit also includes modified input files in the test suite and the modified IDD.

This commits includes two unit tests which exercise two new function. In addition, some unit test errors that were introduced in the previous commit were corrected. These errors occurred because of the change to the Construction:InternalSource input.

This commit includes modifications and additions to the existing documentation for radiant systems and Construction:InternalSource input. This includes changes in the IO Ref and the Eng Ref. In addition, a new transition rule was added to accomodate the new field in the input.

Myoldmopar

Overall this is looking good. I won't mark it approved as I'd like to get a couple things resolved, and it also needs transition rules. One note is that I was unclear from just the text exactly how this 2D heat transfer interacted with the uniform boundary condition on each surface's surface heat balance. If you can point me to an existing image in the docs, or if you can add a new one, that would be great. Second note is the IDD field name with Request at the end. I'd like to get other's feelings on that field name, as the trailing Request makes me think this field is actually the request to get 2D heat transfer, but it's really the position itself.

Myoldmopar · 2020-06-04T14:24:29Z

...neering-reference/src/simulation-models-encyclopedic-reference-004/radiant-system-models.tex

@@ -416,6 +416,25 @@ \subsubsection{State Space Formulation}\label{state-space-formulation-1}

 Other similarities between the two solution methods are evident.~ It is interesting to note that as with the Laplace method there is no alteration of the CTFs calculated by the state space method.~ Thus, the principle of superposition is still valid.~ Furthermore, the introduction of the source term did not substantially increase the computing effort required to calculate the additional transfer functions.~ In the Laplace method, this was shown by the common roots, B(s), shared by both the CTFs and the QTFs.~ In the state space method, it can be noted that the A matrices in Equations~\ref{eq:QTFDeriv640} and~\ref{eq:QTFDeriv658} are identical.~ Since the state space method requires the inversion and the exponentiation of the A matrix only, the additional QTF terms will not require a substantial amount of additional computing time for their calculation.

+\subsubsection{Two-Dimensional Solutions for Radiant Systems}\label{two-dimensional-solutions-for-radiant-systems}
+
+One distinct advantage of the State Space method presented in the previous section over the LaPlace Transform method is that it can be adapted to more than one dimension.  In fact, as long as one can apply a network of nodes to a problem, the State Space method can be adapted to it.  For EnergyPlus, the biggest implication is that conduction through a construction can be expanded from one-dimensional in nature to two-dimensional.  This is particularly important in a hydronic radiant system where the presence of water tubes is clearly more than one-dimensional.


I don't think the P in Pierre-Simon Laplace is capitalized, but I could be wrong. Maybe someone that knows more French than myself could confirm -- @jmarrec? @mjwitte?

@Myoldmopar My bad. Various sources on the internet all agree--it should be Laplace. I'll fix this (and any others I find in this section.

@Myoldmopar I almost forgot...did the transition rule not get uploaded somehow? GitHub is showing that Rules9-3-0-to-9-4-0.md had my changes in one of the commits that happened pretty recently.

The rules are there but we need the actual Fortran transition implementation to be added.

@Myoldmopar Oh...I missed that. Not sure I've done this before but I can figure this out. I mean, I used to program in F90, so... Anyway, I'll work on that.

(yes, capitalized as Laplace, or "Theplace" in English :))

👍 🇫🇷

All Laplace mentions now are spelled Laplace (in the next commit).

Also updated the Fortran transition implementation to reflect the new field in Construction:InternalSource (will show up in the upcoming commit)

Myoldmopar · 2020-06-04T14:35:13Z

...neering-reference/src/simulation-models-encyclopedic-reference-004/radiant-system-models.tex

+
+\begin{itemize}
+\item
+Even though the solution internal to the surface is two-dimensional, in order to work within the confines of the standard EnergyPlus heat balance, the boundary condition at both the inside and outside face of the surface is that the temperature across the surface is the same at all points or that the surfaces are still isothermal.  The point of this is so that once the conduction transfer functions are calculated that the the surface heat balance formulations remain the same as any other surface that is using a one-dimensional assumption in EnergyPlus.


Yeah, I was going to ask about this. The outside of the wall is a single condition. The inside of the wall is a single condition. Inside, what exactly drives the heat transfer to be anything but one dimensional? I think I understand that the impact is the fluid actually passing along this uniform surface acting like a heat exchanger with the flow passing along a fixed surface temperature. But in a lot of those situations, once you make these concessions and idealize the boundary conditions that far, the math often results in a simpler solution. I'm just wondering what kind of accuracy/performance/flexibility you will gain from going 2D inside the wall.

Also is there an image of the 2D heat transfer/configurations you are modeling?

@Myoldmopar The assumptions to fit in with the rest of the 1-D solution techniques is definitely limiting, but keep in mind that when you go to a 2-D solution you go from a surface source to a line source. A diagram will probably explain this better though. I just checked out my dissertation and found some diagrams that could probably help. I will work to get one of more of these integrated into the Eng Ref. I'm not sure why there hasn't been any 2-D description in the docs before. I was likely an oversight on my part. Though it has some limits, it's actually pretty cool.

In the work that I did in verifying that this stuff is working, I did move the user temperature location from where the source hits to the same depth but at the other end of the domain in the perpendicular direction. There is definitely a difference in temperature there. In a 1-D solution, everything along the plane where the source is added would be at the same temperature.

Thanks for the clarification. Pretty sure I've read that document once before...a digital copy is probably sitting on one of my backup drives somewhere. I think a diagram would definitely be helpful. Carry on...

@Myoldmopar I'll definitely incorporate at least one. And I'm impressed--you read it and survived to tell the tale. I tell all my students if they ever suffer from insomnia to let me know and I'll get them a copy. 😂

Three figures and some supporting text added (in the next commit) that should hopefully make things a lot clearer.

Myoldmopar · 2020-06-04T14:37:25Z

src/EnergyPlus/DataHeatBalance.cc

+        } else {    // Valid value between 0 and 1
+            return userValue;
+        }
+    }


Myoldmopar · 2020-06-04T14:39:07Z

src/EnergyPlus/DataHeatBalance.cc

+    {
+        sourceNodeLocation = 0;
+        userTempNodeLocation = 0;
+        if (!this->SourceSinkPresent) return;


I'd move this check outside the function so that the function isn't even called if !this->SourceSinkPresent

@Myoldmopar I thought of that route initially, but because sourceNodeLocation and userTempNodeLocation have to be defined as 0 when the !SourceSinkPresent, it seemed to make more sense to leave this stuff in the function. I can move those assignments outside the function but I feel like the code isn't as clean then. Given all that, do you still feel like I should skip this function if not a SourceSink construction?

How often is this being called? It's a bit nit-picky of me, but I'll just say that if this is called a lot, it's probably worth skipping the call entirely, and just setting sourceNodeLocation and userTempNodeLocation to zero outside of the function. The only reason is that you would end up benefitting slightly from not having to setup the function call arguments. However, I should add some caveats, as I understand them. The allocations of the function call are all made on the stack, which is, well, fast. You are returning void, so that's not causing a performance hit. Your arguments, however, reveal a potential issue. The third argument Nodes is not passed by reference. I believe this is going to end up making a new copy of the Array1D_int -- though assembly would verify what's going on under the hood with the Objexx array. In addition, I see your point about code cleanliness. With all these random thoughts in mind, my new suggestion:

Make this function accept an Array1D_int & Nodes as the third argument

Leave it as it is otherwise

@Myoldmopar Sounds good--I will make that change. This isn't called very often by the way. Just once per construction that shows up in the user's input file. The CTF calculations are only done once so it's not like this is happening every time step or something like that.

Changed this and it will show up in the next commit.

Myoldmopar · 2020-06-04T19:15:00Z

idd/Energy+.idd.in

@@ -8841,6 +8841,15 @@ Construction:InternalSource,
       \units m
       \note uniform spacing between tubes or resistance wires in direction
       \note perpendicular to main intended direction of heat transfer
+  N5 , \field Two-Dimensional Position of Interior Temperature Calculation Request


The word Request at the end feels a little odd to me. If others think it is fine, then go with it, but I feel like it might be OK without Request, or with a different word.

I can't see the rest of the fields on this object, but just confirm that min-fields doesn't need to be updated here.

@Myoldmopar Ok, I'm good with changing this field by removing the word Request. It's hard to find a short phrase that really captures the intent without being a full sentence. I guess that is why we have docs?🤷‍♂️

Perhaps the master of the IDD @mjwitte has an opinion... It could be that leaving Request on there is actually best...

Changed this so that the word Request is gone. I think that the removal of this makes better sense than leaving it. Will show up in the next commit.

I agree with dropping request. An even better field name might be "Two-Dimensional Temperature Calculation Position
The note should mention this field is ignored for 1-d calc?

Other cleanups for this object would be nice:

Construction:InternalSource, \memo Start with outside layer and work your way to the inside Layer \memo Up to 10 layers total, 8 for windows ** This can't be used for window surfaces, can it? So delete ", 8 for windows". \memo Enter the material name for each layer ** \min-fields 7 A1 , \field Name \required-field \type alpha \reference ConstructionNames N1 , \field Source Present After Layer Number \required-field \type integer \minimum 1 ** \maximum 10 \note refers to the list of materials which follows N2 , \field Temperature Calculation Requested After Layer Number \required-field \type integer ** \minimum 1 ** \maximum 10 \note refers to the list of materials which follows N3 , \field Dimensions for the CTF Calculation \required-field \type integer \minimum 1 \maximum 2 \note 1 = 1-dimensional calculation, 2 = 2-dimensional calculation N4 , \field Tube Spacing \required-field \type real \units m ** \minimum> 0 ??? \note uniform spacing between tubes or resistance wires in direction \note perpendicular to main intended direction of heat transfer

@Myoldmopar @mjwitte Ok, I've made the changes that Mike suggested in the name and the other IDD changes (plus one more). I also clarified the max and min values for each of these in the IO Reference. That has all been committed to GitHub and I also made another commit merging the latest from develop.

Myoldmopar · 2020-06-04T19:15:57Z

src/EnergyPlus/DataHeatBalance.cc

+            userTempNodeLocation = ((userTempNodeLocation - 1) * NumOfPerpendNodes)
+                                    + round(this->userTemperatureLocationPerpendicular * (NumOfPerpendNodes - 1)) + 1;
+
+    }


This is a good function, the only comment would be that some of the parentheses are unnecessary.

@Myoldmopar Yeah, I admit it. I tend to be really distinct with order of operations and overuse parentheses to make it crystal clear what I'm trying to do. I can get rid of the extras.

Changes made and they will show up in the next commit.

Myoldmopar · 2020-06-04T19:16:30Z

src/EnergyPlus/LowTempRadiantSystem.cc

@@ -1744,12 +1744,13 @@ namespace LowTempRadiantSystem {
                    TotalEffic = CFloRadSys(RadNum).WaterVolFlowMax * CFloRadSys(RadNum).NomPumpHead / CFloRadSys(RadNum).NomPowerUse;
                    CFloRadSys(RadNum).PumpEffic = TotalEffic / CFloRadSys(RadNum).MotorEffic;
                    static constexpr auto fmt = "Check input.  Calc Pump Efficiency={:.5R}% {}, for pump in radiant system {}";
+                    Real64 pumpEfficiency = CFloRadSys(RadNum).PumpEffic * 100.0;


Addressed various review comments with changes and additions to code, documentation, IDD, and transition F90 program.

RKStrand · 2020-06-05T21:38:08Z

@Myoldmopar Ok, I've made changes to address all of your comments ("details" above) and committed a new version (and then went ahead and merged develop in as well for good measure. Hopefully this all comes back clean and the changes are satisfactory.

Some additional comments were received regarding the new input field and some other clean-up changes to the IDD.

Myoldmopar

Changes look good.

Myoldmopar · 2020-06-24T18:36:52Z

idd/Energy+.idd.in

       \note uniform spacing between tubes or resistance wires in direction
       \note perpendicular to main intended direction of heat transfer
+  N5 , \field Two-Dimensional Temperature Calculation Position


Myoldmopar · 2020-06-24T18:38:02Z

src/Transition/CreateNewIDFUsingRulesV9_4_0.f90

+                  OutArgs(1:5)=InArgs(1:5)
+                  OutArgs(6) = '0.0'
+                  OutArgs(7:CurArgs+1)=InArgs(6:CurArgs)
+                  CurArgs = CurArgs + 1


Myoldmopar · 2020-06-24T18:40:32Z

@RKStrand I am going to pull develop into this branch locally to resolve the unrelated conflict in the transition rules file. CI reported a failing example file in that last commit, so I'll run that locally. It may just be that a new IDF was added and needs the transition rule manually applied. I'll also build the transition binary and test it out.

Myoldmopar · 2020-06-24T19:11:06Z

I pulled develop and resolved the tiny conflict no problem. The build went fine as well. Running the unit test suite shows everything is OK.

I was able to reproduce the failing GeneratorswithPV file. It is failing because the tube spacing in the single internal source construction is zero, and it needs >0 tube spacing. I put .1524 in there to get it running. But it had zero in there before. @RKStrand suggestions? What was it using before when zero slipped through?

I wasn't able to reproduce the failing RadLoTempCFloCtrlOpt2 file, but that occurred in a debug build, so I'm starting a debug build locally and will report back.

Transition binary built and ran. It throws a warning about a field being out of range, but the diff between IDFs looks correct, with the new field added as zero. I think it's all fine and could be a problem with the IDD I had dropped into the folder.

Myoldmopar · 2020-06-24T19:26:01Z

A debug build revealed a divide by zero causing a crash. It is occurring during the evaluation of the water injection rate. I believe this one:

EnergyPlus/src/EnergyPlus/LowTempRadiantSystem.cc

Lines 4047 to 4051 in 0fab60e

    
           this->WaterInjectionRate = 
        
               (this->WaterMassFlowRate * 
        
                (this->WaterInletTemp - this->WaterOutletTemp) / 
        
                (SysWaterInTemp - this->WaterOutletTemp)) - 
        
               (this->PumpHeattoFluid / (CpFluid * (SysWaterInTemp - this->WaterOutletTemp)));

. At this point in the simulation, the SysWaterInletTemp is 7.0, and the this->WaterOutletTemp is also 7. The delta T of zero causes an overflow. I'm not sure exactly what you want to do in all these cases @RKStrand but they need to be protected, and the injection rate is calculated multiple times using a similar block.

I'll push my changes up so that the conflict is gone and what not, so pull down the branch before making any fixes.

Myoldmopar · 2020-06-24T19:27:14Z

Also, please decide whether my GeneratorswithPV.idf fix is reasonable. I set tube spacing from 0. to 0.1524. It may cause diffs in results if that is not what the code was defaulting to before this change.

Changes to the IDD to allow 0.0 for tube spacing and to avoid divide by zero errors in the constant flow system.

Myoldmopar · 2020-06-25T14:09:35Z

@RKStrand this is in really great shape now. There is the one file with big diffs (both time series as well as meter file). It's been showing diffs since the very beginning of this PR, so it's nothing that slipped in with the later commits. I'm assuming it is intentional, but I need you to confirm that. Looking at the diff summary shows that they are actually pretty good size diffs. I think if you can resolve/justify those diffs, this should be good to go.

Myoldmopar · 2020-06-25T14:11:02Z

Oh, and the Linux results with the diff showed "s3" errors. This is because the clock on our CI machine had gone slightly out of sync and the authentication could not be validated when the machine tried to access our s3 account. Anyway, I deleted those results so CI may try to re-run that last commit. If it does, I expect more diffs because of a recent commit to develop, but hopefully it would at least show successful pushes to the s3 bucket.

RKStrand · 2020-06-25T14:26:20Z

@Myoldmopar Yup, as mentioned above in the PR description checklist, that file should have diffs. I tried to look at them in this latest commit but for some reason, when I went to look at them, it wasn't showing me any of the diff files. The reason it should have diffs is that the control point and user temperature location changed from in line with the source tube to mid-point between adjacent tubing. So, those temperature should be significantly different. When I looked at this when that change was first made, the results looked like I would expect.

Myoldmopar · 2020-06-25T14:30:21Z

OK, let's call this a wrap then. Thanks @RKStrand !

RKStrand · 2020-06-25T14:31:26Z

Sweet! Thanks, @Myoldmopar!!!

RKStrand added 7 commits May 28, 2020 18:08

Merge branch 'develop' into 8021-NewCFloElecRadCtrlsWith2DEnhancement

884b6d2

Merge branch 'develop' into 8021-NewCFloElecRadCtrlsWith2DEnhancement

7d60180

Merge branch 'develop' into 8021-NewCFloElecRadCtrlsWith2DEnhancement

1842e39

RKStrand added the NewFeature Includes code to add a new feature to EnergyPlus label Jun 3, 2020

RKStrand requested a review from Myoldmopar June 3, 2020 21:33

RKStrand self-assigned this Jun 3, 2020

Myoldmopar reviewed Jun 4, 2020

View reviewed changes

RKStrand added 2 commits June 5, 2020 16:35

Address Review Comments

82f8568

Addressed various review comments with changes and additions to code, documentation, IDD, and transition F90 program.

Merge branch 'develop' into 8021-NewCFloElecRadCtrlsWith2DEnhancement

fb4c80e

RKStrand added 2 commits June 8, 2020 15:16

Changes to Address More Review Comments

5be2763

Some additional comments were received regarding the new input field and some other clean-up changes to the IDD.

Merge branch 'develop' into 8021-NewCFloElecRadCtrlsWith2DEnhancement

0fab60e

Myoldmopar approved these changes Jun 24, 2020

View reviewed changes

Merge develop, set tube spacing to >0, one debug file still fails

70fa01f

Corrections Based on Additional Review

fc2b9aa

Changes to the IDD to allow 0.0 for tube spacing and to avoid divide by zero errors in the constant flow system.

Myoldmopar merged commit 6846c7a into develop Jun 25, 2020

Myoldmopar deleted the 8021-NewCFloElecRadCtrlsWith2DEnhancement branch June 25, 2020 14:30

mdahlhausen mentioned this pull request Sep 14, 2020

Add New EnergyPlus Radiant System Enhancements to OpenStudio API NREL/OpenStudio#4076

Closed

New Constant Flow and Electric Radiant System Controls and 2-D Modeling Capabilities #8042

New Constant Flow and Electric Radiant System Controls and 2-D Modeling Capabilities #8042

Conversation

RKStrand commented Jun 3, 2020

Pull request overview

Pull Request Author

Reviewer

Myoldmopar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RKStrand commented Jun 5, 2020

Myoldmopar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Myoldmopar commented Jun 24, 2020

Myoldmopar commented Jun 24, 2020

Myoldmopar commented Jun 24, 2020

Myoldmopar commented Jun 24, 2020

Myoldmopar commented Jun 25, 2020

Myoldmopar commented Jun 25, 2020

RKStrand commented Jun 25, 2020

Myoldmopar commented Jun 25, 2020

RKStrand commented Jun 25, 2020