Add the cubic spline interpolation option for PerformancePrecisionTradeoffs #8946

LipingWang · 2021-08-04T01:10:57Z

Pull request overview

A new override option was added as Mode06 for the field Override Mode of the PerformancePrecisionTradeoffs object. Mode06 offers an alternative method to calculate saturated temperature given pressure in replacement of the psychrometric function PsyTsatFnPb. The previous options for the Override Mode of the object including Mode06 and Mode07 in version 9.5 were switched to Mode07 and Mode08 in version 9.6, respectively.

Code Changes

Added the new override option (Mode06) for the field Override Mode of the PerformancePrecisionTradeoffs object in SimulationManager.cc;
Modified and updated the existing override modes of this object in SimulationManager.cc;
Added a function of cubic spline interpolation (CSplineint) to Psychrometrics.cc;
Modified the existing psychrometric function PsyTsatFnPb with the option to use cubic spline interpolation for calculating saturated temperature;
IDD: added the new option as described in Energy+.idd;
Version update: added transition rules and descriptions to CreatNewIDFUsingRulesV9_6_0.f90 and Rules9-5-0-to9-6-0.md;
Reference: added two additional runs including the new override option and updated the testing results and InputOutputReference;
Unit tests: added two unit tests for the verification of interpolation sample data and result comparison between cubic spline interpolation and the original psychrometric function PsychTsatFnPb.

NOTE: ENHANCEMENTS MUST FOLLOW A SUBMISSION PROCESS INCLUDING A FEATURE PROPOSAL AND DESIGN DOCUMENT PRIOR TO SUBMITTING CODE

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.

amirroth · 2021-08-04T01:35:46Z

src/EnergyPlus/Psychrometrics.hh

@@ -129,7 +129,405 @@ namespace Psychrometrics {
                                // 14 - HR | 15 - max iter | 16 - HR | 17 - max iter | 18 -
                                // PsyTwbFnTdbWPb_raw (raw calc) | 19 - PsyPsatFnTemp_raw
                                // (raw calc)
-
+// sample bin size =64 Pa; sample size =1651 (continous)
+     constexpr std::array<Real64, 1651> tsat_fn_pb_x = {


I don't think this array is necessary since the contents are just index * 64, right?

Good point, the array is just 64x an integer so we can probably either populate it with a for loop or eliminate it.

There is no point in populating it with a loop. If you are going to do that, you may as well leave it as is. Just replace the lookup with a multiplication.

@amirroth @JasonGlazer Thanks a lot for your comments! I will update the code to eliminate the pressure input array and replace it with a multiplication.

amirroth · 2021-08-04T01:38:54Z

src/EnergyPlus/PsychCacheData.hh

@@ -100,7 +100,7 @@ constexpr Int64 psatcache_mask = psatcache_size - 1;
 #endif
 #ifdef EP_cache_PsyTsatFnPb
 constexpr int tsatcache_size = 1024 * 1024;
-constexpr int tsatprecision_bits = 24;
+constexpr int tsatprecision_bits = 20;


I think if convTol is set to 0.01C, this can go down to 16.

I asked @LipingWang to look at this earlier and we found that while it increases speed slightly to go from 20 to 16, it also increases the differences in energy, so we decided to stick with 20. This was on a small set of buildings and we can evaluate that with a larger set of buildings to get a more accurate picture if you want.

Myoldmopar · 2021-08-06T13:56:40Z

Is this actually planned for 9.6 IO Freeze or is it just a sandbox to evaluate the approach?

JasonGlazer · 2021-08-06T14:13:11Z

It is for the 9.6.0 IO freeze.

…deOff_CSpline

JasonGlazer · 2021-08-18T14:26:58Z

I think this is ready for review and merge so I'm taking it out of draft state. At this point, only the MacOS CI tests have run but I expect that it just that the CI machines are backed up with jobs. I am putting myself down as a reviewer.

Myoldmopar

This looks generally fine. I will tweak the fortran file to fix the indentation, but otherwise the implementation is fine. The unit test seems ... odd. I'm not going to hold the PR for a different unit test at this point, but I would like a comment about it and most likely a modification to the unit test soon so we don't have a huge array just duplicated out in the unit test that becomes a maintenance problem if something changes.

Myoldmopar · 2021-08-19T20:28:44Z

doc/input-output-reference/src/overview/group-simulation-parameters.tex

+16 & FALSE & CarrollMRT & MODE05 & 1 & 1 & TRUE  & 60 & Normal & 0.30 & 0.002 & 47.31  \tabularnewline
+17 & FALSE & CarrollMRT & MODE06 & 1 & 1 & TRUE  & 60 & Interpolate & 0.30 & 0.002 & 45.62  \tabularnewline
+18 & FALSE & CarrollMRT & MODE07 & 1 & 1 & TRUE  & 60 & Interpolate & 1.00 & 0.002 & 45.23  \tabularnewline
+19 & FALSE & CarrollMRT & MODE08 & 1 & 1 & TRUE  & 60 & Interpolate & 1.00 & 0.1   & 44.09  \tabularnewline


Myoldmopar · 2021-08-19T20:28:57Z

doc/input-output-reference/src/overview/group-simulation-parameters.tex

 Advanced& Allow direct input of convergence field values\tabularnewline

 \bottomrule
 \end{longtable}

-The increasing mode number roughly corresponds with increased speed. Mode01 to Mode05 are overriding inputs in other parts of the IDF/epJSON file and is equivalent to changing those input directly. Mode06 and Mode07 are changing convergence parameters previously not available to be modified by the user.
+The increasing mode number roughly corresponds with increased speed. Mode01 to Mode05 are overriding inputs in other parts of the IDF/epJSON file and is equivalent to changing those input directly. Mode06 offers an alternative method to calculate saturated temperature given pressure in replacement of the psychrometric function PsyTsatFnPb. Mode07 and Mode08 are changing convergence parameters previously not available to be modified by the user.


Myoldmopar · 2021-08-19T20:29:30Z

idd/Energy+.idd.in

@@ -483,6 +483,7 @@ PerformancePrecisionTradeoffs,
      \key Mode05
      \key Mode06
      \key Mode07
+      \key Mode08


OK, so Mode06 was inserted, making a new 08 one. Transition rules to follow I see. 👍

Myoldmopar · 2021-08-19T20:31:07Z

src/EnergyPlus/Psychrometrics.hh

+        -1.9E-09,     -2.8E-09,     -2E-10,       -0.000000004, -2.5E-09,     -1E-10,       -4.6E-09,     -3E-10,       -2.5E-09,     -3.8E-09,
+        -3E-10,       -2.6E-09,     -2.3E-09,     -2.1E-09,     -2.2E-09,     -2.1E-09,     -2.4E-09,     -2.1E-09,     -2.2E-09,     -0.000000002,
+        -2.4E-09,     -9E-10,       -3.3E-09,     -2.2E-09,     -4E-10,       -4.1E-09,     -1.4E-09,     -2.9E-09,     4E-10,        -5.5E-09,
+        2.7E-09}; // namespace Psychrometrics


Myoldmopar · 2021-08-19T20:32:23Z

src/Transition/CreateNewIDFUsingRulesV9_6_0.f90

@@ -426,7 +426,16 @@ SUBROUTINE CreateNewIDFUsingRules(EndOfFile,DiffOnly,InLfn,AskForInput,InputFile
              ! If your original object starts with R, insert the rules here

              ! If your original object starts with S, insert the rules here
-
+			  CASE('PERFORMANCEPRECISIONTRADEOFFS')


Mixed tab/space here causing weird indent, I'll fix it since I need to resolve a tiny conflict anyway.

@Myoldmopar thank you!

Myoldmopar · 2021-08-19T20:33:12Z

src/Transition/InputRulesFiles/Rules9-5-0-to-9-6-0.md

+Summary: A new override option including cubic spline interpolations in replacement of original psychrometric function PsyTsatFnPb was added as Mode06 in this object. The previous options for the override mode of the object including Mode06 and Mode07 in version 9.5 were switched to Mode07 and Mode08 in version 9.6, respectively.
+
+Fields 1-2 remains the same.
+Fields 3 has been updated by adding a new override mode option. 


Could be described a bit better in this line, but I guess the paragraph above basically says it.

Myoldmopar · 2021-08-19T20:34:46Z

tst/EnergyPlus/unit/Psychrometrics.unit.cc

+        100.6190344, 100.636391,   100.6537386,  100.6710809,  100.6884104,  100.7057304,  100.7230452,  100.7403472,  100.7576436,  100.7749305,
+        100.7922053, 100.809475,   100.8267359,  100.8439874,  100.8612302,  100.8784641,  100.8956893,  100.9129053,  100.9301125,  100.947311,
+        100.9645009, 100.9816821,  100.9988569,  101.0160203,  101.0331752,  101.0503248,  101.067462,   101.0845908,  101.101711,   101.1188265,
+        101.1359293};


Is this just the same array as inside the code? I'm not sure I understand what this unit test is doing then.

@Myoldmopar I should have added more details for this unit test. Yes, this sample array tsat_fn_pb_tsat is the same sample array for saturated temperature as inside the code tsat_fn_pb_y. The purpose of this unit test is to test the correctness of the sample data array. The sample data were extracted from the original psychrometric function PsyTsatFnPb every 64 Pa in the range of 64 Pa to 105,664 Pa. The sample data for saturated temperature from tsat_fn_pb_tsat was compared to the results from the original psychrometric function PsyTsatFnPb. So we can double-check the correctness of the sample data inside the code.

@LipingWang the idea of the unit test is good but instead of repeating the array, we should be testing the array that is created in the header file and used by the interpolation function.

@JasonGlazer @Myoldmopar sounds great!

Myoldmopar · 2021-08-19T20:43:43Z

Tested out the branch, everything built and tested just fine. Played with the transition rules and it transitions objects as expected, whether they don't have that field at all, have a field value that should stay the same, or have a field value that should be converted. This seems good to go in, but would like to follow-up with at least commentary about the unit test later. Thanks @LipingWang and thanks @JasonGlazer for the review.

amirroth · 2021-08-20T13:41:37Z

src/EnergyPlus/Psychrometrics.cc

@@ -1439,6 +1444,26 @@ namespace Psychrometrics {

        return Temp;
    }
+    Real64 CSplineint(int const n,     // sample data size
+                      const Real64 &x) // given value of x


There is no reason to pass scalars by const &. If this is only an input, it should be passed by value. The only parameters that should be passed by reference are output parameters (passing by reference is the only) and objects (passing by value will trigger the copy constructor which 99% of the time you don't want to do).

@amirroth Many thanks for your comments and the details! I will drop the const &.

LipingWang added 7 commits July 28, 2021 13:32

CSpline 64 Pa + 20 Bits

9ff9149

convTol

0895545

Merge branch 'NREL:develop' into Branch_TradeOff_CSpline

f5fafd1

unit test for psychrometrics

d867236

unit test and code update

4601908

I/O reference (draft)

57e14e6

clang-formate

872ecf1

amirroth reviewed Aug 4, 2021

View reviewed changes

JasonGlazer marked this pull request as draft August 4, 2021 14:24

JasonGlazer assigned JasonGlazer and LipingWang and unassigned JasonGlazer Aug 4, 2021

JasonGlazer added the NewFeature Includes code to add a new feature to EnergyPlus label Aug 4, 2021

JasonGlazer added this to the EnergyPlus 9.6 IOFreeze milestone Aug 4, 2021

LipingWang added 7 commits August 4, 2021 20:47

simplify pressure input

dc25d7d

clean up

dd7de8a

Clang format

8518700

Clang format

4845b7b

Clang format

50534cd

Clang format

40cbc89

Clang format

f3b1f64

LipingWang added 6 commits August 6, 2021 23:11

Clang format

79fc3e2

Clang format

edf69bd

clang format

084f940

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

0d78c9a

…deOff_CSpline

Clang format

2d4e85b

clang format

489e0e0

mjwitte and others added 5 commits August 10, 2021 10:12

Fix format Psychrometrics.hh

bb2e35a

Merge branch 'NREL:develop' into Branch_TradeOff_CSpline

bc8f0d7

tsatprecision_bits test

205937f

Clang format CacheData.hh

29a3710

add transition rules & remove unused &state

705728f

mjwitte added the IDDChange Code changes impact the IDD file (cannot be merged after IO freeze) label Aug 17, 2021

LipingWang changed the title ~~Test the new option for the PerformancePrecisionTradeoffs object~~ Add a new option using cubic spline interpolations for the PerformancePrecisionTradeoffs object Aug 17, 2021

LipingWang changed the title ~~Add a new option using cubic spline interpolations for the PerformancePrecisionTradeoffs object~~ Add a new option using cubic spline interpolations for PerformancePrecisionTradeoffs Aug 17, 2021

Format update

89afc3e

LipingWang changed the title ~~Add a new option using cubic spline interpolations for PerformancePrecisionTradeoffs~~ Add the cubic spline interpolation option for PerformancePrecisionTradeoffs Aug 17, 2021

JasonGlazer marked this pull request as ready for review August 18, 2021 14:26

JasonGlazer requested review from Myoldmopar, mjwitte and JasonGlazer August 18, 2021 14:27

Merge upstream develop; resolve conflict in input rules file

8ce8c9a

Myoldmopar reviewed Aug 19, 2021

View reviewed changes

Tweak indent in transition code

74636cb

Myoldmopar merged commit d763b10 into NREL:develop Aug 19, 2021

amirroth reviewed Aug 20, 2021

View reviewed changes

JasonGlazer mentioned this pull request Aug 30, 2021

Change from 24 bit to 20 bit caching for PsyTsatFnPb when using PerformancePrecisionTradeoffs Mode06 or higher #9024

Merged

20 tasks

LipingWang mentioned this pull request Sep 1, 2021

Interpolation set draft for test runs #8872

Closed

20 tasks

shorowit mentioned this pull request Sep 2, 2021

Update to EnergyPlus v9.6/OpenStudio v3.3 NREL/OpenStudio-HPXML#839

Closed

10 tasks

shorowit mentioned this pull request Sep 29, 2021

Independently allow cubic spline interpolation option in PerformancePrecisionTradeoffs #9107

Open

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add the cubic spline interpolation option for PerformancePrecisionTradeoffs #8946

Add the cubic spline interpolation option for PerformancePrecisionTradeoffs #8946

LipingWang commented Aug 4, 2021 •

edited

Loading

amirroth Aug 4, 2021

JasonGlazer Aug 4, 2021

amirroth Aug 4, 2021

LipingWang Aug 4, 2021

amirroth Aug 4, 2021

JasonGlazer Aug 4, 2021

Myoldmopar commented Aug 6, 2021

JasonGlazer commented Aug 6, 2021

JasonGlazer commented Aug 18, 2021

Myoldmopar left a comment

Myoldmopar Aug 19, 2021

Myoldmopar Aug 19, 2021

Myoldmopar Aug 19, 2021

Myoldmopar Aug 19, 2021

Myoldmopar Aug 19, 2021

LipingWang Aug 19, 2021

Myoldmopar Aug 19, 2021

Myoldmopar Aug 19, 2021

LipingWang Aug 19, 2021

JasonGlazer Aug 20, 2021

LipingWang Aug 20, 2021

Myoldmopar commented Aug 19, 2021

amirroth Aug 20, 2021

LipingWang Aug 20, 2021

Add the cubic spline interpolation option for PerformancePrecisionTradeoffs #8946

Add the cubic spline interpolation option for PerformancePrecisionTradeoffs #8946

Conversation

LipingWang commented Aug 4, 2021 • edited Loading

Pull request overview

Code Changes

Pull Request Author

Reviewer

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 Aug 6, 2021

JasonGlazer commented Aug 6, 2021

JasonGlazer commented Aug 18, 2021

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

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 Aug 19, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LipingWang commented Aug 4, 2021 •

edited

Loading