Fix #10574 - Deal with BIPVT for WaterHeater sizing and throw error if Total Collector area found is zero #10577

jmarrec · 2024-06-19T23:41:32Z

Pull request overview

Fix Fatal error for autosizing when using WaterHeater:Mixed #10574
Deal with BIPVT for WaterHeater sizing and,
throw error if Total Collector area found is zero

The issue is specific to the Solar Loop Water Heater. It's really a very specific combination of factors that lead to the issue:

Tank sizing uses the PerSolarCollectorArea
The solar collectors in the model are PhotovoltaicThermal ones (BIPVT), not the regular FlatPlatWater ones
- This is currently not handled by the routine for calculating the area
Tank has "Ambient Temperature Indicator" set to "Zone"

As a result the collector area is found to be 0.0, there are zero warnings/errors reported.

The tank volume becomes zero, so the mass of the fluid is also 0, and in CalcTankTemp, you end up with a NaN due to a divide by zero.

This tank has "Ambient Temperature Indicator" set to "Zone", and that NaN propagates to the zone internal gains, and you end up with a DualSetPointWithDeadBand error due to that.

Tested that I can reproduce the crash with the Defect File (I put it on DevSupport) and it runs to completion after fix

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.

``` [ RUN ] EnergyPlusFixture.WH_Sizing /Users/julien/Software/Others/EnergyPlus2/tst/EnergyPlus/unit/WaterThermalTanks.unit.cc:5690: Failure Expected equality of these values: 5.00 Which is: 5 Tank.Sizing.TotalSolarCollectorArea Which is: 0 /Users/julien/Software/Others/EnergyPlus2/tst/EnergyPlus/unit/WaterThermalTanks.unit.cc:5692: Failure Expected equality of these values: 1.0 Which is: 1 Tank.Volume Which is: 0 /Users/julien/Software/Others/EnergyPlus2/tst/EnergyPlus/unit/WaterThermalTanks.unit.cc:5711: Failure Value of: std::isnan(Tank.AmbientZoneGain) Actual: true Expected: false /Users/julien/Software/Others/EnergyPlus2/tst/EnergyPlus/unit/WaterThermalTanks.unit.cc:5712: Failure Expected equality of these values: 0.0 Which is: 0 Tank.AmbientZoneGain Which is: nan ```

…f Total Collector area found is zero

…eed to check if we need to trigger the getinput for them

jmarrec · 2024-06-19T23:52:06Z

src/EnergyPlus/WaterThermalTanks.cc

+        for (int CollectorNum = 1; CollectorNum <= state.dataPhotovoltaicThermalCollector->NumPVT; ++CollectorNum) {
+            auto const &collector = state.dataPhotovoltaicThermalCollector->PVT(CollectorNum);
+            this->Sizing.TotalSolarCollectorArea += collector.AreaCol;


Also check the BIPVT collectors

jmarrec · 2024-06-19T23:53:41Z

src/EnergyPlus/WaterThermalTanks.cc

+        if (this->VolumeWasAutoSized) {
+            if (this->Sizing.TotalSolarCollectorArea > 0) {
+                tmpTankVolume = this->Sizing.TotalSolarCollectorArea * this->Sizing.TankCapacityPerCollectorArea;
+            } else {
+                ShowFatalError(state,
+                               format("{}: Tank=\"{}\", requested sizing for volume with PerSolarCollectorArea but total found "
+                                      "area of Collectors is zero.",
+                                      RoutineName,
+                                      this->Name));
+            }
+        }


If VolumeWasAutosized, and the collector area found is 0, we throw.

Otherwise we'd end up with the same issue, the volume is 0, the Mass of tank fluid (kg) m is therefore 0 when CalcTankTemp is called, and you end up with a divide by zero.

EnergyPlus/src/EnergyPlus/WaterThermalTanks.cc

Line 7553 in 3c8844f

a = Q / (m * Cp);

Looks good.

jmarrec · 2024-06-19T23:55:06Z

src/EnergyPlus/WaterThermalTanks.cc

@@ -11201,9 +11202,6 @@ void WaterThermalTankData::SizeTankForSupplySide(EnergyPlusData &state)

    static constexpr std::string_view RoutineName("SizeTankForSupplySide");

-    Real64 Tstart = 14.44;


TODO: should this return early?

if (!this->VolumeWasAutoSized && !this->MaxCapacityWasAutoSized) { return; }

SizeStandAloneWaterHeater does avoid doing work

EnergyPlus/src/EnergyPlus/WaterThermalTanks.cc

Line 11555 in 3c8844f

if (this->VolumeWasAutoSized || this->MaxCapacityWasAutoSized) {

We could, did you check if it triggered any failures or diffs when you return early? If it causes any issues, then I say we push that to a later PR. If it is a no diff/issue and we just gain a little efficiency, then that's great.

jmarrec · 2024-06-19T23:58:00Z

src/EnergyPlus/WaterThermalTanks.cc

+            auto const &collector = state.dataSolarCollectors->Collector(CollectorNum);
+            this->Sizing.TotalSolarCollectorArea += state.dataSurface->Surface(collector.Surface).Area;


TODO: Should we make this should only count the collectors that serve this WH?!

const int thisPlantNum = this->SrcSidePlantLoc.loopNum; const int plantNum = collector.plantLoc.loopNum; if (plantNum == thisPlantNum) { this->Sizing.TotalSolarCollectorArea += state.dataSurface->Surface(collector.Surface).Area; }

The documentation does say that it uses all of the collectors in the model, but I think that is weird.

https://bigladdersoftware.com/epx/docs/24-1/input-output-reference/group-water-heaters.html#field-design-mode

PerSolarCollectorArea
This design method scales tank volume based on the collector area for a solar hot water collector. The collector area is summed for all the collectors in the model and each tank is sized for the total. The collector area is determined from input for Solar Collectors defined elsewhere in the input file.

That is an interesting idea. Are there any corner cases we would need to consider further? Multiple WH on the same plant loop or something? I don't think any corner cases would cause problems, but it may just continue to use non-ideal sizing values or something. Either way, I'm not sure we should worry about that in this scope.

jmarrec · 2024-06-19T23:58:49Z

tst/EnergyPlus/unit/WaterThermalTanks.unit.cc

@@ -5427,3 +5435,279 @@ TEST_F(EnergyPlusFixture, setBackupElementCapacityTest)
    expectedAnswer = -456.0;
    EXPECT_NEAR(DSup.BackupElementCapacity, expectedAnswer, allowedTolerance);
 }
+
+TEST_F(EnergyPlusFixture, MixedTank_PVT_Per_VolumeSizing_PerSolarCollectorArea)


New unit test.

A PlantLoop with a WHMixed on the supply, a BIPVT collector on the demand.

WHMixed was Autosized Volume, Sizing is PerCollectorArea

jmarrec · 2024-06-19T23:59:15Z

tst/EnergyPlus/unit/WaterThermalTanks.unit.cc

+    state->dataPlnt->PlantFirstSizesOkayToFinalize = true;
+    state->dataPlnt->PlantFinalSizesOkayToReport = true;
+    Tank.onInitLoopEquip(*state, plantLocSupply);
+    EXPECT_ENUM_EQ(DataPlant::LoopSideLocation::Supply, Tank.SrcSidePlantLoc.loopSideNum);
+    EXPECT_EQ(1, Tank.SrcSidePlantLoc.loopNum);
+
+    // compare_eio_stream(delimited_string({
+    //     "Water Heater Information,WaterHeater:Mixed,SOLAR LOOP WATER HEATER,-99998.9999,0.0,0.000,0.0000",
+    //     "! <Component Sizing Information>, Component Type, Component Name, Input Field Description, Value",
+    //     " Component Sizing Information, WaterHeater:Mixed, SOLAR LOOP WATER HEATER, Tank Volume [m3], 1.00000",
+    // }));
+    EXPECT_DOUBLE_EQ(5.00, Tank.Sizing.TotalSolarCollectorArea);
+    EXPECT_DOUBLE_EQ(0.2, Tank.Sizing.TankCapacityPerCollectorArea);
+    EXPECT_DOUBLE_EQ(1.0, Tank.Volume);


Autosizing works with BIPVT

jmarrec · 2024-06-19T23:59:30Z

tst/EnergyPlus/unit/WaterThermalTanks.unit.cc

+    state->dataGlobal->HourOfDay = 0;
+    state->dataGlobal->TimeStep = 1;
+    state->dataGlobal->TimeStepZone = 1.0 / 60.0; // one-minute system time step
+    state->dataHVACGlobal->TimeStepSys = state->dataGlobal->TimeStepZone;
+    state->dataHVACGlobal->TimeStepSysSec = state->dataHVACGlobal->TimeStepSys * Constant::SecInHour;
+    Tank.SavedTankTemp = 60.0;
+    Tank.TankTemp = 60.0;
+    Tank.AmbientTempZone = 20.0;
+    Tank.AmbientTemp = 20.0;
+    Tank.UseInletTemp = 20.0;
+    Tank.SetPointTemp = 55.0;
+    Tank.SetPointTemp2 = Tank.SetPointTemp;
+    Tank.TimeElapsed = 0.0;
+
+    Tank.SourceMassFlowRate = 0.0;
+
+    Tank.CalcWaterThermalTankMixed(*state);
+    EXPECT_FALSE(std::isnan(Tank.AmbientZoneGain));
+    EXPECT_DOUBLE_EQ(0.0, Tank.AmbientZoneGain); // Didn't define on/off cycle losses


Specifically test for the NaN encountered.

Myoldmopar · 2024-07-03T17:48:13Z

There are still several TODO's so I'm putting this as waiting on developer for now.

jmarrec · 2024-07-08T13:43:08Z

@Myoldmopar These TODOs are meant for discussion with you/the reviewer.

it's stuff I noticed, that is extra, and that maybe should be addressed, maybe here (in the case of early returns), maybe into another (counting per zone)

Myoldmopar · 2024-07-15T15:07:06Z

@Myoldmopar These TODOs are meant for discussion with you/the reviewer.

I see, I should have looked closer. Back when the PR list was like 60 I was looking quickly for PRs that appeared 'not done', and the TODO's were a red flag to me. :) I'll check it out in more detail now.

Myoldmopar

My feeling on the TODO's is to just wait until a later PR. This seems good to me. Anyone have any issues?

Myoldmopar · 2024-07-15T15:19:51Z

src/EnergyPlus/WaterThermalTanks.cc

+            auto const &collector = state.dataSolarCollectors->Collector(CollectorNum);
+            this->Sizing.TotalSolarCollectorArea += state.dataSurface->Surface(collector.Surface).Area;


That is an interesting idea. Are there any corner cases we would need to consider further? Multiple WH on the same plant loop or something? I don't think any corner cases would cause problems, but it may just continue to use non-ideal sizing values or something. Either way, I'm not sure we should worry about that in this scope.

Myoldmopar · 2024-07-15T15:22:39Z

src/EnergyPlus/WaterThermalTanks.cc

@@ -11201,9 +11202,6 @@ void WaterThermalTankData::SizeTankForSupplySide(EnergyPlusData &state)

    static constexpr std::string_view RoutineName("SizeTankForSupplySide");

-    Real64 Tstart = 14.44;


We could, did you check if it triggered any failures or diffs when you return early? If it causes any issues, then I say we push that to a later PR. If it is a no diff/issue and we just gain a little efficiency, then that's great.

Myoldmopar · 2024-07-15T15:23:10Z

src/EnergyPlus/WaterThermalTanks.cc

+        if (this->VolumeWasAutoSized) {
+            if (this->Sizing.TotalSolarCollectorArea > 0) {
+                tmpTankVolume = this->Sizing.TotalSolarCollectorArea * this->Sizing.TankCapacityPerCollectorArea;
+            } else {
+                ShowFatalError(state,
+                               format("{}: Tank=\"{}\", requested sizing for volume with PerSolarCollectorArea but total found "
+                                      "area of Collectors is zero.",
+                                      RoutineName,
+                                      this->Name));
+            }
+        }


Looks good.

Myoldmopar · 2024-07-15T16:11:04Z

src/EnergyPlus/WaterThermalTanks.cc

-                this->Sizing.TotalSolarCollectorArea += state.dataSurface->Surface(state.dataSolarCollectors->Collector(CollectorNum).Surface).Area;
+                auto const &collector = state.dataSolarCollectors->Collector(CollectorNum);
+                this->Sizing.TotalSolarCollectorArea += state.dataSurface->Surface(collector.Surface).Area;
+            }


I'm so tempted to say things like "range-based loop" and "std::accumulate", both of which could help remove the 1-based array dependence, but I won't for now. https://godbolt.org/z/35jEcv1E5

Myoldmopar · 2024-07-25T20:33:37Z

This is still fine, just let me pull develop and test locally.

Myoldmopar · 2024-07-25T20:41:32Z

Yep, still good here. Merging, thanks @jmarrec

jmarrec added 2 commits June 20, 2024 01:29

Add a test for #10574

0b2affa

jmarrec added the Defect Includes code to repair a defect in EnergyPlus label Jun 19, 2024

jmarrec self-assigned this Jun 19, 2024

jmarrec added 2 commits June 20, 2024 01:48

Fix #10574 - Deal with BIPVT for WaterHeater sizing and throw error i…

df5042f

…f Total Collector area found is zero

The CollectorData and PVTCollectorStruct use a factory, so we don't n…

4ee8bcb

…eed to check if we need to trigger the getinput for them

jmarrec force-pushed the 10574_WaterHeater_Autosizing_NaN branch from 7567778 to 4ee8bcb Compare June 19, 2024 23:50

jmarrec commented Jun 19, 2024

View reviewed changes

jmarrec mentioned this pull request Jun 20, 2024

Fatal error for autosizing when using WaterHeater:Mixed #10574

Closed

Myoldmopar approved these changes Jul 15, 2024

View reviewed changes

Myoldmopar merged commit 047575a into develop Jul 25, 2024
15 checks passed

Myoldmopar deleted the 10574_WaterHeater_Autosizing_NaN branch July 25, 2024 20:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #10574 - Deal with BIPVT for WaterHeater sizing and throw error if Total Collector area found is zero #10577

Fix #10574 - Deal with BIPVT for WaterHeater sizing and throw error if Total Collector area found is zero #10577

jmarrec commented Jun 19, 2024 •

edited

Loading

jmarrec Jun 19, 2024

jmarrec Jun 19, 2024

Myoldmopar Jul 15, 2024

jmarrec Jun 19, 2024

Myoldmopar Jul 15, 2024

jmarrec Jun 19, 2024 •

edited

Loading

Myoldmopar Jul 15, 2024

jmarrec Jun 19, 2024

jmarrec Jun 19, 2024

jmarrec Jun 19, 2024

Myoldmopar commented Jul 3, 2024

jmarrec commented Jul 8, 2024

Myoldmopar commented Jul 15, 2024

Myoldmopar left a comment

Myoldmopar Jul 15, 2024

Myoldmopar Jul 15, 2024

Myoldmopar Jul 15, 2024

Myoldmopar Jul 15, 2024 •

edited

Loading

Myoldmopar commented Jul 25, 2024

Myoldmopar commented Jul 25, 2024

		@@ -11201,9 +11202,6 @@ void WaterThermalTankData::SizeTankForSupplySide(EnergyPlusData &state)

		static constexpr std::string_view RoutineName("SizeTankForSupplySide");

		Real64 Tstart = 14.44;

		auto const &collector = state.dataSolarCollectors->Collector(CollectorNum);
		this->Sizing.TotalSolarCollectorArea += state.dataSurface->Surface(collector.Surface).Area;

Fix #10574 - Deal with BIPVT for WaterHeater sizing and throw error if Total Collector area found is zero #10577

Fix #10574 - Deal with BIPVT for WaterHeater sizing and throw error if Total Collector area found is zero #10577

Conversation

jmarrec commented Jun 19, 2024 • edited Loading

Pull request overview

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

jmarrec Jun 19, 2024 • edited Loading

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 Jul 3, 2024

jmarrec commented Jul 8, 2024

Myoldmopar commented Jul 15, 2024

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

Myoldmopar Jul 15, 2024 • edited Loading

Choose a reason for hiding this comment

Myoldmopar commented Jul 25, 2024

Myoldmopar commented Jul 25, 2024

jmarrec commented Jun 19, 2024 •

edited

Loading

jmarrec Jun 19, 2024 •

edited

Loading

Myoldmopar Jul 15, 2024 •

edited

Loading