Add convergence check for co2 and contaminant #10500

lgu1234 · 2024-05-09T18:01:15Z

Pull request overview

Fixes Error with Zone Air Contaminant Balance #10348
DESCRIBE PURPOSE OF THIS PULL REQUEST

The convergence check for CO2 and generic contaminant was added based on decision in the issue.

The differences are found in the test file, located in EnergyPlusDevSupport.

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.

[ X] Title of PR should be user-synopsis style (clearly understandable in a standalone changelog context)
[X ] Label the PR with at least one of: Defect, Refactoring, NewFeature, Performance, and/or DoNoPublish
[X ] Pull requests that impact EnergyPlus code must also include unit tests to cover enhancement or defect repair
[ X] Author should provide a "walkthrough" of relevant code changes using a GitHub code review comment process
If any diffs are expected, author must demonstrate they are justified using plots and descriptions
If changes fix a defect, the fix should be demonstrated in plots and descriptions
[X ] If any defect files are updated to a more recent version, upload new versions here or on DevSupport
If IDD requires transition, transition source, rules, ExpandObjects, and IDFs must be updated, and add IDDChange label
If structural output changes, add to output rules file and add OutputChange label
If adding/removing any LaTeX docs or figures, update that document's CMakeLists file dependencies

Reviewer

This will not be exhaustively relevant to every PR.

lgu1234 · 2024-05-09T18:49:38Z

src/EnergyPlus/DataConvergParams.hh

+        std::array<bool, 3> HVACGenContamNotConverged = {false};                                   // Flag to show mass flow convergence
+        std::array<Real64, ConvergLogStackDepth> HVACGenContamDemandToSupplyTolValue = {0.0};      // Queue of convergence "results"
+        std::array<Real64, ConvergLogStackDepth> HVACGenContamSupplyDeck1ToDemandTolValue = {0.0}; // Queue of convergence "results"
+        std::array<Real64, ConvergLogStackDepth> HVACGenContamSupplyDeck2ToDemandTolValue = {0.0}; // Queue of convergence "results"


Add more variables for CO2 and contaminant convergence check

lgu1234 · 2024-05-09T18:50:31Z

src/EnergyPlus/HVACInterfaceManager.cc

+        }
+        if (state.dataContaminantBalance->Contaminant.GenericContamSimulation) {
+            airLoopConv.HVACGenContamNotConverged[0] = false;
+        }


Initialize bool variables with false

lgu1234 · 2024-05-09T18:51:26Z

src/EnergyPlus/HVACInterfaceManager.cc

+                OutOfToleranceFlag = true; // Something has changed--resimulate the other side of the loop
+            }
+        }
+


Convergence check for AirSystemDemandSide

lgu1234 · 2024-05-09T18:51:58Z

src/EnergyPlus/HVACInterfaceManager.cc

+                airLoopConv.HVACGenContamNotConverged[1] = true;
+                OutOfToleranceFlag = true; // Something has changed--resimulate the other side of the loop
+            }
+        }


Convergence check for AirSystemSupplySideDeck1

lgu1234 · 2024-05-09T18:52:06Z

src/EnergyPlus/HVACInterfaceManager.cc

+                airLoopConv.HVACGenContamNotConverged[2] = true;
+                OutOfToleranceFlag = true; // Something has changed--resimulate the other side of the loop
+            }
+        }


Convergence check for AirSystemSupplySideDeck2

lgu1234 · 2024-05-09T18:52:46Z

src/EnergyPlus/HVACManager.cc

+                                                     HistoryTrace));
+                        }
+                    } // Generic contaminant not converged
+


Provide error messages if convergence is not met.

Could all this repeated stuff be put into a single function and called with an array reference or two passed in?

Myoldmopar

I'm going to hold off on marking approve/reject right now. I would love to get a response about the array index comments. Honestly, when there is a check on a contam tolerance value at array index [0], and then assigning not converged[2] to a value, I cannot tell if that's right or a typo. It would be great to know how big of an effort it would be to "at least" give those zeroes, ones, and twos meaningful names through constexpr int variables. Or an enum. Or something else. If it's enormous, I won't hold this fix up for that.

Myoldmopar · 2024-05-11T00:12:49Z

src/EnergyPlus/HVACInterfaceManager.cc

+        }
+        if (state.dataContaminantBalance->Contaminant.GenericContamSimulation) {
+            airLoopConv.HVACGenContamNotConverged[1] = false;
+        }


Oof, I really don't like further propagating these numbers. As far as I can tell, the index in this array is based on where we are calling from? Is that actually how it works? I'm not saying we need to go in and gut things for this fix, but it's just something that I think should be cleaned up. At a minimum, shouldn't we at least have a constexpr variable for "0", "1", and "2"? And more importantly, does it make sense for these to be arrays at all? Feels more like we should have a little struct and just create 3 different variable instances of that struct. Rant over.

@Myoldmopar Your understanding is correct. The index in the array represents a call-from type as DataConvergParams::CalledFrom. I also agree to have a constexpr variable for "0", "1", and "2" with clear representation. Since constexpr variable change is beyond the current issue scope, I would like to suggest to create a new issue to handle a new constexpr variable.

Myoldmopar · 2024-05-11T00:13:37Z

src/EnergyPlus/HVACInterfaceManager.cc

+            if (airLoopConv.HVACGenContamSupplyDeck2ToDemandTolValue[0] > DataConvergParams::HVACGenContamToler) {
+                airLoopConv.HVACGenContamNotConverged[2] = true;
+                OutOfToleranceFlag = true; // Something has changed--resimulate the other side of the loop
+            }


Yeah, checking the value of variable [0] and then saying converged[2] = true. This is not easy to read and understand.

@Myoldmopar a new constexpr variable suggested by you may address your concern.

So this DemandTolValue[0] is not related to the same indexing that the converged[2] was referencing? It doesn't have to be changed now, and using CalledFrom is helpful indeed.

Myoldmopar · 2024-05-11T00:14:56Z

src/EnergyPlus/HVACManager.cc

+                                                     HistoryTrace));
+                        }
+                    } // Generic contaminant not converged
+


Could all this repeated stuff be put into a single function and called with an array reference or two passed in?

lgu1234 · 2024-05-13T13:39:44Z

@Myoldmopar The convergence check for CO2 and generic contaminant mimics the convergence check for temperature and others. That is why I adopt index of the array of NotConverged as (0, 1, 2). I agree the index can be replaced by a new constexpr variable or enum. Since the change is beyond the current issue scope, it is better to have a new issue to handle the change.

lgu1234 · 2024-05-13T14:33:05Z

@Myoldmopar Since the index of the array represents calledFrom type, here is my suggestion for the index. It should be noted that CalledFrom is defined as enum already.
from
airLoopConv.HVACMassFlowNotConverged[0] = false;
Revide
airLoopConv.HVACMassFlowNotConverged[(int)CalledFrom] = false;

Please let me know what you think.

lgu1234 · 2024-05-20T17:20:38Z

@Myoldmopar A function was created to handle not convergence errors in different categories, mass flow rate, humidity ratio, temperature, energy, CO2 and generic contaminant.

lgu1234 · 2024-05-20T17:21:29Z

A new test file was uploaded to test no convergence errors.

lgu1234 · 2024-05-20T18:31:00Z

@Myoldmopar The Win64-Windows-10-VisualStudio-16 — Build Failed

I recompiled E+ on my local computer using Visual Studio. It is OK to generate exe files for both release and debug. Is ti possible to let me know how fix this failure? Thanks.

lgu1234 · 2024-05-21T17:59:54Z

@Myoldmopar It is ready for review again.

Myoldmopar

I'm requesting changes, but I'm going to make them myself. I'll get them pushed up and wait for one more round of CI and then get this merged. Thanks @lgu1234

Myoldmopar · 2024-05-22T13:11:29Z

src/EnergyPlus/HVACInterfaceManager.cc

-        airLoopConv.HVACHumRatNotConverged[0] = false;
-        airLoopConv.HVACTempNotConverged[0] = false;
-        airLoopConv.HVACEnergyNotConverged[0] = false;
+        airLoopConv.HVACMassFlowNotConverged[(int)CalledFrom] = false;


Doesn't need to be changed here, but you are welcome to do this cast one time and create a new int from it. That way the array lookup is a bit cleaner. It won't affect performance or anything, but could help readability:

int iCall = (int)calledFromEnum; airLoopConv.HVACMassFlowNotConverged[iCall] = false; // ...lots more uses of iCall without having (int) everywhere...

Myoldmopar · 2024-05-22T13:14:29Z

src/EnergyPlus/HVACInterfaceManager.cc

+            if (airLoopConv.HVACGenContamSupplyDeck2ToDemandTolValue[0] > DataConvergParams::HVACGenContamToler) {
+                airLoopConv.HVACGenContamNotConverged[2] = true;
+                OutOfToleranceFlag = true; // Something has changed--resimulate the other side of the loop
+            }


So this DemandTolValue[0] is not related to the same indexing that the converged[2] was referencing? It doesn't have to be changed now, and using CalledFrom is helpful indeed.

Myoldmopar · 2024-05-22T13:17:14Z

src/EnergyPlus/HVACManager.cc

-                        }
-                    } // energy not converged
-
+                    // mass flow rate


Future work: create a reference to reuse to avoid these lookups and collapse all these long lines:

auto & thisSys = state.dataConvergeParams->AirLoopConvergence(AirSysNum); ConvergenceErrors(state, thisSys.HVACMassFlowNotConverged, thisSys.HVACFlowDemandToSupplyTolValue, thisSys.HVACFlowSupplyDeck1ToDemandTolValue, thisSys.HVACFlowSupplyDeck2ToDemandTolValue, AirSysNum, 1);

@Myoldmopar I tried to use references for all arrays and was unable to pass compile.

Myoldmopar · 2024-05-22T13:18:51Z

src/EnergyPlus/HVACManager.cc

+    if (index == 3) CaseName = "temperature";
+    if (index == 4) CaseName = "energy";
+    if (index == 5) CaseName = "CO2";
+    if (index == 6) CaseName = "generic contaminant";


Oooh this should be a constexpr array of strings. I'm going to stop short of requiring it here, but this should just be a lookup.

Myoldmopar · 2024-05-22T13:19:52Z

src/EnergyPlus/HVACManager.cc

+                       std::array<bool, 3> HVACNotConverged,
+                       std::array<Real64, 10> DemandToSupply,
+                       std::array<Real64, 10> SupplyDeck1ToDemand,
+                       std::array<Real64, 10> SupplyDeck2ToDemand,


Hmmmm... are these going to be copied? This is something that might actually have to be changed. @lgu1234 I will likely make a pass at cleaning this up and maybe touch some of the other random notes I've made here.

Myoldmopar · 2024-05-22T14:44:53Z

Alright, I addressed my own comments and it should be ready. I'll let CI confirm things, but then this should be ready for merge.

lgu1234 · 2024-05-22T15:22:11Z

@Myoldmopar Your changes are better. Thanks.

Myoldmopar · 2024-05-29T15:32:55Z

I ran a quick local test with develop, still happy, let's merge. Thanks @lgu1234

lgu1234 added 2 commits May 9, 2024 13:51

Initial commit

95c98b4

Merge branch 'develop' into Add-more-convergence-check-for-CO2-and-Cont

953f3da

lgu1234 added the Defect Includes code to repair a defect in EnergyPlus label May 9, 2024

lgu1234 added 2 commits May 9, 2024 14:16

Clang format

e1d5f06

More clang format

73ca4a1

lgu1234 commented May 9, 2024

View reviewed changes

Myoldmopar reviewed May 11, 2024

View reviewed changes

lgu1234 added 4 commits May 13, 2024 13:37

Address comments

2d0b9c4

Merge branch 'develop' into Add-more-convergence-check-for-CO2-and-Cont

087377a

Address more comments

20386ed

Merge branch 'develop' into Add-more-convergence-check-for-CO2-and-Cont

b9ce7b8

lgu1234 added 2 commits May 21, 2024 10:25

Clang format

0e7ed3d

Correct a data type

e71806a

Myoldmopar requested changes May 22, 2024

View reviewed changes

Myoldmopar added 2 commits May 22, 2024 09:37

Cleanups

22083f7

Fix unit test

434b1dd

Myoldmopar approved these changes May 22, 2024

View reviewed changes

Myoldmopar merged commit 1b333b5 into develop May 29, 2024
15 checks passed

Myoldmopar deleted the Add-more-convergence-check-for-CO2-and-Cont branch May 29, 2024 15:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add convergence check for co2 and contaminant #10500

Add convergence check for co2 and contaminant #10500

lgu1234 commented May 9, 2024

lgu1234 May 9, 2024

lgu1234 May 9, 2024

lgu1234 May 9, 2024

lgu1234 May 9, 2024

lgu1234 May 9, 2024

lgu1234 May 9, 2024

Myoldmopar May 11, 2024

Myoldmopar left a comment

Myoldmopar May 11, 2024

lgu1234 May 13, 2024

Myoldmopar May 11, 2024

lgu1234 May 13, 2024

Myoldmopar May 22, 2024

Myoldmopar May 11, 2024

lgu1234 commented May 13, 2024

lgu1234 commented May 13, 2024

lgu1234 commented May 20, 2024

lgu1234 commented May 20, 2024

lgu1234 commented May 20, 2024

lgu1234 commented May 21, 2024

Myoldmopar left a comment

Myoldmopar May 22, 2024

Myoldmopar May 22, 2024

Myoldmopar May 22, 2024

lgu1234 May 22, 2024

Myoldmopar May 22, 2024

Myoldmopar May 22, 2024

Myoldmopar commented May 22, 2024

lgu1234 commented May 22, 2024

Myoldmopar commented May 29, 2024

Add convergence check for co2 and contaminant #10500

Add convergence check for co2 and contaminant #10500

Conversation

lgu1234 commented May 9, 2024

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

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

lgu1234 commented May 13, 2024

lgu1234 commented May 13, 2024

lgu1234 commented May 20, 2024

lgu1234 commented May 20, 2024

lgu1234 commented May 20, 2024

lgu1234 commented May 21, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Myoldmopar commented May 22, 2024

lgu1234 commented May 22, 2024

Myoldmopar commented May 29, 2024