Absorption chillerheater plant loop fixes #8556

jcyuan2020 · 2021-02-22T18:03:15Z

Pull request overview

Fixes Exhaust or gas fired chillerheater plant loop branch check preventing pumps or other elements to be placed in front of the chillerheater #8555
Fixes Exhaust (and gas fired) absorption chillerheater simulation fatal out on zero condenser flow rate #8554
The PR addressed an unexpected outcome (Exhaust or gas fired chillerheater plant loop branch check preventing pumps or other elements to be placed in front of the chillerheater #8555) in the exhaust fired absorption chiller heater loop branch. The original code prevents another element--mostly typically a pump, but could be anything such as a simple pipe) to be placed in front of the exhaust absorption chillerheater. Otherwise, the simulation program will report fatal errors and terminate the simulation. The PR also addressed issue Exhaust (and gas fired) absorption chillerheater simulation fatal out on zero condenser flow rate #8554 that when condenser flow rate becomes zero for whatever reasons, the program would report fatal error and terminate the program. With the fix in this PR, the program will use a similar treatment for other water cooled chiller models, in which it will still report severe errors but allow the simulation to continue.
These changes are applied to both the exhaust-fired absorption chillerheater and the gas-fired absorption chillerheater.

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.

… absorption chiller. Compiles ok.

…ranch hacky check. Compiles ok.

…well. Compiles ok.

…es ok. Tests passed.

jcyuan2020 · 2021-03-01T21:03:30Z

src/EnergyPlus/ChillerGasAbsorption.cc

+                                  .LoopSide(calledFromLocation.loopSideNum)
+                                  .Branch(calledFromLocation.branchNum)
+                                  .TotalComponents;
+


The revised code will check all components in a branch, rather than just checking the branch head element as before.

jcyuan2020 · 2021-03-01T21:06:35Z

src/EnergyPlus/ChillerGasAbsorption.cc

+                                       .Branch(calledFromLocation.branchNum)
+                                       .Comp(iComp)
+                                       .NodeNumIn;
+


Still using the `hacky' way, but applied to all the components in the loop branch.

jcyuan2020 · 2021-03-01T21:08:42Z

src/EnergyPlus/ChillerGasAbsorption.cc

+            } else {
+                matchfound = false;
+            }
+        }


If match found, update the loads values accordingly, break and return. Otherwise, continue to the next component check.

jcyuan2020 · 2021-03-01T21:13:11Z

src/EnergyPlus/ChillerExhaustAbsorption.cc

+                                  .Branch(calledFromLocation.branchNum)
+                                  .TotalComponents;
+
+        for (int iComp = 1; iComp <= branchTotalComp; iComp++) {


The revised code checks all the elements in an (exhaust or gas-fired) absorption chiller heat plant loop branch, to identity which (chilled water, hot water, or condensing water) branch it is simulating. This is in contrast to the original code where only head branch component is checked.

jcyuan2020 · 2021-03-01T21:15:12Z

src/EnergyPlus/ChillerExhaustAbsorption.cc

+            } else {
+                brIdentity = 0;
+            }
+        }


Use an integer number (brIndentity) to identify the loop branch property: 1 for chilled water; 2 for hot water; 3 for condensing water; 0 indicates problem (no match).

This is kinda odd. If it is just an identifier and the value doesn't matter, this should be an enum class. I would like to see that changed before this is merged. I can make the change if needed, just let me know.

Much nicer than the magic numbers, thank you!

jcyuan2020 · 2021-03-01T21:17:08Z

src/EnergyPlus/ChillerExhaustAbsorption.cc

-
-        // Match inlet node name of calling branch to determine if this call is for heating or cooling
-        if (BranchInletNodeNum == this->ChillReturnNodeNum) { // Operate as chiller
+        if (brIdentity == 1) {
            this->InCoolingMode = RunFlag != 0;


After the current branch is identified, run the corresponding functions/code blocks to execute the simulation. Slightly reorganized from the original code.

jcyuan2020 · 2021-03-01T21:18:31Z

src/EnergyPlus/ChillerExhaustAbsorption.cc

@@ -131,23 +131,47 @@ namespace EnergyPlus::ChillerExhaustAbsorption {

    void ExhaustAbsorberSpecs::simulate(EnergyPlusData &state, const PlantLocation &calledFromLocation, bool FirstHVACIteration, Real64 &CurLoad, bool RunFlag)


The changes are applied to both the Exhaust fired absorption chiller heater, and the gas fired absorption chiller heater. For each types of absorption chiller heater, the getDesignCapacities() and the simulate() functions would be check to identity the loop/branch type (chilled water, hot water, or condenser water).

jcyuan2020 · 2021-03-01T21:27:48Z

src/EnergyPlus/ChillerExhaustAbsorption.cc

@@ -1626,7 +1672,7 @@ namespace EnergyPlus::ChillerExhaustAbsorption {
                } else {
                    ShowSevereError(state, "CalcExhaustAbsorberChillerModel: Condenser flow = 0, for Exhaust Absorber Chiller=" + this->Name);
                    ShowContinueErrorTimeStamp(state, "");
-                    ShowFatalError(state, "Program Terminates due to previous error condition.");
+                    // ShowFatalError(state, "Program Terminates due to previous error condition.");


Now do not show the fatal message and terminate any more--this is aligned with the kind of common treatment used in other type of chiller (electric or absorption) models.

Hmmm.... OK, so if we show a severe then we need to fatal. If we aren't going to fatal then this needs to be a warning. But if it's a warning that could occur multiple times should we do a recurring warning message to avoid huge error files?

Myoldmopar

I think we should fix the magic numbers and make them an enum. If you aren't able to make that change, I can do it, just let me know.

Myoldmopar · 2021-03-05T18:43:02Z

src/EnergyPlus/ChillerExhaustAbsorption.cc

+            } else {
+                brIdentity = 0;
+            }
+        }


This is kinda odd. If it is just an identifier and the value doesn't matter, this should be an enum class. I would like to see that changed before this is merged. I can make the change if needed, just let me know.

jcyuan2020 · 2021-03-05T20:00:09Z

I think we should fix the magic numbers and make them an enum. If you aren't able to make that change, I can do it, just let me know.

Thanks! Updating it. Will test and push again.

…er model changes.

…odel changes.

Myoldmopar · 2021-03-05T20:41:59Z

src/EnergyPlus/ChillerExhaustAbsorption.cc

+            CONDENSER,
+            NOMATCH
+        };
+        brLoopType brIdentity(NOMATCH);


This is much closer, but not quite there. I'm confused why this enum is repeated in the two cases. It should be declared once. Please:

move this one enum declaration to either the top of the ChillerExhaustAbsorption.cc file, or in the header,

make it an enum class instead of a raw enum,

change the enum name from camelCase to PascalCase,

change the member names from SCREAMINGCASE to PascalCase,

and make the accompanying change for the enum class by using the namespace below (BrLoopType::Chiller)

This is much closer, but not quite there. I'm confused why this enum is repeated in the two cases. It should be declared once. Please:

* move this one enum declaration to either the top of the ChillerExhaustAbsorption.cc file, or in the header, * make it an `enum class` instead of a raw `enum`, * change the enum name from `camelCase` to `PascalCase`, * change the member names from `SCREAMINGCASE` to `PascalCase`, * and make the accompanying change for the `enum class` by using the namespace below (`BrLoopType::Chiller`)

I will make the changes. For why it was done twice in the two functions: actually these variables are very "local" to the simulate() functions so I did not want to bump up their scope.
It might be a little bit debatable since it is just a type definition. Or we can make the scope an even higher level up, so that both the ChillerExhaustAbsorption.cc (or hh) and the ChillerGasAbsorption.cc (or .hh) can see the definition.

Any comments or thoughts?

Wherever it makes sense is fine with me. But duplicating the same list of enumarations in two places smells like it needs to be DRYd up. For now, I say just move it up to the file level here so it can be shared between these two functions without duplication.

And you are correct about the type declaration comment, the scope here doesn't matter since an enum is just optimized away quickly, so it won't affect runtime. The scope should simply be determined by how many components this can be applied to.

So putting the definition in EnergyPlus/Plant/Enum.hh seems to be a good choice; then add #inlcude<EnergyPlus/Plant/Enum.hh> to both ChillerExhaustAbsorption.cc and ChillerGasAbsorption.cc.

Or definition in EnergyPlus/Plant/DataPlant.hh, which is already included in both both ChillerExhaustAbsorption.cc and ChillerGasAbsorption.cc.

See the new commits.

Myoldmopar

OK, I need one more reply back about the severe/warning/recurring comment. I actually do think it's worth changing to at least a warning message, but I'm curious about the recurring change as well. @jcyuan2020 what do you think? Would this warning end up coming out in the error file many many times? Or just once? If it's just a one time sort of message then a regular warning is fine, and I can quickly make that change and get this merged. If it needs to be a recurring message I may just push this back for that one more change from you.

Or, I could have totally missed something and you can correct me.

Thanks for this effort, either way this is very nearly done.

Myoldmopar · 2021-03-06T02:29:56Z

src/EnergyPlus/ChillerExhaustAbsorption.cc

+            } else {
+                brIdentity = 0;
+            }
+        }


Much nicer than the magic numbers, thank you!

Myoldmopar · 2021-03-06T02:31:02Z

src/EnergyPlus/ChillerExhaustAbsorption.cc

@@ -1626,7 +1672,7 @@ namespace EnergyPlus::ChillerExhaustAbsorption {
                } else {
                    ShowSevereError(state, "CalcExhaustAbsorberChillerModel: Condenser flow = 0, for Exhaust Absorber Chiller=" + this->Name);
                    ShowContinueErrorTimeStamp(state, "");
-                    ShowFatalError(state, "Program Terminates due to previous error condition.");
+                    // ShowFatalError(state, "Program Terminates due to previous error condition.");


Hmmm.... OK, so if we show a severe then we need to fatal. If we aren't going to fatal then this needs to be a warning. But if it's a warning that could occur multiple times should we do a recurring warning message to avoid huge error files?

Myoldmopar · 2021-03-06T02:32:10Z

src/EnergyPlus/Plant/Enums.hh

+        Heater,
+        Condenser,
+        NoMatch
+    };


Yup, great! In a future set of changes the actual type name might be tailored a little more to the actual application, but this is fine for now.

@Myoldmopar Thanks! Yes, the warnings are usually recurring. So it makes sense to condense them.

…lop branch.

Myoldmopar · 2021-03-20T02:59:59Z

My concern was still not fully addressed. It looks to be like we have a ShowSevereError that now no longer fatals. That is an invalid pattern -- even though yes there may still be a few of them lurking in the code. If it is a severe error, then it requires a fatal. If it is not going to fatal then it needs to be a warning.

I suspect it needs to be a warning.

Now, if it is a warning, it could occur many times in the course of a single simulation. We don't want our error files to be thousands of lines long, so we need to instead use the ShowRecurringWarning style error messages so that we only print the first one plus a summary at the end.

Right now I think a user could end up with dozens? thousands?? of severe errors in their error file, when it should've just been a recurring warning.

I don't think this is quite ready.

jcyuan2020 · 2021-03-20T03:16:18Z

My concern was still not fully addressed. It looks to be like we have a ShowSevereError that now no longer fatals. That is an invalid pattern -- even though yes there may still be a few of them lurking in the code. If it is a severe error, then it requires a fatal. If it is not going to fatal then it needs to be a warning.

I suspect it needs to be a warning.

Now, if it is a warning, it could occur many times in the course of a single simulation. We don't want our error files to be thousands of lines long, so we need to instead use the ShowRecurringWarning style error messages so that we only print the first one plus a summary at the end.

Right now I think a user could end up with dozens? thousands?? of severe errors in their error file, when it should've just been a recurring warning.

I don't think this is quite ready.

Thanks for the comments! I thought the original question was whether this would be a recurring warning, and my answer was that these warnings could indeed be recurring:
See here #8556 (comment)

As to whether the severe warnings should report fatal, currently this change is trying to make it similar to the way how other chiller and regular absorption chiller models handle such a stiuation---I think for those chiller models, it would be just report severe error but does not fatal out on it.

So for the recurrent ones, do you want me change it to using the recurrent errors mode? I can make that change for the two absorption chillerheater models involved in this PR.

BTW, if related, I think for the other chiller models (e.g. electric or regular absorption), the use of recurrent ones had not propagated in yet.

Myoldmopar · 2021-03-20T13:21:09Z

@jcyuan2020 thanks for the further details. I'm not excited about dropping this in with the severe-no-fatal and non-recurring errors, but indeed this is happening with the other objects, so this is not a widespread issue. Given our timing I think I will merge this in after I finish testing it locally.

Myoldmopar · 2021-03-20T13:41:58Z

This passes fine locally with develop pulled in. I'll merge it now. @jcyuan2020 please open an issue to address after the release where the severe-no-fatal code paths are cleaned up in relevant objects and also the messages should become recurring. Thanks!

jcyuan2020 · 2021-03-20T16:39:38Z

@Myoldmopar Thanks! The issue has been posted---Issue #8638

jcyuan2020 force-pushed the I8554_I8555_chillerheater_plant_loop_fixes branch from 8fed19b to e404dfb Compare February 24, 2021 04:39

mjwitte added the Defect Includes code to repair a defect in EnergyPlus label Feb 26, 2021

mjwitte added this to the EnergyPlus 9.5.0 milestone Feb 26, 2021

jcyuan2020 force-pushed the I8554_I8555_chillerheater_plant_loop_fixes branch from 6814516 to 1d49d51 Compare February 26, 2021 19:29

jcyuan2020 marked this pull request as ready for review March 1, 2021 17:39

mjwitte requested a review from Myoldmopar March 1, 2021 17:50

jcyuan2020 added 10 commits March 1, 2021 14:31

Do not show fatal message in absorption chiller zero condenser flow.

57ca95f

Apply fix to getDesignCapacties loop identification issue for exhaust…

80f1fb1

… absorption chiller. Compiles ok.

Revised the simulation module for exhaust fired chillerheater about b…

a4d9b98

…ranch hacky check. Compiles ok.

Fix additional state variables issue. Compiles ok (fixed).

e301d27

Apply the plant loop fixes to the direct fired absorption chiller as …

46ae933

…well. Compiles ok.

Reorganized the simualate check module.

19e0e99

Unit test cases developed. Compiles ok. Tests passed.

cccf8f7

Add another unit test for gas fired absorption chiller heater. Compil…

5c67cc0

…es ok. Tests passed.

Comments added to unit tests.

0d5d356

Clean up redudant comments.

c3940d6

jcyuan2020 force-pushed the I8554_I8555_chillerheater_plant_loop_fixes branch from bad52a7 to c3940d6 Compare March 1, 2021 20:33

jcyuan2020 commented Mar 1, 2021

View reviewed changes

Myoldmopar requested changes Mar 5, 2021

View reviewed changes

jcyuan2020 added 2 commits March 5, 2021 14:28

Use enum class for loop types in exhaust-fired absorption chillerheat…

1bc932d

…er model changes.

Use enum class for loop types in gas-fired absorption chillerheater m…

d016c33

…odel changes.

Myoldmopar reviewed Mar 5, 2021

View reviewed changes

jcyuan2020 added 2 commits March 5, 2021 15:58

A few more enum changes and make the enum definintion common to share.

5608c82

Enum changes and common share propagated to both cases.

0dec544

Myoldmopar reviewed Mar 6, 2021

View reviewed changes

2021-03-17 Resolved conflicts encountered while merging upstream deve…

01507f3

…lop branch.

Myoldmopar approved these changes Mar 20, 2021

View reviewed changes

Myoldmopar merged commit 74f2aeb into NREL:develop Mar 20, 2021

jcyuan2020 mentioned this pull request Mar 20, 2021

Use recurring error warnings for condenser related chiller (severe) warnings #8638

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Absorption chillerheater plant loop fixes #8556

Absorption chillerheater plant loop fixes #8556

jcyuan2020 commented Feb 22, 2021 •

edited

Loading

jcyuan2020 Mar 1, 2021

jcyuan2020 Mar 1, 2021

jcyuan2020 Mar 1, 2021

jcyuan2020 Mar 1, 2021

jcyuan2020 Mar 1, 2021

Myoldmopar Mar 5, 2021

Myoldmopar Mar 6, 2021

jcyuan2020 Mar 1, 2021

jcyuan2020 Mar 1, 2021

jcyuan2020 Mar 1, 2021

Myoldmopar Mar 6, 2021

Myoldmopar left a comment

Myoldmopar Mar 5, 2021

jcyuan2020 commented Mar 5, 2021

Myoldmopar Mar 5, 2021 •

edited

Loading

jcyuan2020 Mar 5, 2021 •

edited

Loading

Myoldmopar Mar 5, 2021

jcyuan2020 Mar 5, 2021

jcyuan2020 Mar 5, 2021

jcyuan2020 Mar 5, 2021

Myoldmopar left a comment

Myoldmopar Mar 6, 2021

Myoldmopar Mar 6, 2021

Myoldmopar Mar 6, 2021

jcyuan2020 Mar 6, 2021

Myoldmopar commented Mar 20, 2021

jcyuan2020 commented Mar 20, 2021 •

edited

Loading

Myoldmopar commented Mar 20, 2021

Myoldmopar commented Mar 20, 2021

jcyuan2020 commented Mar 20, 2021

		@@ -131,23 +131,47 @@ namespace EnergyPlus::ChillerExhaustAbsorption {

		void ExhaustAbsorberSpecs::simulate(EnergyPlusData &state, const PlantLocation &calledFromLocation, bool FirstHVACIteration, Real64 &CurLoad, bool RunFlag)

Absorption chillerheater plant loop fixes #8556

Absorption chillerheater plant loop fixes #8556

Conversation

jcyuan2020 commented Feb 22, 2021 • 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

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 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcyuan2020 commented Mar 5, 2021

Myoldmopar Mar 5, 2021 • edited Loading

Choose a reason for hiding this comment

jcyuan2020 Mar 5, 2021 • 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 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

Myoldmopar commented Mar 20, 2021

jcyuan2020 commented Mar 20, 2021 • edited Loading

Myoldmopar commented Mar 20, 2021

Myoldmopar commented Mar 20, 2021

jcyuan2020 commented Mar 20, 2021

jcyuan2020 commented Feb 22, 2021 •

edited

Loading

Myoldmopar Mar 5, 2021 •

edited

Loading

jcyuan2020 Mar 5, 2021 •

edited

Loading

jcyuan2020 commented Mar 20, 2021 •

edited

Loading