Cooling coil source side energy is now balanced for gshp + stratified tank + desuperheater test file.

For a gshp model with Coil:Cooling:WaterToAirHeatPump:EquationFit (no heat reclaim):

This first commit fixed (Cooling Coil Electric Energy + Cooling Coil Total Cooling Energy) to be equal to Cooling Coil Source Side Heat Transfer Energy. Its annual energy impact:

@rraustad Looking at these comments, it makes me confused if multiple desuperheater users are desired scenarios: https://github.com/NREL/EnergyPlus/blob/develop/src/EnergyPlus/HeatingCoils.cc#L1108-L1110
I am wondering which of the followings should it be to solve the reclaimed heat issue:

1. One source coil can only serve one desuperheater (water heating or heating), then should add error checking to avoid duplicate heat source.
2. One source coil at maximum is able to serve one water heating desuperheater + one heating desuperheater;
3. Multiple desuperheater users are allowed to use the same source coil with documentation giving some notice eg. the sum of reclaimed portions from the same source coil shouldn't exceed suggested values.

The current stage looks to be designed for at least scenario 2 or scenario 3 because it decremented available reclaimed heat capacity across namespaces.

Well, that error checking was added so that there was no more than 1 desuperheater per source. The reason is that a desuperheater can condense refrigerant which would change the performance of the source.

Coil:Heating:Desuperheater,
   \memo Desuperheater air heating coil. The heating energy provided by this coil is reclaimed
   \memo from the superheated refrigerant gas leaving a compressor and does not impact the
   \memo performance of the compressor.

If you run the hot gas off a compressor to 2 desuperheaters, and each consumed 30% of the available heat reclaim energy, it is likely that the refrigerant would condense (either in the desuperheater or the refrigeration condenser) and the refrigeration system would have extra sub-cooling and result in extra cooling on the air-side of the refrigeration system at the cooling coil. It's also likely that 1 desuperheater would condense refrigerant but this would be a small (model assumption) amount of sub-cooling if max heat reclaim is limited and not grossly affect the refrigeration system performance.

I couldn't find in the IDD (and did not look at the code) where a max reclaim efficiency is used (i.e., which input limits max reclaim energy).

Coil:Heating:Desuperheater,
  N1 , \field Heat Reclaim Recovery Efficiency
          \type real
          \minimum 0.0

Coil:WaterHeating:Desuperheater,
  N2 , \field Rated Heat Reclaim Recovery Efficiency
         \type real
         \minimum> 0.0

I recall 30% when this object was originally added to E+. You have looked at the code and might know how limits to recovered heat is addressed. You are right that if 2 desuperheaters consumed no more than 30% of the total available heat reclaim energy then that should be OK (i.e., same as 1 desuperheater consuming 30%). Since the energy bucket is decremented each time it is used then more than 1 desuperheater per source should be OK, but would the user be aware if the 2nd desuperheater did not having enough heat available to do the job expected? So using more than 1 desuperheater per source does push the envelope. I do like allowing users to model whatever they like, but we can't allow users to provide extra sub-cooling without including that sub-cooling in the cooling coil model (which we do not do now and only a component model could handle this without additional inputs from the user). I do like your items number 2 and 3 and we have to be sure to model these correctly so that we do not do something that would impact cooling coil performance without accounting for the change in performance.

I hope this clarifies a few things as you move forward.

Desuperheater was now using reclaimed heat and saved tank source side outlet temperature(temp from last time step) to calculate tank source side inlet temperature for current time step, then called tank calculation functions. But later tank calculation function was using timestep-averaged tank tamperature to recalculate source side heat transfer. This caused energy discrepancy between desuperheater heater rate and tank source side heat transfer rate. I think desuperheater should be called multiple times using updated tank temperature to ensure reclaimed heat was correctly fed into the tank source side (achieve some convergence).
This issue caused a big energy discrepancy of a test file with desuperheater connected to a storage tank:

@rraustad Do you mind taking a look at this? It's pretty close to completion, looks like it has fixed all targeted bugs. Some tests showed large diffs due to Qsource fix, another few due to desuperheater fixes. Any suggestion to improve these changes will be greatly appreciated!

@yzhou601 I don't use VS all that much. However, I believe you have to add the ClangFormat VS Extension, then run the commands as shown.

Actually, clang is built-in with VS2017, but there are options to activate clang, and to format automatically or manually. I use manual and apply as needed to avoid excess formatting diffs.
Tools - Options - Text Editor - C/C++ - Formatting - General.

If you go with manual formatting, then apply it from Edit - Advanced - Format Selection.

@mjwitte Thanks! So this manual method is applied to each file, right? Is there any way to run clang on the entire solution? For example, after making changes or resolving merge conflicts from different files, it will be great if I can run clang once across all source code files before push them up, and ci clang check won't be a concern.

@yzhou601 At most, only run it on the main files you have modified. If you have added a line or two in other files, do not run it there. If you run it across the entire code base, you're likely to cause merge conflicts with most of the dev team.

@yzhou601 At most, only run it on the main files you have modified. If you have added a line or two in other files, do not run it there. If you run it across the entire code base, you're likely to cause merge conflicts with most of the dev team.

Oh ok, I thought other files are already formatted with the same clang. Good to know, then I think I'm fine so far.

@mitchute Do you mind starting looking at this PR whenever you're available. I was kind of stuck not knowing what's the next step. If there's any issue, let me know and we can talk about how to address. I'm kind of overwhelmed by getting CI results the same as before pulling in new Master branch, I think they're looking much closer now. This PR solves following issues:

1. Heat reclaim efficiency is not correctly applied. It was caused by the "-=" operated multiple times in one timestep on heat reclaim capacity. It got fixed by restructuring the related data structure and tracking each heat-reclaim user independently. This fix will basically impact results of desuperheaters connected with DX cooling coils, introducing ci diffs.
2. Qsource didn't account for the startup loss electric energy for WaterToAirHeatPump:EquationFit coils. This issue caused coil cooling energy + electric energy != coil source side heat transfer energy. This fix might impact coil performance, introducing ci diffs.
3. The output Desuperheater Water Heater Heating Energy is not the same, or even close to Water Heater Source Side Heat Transfer Energy. There's a discrepancy between desuperheater and water thermal tank parts, caused calculated reclaim heat not totally applied to tank side, thus made reported variables look energy-unbalanced. This was fixed by adding a loop to iterate between these two functions more times until they're on the same page. This fix might have a performance impact(not quite sure how much).

Oh yeah, I was going to look at this but had forgotten. Thanks for the reminder.

For starters, could you come up with a better PR title? This one appears to have come straight from the GitHub default, which is the initial git commit message on this branch. The PR titles get summarized on the changelog for each release, as shown here for example: https://github.com/NREL/EnergyPlus/releases/tag/v9.2.0

I'll work on the review.

@yzhou601 are the example files linked over on #7251 still valid test cases for this issue? See files linked here: #7251 (comment)

There's one unit test failure after pulling in develop, however, I can't replicate it on mac or linux. Let's see what happens after CI has a chance to run again.

Not sure what's going on here. I've run the failing unit test on Mac and Linux in debug and release modes and I don't see any problems. I guess I'll look at the rest of the results for now.

@yzhou601 are the example files linked over on #7251 still valid test cases for this issue? See files linked here: #7251 (comment)

Yes this is what's supposed to be fixed here. So the example files are valid to test.
Though I've been using another test file for my local test. Let me know if those two get fixed as expected if you want to test them.

Not sure what's going on here. I've run the failing unit test on Mac and Linux in debug and release modes and I don't see any problems. I guess I'll look at the rest of the results for now.

I ran with Windows debug mode, it also passed.

I've checked the SingeSpeed file referenced here (#7251 (comment)) and the results are looking good. TheMultiSpeed coil appears to be broken, so I didn't dig into that one.

The heat reclaimed now matches what is being specified in the IDF.

0.25,                    !- Rated Heat Reclaim Recovery Efficiency

Previously, the Water Heater Source Side Heat Transfer Energy was not matching the Desuperheater Water Heater Heating Energy as shown in the next plot.

Annual energy between the two were also off by about 8%.

This now appears to be fixed by adding some additional iterations between these components and the results look much better.

Annual energy totals also match up more closely.

I also compared DOAToWaterToAirHeatPumpInlet.idf which is one of the other "big" diff files. The results are nearly identical. The condenser inlet temperatures looks to be the variable with the largest diffs.

As noted, there are going to be diffs due to changes during the warmup periods, and there are a few other assorted power diffs, but overall, they look reasonable.

As far as CI results go, this looks good with the exception of the ZoneUnitarySysTest.UnitarySystemModel_FlowPerCoolingCapacityTest unit test failure which I can't replicate locally.

I'm going to pull in develop one more time in hopes of that being fixed. We will look into it after that if it still fails.

@Myoldmopar @mjwitte @rraustad would one of you take a look at this when you get a minute? The numeric diffs appear to be justified. However, I want to get a second look at the err diffs. Most of the err diffs are related to changes to the number of times the plant loop temperature limits are exceeded. Admittedly, most of these errors were there before, but now the number of occurrences for most of these errors has gone up, say, for example from 6 to 15.

And then there's this error message from ASHRAE9012016_OfficeLarge_Denver:

    *************  ** Warning ** GetSpecificHeatGlycol: Temperature out of range (too low) for fluid [WATER] specific heat **
-   *************  **   ~~~   **   This error occurred 575 total times;
-   *************  **   ~~~   **   during Warmup 0 times;
-   *************  **   ~~~   **   during Sizing 0 times.
-   *************  **   ~~~   **   Max=-6.689049E-002 {C}  Min=-562.450726 {C}
+   *************  **   ~~~   **   This error occurred 551 total times;
+   *************  **   ~~~   **   during Warmup 0 times;
+   *************  **   ~~~   **   during Sizing 0 times.
+   *************  **   ~~~   **   Max=-0.135791 {C}  Min=-562.353545 {C}

Huh??? Sub absolute zero temperatures and we don't fatal out?

I'm going to hold off on this until we can take a little closer look at it.

Hmm, those numbers hint that a large amount of heat is being extracted from a small flow rate. The question is whether that's coming from the desuperheater portion of the simulation, or elsewhere that's unrelated to this fix.

The only thing I can suggest is to break at the warning and then use the call stack to see what calculation/condition caused the issue.

Thanks @mjwitte and @rraustad for the comments.

@yzhou601 could you dig in here a little and see whether this is your code or just some other lingering issue?

@mitchute The ASHRAE9012016_OfficeLarge_Denver doesn't have any desuperheater object, so it shouldn't be related to those changes to desuperheater functions.
I broke at the warning and checked the call stack, the issue basically came from CoolSys1, where there're chillers on supply side + some water cooling coils on demand side. The supply side is where most warnings came from.
I can't see why my changes increased the occurrences of these errors by tracking breakpoints for limited times, because almost all the breaks were caused by chiller side of CoolSys1 within scope of my observation. But I did see that this file has Coil:Cooling:WaterToAirHeatPump:EquationFit, to which I made some changes here to include startup losses. The changes are fairly straightforward per Richard's suggestion.

@Myoldmopar any last thoughts before this goes in? Numeric diffs all look justified. The only one that I'm not sure about is the UtilityRoutines_appendPerfLog2 unit test failure.

I have no issues with this going in. I'm looking at the convection issue again, and don't worry about that other failure -- it's nothing related to this. Merge at will.