Update GHE Interpolation Routine #9535
Conversation
mitchute
added
the
Refactoring
| @@ -2957,99 +2956,40 @@ Real64 GLHESlinky::calcHXResistance(EnergyPlusData &state) | |||
|
|
|||
| //****************************************************************************** | |||
|
|
|||
| Real64 GLHEBase::interpGFunc(Real64 const LnTTsVal // The value of LN(t/TimeSS) that a g-function | |||
| ) const | |||
| Real64 GLHEBase::interpGFunc(Real64 const LnTTsVal) const | |||
There was a problem hiding this comment.
Let's see, this function is only about 100x easier to read and understand now. This is great. I might have a few suggested tweaks, but this seems awesome.
| // to find the correct g-function value for a | ||
| // known value of the natural log of (T/Ts) | ||
| auto &x = this->myRespFactors->LNTTS; | ||
| auto &y = this->myRespFactors->GFNC; |
There was a problem hiding this comment.
Normally, using references to lists is not the right approach, but this is a bit different. In this case, you don't actually know which index you are going to use yet. Using x and y here make this logic extremely easy to read and understand. 👍
| // Lund, Sweden, June 1987. | ||
| auto x_begin = x.begin(); | ||
| auto x_end = x.end(); | ||
| double x_val = LnTTsVal; |
There was a problem hiding this comment.
Is it worth using references for any of these? I'm not sure how heavy an iterator is, but usually we use references for anything heavier than POD. Also the references could be const I think just to make it clear.
| double x_high = x[u_idx]; | ||
| double y_low = y[l_idx]; | ||
| double y_high = y[u_idx]; | ||
| return (x_val - x_low) / (x_high - x_low) * (y_high - y_low) + y_low; |
There was a problem hiding this comment.
These intermediate variables need to be Real64. I was going to say you could also remove them entirely and just use the lookups in the function, but frankly, this makes it just so easy to see the interpolation, that I'd say leave them.
| EXPECT_NEAR(thisGHE.interpGFunc(-14.293207), -2.001293, tolerance); | ||
| EXPECT_NEAR(thisGHE.interpGFunc(-14.260100), -1.978820, tolerance); | ||
| } | ||
|
|
|
@mitchute, I'm curious if you'd consider using btwxt for the interpolation? It is a generalized n-dimensional interpolation library that is already linked to EnergyPlus and used for tabular interpolation elsewhere in the code. How many dimensions are you interpolating here? |
|
This is just a simple 1D interpolation. We could use btwxt here - I don't have a strong preference either way. I had to roll my own for a separate project, and I had this to hand already so that's why I threw it in. |
|
That's fair. Btwxt might be overkill if it's always 1-D. |
|
I fully agree that btwxt would be fine here, but also that's for sure overkill here in this always-1-D case. For now let's forge ahead with this super minimal updated version. |
|
@Myoldmopar comments have been addressed and this is ready for review. |
|
The tiny diffs are acceptable, but I don't want to muddy other CI results this afternoon, so I'll hold off from merging this until later. But it's ready. |
|
This one's next, testing locally now. |
|
All happy with latest develop pulled in, merging now, thanks @mitchute |
Pull request overview
In working on something else, I noticed that the GHE interpolation routine was pretty convoluted and more complicated than it needed to be. This updates it to be more straightforward and to use features from the stdlib. I've also added a few more unit tests.
I'm seeing small diffs, which I never fully got to the root cause of. When I write the interpolation input/output to a file for the DOAToWaterToAirHPSupply example file and then compare it against the scipy interpolation routine, I get a slightly better RMSE for the updated function. I don't think it's worth digging in any further. Both versions were working, but this one is more modern and is slightly more accurate.
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.