Remove unused second flow rate input from vertical GLHEs

[mitchute]

@Myoldmopar CI says this is good but I'll let you take a quick pass at reviewing it. I think it will need a new transition rule since this removes an unused field from the IDF/IDD objects. Let me know if you want me to help on that. I will submit the doc changes here shortly.

[mjwitte]

@mmAtTs Ignore the question about IDD changes, I had followed the commit link rather than landing here, somehow. Adding cross reference to the issue here. Fixes #4660

[mitchute]

@mjwitte Eng. Ref. does not mention the fields specifically and the I/O Ref. only says, basically: "this is the max flow rate" and "this is the design flow rate." All this fix does is clarify to the user that there really is only one field that they need, which is the design flow rate. AND, it gets them labeled correctly. Before this, the only field being used was the Max Flow Rate, which was being treated as the Design Flow Rate by E+. The Design Flow Rate field was not being used at all. See the following for reference:
https://github.com/NREL/EnergyPlus/compare/4660-GLHE_Vertical_Input_Fixes#diff-98ca5f748365bfb6b2c62bf804d7f532R756

https://github.com/NREL/EnergyPlus/compare/4660-GLHE_Vertical_Input_Fixes#diff-3290c38a47a0cedf1cb5132ba3769d64R3157

We could implement your suggestion, but I'd have to go back into documentation and figure out exactly what the authors intended. Perhaps @Myoldmopar could comment on this. In the past we've discussed some issues associated with flow rates and this model.

Regarding why some of the fields had different values, I have to assume people took the fields at face value without checking them.

[mjwitte]

@mmAtTs Ok thanks. No need to revisit, thanks for the cleanup.

[Myoldmopar]

@mmAtTs One last merge of develop as of right now into this branch to get all clean dashboard results, then it's mergeable. Are you able to post the doc modifications here for the input change?

[mitchute]

@Myoldmopar Yes, I'll put them up right now.

[Myoldmopar]

Good grief look at all that green!! 😉

Once develop is stabilized later this morning I'll merge this in.

[DeadParrot]

@Myoldmopar @mmAtTs Just a heads up that I'm getting a core dump for the GSHP-GLHE and GSHPSimple-GLHE cases (array bounds violation detected with debug Linux build). In this line in GroundHeatExchangers::INTERP:

if ( LnTTsVal <= VerticalGlhe( GlheNum ).LNTTS( 1 ) ) {

LNTTS doesn't have an element with index 1 so it is probably unallocated (or allocated with zero size). Haven't tracked it down further but seems probable that this merge is the cause. LNTTS is allocated to the size of NPairs, which is tied to rNumericArgs( 15 ). Maybe the input arg numbering changes didn't get applied here?

[mitchute]

@Myoldmopar @DeadParrot: from develop, I'm not seeing any issues related to that line. I did see, however, that I missed updating the error messages calls, so I will need to submit a fix for that. Regarding the uninitialized LNTTS array, I don't have an answer at the moment. When I test it on my machine (Win 7, MSVS) it all looks good. Any thoughts of what else I could try?

[Myoldmopar]

I'm testing on Linux now; I'll let you know if I also see it.

[Myoldmopar]

So, the GSHP-GLHE file seemed to test fine, no issues at all, with a fresh debug build of develop right now. ... I'll also test the simple file next.

[Myoldmopar]

Yes the GSHPSimple file runs fine over here for me as well. @DeadParrot can you confirm that develop is still broken for you as of right now (04ae45c)?

[DeadParrot]

@Myoldmopar @mmAtTs OK, let me experiment further. It is possible my run picked up the 8.2.0 idd instead of the new one and that would explain the argument numbering problem.

[DeadParrot]

@Myoldmopar @mmAtTs When I run the updated idd and idf it does run OK for me so it is probably a false alarm -- sorry for the noise. I have some non-GLHE cases core dumping that I still have to figure out but they are probably also due to the wrong idd/idf version.

Questions:

1. How serious/common is the issue this fixes?
2. What is the best way of setting up an "installed" version of EnergyPlus to test against before an official installer is posted?

[Myoldmopar]

By "this issue", do you mean removing the unused input field? If so, it was simply a field left over from some older model implementation that just never got noticed. It wasn't used, so @mmAtTs removed it and just used the single flow rate input.

For testing out an installer, in cmake just turn on BUILD_PACKAGE, and the run make package. It will build up a full installer and you can install it on your machine. I usually have multiple installed at once.

For testing in general, I just run ctest -R "<case name>" from the cmake build folder. That automatically executes the correct binaries, idd, and idf for that exact build, so no worries on conflicts.

[DeadParrot]

@Myoldmopar Thanks for the tips. Not sure yet how to turn on BUILD_PACKAGE from the cmake command line.

I said "issue" because this PR title says "fix" but I didn't see a fix other than the input removal you describe so I thought I was missing something. Sounds like I wasn't.

[kbenne]

Use the curses interface on the command line. ccmake.

On Jan 22, 2015, at 6:41 PM, Stuart Mentzer <notifications@github.zerozr99.workers.devmailto:notifications@github.com> wrote:

@Myoldmoparhttps://github.com/Myoldmopar Thanks for the tips. Not sure yet how to turn on BUILD_PACKAGE from the cmake command line.

I said "issue" because this PR title says "fix" but I didn't see a fix other than the input removal you describe so I thought I was missing something. Sounds like I wasn't.

Reply to this email directly or view it on GitHubhttps://github.com//pull/4661#issuecomment-71128617.

[DeadParrot]

@Myoldmopar @kbenne Thanks. The package build works fine on Fedora from cmake-gui (didn't try ccmake yet). I'm not sure why it makes sh, gz and Z versions of the installer but the sh installed OK on Fedora for me. (The official release builds for 8.2.7 are still rolling out and the Linux installer hasn't shown up yet.)

[kbenne]

By default cpack will turn on all of the packaging options that are supported by your configuration. You can turn them off using the advanced options in the cmake configuration. Here it is in the curses interface. You type "t" to see the advanced options.

It is possible to pass configuration options using the non interactive (non curses) cmake command using something like cmake -DBUILD_PACKAGE:BOOL=ON. Normally this is tedious for a human. We use it mostly in the continuous integration system.

[DeadParrot]

@kbenne Great info! Thanks. I like to have the scriptable method for everything saved away.