Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GHE g-function calculation enhancements #8708

Merged
merged 106 commits into from
Aug 19, 2021

Conversation

j-c-cook
Copy link
Contributor

@j-c-cook j-c-cook commented Apr 10, 2021

Pull request overview

A C++ library, named cpgfunctionEP, with the ability to compute
g-functions has been developed by Jack C. Cook, a student of Dr.
Jeffrey D. Spitler at Oklahoma State University.

The cpgfunction library is a low level implementation of Massimo
Cimmino's g-function methodology. The code was developed with the aim
of increasing performance (compared to Cimmino's open source Python
implementation, pygfunction), in both memory and time to be used in
the generation of g-function databases on high performance computing
clusters (HPCC). The resulting program makes it more practical to
compute g-functions for larger ground heat exchangers on a desktop
computer.

This proposal specifically aims to address issue #6651, to provide
EnergyPlus with an enhanced g-function calculation. The library will be
located in the third party folder.

Pull Request Author

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

  • Title of PR should be user-synopsis style (clearly understandable in a standalone changelog context)
  • Label the PR with at least one of: Defect, Refactoring, NewFeature, Performance, and/or DoNoPublish
  • Pull requests that impact EnergyPlus code must also include unit tests to cover enhancement or defect repair
  • Author should provide a "walkthrough" of relevant code changes using a GitHub code review comment process
  • If any diffs are expected, author must demonstrate they are justified using plots and descriptions
  • If changes fix a defect, the fix should be demonstrated in plots and descriptions
  • If any defect files are updated to a more recent version, upload new versions here or on DevSupport
  • If IDD requires transition, transition source, rules, ExpandObjects, and IDFs must be updated, and add IDDChange label
  • If structural output changes, add to output rules file and add OutputChange label
  • If adding/removing any LaTeX docs or figures, update that document's CMakeLists file dependencies

Reviewer

This will not be exhaustively relevant to every PR.

  • Perform a Code Review on GitHub
  • If branch is behind develop, merge develop and build locally to check for side effects of the merge
  • If defect, verify by running develop branch and reproducing defect, then running PR and reproducing fix
  • If feature, test running new feature, try creative ways to break it
  • CI status: all green or justified
  • Check that performance is not impacted (CI Linux results include performance check)
  • Run Unit Test(s) locally
  • Check any new function arguments for performance impacts
  • Verify IDF naming conventions and styles, memos and notes and defaults
  • If new idf included, locally check the err file and other outputs

j_c_cook added 2 commits April 9, 2021 18:40
A new feature proposal draft for a fast and accurate g-function
calculation is included in this commit. A C++ library, named
cpgfunction, with the ability to compute g-functions has been
developed by Jack C. Cook, a student of Dr. Jeffrey D. Spitler at
Oklahoma State University.

The cpgfunction library is a low level implementation of Massimo
Cimmino's g-function methodology. The code was developed
with the aim of increasing performance (compared to Cimmino's open
source Python implementation, pygfunction), in both memory and time to
be used in the generation of g-function databases on high performance
computing clusters (HPCC). The resulting program makes it more practical
to compute g-functions for larger ground heat exchangers on a desktop
computer.

More information about the g-function calculation can be found in the
NFP.

This proposal specifically aims to address issue NREL#6651, to provide
EnergyPlus with a 3rd party g-function calculation.
The NFP mentions proposals for new keys for the
GroundHeatExchanger:System. This would result in new lines in IDF and
IDD sections. Sample IDF and IDD are presented with proposed changes.
It is helpful for the reader to quickly be able to distinguish the
new lines.

The code blocks in markdown do not allow for highlighting (e.g. bold,
underline, etc.). Additionally, html font highlighting did not appear
to render in the .md file on github. The changed lines were placed
below, and were intended to be highlighted in red (rendering md in atom
offline looked fine), but were not. Therefore, the proposed changed
lines now begin with a ">" to show distinction.
@j-c-cook j-c-cook force-pushed the 6651_cpgfunction_thirdparty branch from c6063ec to e042488 Compare April 10, 2021 01:34
@mitchute mitchute added the NewFeature Includes code to add a new feature to EnergyPlus label Apr 10, 2021
@mitchute mitchute added this to the EnergyPlus Future milestone Apr 10, 2021
@mitchute
Copy link
Collaborator

This NFP is in good shape.

One final comment regarding the NFP: it looks like the gains from the "adaptive integration scheme" are really going to be significant here. You have a plot showing the memory reductions in going from pygfunction to cpgfunction, and then to cpgfunction with the ADS (plot with red arrows). If you have it, it would be nice to show a similar plot for time reductions. This is obviously dependent on the GHE configuration, but an example would be useful. If you don't have it right now, we don't need to hold this up for it.

This is really going to be a nice addition to E+!

FYI: @j-c-cook @jeffreyspitler

@jmarrec
Copy link
Contributor

jmarrec commented Apr 20, 2021

Just a tiny note, note that the diff language marker can be used for your IDD snippets instead of adding a > to mark new lines.

eg:

! IDF example using GHE:Vertical:Array input
GroundHeatExchanger:System,
  Vertical GHE 1x4 Std,    !- Name
  GLHE Inlet,              !- Inlet Node Name
  GLHE Outlet,             !- Outlet Node Name
  0.004,                   !- Design Flow Rate {m3/s}
  Site:GroundTemperature:Undisturbed:KusudaAchenbach,  !- Undisturbed Ground Temperature Model Type
  KATemps,                 !- Undisturbed Ground Temperature Model Name
  2.5,                     !- Ground Thermal Conductivity {W/m-K}
  2.5E+06,                 !- Ground Thermal Heat Capacity {J/m3-K}
  ,                        !- GHE:Vertical:ResponseFactors Object Name
+ UHFcalc,                 !- g-function Calculation Model Name
  GHE-Array;               !- GHE:Vertical:Array Object Name

it also works for modifications

- something old
+ something new

@nrel-bot-2c
Copy link

@j-c-cook @lgentile it has been 28 days since this pull request was last updated.

j_c_cook added 21 commits June 8, 2021 19:40
As noted by @jmarrec, there is a way to highlight lines that conain
  - something old
  + something new

NREL#8708 (comment)
For the diff highlighting to work on github, the !'s, +'s and -'s must
be at the beginning of a new line.
On the EnergyPlus call Wednesday, June 30, 2021, a path was laid out
for the future of cpgfunction. The project has been forked. There now
exists a fork by the name of cpgfunctionEP.

cpgfuncitonEP (a fork of cpgfunction) will contain neither Fortran
nor Boost dependencies.
@mitchute
Copy link
Collaborator

I've run this branch on two different Linux boxes, in release and debug modes. It looks like the segfaults are now gone. @Myoldmopar over to you for a final review and merge.

@Myoldmopar
Copy link
Member

Building now, thanks @mitchute !

@Myoldmopar
Copy link
Member

Concur, everything is passing now. I'll be happy to resolve the conflict and do final review here.

Copy link
Member

@Myoldmopar Myoldmopar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks really close. I noted a few minor tweaks that I'm going to make in a follow-up commit. This also needs a minor transition rule which @mitchute is doing now. Otherwise this should be ready to go in.

if (state.dataGlobal->numThread == 0) {
DisplayString(state, "Invalid value for -j arg. Defaulting to 1.");
state.dataGlobal->numThread = 1;
} else if (state.dataGlobal->numThread > (int)std::thread::hardware_concurrency()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting. I'm not sure we really need to take the burden of restricting this. And on some platforms where this cannot be determined, it is required to return 0, thus making EnergyPlus fail. I think I'll take this out.

@@ -149,6 +152,8 @@ namespace CommandLineInterface {

opt.add("", false, 0, 0, "Display version information", "-v", "--version");

opt.add("1", false, 1, 0, "Multi-thread with N threads; 1 thread with no arg.", "-j", "--jobs");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm adding a little phrase in here to describe the GHE usage so it shows up in the usage() display.

@Myoldmopar
Copy link
Member

Thanks for the transition update @mitchute. I took a GSHP file that did not have the full list of fields and it left the object alone. I took GSHP-GLHE-CalcGFunctions.idf and transitioned it to find the new default field name put in there. It's working as expected. I have a few small tweaks and will do final testing, but I anticipate this is good to go in next/shortly.

@Myoldmopar
Copy link
Member

Alright, this is looking great. I pulled in develop one more time and did a full build and test. Unit and integration tests are passing fine, and there are no unexpected regressions. This is good to go in. I'll push my small commit up and then get it merged. Thanks @j-c-cook and @mitchute !

@Myoldmopar
Copy link
Member

Forgot about the Windows warnings, so addressed them real quick. Heads up @j-c-cook these changes were in the new third party code.

@@ -49,7 +49,7 @@ namespace gt::segments {
nq = jcc::interpolation::interp1d(drilling_depth,
drilling_depths[0],
ideal_segment_lengths[0]);
} else if(heights[0] < height < heights[n1]) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In C++, x < y < z doesn't do what you think it does. C++ does not have chained operation. This will perform one binary comparison, which in this case returns a boolean, then try to compare that boolean to a floating point with the second binary operation. I'm assuming you just want to do x < y && y < z, so that's what I did here. This nifty syntax works in Python but not C++.

@@ -9,7 +9,7 @@ void jcc::decomposition(vector<vector<double> > &A, int &n, vector<int> &indx,
double &d) {
const double TINY = 1.0e-20; // a small number
int i, imax, j, k;
double big, dum, sum, temp;
double big, dum, sum;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unused variable removd.

@@ -376,7 +376,7 @@ class Adaptive<BaseMethod,Estimator,true,true,false,true> :
template<typename BM>
Adaptive<BM> adaptive(const BM& bm)
{
return Adaptive<BM>(bm, 1.e-5, 1.e-10);
return Adaptive<BM>(bm, 1.e-5f, 1.e-10f);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Explicitly creating float literals instead of double to avoid the warning.

@Myoldmopar
Copy link
Member

Alright, I'm merging this now. Thanks all.

@Myoldmopar Myoldmopar merged commit 2632627 into NREL:develop Aug 19, 2021
@Myoldmopar Myoldmopar deleted the 6651_cpgfunction_thirdparty branch August 19, 2021 18:08
@rraustad
Copy link
Contributor

rraustad commented Aug 20, 2021

@j-c-cook why do I have to add #include <algorithm> to boreholes.cpp and adaptive.h to get MS Visual Studio Professional 2019 Version 16.5.0 to compile? In boreholes.cpp was a max statement where MSVS said identifier max not found and in adaptive.h is a std::max and MSVS says max is not a member. When I type in std::m I don't see max in the pop up list as an available function. I also don't understand why adding #include <algorithm> in adaptive.h corrects a statement of std::max ?? since I did not change that line from std::max to max.

Do you have any insight into this?

image

@rraustad
Copy link
Contributor

rraustad commented Aug 20, 2021

In develop boreholes.cpp I see this in MSVS.

image

@j-c-cook
Copy link
Contributor Author

j-c-cook commented Aug 20, 2021

I cannot say anything definitive about how MS Visual Studio Professional 2019 Version 16.5.0 operates. I do nearly all development in Kubuntu 20.04.2, and compiled the code in Windows 10 only when the CI machine returned errors. I was under the impression that all errors in a Windows build or test would have been caught by the CI machines, and that the code would not have been merged before those errors were found. However, unforeseen bugs happen.

To eradicate this bug will I need to download a Visual Studio IDE specifically, or does compiling from the command prompt provide the same error?

@rraustad
Copy link
Contributor

I can just commit this small change as it does not appear to change anything other than which headers are included. I just wanted to ask before I made that commit. It's only 2 lines of code, 1 in each file, and only a #include. Why I see this compiler warning and others who use MSVS do not is a mystery to me (other than they haven't pulled develop yet).

@j-c-cook
Copy link
Contributor Author

Okay thank you! I'll make sure those changes propagate their way back into cpgfunctionEP's repo, along with some refactors Edwin had made as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
IDDChange Code changes impact the IDD file (cannot be merged after IO freeze) NewFeature Includes code to add a new feature to EnergyPlus
Projects
None yet
Development

Successfully merging this pull request may close these issues.