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.

Sign up for GitHub

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

Jump to bottom

FluidProperties API #10616

Merged

Myoldmopar merged 11 commits into develop from FluidAPI

Jul 24, 2024

Collaborator

amirroth commented Jul 21, 2024

This is a cleanup and quick refactor of the FluidProperties module. It is enabled by the recent init_state() PR which calls EnergyPlusData::initi_state() early in the simulation. FluidProperties::init_state() is the second call made by EnergyPlusData::init_state() ensuring that FluidProperty are created before any objects that reference them.

This allows us to replace the GetPropertyGlycol(state, glycolName, arg1, arg2, glycolIndex, calledFrom) API which loads FluidProperty data on first call for any fluid, and has to locate the fluidIndex on first call to each fluid from each context with a cleaner GlycolProps::getProperty(state, arg1, arg2, calledFrom) API that relies on the fluid object being located on construction. That is the main change in this PR. For now, the old API has been retained and has been turned into a skinny-wrapper for the new API. Only one module (HVACVariableRefrigerantFlow) has been moved to the new API. When the rest of the modules have moved, either opportunistically or in a pass, the old API can be deleted.

Several other changes:

GetFluidPropertiesInput was refactored. The old code used nested loops where the outer loop iterated over fluids, then for each property of each fluid iterated over FluidProperties objects to find the right object. With this approach, FluidProperties objects were read and multiple times. In the worst case, each object had to be read N times where is the number of FluidProperties objects. The new code uses a single loop that iterates over FluidProperties objects and acts based on which property and which fluid they refer to. In addition to being about 1,200 lines shorter, this refactor also sped up the DataSetFixture.FluidProperties unit test by a factor of 3, from 55 seconds to 18.
There are two local performance optimizations. One caches the index of most recent temperature "bin" used in a given call to avoid finding that bin again in the next call. Another precomputes (Property(bin+1) - Property(bin)) / (Temp(bin+1) - (Temp(bin)) for every bin. This expression is the scaling factor used in interpolation of property values within a bin. I don't know how much time these save because the performance suite CI machines appear to be down. These optimizations are guarded by #ifdef PERFORMANCE_OPT which is currently not defined.
There are about 30 files with small diffs due to reordered/regrouped computations and one "large" diff which is a change in timestamp by 10 minutes. Turning on the performance optimizations mentioned in the previous bullet increases the number of files with diffs by a factor of four. Again, there is no such thing as true commutativity and associativity in fixed-precision math. Order of operations matters. It may only matter in the 14th significant digit, but it matters.

amirroth added 8 commits

June 26, 2024 08:02


          namespace Fluid

5d4b8d2


          Fluid properties API with one usage example (VRF)

f864177


          merge develop, resolve conflicts

26ef273


          Some local cleanup

907bf19


          GetInput cleanup

a242572


          Fix initialization and API issues

bf4b914


          More fixes and performance testing

90faf9d


          Disable performance optimizations

08e24a1

amirroth requested a review from Myoldmopar

July 21, 2024 01:46

amirroth assigned amirroth and Myoldmopar

amirroth added DoNotPublish Refactoring labels

amirroth added this to the EnergyPlus 24.2 milestone

amirroth commented

View reviewed changes

src/EnergyPlus/FluidProperties.hh

               #ifdef EP_cache_GlycolSpecificHeat
                   int constexpr t_sh_cache_size = 1024 * 1024;
                   int constexpr t_sh_precision_bits = 24;
                   std::uint64_t constexpr t_sh_cache_mask = (t_sh_cache_size - 1);
               #endif
-                  struct FluidPropsRefrigerantData
+                  enum class RefrigError

Collaborator Author

amirroth Jul 21, 2024

Error enums.

src/EnergyPlus/FluidProperties.hh

-                      }
+                      std::array<ErrorCountIndex, (int)RefrigError::Num> errors;
+                      Real64 getQuality(EnergyPlusData &state,

Collaborator Author

amirroth Jul 21, 2024

New object oriented API.

src/EnergyPlus/FluidProperties.hh

-                      int CondLowTempIndex;       // Low Temperature Min Index for Cond (>0.0)
-                      int CondHighTempIndex;      // High Temperature Max Index for Cond (>0.0)
+              #ifdef PERFORMANCE_OPT
+                      int LoRhoTempIdxLast = 1;

Collaborator Author

amirroth Jul 21, 2024

For performance optimization.

src/EnergyPlus/FluidProperties.hh

-                  constexpr static std::string_view EthyleneGlycol("EthyleneGlycol");
-                  constexpr static std::string_view PropyleneGlycol("PropyleneGlycol");
+              #undef PERFORMANCE_OPT

Collaborator Author

amirroth Jul 21, 2024

Change this to #define PERFORMANCE_OPT to enable the local optimizations.

src/EnergyPlus/FluidProperties.hh

+                                          int &RefrigIndex,               // Index to Refrigerant Properties
+                                          std::string_view CalledFrom     // routine this function was called from (error messages)
+                  );
                   Real64 GetSatPressureRefrig(EnergyPlusData &state,

Collaborator Author

amirroth Jul 21, 2024

Old API is still here. Can get rid of it once all uses have been converted.

src/EnergyPlus/FluidProperties.hh Outdated

@@ @@ -666,7 +636,12 @@ struct FluidPropertiesData : BaseGlobalStruct @@
                   void clear_state() override
                   {
-                      new (this) FluidPropertiesData();
+                      for (int i = 1; i <= refrigs.isize(); ++i) delete refrigs(i);

Collaborator Author

amirroth Jul 21, 2024

Using arrays of pointers to fluid objects, so remember to delete in clear_state.

src/EnergyPlus/HVACVariableRefrigerantFlow.cc

-                      if (Util::FindItemInList(thisVrfFluidCtrl.RefrigerantName, state.dataFluidProps->RefrigData, state.dataFluidProps->NumOfRefrigerants) == 0) {
-                          ShowSevereError(state, cCurrentModuleObject + " = " + thisVrfFluidCtrl.Name);
-                          ShowContinueError(state, "Illegal " + cAlphaFieldNames(4) + " = " + cAlphaArgs(4));
+                      thisVrfFluidCtrl.refrigName = cAlphaArgs(4);

Collaborator Author

amirroth Jul 21, 2024

Grabbing the pointer to the refrigerant here so that we can use the new fluid API.

Member

Myoldmopar Jul 24, 2024

Got it. And as with the plant component work, the refrig pointer is not owning. It's just an address, and we need to check null before using it. Good to go.

Collaborator Author

amirroth Jul 24, 2024

You don't really need to check it, unless the option of not having a fluid at all is a legitimate option. Pointer ownership is about who has to free the memory when it is no longer used, not about checking for nullptr.

Member

Myoldmopar Jul 24, 2024

I just mean check for null once during get input routines in case there is an invalid refrigerant string passed in from user input to the GetRefrig function. Once state is all setup, definitely shouldn't be checking it over and over again.

src/EnergyPlus/HVACVariableRefrigerantFlow.cc

-                  RefTHigh = state.dataFluidProps->RefrigData(RefrigerantIndex).PsHighTempValue; // High Temperature Value for Ps (max in tables)
-                  RefPLow = state.dataFluidProps->RefrigData(RefrigerantIndex).PsLowPresValue;   // Low Pressure Value for Ps (>0.0)
-                  RefPHigh = state.dataFluidProps->RefrigData(RefrigerantIndex).PsHighPresValue; // High Pressure Value for Ps (max in tables)
+                  RefMinPe = this->refrig->getSatPressure(state, RefMinTe, RoutineName);

Collaborator Author

amirroth Jul 21, 2024

New refrigerant API.

src/EnergyPlus/UtilityRoutines.cc

                       state, " ** Severe  ** " + Message, MsgIndex, ReportMaxOf, ReportMinOf, ReportSumOf, ReportMaxUnits, ReportMinUnits, ReportSumUnits);
               }
+              void ShowRecurringSevereErrorAtEnd(EnergyPlusData &state,

Collaborator Author

amirroth Jul 21, 2024

Added this reduced argument wrapper for ShowRecurringError.

third_party/ObjexxFCL/src/ObjexxFCL/Array1S.hh

		@@ -213,6 +213,18 @@ public: // Assignment: Array

		public: // Assignment: Value

		template< typename U, Size s, class = typename std::enable_if< std::is_assignable< T&, U >::value >::type >

Collaborator Author

amirroth Jul 21, 2024

Added template for assigning std::array to Array1S (i.e., Array1 slices of higher dimensional ArrayXD).

Member

Myoldmopar Jul 24, 2024

Not gonna lie, it's a little offputting to see more being added to Array1D after we've spent so much time stripping it down. Nothing red flag, just surprising. I wonder if this method was already available in the past and it wasn't used so it was removed at some point.

Collaborator Author

amirroth Jul 24, 2024

I get it. I wasn't happy about this either. I added it so that built-in glycols and user-defined glycols could be handled with the same code. The real solution is to just convert this entire module to std::vector which is eminently doable since it is quite self contained, but just something that I didn't feel like doing in this PR. I do have another pass planned having to do with caching, so I might do it then.

Myoldmopar added 2 commits

July 24, 2024 10:33


          Apply clang format

c842eec


          Fix format issue with custom_check

00ce966

Myoldmopar approved these changes

View reviewed changes

Member

Myoldmopar left a comment

Big changes here! The new API looks great. Since I'm already working on the EnergyPlus C API, I'll try to convert it over to the new interface in there as well. I made a couple comments, but nothing worth any extra changes. I also applied clang format and fixed the format string failure in custom check. I'll let CI make one more pass and this should be ready.

src/EnergyPlus/HVACVariableRefrigerantFlow.cc

-                      if (Util::FindItemInList(thisVrfFluidCtrl.RefrigerantName, state.dataFluidProps->RefrigData, state.dataFluidProps->NumOfRefrigerants) == 0) {
-                          ShowSevereError(state, cCurrentModuleObject + " = " + thisVrfFluidCtrl.Name);
-                          ShowContinueError(state, "Illegal " + cAlphaFieldNames(4) + " = " + cAlphaArgs(4));
+                      thisVrfFluidCtrl.refrigName = cAlphaArgs(4);

Member

Myoldmopar Jul 24, 2024

Got it. And as with the plant component work, the refrig pointer is not owning. It's just an address, and we need to check null before using it. Good to go.

third_party/ObjexxFCL/src/ObjexxFCL/Array1S.hh

		@@ -213,6 +213,18 @@ public: // Assignment: Array

		public: // Assignment: Value

		template< typename U, Size s, class = typename std::enable_if< std::is_assignable< T&, U >::value >::type >

Member

Myoldmopar Jul 24, 2024

Not gonna lie, it's a little offputting to see more being added to Array1D after we've spent so much time stripping it down. Nothing red flag, just surprising. I wonder if this method was already available in the past and it wasn't used so it was removed at some point.

Collaborator Author

amirroth commented Jul 24, 2024

I saw the RefrigerantAPI and GlycolAPI objects, but didn't actually know how and more importantly when they could used (e.g., could they be called before init_state?), so I left them with the old API.


          Clang format again?

44cc051

Member

Myoldmopar commented Jul 24, 2024

All good, merging. Thanks @amirroth

Myoldmopar merged commit 8c96cbb into develop

15 checks passed

Myoldmopar deleted the FluidAPI branch

July 24, 2024 21:28

mjwitte mentioned this pull request

Fix false warnings from getSatTemperature #10629

Merged

20 tasks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

DoNotPublish Refactoring