FluidAPI Continued #10872

amirroth · 2024-12-22T03:24:27Z

This is a continuation of this PR which created an object-oriented API for FluidProperties. This PR propagates this API to all uses and comments out the previous procedural API.

In addition, the GlycolAPI and RefrigAPI objects that served as wrappers for the external C API have been deleted. Those held the FluidName and FluidIndex parameters for the old procedural interface, which are no longer used. The new C API implementation using GlycolProps * and RefrigProps * casting them to externally visible types for client management and casting back during API calls.

Conceptually this is just a "simple" search-and-replace PR. If I knew Python as well as @Myoldmopar, I may have been able to do this using an actual search-and-replace.

There are no diffs as expected. This is ready to go.

rraustad · 2024-12-24T07:15:12Z

Local results:

amirroth · 2024-12-24T14:20:30Z

Local results:

Why did this not show up in my inbox? Also, this doesn't jibe with the successful test report below. Is this stale maybe?

rraustad · 2024-12-24T15:28:31Z

5ZoneAirCooledConvCoef_VSFan shows this error for develop and this branch (this type of error should not be a fatal):

** Severe  ** Iteration limit exceeded in calculating variable speed fan powered box 1st stage heating fan speed
**   ~~~   **  Environment=RUN PERIOD 1, at Simulation time=09/30 06:27 - 06:30
**  Fatal  ** PIU control failed for AirTerminal:SingleDuct:ParallelPIU:Reheat:SPACE4-1 VAV REHEAT

AbsorptionChiller_Macro failed to run, for some reason, in develop and this branch (a regression tool issue?).
ShopWithPVandLiIonBattery failed in this branch with a sqlite error:

SQLite3 message, sqlite.err open for processing!

So I don't think these failures are related to changes in this branch. I'm not sure why the VSFan file runs on Linux, or if it even did.

amirroth · 2025-01-03T20:29:09Z

src/EnergyPlus/FluidProperties.cc

@@ -4560,6 +4562,11 @@ namespace FluidProperties {
        return (refrigNum > 0) ? df->refrigs(refrigNum) : nullptr;
    }

+    RefrigProps *GetSteam(EnergyPlusData &state) {


Shortcut for getting built-in Steam properties object.

amirroth · 2025-01-03T20:29:29Z

src/EnergyPlus/FluidProperties.cc

@@ -4591,6 +4598,11 @@ namespace FluidProperties {
        return (glycolNum > 0) ? df->glycols(glycolNum) : nullptr;
    }

+    GlycolProps *GetWater(EnergyPlusData &state) {


Similar shortcut for getting built-in Water fluid properties object.

amirroth · 2025-01-03T20:30:12Z

src/EnergyPlus/Furnaces.cc

@@ -165,8 +165,6 @@ namespace Furnaces {
    // MODULE PARAMETER DEFINITIONS
    static constexpr std::string_view BlankString;

-    constexpr std::string_view fluidNameSteam("STEAM");


There is a direct shortcut now for getting the steam fluid properties object that does not require a lookup by name.

amirroth · 2025-01-03T20:31:26Z

src/EnergyPlus/Furnaces.cc

-                        SteamIndex = 0; // Function GetSatDensityRefrig will look up steam index if 0 is passed
-                        SteamDensity = FluidProperties::GetSatDensityRefrig(
-                            state, fluidNameSteam, state.dataFurnaces->TempSteamIn, 1.0, SteamIndex, getUnitaryHeatOnly);
+                        SteamDensity = FluidProperties::GetSteam(state)->getSatDensity(state, state.dataFurnaces->TempSteamIn, 1.0, getUnitaryHeatOnly);


Saving a pointer to the built-in steam fluid properties object would be a tiny bit faster, but not enough to matter for a one-off instance.

amirroth · 2025-01-03T20:38:07Z

src/EnergyPlus/Humidifiers.cc

-            WaterSpecHeatAvg = 0.5 * (GetSpecificHeatGlycol(state, format(fluidNameWater), TSteam, WaterIndex, CalledFrom) +
-                                      GetSpecificHeatGlycol(state, format(fluidNameWater), Tref, WaterIndex, CalledFrom));
+
+            auto *water = FluidProperties::GetWater(state);


An index-based lookup like FluidProperties::GetPropertyRefrig(state, refrigNum, x, y, z); requires three consecutive indirections: i) state.dataFluidProperties, ii) dataFluidProperties->refrigs, iii) refrigs(refrigNum). Saving the object pointer and making this into a method eliminates these three consecutive lookups. One small step for EnergyPlus.

amirroth · 2025-01-03T20:42:30Z

This is ready to go.

github-actions · 2025-01-04T22:29:03Z

⚠️ Regressions detected on macos-14 for commit `8eb6fce`

Regression Summary

EIO: 22
ERR: 17
ESO Big Diffs: 23
MTR Big Diffs: 13
Table Big Diffs: 26
Table String Diffs: 22
ESO Small Diffs: 2
MTR Small Diffs: 2
GLHE: 2

github-actions · 2025-01-04T23:50:19Z

⚠️ Regressions detected on macos-14 for commit `9628137`

Regression Summary

EIO: 4
Table Big Diffs: 2
ESO Small Diffs: 3
Table Small Diffs: 2

github-actions · 2025-01-05T01:45:16Z

⚠️ Regressions detected on macos-14 for commit `a72aecb`

Regression Summary

EIO: 4
Table Big Diffs: 2
ESO Small Diffs: 3
Table Small Diffs: 2

github-actions · 2025-01-05T18:31:17Z

⚠️ Regressions detected on macos-14 for commit `7857d91`

Regression Summary

EIO: 4
Table Big Diffs: 2
ESO Small Diffs: 3
Table Small Diffs: 2

github-actions · 2025-01-06T04:36:03Z

⚠️ Regressions detected on macos-14 for commit `269c8f0`

Regression Summary

EIO: 6
ESO Big Diffs: 2
MTR Big Diffs: 1
Table Big Diffs: 3
ESO Small Diffs: 3
Table Small Diffs: 3
MTR Small Diffs: 1

github-actions · 2025-01-06T15:50:38Z

⚠️ Regressions detected on macos-14 for commit `b9ef692`

Regression Summary

EIO: 6
ESO Big Diffs: 2
MTR Big Diffs: 1
Table Big Diffs: 3
ESO Small Diffs: 3
Table Small Diffs: 3
MTR Small Diffs: 1

github-actions · 2025-01-06T21:12:39Z

⚠️ Regressions detected on macos-14 for commit `7ec6935`

Regression Summary

EIO: 6
ESO Big Diffs: 2
MTR Big Diffs: 1
Table Big Diffs: 3
ESO Small Diffs: 3
Table Small Diffs: 3
MTR Small Diffs: 1

amirroth · 2025-01-08T16:52:14Z

src/EnergyPlus/api/func.cc

@@ -88,68 +88,68 @@ void registerErrorCallback(EnergyPlusState state, void (*f)(int, const char *))
 Glycol glycolNew(EnergyPlusState state, const char *glycolName)
 {
    auto *thisState = reinterpret_cast<EnergyPlus::EnergyPlusData *>(state);
-    auto *glycol = new EnergyPlus::FluidProperties::GlycolAPI(*thisState, glycolName);
+    auto *glycol = EnergyPlus::Fluid::GetGlycol(*thisState, EnergyPlus::Util::makeUPPER(glycolName));


This call used to go through a GlycolAPI object which held the GlycolName and GlycolIndex fields that were used by the old procedural API. We now have an object-oriented API that doesn't use those fields and so the internal GlycolProps * can be directly cast to the extenrnal Glycol type handle without the need for an intermediate object.

…ci_skip]

Myoldmopar · 2025-01-09T14:31:37Z

Pulled in develop which should get the code integrity check passing. I'll double check things and see if we can get the merge train rolling today with this one.

Myoldmopar

Alright, this is enormous, but a nice cleanup. I could think of some tweaks to make, but they are not necessary right now. With CI happy here, I think this will merge. I'll hold momentarily for clarification on the dangling #ifdef GET_OUT block, but otherwise this seems ready. Thanks @amirroth

Myoldmopar · 2025-01-09T14:32:38Z

src/EnergyPlus/BaseboardRadiator.cc

+        baseboard.AirOutletTemp = AirOutletTemp;
+        baseboard.Power = LoadMet;
+        baseboard.WaterMassFlowRate = WaterMassFlowRate;
+        baseboard.AirMassFlowRate = AirMassFlowRate;


This is especially nice :)

Myoldmopar · 2025-01-09T14:34:36Z

src/EnergyPlus/Data/EnergyPlusData.hh

@@ -388,7 +388,7 @@ struct EnergyPlusData : BaseGlobalStruct
    std::unique_ptr<FansData> dataFans;
    std::unique_ptr<FaultsManagerData> dataFaultsMgr;
    std::unique_ptr<FluidCoolersData> dataFluidCoolers;
-    std::unique_ptr<FluidData> dataFluidProps;
+    std::unique_ptr<FluidData> dataFluid;


I almost dare say just ... fluid ? But that's for another discussion.

I almost dare say just ... fluid ? But that's for another discussion.

Normalize using short names.

Myoldmopar · 2025-01-09T14:38:56Z

src/EnergyPlus/FluidProperties.hh

@@ -521,7 +529,8 @@ namespace FluidProperties {
                              int &GlycolIndex,           // Index to Glycol Properties
                              std::string_view CalledFrom // routine this function was called from (error messages)
    );
-
+#endif // GET_OUT


Are you wanting to leave these #ifdefs ?

I will probably take them out in a later PR that cleans up the caching. I can also take them out now.

Let's defer to later.

amirroth · 2025-01-09T14:43:45Z

Pulled in develop which should get the code integrity check passing. I'll double check things and see if we can get the merge train rolling today with this one.

Can you look at IceThermalStorage.cc:1684? I think that quantity should be specific heat, not density, based on the name of the variable and the way it is used below. I left it as density so that there would be no diffs, but someone should take a look at this.

amirroth · 2025-01-09T14:46:21Z

Alright, this is enormous, but a nice cleanup. I could think of some tweaks to make, but they are not necessary right now. With CI happy here, I think this will merge. I'll hold momentarily for clarification on the dangling #ifdef GET_OUT block, but otherwise this seems ready. Thanks @amirroth

Wait until you see ScheduleAPI.

rraustad · 2025-01-09T14:51:31Z

src/EnergyPlus/IceThermalStorage.cc

+        // BUG? I think this is supposed to be getSpecificHeat, not getDensity
+        Real64 CpFluid = state.dataPlnt->PlantLoop(loopNum).glycol->getDensity(state, 
+                                                                               state.dataLoopNodes->Node(this->PltInletNodeNum).Temp,
+                                                                               RoutineName);


Yes, this should be specific heat. And this could be moved down to line 1718

Real64 DeltaTemp = Qice / CpFluid / this->ITSMassFlowRate;

Should we do that here? Or separately as a bug fix? Oh, another thing to normalize ... no unparenthesized divisions!

Issue #10890 posted

I would do this separate as a specific bug fix. You could do it here if you like, as long as the diffs are explained (which should be straight-forward).

The thing with doing this here is if anyone ever goes back to see what the change was it would be hard to find it.

Yep, I'm in full support of doing this in it's own minimalistic PR rather than adding it in here.

Myoldmopar · 2025-01-09T15:16:32Z

OK, got formatting applied (that's another issue to deal with very soon). The cp vs density change is deferred to later, along with the stray ifdef. I think this is ready as long as the integrity check passes. Any other final thoughts?

amirroth · 2025-01-09T15:20:42Z

OK, got formatting applied (that's another issue to deal with very soon). The cp vs density change is deferred to later, along with the stray ifdef. I think this is ready as long as the integrity check passes. Any other final thoughts?

Yes, Go Birds!

amirroth added 4 commits December 21, 2024 22:22

Propagate fluidAPI to water

61bcdc7

Two quick fixes

ace163b

Propagate FluidAPI to Steam

3d12910

ChillerIndirectAbsorption fix

1db2e95

amirroth requested review from Myoldmopar and rraustad December 23, 2024 15:43

amirroth assigned amirroth and Myoldmopar and unassigned amirroth Dec 23, 2024

amirroth added Refactoring Includes code changes that don't change the functionality of the program, just perform refactoring DoNotPublish Includes changes that shouldn't be reported in the changelog labels Dec 23, 2024

amirroth added this to the EnergyPlus 25.1 milestone Dec 23, 2024

Merge remote-tracking branch 'origin/develop' into FluidAPI2

6b3caaa

amirroth commented Jan 3, 2025

View reviewed changes

Begin propagating FluidAPI to plant loop

4c68eb3

Fix issue with user defined fluid

149f568

Fix some unit tests

afc8f3b

Amir Roth added 3 commits January 5, 2025 11:09

Propagate to a few more modules

b857d5a

And a few more modules

b856e3a

And a few more modules

9bc8316

Several more modules

96a7a97

amirroth marked this pull request as draft January 6, 2025 14:09

Fix steam loop issue

95d9dea

More modules, more, more, more

a812f38

Amir Roth added 5 commits January 6, 2025 20:24

Fix a few issues

6eba628

More modules, only a few more to go

5710c25

Final modules and refrig API

b344762

Fix RefrigeratedCase crash

bfa5089

Cleanup and C API

41374d2

amirroth marked this pull request as ready for review January 8, 2025 14:27

Remove uses of FluidIndex

42f9e68

amirroth commented Jan 8, 2025

View reviewed changes

Merge remote-tracking branch 'origin/develop' into FluidAPI2 [decent_…

767bec7

…ci_skip]

Myoldmopar approved these changes Jan 9, 2025

View reviewed changes

rraustad reviewed Jan 9, 2025

View reviewed changes

Clang format [decent_ci_skip]

236ed07

Myoldmopar merged commit f224405 into develop Jan 9, 2025
6 checks passed

Myoldmopar deleted the FluidAPI2 branch January 9, 2025 16:15

FluidAPI Continued #10872

FluidAPI Continued #10872

Conversation

amirroth commented Dec 22, 2024 • edited Loading

rraustad commented Dec 24, 2024

amirroth commented Dec 24, 2024 • edited Loading

rraustad commented Dec 24, 2024

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

amirroth commented Jan 3, 2025

github-actions bot commented Jan 4, 2025

⚠️ Regressions detected on macos-14 for commit 8eb6fce

github-actions bot commented Jan 4, 2025

⚠️ Regressions detected on macos-14 for commit 9628137

github-actions bot commented Jan 5, 2025

⚠️ Regressions detected on macos-14 for commit a72aecb

github-actions bot commented Jan 5, 2025

⚠️ Regressions detected on macos-14 for commit 7857d91

github-actions bot commented Jan 6, 2025

⚠️ Regressions detected on macos-14 for commit 269c8f0

github-actions bot commented Jan 6, 2025

⚠️ Regressions detected on macos-14 for commit b9ef692

github-actions bot commented Jan 6, 2025

⚠️ Regressions detected on macos-14 for commit 7ec6935

Choose a reason for hiding this comment

Myoldmopar commented Jan 9, 2025

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

amirroth commented Jan 9, 2025

amirroth commented Jan 9, 2025

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 Jan 9, 2025

amirroth commented Jan 9, 2025

amirroth commented Dec 22, 2024 •

edited

Loading

amirroth commented Dec 24, 2024 •

edited

Loading

⚠️ Regressions detected on macos-14 for commit `8eb6fce`

⚠️ Regressions detected on macos-14 for commit `9628137`

⚠️ Regressions detected on macos-14 for commit `a72aecb`

⚠️ Regressions detected on macos-14 for commit `7857d91`

⚠️ Regressions detected on macos-14 for commit `269c8f0`

⚠️ Regressions detected on macos-14 for commit `b9ef692`

⚠️ Regressions detected on macos-14 for commit `7ec6935`