Implement new slinky ground heat exchanger simulation model #4692

Myoldmopar · 2015-01-23T17:06:09Z

Making this a PR for convenience for conversation.

Changes caused regression diffs which need to be addressed.

QGLHE is still not initialized at the beginning of each environment.

DeadParrot · 2015-01-24T00:21:44Z

@mmAtTs Good stuff but why the this->member usage? It is normally not needed within methods to disambiguate and just adds visual clutter. Where you have local variables with the same name as members it would be best to rename the locals to avoid silently hiding the members and confusing future developers.

Myoldmopar · 2015-01-25T17:11:10Z

That was my doing @DeadParrot. Although some of the devs have an understanding of what an instance is, some don't. Putting the this pointer in there was my way of being clear when referencing a class instance variable or method. I'm sure the compiler doesn't care and creates the same assembly either way, so it shouldn't make any difference on performance. I won't take any hard stand on the issue, just keep in mind the status of the development team when discussing it.

DeadParrot · 2015-01-25T17:58:33Z

@Myoldmopar OK. I agree that it makes no difference to the compiled code: this issue is whether it helps users more than it hinders them. I can see some points in its favor. The factors I see are:

Visual clutter that makes it harder to see what is going on (and could push lines to be wider than screen)
Didn't have an analog of this-> in Fortran so it shouldn't be needed as a transitional crutch
It helps Intellisense work better so that is something
Other than that good IDEs should know a variable is the local class member without this->
If you refactor a function into a different class or to a non-class function (happens fairly often) you have to go remove all the old this-> stuff.
Using this-> is probably more comfortable for Python-istas (but the self use in Python is considered a language flaw by many).
If you have a naming ambiguity that requires this-> then renaming the non-member variable is probably a good idea.
I believe using this-> when not needed is not as common but certainly there are those who like it: http://stackoverflow.com/questions/6779645/use-of-this-keyword-in-c

It would be a good idea to set up some standards before the code becomes a mish-mosh of different styles. C++ offers too much stylistic freedom to let everyone do their own thing.

…is->

Still has issue where QGLHE is not initialized at the beginning of each environment. I haven't been able to track this bug down yet.

nrel-bot · 2015-02-02T00:33:55Z

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

…roundHeatExchanger into calcGroundHeatExchanger because a separate function to update the object was no longer needed.

Conflicts: src/EnergyPlus/GroundHeatExchangers.cc

Modifying idd/Energy+.idd.in Adding GSHP:Slinky to the DataPlant.cc and PlantManger.cc, among others. Some cleanup related to merging with develop that was missed earlier.

…t building getInput now.

Finishing the major changes to GetGroundHeatExchangerInput.

…ical slinky g-function code and integrate with plantloops.

…n length.

nrel-bot · 2015-02-13T00:46:53Z

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

mitchute · 2015-02-13T01:48:50Z

@Myoldmopar could you look at this briefly and let me know if I'm headed in a good direction WRT unit testing. I'm pretty much done implementing the g-function generation code and am planning to move on to GLHESlinky::calcGroundHeatExchanger tomorrow.

Myoldmopar · 2015-02-13T03:30:18Z

src/EnergyPlus/GroundHeatExchangers.cc

+
+	Real64
+	GLHESlinky::distance(
+	int const m,


Random note; some of these function signatures should have indented arguments. Others already do. Just noting.

Myoldmopar · 2015-02-13T03:40:22Z

@mmAtTs Holy shoot this is looking so good, it's exciting.

With the performance PR merged in, you are now out of date with develop -- again -- sorry -- but it had to happen sometime. Anyway, I can assist with resolving conflicts. OK, I resolved them and pushed with beef0f7. The conflicts were pretty minor so just take a quick pass at the updated Files Changed tab and make sure nothing looks goofy.

As for your comment, you were saying you were still passing indices into functions? Could you give an example?

DeadParrot · 2015-02-13T04:33:00Z

src/EnergyPlus/GroundHeatExchangers.cc

+		sqrtAlphaT = std::sqrt( diffusivityGround * t );
+
+		distance = distToCenter( m, n, m1, n1 );
+		sqrtDistDepth = std::sqrt( std::pow( distance, 2.0 ) + 4 * std::pow( coilDepth, 2.0 ) );


Performance note: pow_2( x ) is faster than std::pow( x, 2 ) and should be preferred. Yes, these really add up to matter in some of our profiles. We have pow_2 through pow_9 and a few others in ObjexxFCL/Fmath.hh.

Thanks @DeadParrot, your eagle eyes are very helpful! I do have a performance sub-checklist on my review checklist, so I've added this as another thing to watch for in PRs.

mitchute · 2015-02-13T15:07:41Z

@Myoldmopar @DeadParrot thanks for the tips. I will get it cleaned up.

mitchute · 2015-02-13T15:20:32Z

@Myoldmopar Currently the arrays holding the (X,Y,Z) coordinates for each ring live here at the object level. We're looping through all possible combinations of rings here and then within that, to get the distances between any pair of rings, I just pass the correct indices through several functions to the distance function. Would it be easier for unit testing purposes to just pass the (X,Y,Z) center values for each pair of rings through, or can we make this work.

DeadParrot · 2015-02-20T23:05:22Z

@Myoldmopar to clarify how the "member array" hack works in ObjexxFCL, because name() is already set up as a member for all 1D arrays verticalGLHE.name() should work fine without any customization required specific to verticalGLHE, as long as its elements have a name std::string member.

We may want to discourage new use of member array syntax but for common usage such as name() it should work "out of the box". When someone wants to expose a member with a name not already set up then it would require the (discouraged) addition to the appropriate *.Projects.MArray.hh file.

… platforms. Still needed: -Final g-function validation -Final slinky model validation. -Current relative diffs for the vertical borehole models are around 10^-3. One final pass to track those down, if possible. -Potentially the most important issue of all: resolving why "DisplayString" is not printing "Calculating g-Functions" to the terminal.

… midFieldResponseFunction.

Myoldmopar · 2015-02-24T04:08:11Z

Looks like it might still be having trouble with Name. I will look tomorrow morning, but it might be that the .Name() is still being used inside the sim routine. Maybe. Just guessing while on my phone...

Myoldmopar · 2015-02-24T11:54:15Z

@mmAtTs OK, I got it building with commit beef740. I also cleanup up some unneeded trailing semicolons. I didn't actually run it, but it should at least build now. It is very interesting that Windows seemed to build that just fine.

One thing to note is that in the Sim function, we won't be able to do the string comparison to check which type we have; it will be too slow. I think it's probably time to bite the bullet and store pointers to the base class in a single array rather than two separate arrays, one per type. Let's chat today about it.

…nto 1540218-SlinkyGroundHX

… unused timeSS from GLHESlinky::getAnnualTimeConstant.

…sistency with current code.

…inkyGroundHX

…base 10, and vertical GLHEs are log base e.

nrel-bot · 2015-03-03T03:32:01Z

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

Myoldmopar · 2015-03-03T12:50:50Z

@mmAtTs the test results look good, just tiny diff's in a few files, I'm guessing similar to what you were seeing locally. I think this is ready to merge, let me know if you were all doing anything else.

mitchute · 2015-03-03T12:59:48Z

@Myoldmopar let's hold off for a little bit. I just tracked down the source of the tiny diffs. I'm going to push that right now. Pending CI and any recommendations from you or others on the unit tests, it should be good.

…. Other minor cleanups.

…nto 1540218-SlinkyGroundHX

Changing to pass in typeNum rather than type to avoid long string comparisons.

Myoldmopar · 2015-03-03T16:12:40Z

@mmAtTs Yay! 100% passed! 👍

I'll let CI make a pass at 10d0493 and this should drop in after. Great work.

Myoldmopar · 2015-03-03T17:26:12Z

Oh, @mmAtTs the CI results might turn red due to diffs since develop has moved since your commit. I'll look over the results and deem whether we need to merge develop or not.

@mmAtTs

The diffs are all the same from the commit earlier into develop; nothing to do with your branch. Merging in now. Thanks for this @mmAtTs.

Matt Mitchell added 4 commits January 20, 2015 14:37

First pass at modifying GLHE models to make them object oriented.

a5e2e94

More work at introducing OOP to GLHE.

5b35bf7

Changes caused regression diffs which need to be addressed.

More work incorporating OOP in the GLHE code.

1875158

Correcting errors causing regression diffs.

3caafe1

QGLHE is still not initialized at the beginning of each environment.

Matt Mitchell added 3 commits January 29, 2015 16:12

Start at making variables camelCase

85c700a

Cleaning up local variables. Removing, where possible, the use of: th…

77733f1

…is->

Some code cleanup after working some OOP into the GLHE.

122be1d

Still has issue where QGLHE is not initialized at the beginning of each environment. I haven't been able to track this bug down yet.

Matt Mitchell added 4 commits February 4, 2015 11:58

More cleanup removing unnecessary local variables. Moving the updateG…

100179a

…roundHeatExchanger into calcGroundHeatExchanger because a separate function to update the object was no longer needed.

Merge branch 'develop' into 1540218-SlinkyGroundHX

b223a67

Conflicts: src/EnergyPlus/GroundHeatExchangers.cc

Adding input GSHP-Slinky.idf input file.

def1ffa

Modifying idd/Energy+.idd.in Adding GSHP:Slinky to the DataPlant.cc and PlantManger.cc, among others. Some cleanup related to merging with develop that was missed earlier.

Some cleanup from setting up the new model in the plant. Need to star…

4f7eda4

…t building getInput now.

Myoldmopar assigned mitchute Feb 5, 2015

Matt Mitchell added 5 commits February 6, 2015 15:57

Minor changes to IDD and GSHP-Slinky.idf.

ab2fbc0

Finishing the major changes to GetGroundHeatExchangerInput.

Horizontal slinky g-function code implemented. Need to implement vert…

418165e

…ical slinky g-function code and integrate with plantloops.

Added changes to allowing calculation of vertical g-functions.

94aef01

Added field in IDD and IDF to allow user to specify maximum simulatio…

f965b76

…n length.

Merge branch 'develop' into 1540218-SlinkyGroundHX

b0d7d0d

Myoldmopar reviewed Feb 13, 2015
View reviewed changes

Merge develop; resolve conflicts

beef0f7

DeadParrot reviewed Feb 13, 2015
View reviewed changes

Myoldmopar added the NewFeature Includes code to add a new feature to EnergyPlus label Feb 22, 2015

Matt Mitchell added 2 commits February 23, 2015 11:11

Restructuring "name" for VerifyName. Fixed missing parentheses in the…

f24f9fe

… midFieldResponseFunction.

Remove Sim use of .Name(); other minor cleanup

beef740

Matt Mitchell added 10 commits February 24, 2015 08:10

Bug fix where ground temperature was not being updated.

a0a498d

Merge branch '1540218-SlinkyGroundHX' of github.com:NREL/EnergyPlus i…

37ea996

…nto 1540218-SlinkyGroundHX

Moved AGG and SubAGG to the getInput routine for consistency. Removed…

23a57f9

… unused timeSS from GLHESlinky::getAnnualTimeConstant.

Changed slinky g-functions to be generated using base 'e' to keep con…

7026a66

…sistency with current code.

Merge remote-tracking branch 'remotes/origin/develop' into 1540218-Sl…

f055f2f

…inkyGroundHX

Moved more into getGFunc routines because slinky g-functions are log …

517ee87

…base 10, and vertical GLHEs are log base e.

Adding comments. Some other cleanups.

0dfc625

Stubbing out unit tests for GHX's.

3eb936f

Adding unit tests.

ec09436

Merge branch 'develop' into 1540218-SlinkyGroundHX

ac45e44

Matt Mitchell added 4 commits March 3, 2015 07:01

Fixed bug prevHour was not reset at the beginning of each environment…

6513246

…. Other minor cleanups.

Unit test cleanup.

393cbd4

Merge branch '1540218-SlinkyGroundHX' of github.com:NREL/EnergyPlus i…

ca5e498

…nto 1540218-SlinkyGroundHX

Changing EXPECT_DOUBLE_EQ to EXPECT_NEAR in the unit tests.

10d0493

Changing to pass in typeNum rather than type to avoid long string comparisons.

Myoldmopar added a commit that referenced this pull request Mar 3, 2015

Merge pull request #4692 from NREL/1540218-SlinkyGroundHX

c2c2631

The diffs are all the same from the commit earlier into develop; nothing to do with your branch. Merging in now. Thanks for this @mmAtTs.

Myoldmopar merged commit c2c2631 into develop Mar 3, 2015

Myoldmopar deleted the 1540218-SlinkyGroundHX branch March 3, 2015 20:11

Myoldmopar changed the title ~~1540218 slinky ground hx~~ Implement new slinky ground heat exchanger simulation model Mar 7, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement new slinky ground heat exchanger simulation model #4692

Implement new slinky ground heat exchanger simulation model #4692

Myoldmopar commented Jan 23, 2015

DeadParrot commented Jan 24, 2015

Myoldmopar commented Jan 25, 2015

DeadParrot commented Jan 25, 2015

nrel-bot commented Feb 2, 2015

nrel-bot commented Feb 13, 2015

mitchute commented Feb 13, 2015

Myoldmopar Feb 13, 2015

Myoldmopar commented Feb 13, 2015

DeadParrot Feb 13, 2015

Myoldmopar Feb 13, 2015

mitchute commented Feb 13, 2015

mitchute commented Feb 13, 2015

DeadParrot commented Feb 20, 2015

Myoldmopar commented Feb 24, 2015

Myoldmopar commented Feb 24, 2015

nrel-bot commented Mar 3, 2015

Myoldmopar commented Mar 3, 2015

mitchute commented Mar 3, 2015

Myoldmopar commented Mar 3, 2015

Myoldmopar commented Mar 3, 2015

Implement new slinky ground heat exchanger simulation model #4692

Implement new slinky ground heat exchanger simulation model #4692

Conversation

Myoldmopar commented Jan 23, 2015

DeadParrot commented Jan 24, 2015

Myoldmopar commented Jan 25, 2015

DeadParrot commented Jan 25, 2015

nrel-bot commented Feb 2, 2015

nrel-bot commented Feb 13, 2015

mitchute commented Feb 13, 2015

Myoldmopar Feb 13, 2015

Choose a reason for hiding this comment

Myoldmopar commented Feb 13, 2015

DeadParrot Feb 13, 2015

Choose a reason for hiding this comment

Myoldmopar Feb 13, 2015

Choose a reason for hiding this comment

mitchute commented Feb 13, 2015

mitchute commented Feb 13, 2015

DeadParrot commented Feb 20, 2015

Myoldmopar commented Feb 24, 2015

Myoldmopar commented Feb 24, 2015

nrel-bot commented Mar 3, 2015

Myoldmopar commented Mar 3, 2015

mitchute commented Mar 3, 2015

Myoldmopar commented Mar 3, 2015

Myoldmopar commented Mar 3, 2015