GHE Updates #8491

mitchute · 2021-01-29T17:14:02Z

Pull request overview

Moves GHE data to state
Moves GHE SetupOutputVars to shared member method. Note that this causes rdd, mdd, and aud diffs. See here for the offending commit: 68ab73f
Moves GHE input processing to ctor's that accept JSON input. This is a potential prototype for future refactoring when moving away from the traditional input processing methods.
Cleans up local variables
Add some missing unit test support

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.

…GLHEVertProps constructor to a JSON constructor

mjwitte · 2021-01-29T17:26:58Z

src/EnergyPlus/GroundHeatExchangers.cc

+            this->verticalConfig = false;
+        }
+
+        this->coilDiameter = j["coil_diameter"];


@mitchute This is an interesting direction to move in.

What happens if the field name string is misspelled? Will it throw an error or simply return a zero or blank?

What happens if the field isn't present in the incoming object? Will it be present in the object data with it's default if it has one?
I still feel that we need an input fetching utility to manage this as suggested in Add helper functions to validate and get an epJSON object and field value #7632 .
Or maybe fields with defaults are already populated in the JSON data structure when the input is read in? I've never been sure about that.

@mjwitte

Re 1. I just tested this by misspelling one field, depth_of_top_of_borehole. The following is the error message thrown.

** Severe ** [json.exception.type_error.302] type must be number, but is number

Not super helpful, but one would hope that these sort of errors never make it past the developer/reviewer since the model likely would never run. We should give this a little more thought.

Re 2. We handle fields that could be missing and which do NOT have default values like the following:

if (j.find("ghe_vertical_responsefactors_object_name") != j.end())

In this case, the field will not be present if it's not present in the input. I'd assume that fields with default values will have the default filled in automatically, but I have not tested that.

FFR, some issues I had with this transition are listed here:

The string fields come in as-is from the input. I found myself needing to upper case everything before comparing strings, whereas before I think some of this was done automatically. We'll need a way to keep some fields case sensitive, such as paths. However, the majority of strings don't need that, so when we go to scale this up we'll need to decide how to best handle it.

Currently, we have cAlphaFieldNames and cNumericFieldNames which allows us to programmatically get the field names. I didn't figure out a way to get these using JSON, but it probably exists. Either way, the names are likely to come in as the JSON key names. We may want a way to provide the user with the IDF-like field names? I.e. "inlet_node_name" vs. "Inlet Node Name".

This was my first real attempt at using the JSON inputs, so perhaps these issues were just due to my inexperience.

I am really excited about this pattern. And one of my favorite reasons is that unit testing the input processing and catching casing/other issues will be easy. Just call the constructor will different JSON objects and verify the created object matches the expectation. No need to do any other setup (unless the constructor is accessing something that needs to be set up).

Myoldmopar

I really like this. Having the input processor routine rewritten as an object constructor that accepts JSON input is something we've wanted for quite some time. We can critique this moving forward and optimize it as we improve more and more components.

Myoldmopar · 2021-02-01T15:58:14Z

src/EnergyPlus/GroundHeatExchangers.cc

+            this->verticalConfig = false;
+        }
+
+        this->coilDiameter = j["coil_diameter"];


I am really excited about this pattern. And one of my favorite reasons is that unit testing the input processing and catching casing/other issues will be easy. Just call the constructor will different JSON objects and verify the created object matches the expectation. No need to do any other setup (unless the constructor is accessing something that needs to be set up).

…_mods

Myoldmopar

All good here! Let me make sure the branch is up to date with develop and if not I'll just build it locally anyway prior to merging. Thanks @mitchute

Myoldmopar · 2021-02-05T20:20:13Z

src/EnergyPlus/GroundHeatExchangers.cc

-            sqrtDistDepth = std::sqrt(pow_2(distance1) + 4 * pow_2(coilDepth));
-            errFunc1 = std::erfc(0.5 * distance1 / sqrtAlphaT);
-            errFunc2 = std::erfc(0.5 * sqrtDistDepth / sqrtAlphaT);
+            Real64 sqrtDistDepth = std::sqrt(pow_2(distance1) + 4 * pow_2(this->coilDepth));


Localized variables, great.

Myoldmopar · 2021-02-05T20:20:31Z

src/EnergyPlus/GroundHeatExchangers.cc

                            // if the ring(n1, m1) is the near-field ring of the ring(n,m)
-                            if (disRing <= 2.5 + coilDiameter) {
+                            if (disRing <= 2.5 + this->coilDiameter) {


Member variables, awesome.

Myoldmopar · 2021-02-05T20:20:55Z

src/EnergyPlus/GroundHeatExchangers.cc

+        this->pipe.cp = j["pipe_specific_heat"];
+        this->pipe.outDia = j["pipe_outer_diameter"];
+        this->pipe.outRadius = this->pipe.outDia / 2.0;
+        this->pipe.thickness = j["pipe_thickness"];


Once again, really excited about this.

Myoldmopar · 2021-02-05T20:21:18Z

src/EnergyPlus/GroundHeatExchangers.cc

            }
        }
    }

    //******************************************************************************

+    void GLHEBase::setupOutput(EnergyPlusData &state)


Reorganization, causing rdd diffs. OK.

Myoldmopar · 2021-02-05T20:21:40Z

src/EnergyPlus/GroundHeatExchangers.hh


        void calcShortTimestepGFunctions(EnergyPlusData &state);

        void calcLongTimestepGFunctions(EnergyPlusData &state);

-        void calcGFunctions(EnergyPlusData &state);
+        void calcGFunctions(EnergyPlusData &state) override;


Proper override specification. Good.

Myoldmopar · 2021-02-05T20:21:59Z

src/EnergyPlus/StateManagement.cc

@@ -183,7 +182,6 @@ void EnergyPlus::clearAllStates(EnergyPlusData &state)
    Furnaces::clear_state();
    General::clear_state();
    GeneralRoutines_clear_state(); // GeneralRoutines does not have a namespace
-    GroundHeatExchangers::clear_state();


One less state, woohoo!

Myoldmopar · 2021-02-05T20:23:48Z

Branch is 36 commits ahead of develop so the latest CI results are still good. Windows failures are unrelated, and otherwise everything seems happy. Good to go, thanks @mitchute !

mitchute added 14 commits January 20, 2021 07:27

moving GHE data to state

979a6ea

apply formatting and other similar changes, fix C++ warnings

3d282ff

cleaning up local vars

0c10a62

move some parts to stand-alone initEnvironment methods

9efa243

move some parts to stand-along initEnvironment methods

64383ef

de-tabbify GHE files, remove 'struct' from GHE struct names, convert …

61dba39

…GLHEVertProps constructor to a JSON constructor

move GHE response factor input to JSON constructor

d4b8582

move GHE vert array input to JSON constructor

58482bf

move GHE vert single BH input to JSON constructor

37d3128

move GHE System input to JSON constructor

30f1d89

move SetupOutputVars to member method

aee166c

moving GHE Slinky input to JSON constructor

68ab73f

delete old input

70cb675

new unit tests, remove unnecessary bounds checking

1f75211

mitchute added the Refactoring Includes code changes that don't change the functionality of the program, just perform refactoring label Jan 29, 2021

mitchute added this to the EnergyPlus 9.5.0 milestone Jan 29, 2021

mitchute requested a review from Myoldmopar January 29, 2021 17:14

mitchute self-assigned this Jan 29, 2021

mjwitte reviewed Jan 29, 2021

View reviewed changes

Merge branch 'develop' into ghe_mods

5fb8108

Myoldmopar approved these changes Feb 1, 2021

View reviewed changes

mitchute added 4 commits February 1, 2021 10:17

add unit test to execise JSON ctor for slinky GHE

2410d71

Merge branch 'develop' into ghe_mods

bea5023

Merge branch 'develop' into ghe_mods

5abde26

Merge branch 'ghe_mods' of github.com:energy-plus/EnergyPlus into ghe…

dc795ea

…_mods

Myoldmopar approved these changes Feb 5, 2021

View reviewed changes

Myoldmopar merged commit 883e5bc into NREL:develop Feb 5, 2021

Myoldmopar deleted the ghe_mods branch February 5, 2021 20:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GHE Updates #8491

GHE Updates #8491

mitchute commented Jan 29, 2021 •

edited

Loading

mjwitte Jan 29, 2021

mitchute Jan 29, 2021 •

edited

Loading

Myoldmopar Feb 1, 2021

Myoldmopar left a comment

Myoldmopar Feb 1, 2021

Myoldmopar left a comment

Myoldmopar Feb 5, 2021

Myoldmopar Feb 5, 2021

Myoldmopar Feb 5, 2021

Myoldmopar Feb 5, 2021

Myoldmopar Feb 5, 2021

Myoldmopar Feb 5, 2021

Myoldmopar commented Feb 5, 2021

GHE Updates #8491

GHE Updates #8491

Conversation

mitchute commented Jan 29, 2021 • edited Loading

Pull request overview

Pull Request Author

Reviewer

Choose a reason for hiding this comment

mitchute Jan 29, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Myoldmopar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

Myoldmopar commented Feb 5, 2021

mitchute commented Jan 29, 2021 •

edited

Loading

mitchute Jan 29, 2021 •

edited

Loading