Add Vertical (1-D) Solution to Earth Tube Object #10138

RKStrand · 2023-08-07T19:11:04Z

Pull request overview

Fixes Enhance ZoneEarthtube object capabilities #6627 (first phase--vertical solution)
This pull request contains the code, unit tests, IDD changes, new test file, and documentation for the earth tube enhancement that implements a 1-D solution simulating the vertical temperature variation resulting from the presence of an earth tube. The implementation follows as closely as possible the NFP/design document that went through a thorough review spanning several technicalities calls. Evidence that the solution is correctly integrated and producing expected results will be attached to this pull request. Also, a "virtual walkthrough" explaining the code changes will also be attached.

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.

Reviewer

This will not be exhaustively relevant to every PR.

Initial submission of the new feature proposal for the 1-D (vertical) earth tube enhancement to EnergyPlus.

Changes were made to the NFP to include the comments made during the 5/3/23 technicalities meeting regarding the assumptions made about the earth tube node and contacting an expert on earth tubes from outside the development team.

Changes made to the NFP based on comments/conversation that took place over Slack on 5/11/23.

Modifications as a result of last technicalities call.

First drafts of the documentation in both the IO Ref and Eng Ref. Change of a variable name to better reflect what it actually is. Additions to the IDD.

this doesn't work, but i need to merge in other things

First successful build!

Correction of minor issue

Fixed an issue with a blank solution type. It should now interpret the input when solution type is blank correctly (default back to Basic). There should be no diffs in the ci results (hopefully).

Various minor issues fixed. This now runs but the results are not correct for the vertical solution.

Working code with results that look like one would expect. More testing and then unit tests and docs final edits to come.

Addition of two unit tests and a new regression test. Minor documentation additions and tweaks.

RKStrand · 2023-08-07T19:12:09Z

Supporting graphs:
EarthTubeSupportingGraphs.pdf

RKStrand · 2023-08-07T19:12:35Z

"Virtual" Walkthrough Notes:
Earth Tube Enhancement.pdf

Myoldmopar

This is really great, and looks to be in good shape. I had a few comments, so I'll hold from marking it approve/reject right now. Let me know what you think about my comments.

Myoldmopar · 2023-08-07T19:18:20Z

.gitignore

@@ -89,3 +89,4 @@ dist
 # if you generate sphinx docs, it builds a dummy version of the schema in the idd folder, just ignore it
 /idd/Energy+.schema.epJSON
 /idd/Energy+.schema.epJSON.in
+design/FY2023/earth_tube_solution_space_diagram.pdf


Why would this file be ignored? I don't anticipate it changing, so it shouldn't matter much, but I think it should be included.

Ok. I had to go back and look up what was going on here. It's essentially a duplicate file. I created the diagram in MS Powerpoint, saved it as a PDF, then opened in Preview and saved it as a .png to match the file type of other documents. Since it was a duplicate and just a transition file, I didn't think it was worth cluttering the GitHub space. It's essentially the same as the file of the same name with a .png extension. You still want this? I'm ok either way.

Myoldmopar · 2023-08-07T19:19:31Z

doc/input-output-reference/src/overview/group-airflow.tex

@@ -1884,6 +1931,22 @@ \subsubsection{Outputs}\label{zoneearthtube-outputs}

 This is the humidity ratio of the air entering the zone after passing through the earth tube {[}kgWater/kgDryAir{]}.

+\paragraph{Earth Tube Node Temperature <X> {[}C{]}}\label{earth-tube-node-temperatures-c}
+
+This will generate the internal node temperatures {[}C{]} for the earth tube as a result of the 1-D finite difference model.  This is only valid for the 1-D (Vertical) model.


This will report the internal node temperatures...

Right? I don't think this will trigger the generation of anything.

Yes, correct--it just triggers the reporting. These always get calculated. And before I forget, thanks for reviewing this!

Myoldmopar · 2023-08-07T19:20:29Z

doc/input-output-reference/src/overview/group-airflow.tex

+
+This will report the theoretical undisturbed ground temperature at the upper boundary for the 1-D finite difference model.  This is only valid for the 1-D (Vertical) model.
+
+\paragraph{Earth Tube Lower Boundary Ground Temperature {[}C{]}}\label{earth-tube-upper-boundary-temperature-c}


GitHub's doc build is complaining about the multiply defined label here. Need the relabel as -lower-boundary ...

Oh, rats. Copied and pasted and then forgot to update that part of the statement. I'll fix that (hopefully first thing tomorrow).

Myoldmopar · 2023-08-07T19:20:46Z

idd/Energy+.idd.in

      \note  "D" in Equation
      \type real
      \default 0
+ A5,  \field Earth Tube Model Type


Yay for no transition! Right?

Yes, correct. Intentionally done that way. The old model is still the default (notice that the existing earth tube idf in the test suite did not get changed and it runs with no diffs).

Myoldmopar · 2023-08-07T19:21:12Z

idd/Energy+.idd.in

+      \object-list EarthTubeParameterNames
+
+ZoneEarthtube:Parameters,
+      \memo Parameters that apply to the vertical model for an earth tube


Should we do a min-fields here?

Well, I guess that technically everything has a default so if the user just gave a name and ignored the rest it would just take all the defaults? I wasn't sure so I just left that out. I can put it in if you feel like this is important.

Myoldmopar · 2023-08-07T19:25:57Z

src/EnergyPlus/EarthTube.cc

@@ -543,6 +631,188 @@ void CheckEarthTubesInZones(EnergyPlusData &state,
    }
 }

+void initEarthTubeVertical(EnergyPlusData &state)


It would be really great if this function did not loop over all the earth tubes. If this could just be a one-time init flag on each earth tube, it would be preferable. I actually don't think it's too much of an effort to change, but I'm open to discussion.

That could be done. The loop over all earth tubes could happen one level up with the continue for a non-vertical model one happening at that point. It would eliminate the need for the multiple "continues" in every loop and would probably eliminate the need for loops in the initEarthTubeVertical subroutine. I'll take a look at this when I make other changes requested above.

Honestly, if it's working, I would just leave it alone. We have a new way to call each plant object "one time". This is nice because then the components can avoid having to constantly check "if one time ... if one time". As such, plant components definitely shouldn't be looping over each plant instance to do one time inits. I am a little foggy it seems, and I wanted to apply that same plant logic here to the earth tubes. But it's not necessary. Just leave this, thanks.

Ok. It is working so I'm also happy to leave it alone.

Myoldmopar · 2023-08-07T19:27:53Z

src/EnergyPlus/EarthTube.cc

+                thisEarthTube.tUndist[nodeNum] = thisEarthTube.calcUndisturbedGroundTemperature(state, thisEarthTube.depthNode[nodeNum]);
+            }
+        }
+    } // ...end of BeginDayFlag block


Will this get hit multiple times while BeginDayFlag is true? Do you need a second flag like "MyBeginDayFlag" so that it only happens once per day?

I'll double check. I was trying to avoid a duplicate calculation but I may have missed something.

Yeah, this was a problem. I took a page out of the ElectricPowerServiceManager.cc and decided to use "timeElapsed" to avoid duplicate initializations and improper updates. Will be testing this out here shortly.

Myoldmopar · 2023-08-07T19:28:14Z

src/EnergyPlus/EarthTube.cc

+{
+    // Calculate c' for when effectiveness is zero.  Will use these values when there is no air flow through the earth tube
+    // and also use the values in the top portion of the solution (before the earth tube node) since these will not change.
+    this->cPrime0[0] = this->cCoeff0[0] / this->bCoeff[0];


Nice little member method!

Thanks! 😀

Myoldmopar · 2023-08-07T19:32:07Z

src/EnergyPlus/EarthTube.cc

+        bool tempShutDown = false;
+        // Don't simulate for Basic Solution if the zone is below the minimum temperature limit
+        if (thisZoneHB.MAT < thisEarthTube.MinTemperature) tempShutDown = true;
+        // Don't simulate for Basic Solution if the zone is above the maximum temperature limit
+        if (thisZoneHB.MAT > thisEarthTube.MaxTemperature) tempShutDown = true;


Can this section just be:

bool tempShutDown = thisZoneHB.MAT > earthTube.MaxTemperature || thisZoneHB.MAT < earthTube.MinTemperature;

Oooo, I guess I hadn't thought about such an assignment being possible and was trying to maintain as much similarity to the old code. But this looks slick (or as the kids say these days "sick"), so I'll implement this also. This "old dog" (me) needs to learn a few new tricks...

Yeah, in Python it could just use an interval comparison 😱

tempShutDown = not (earthTube.MinTemperature < thisZoneHB.MAT < earthTube.MaxTemperature)

I had such grand hopes last school year. My now 16 yo was still doing homeschooling and so we were working through a Python intro course together. Unfortunately I had too much to do to keep up with him. Anyway, that sounds cool!

Myoldmopar · 2023-08-07T19:34:10Z

src/EnergyPlus/EarthTube.hh

@@ -85,7 +93,7 @@ namespace EarthTube {
        Real64 FanPressure = 0.0;
        Real64 FanEfficiency = 0.0;
        Real64 FanPower = 0.0;
-        Real64 GroundTempz1z2t = 0.0; // ground temp between z1 and z2 at time t
+        Real64 GroundTempt = 0.0; // ground temp at the depth of the earth tube midpoint at time t


Lead us not to GroundTempt-ation.

🤣🤣🤣...great comment! Yeah, I noticed the "tempt" part and chucked before just deciding that it was at least consistent with what the old name was. Well, maybe it should have been GroundTempzt then...but since it was a single temperature and I wanted that to be very clear.

Modifications to address review comments.

RKStrand · 2023-08-08T17:07:20Z

@Myoldmopar Just pushed code mods that will hopefully address the comments you had. Should come back all green unless I missed something. And if I missed a comment, let me know. Thanks!

Used the wrong term to size vectors. Should fix the issues with Linux debug errors.

Found a problem with one of the loops in the vertical algorithm, resulted from bad job moving stuff around. Xcode was very forgiving, Ubuntu not so much. This should make them both happy.

Myoldmopar

Thanks for addressing my comments! I'll do a quick build/test now but I assume this will merge shortly.

Myoldmopar · 2023-08-15T20:20:34Z

All good here locally, merging this one in. Thanks for this enhancement @RKStrand !

jmarrec · 2023-09-12T14:32:00Z

idd/Energy+.idd.in

+ N3,  \field Earth Tube Dimensionless Boundary Above
+      \note When set to 1.0, the upper boundary is one earth tube radius below ground.
+      \type real
+      \units dimensionless
+      \minimum 0.25
+      \maximum 1.0
+      \default 1.0 
+ N4,  \field Earth Tube Dimensionless Boundary Above
+      \note When set to 1.0, the upper boundary is one earth tube radius below ground.
+      \type real
+      \units dimensionless
+      \minimum 0.25
+      \maximum 1.0
+      \default 0.25


N4 is below. Note should be updated too.

Yes, you are correct--N4 should say below. Sorry!

@Myoldmopar Any chance we can slide in a typo correction to the field name in the IDD or is that not allowed. Sorry about the bad cut/paste/non-edit.

jmarrec · 2023-09-12T14:32:58Z

design/FY2023/NFP-EarthTube-1DEnhancement.md

I think this is an outdated document compared to the Design-Document one no?

Yes, the design document is more up-to-date.

@RKStrand

Note sure about the \note @RKStrand cf #10138 (review)

RKStrand added 20 commits April 24, 2023 10:59

NFP - initial submission

a745abb

Initial submission of the new feature proposal for the 1-D (vertical) earth tube enhancement to EnergyPlus.

Revision (Version 2) of NFP

e23014d

Changes were made to the NFP to include the comments made during the 5/3/23 technicalities meeting regarding the assumptions made about the earth tube node and contacting an expert on earth tubes from outside the development team.

Changes from Slack Convo 5/11/23

32ffa7a

Changes made to the NFP based on comments/conversation that took place over Slack on 5/11/23.

Merge branch 'develop' into 5114327-EarthTube1DVerticalEnhancement

04d98dd

Create Design-Document-EarthTube-1DEnhancement.md

7e71a22

Merge branch 'develop' into 5114327-EarthTube1DVerticalEnhancement

1f0dd9d

Version 5 of Design Document

db2e651

Modifications as a result of last technicalities call.

Merge branch 'develop' into 5114327-EarthTube1DVerticalEnhancement

201ddea

Merge branch 'develop' into 5114327-EarthTube1DVerticalEnhancement

86cfaa2

Documentation First Draft and Miscellaneous

353170b

First drafts of the documentation in both the IO Ref and Eng Ref. Change of a variable name to better reflect what it actually is. Additions to the IDD.

Merge branch 'develop' into 5114327-EarthTube1DVerticalEnhancement

2161c77

intermediate commit

2febfe7

this doesn't work, but i need to merge in other things

Merge branch 'develop' into 5114327-EarthTube1DVerticalEnhancement

c299795

Fix build and format issues

6699d6d

First successful build!

Fix minor format issue

d1370a2

Correction of minor issue

Fix input problem for blank type

b22afa2

Fixed an issue with a blank solution type. It should now interpret the input when solution type is blank correctly (default back to Basic). There should be no diffs in the ci results (hopefully).

Fix a variety of minor issues

9084340

Various minor issues fixed. This now runs but the results are not correct for the vertical solution.

It's working, it's working!

3a5796f

Working code with results that look like one would expect. More testing and then unit tests and docs final edits to come.

Merge branch 'develop' into 5114327-EarthTube1DVerticalEnhancement

ae8bdd3

Unit and Regression Test

a251c56

Addition of two unit tests and a new regression test. Minor documentation additions and tweaks.

RKStrand added the NewFeature Includes code to add a new feature to EnergyPlus label Aug 7, 2023

RKStrand added this to the EnergyPlus 23.2 IOFreeze milestone Aug 7, 2023

RKStrand requested review from Myoldmopar and mjwitte August 7, 2023 19:11

RKStrand self-assigned this Aug 7, 2023

Myoldmopar reviewed Aug 7, 2023

View reviewed changes

Review Comment Edits

992d459

Modifications to address review comments.

RKStrand added 2 commits August 8, 2023 16:41

Fix to allocation of vectors

1a7b1b2

Used the wrong term to size vectors. Should fix the issues with Linux debug errors.

Negative index correction

a445934

Found a problem with one of the loops in the vertical algorithm, resulted from bad job moving stuff around. Xcode was very forgiving, Ubuntu not so much. This should make them both happy.

Myoldmopar approved these changes Aug 15, 2023

View reviewed changes

Myoldmopar merged commit 2a89af7 into develop Aug 15, 2023
10 checks passed

Myoldmopar deleted the 5114327-EarthTube1DVerticalEnhancement branch August 15, 2023 20:20

jmarrec reviewed Sep 12, 2023

View reviewed changes

jmarrec added a commit that referenced this pull request Sep 12, 2023

Duplicated ZoneEarthTube:Parameters field name

344cf13

Note sure about the \note @RKStrand cf #10138 (review)

jmarrec mentioned this pull request Sep 12, 2023

V23.2.0-IOFreeze: IDD and transition fixes #10215

Merged

20 tasks


		This will report the theoretical undisturbed ground temperature at the upper boundary for the 1-D finite difference model. This is only valid for the 1-D (Vertical) model.

		\paragraph{Earth Tube Lower Boundary Ground Temperature {[}C{]}}\label{earth-tube-upper-boundary-temperature-c}

Add Vertical (1-D) Solution to Earth Tube Object #10138

Add Vertical (1-D) Solution to Earth Tube Object #10138

Conversation

RKStrand commented Aug 7, 2023

Pull request overview

Pull Request Author

Reviewer

RKStrand commented Aug 7, 2023

RKStrand commented Aug 7, 2023

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

RKStrand commented Aug 8, 2023

Myoldmopar left a comment

Choose a reason for hiding this comment

Myoldmopar commented Aug 15, 2023

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