EnergyPlus Crash Due to Zero Input Value for Variable Speed Coil Total Cooling

[RKStrand]

Pull request overview

• Fixes EnergyPlus Stuck with Unhandeled Exception #10386
• A user had an input file where the total cooling capacity for a DX variable speed coil was set to zero for a 10 speed coil that clearly had cooling capacity for each "stage". While zero could potentially be a valid answer in some situations, a zero here is not valid. The problem then was that fraction of operation was then being divided by a zero capacity. The NaN value did not cause a problem right at the point of the divide by zero but rather later it ended up causing an infinite loop in a psychrometric routine. This was fixed by checking the total capacity for a zero (or non-valid negative number) and reseting it to the highest stage capacity to avoid the divide by zero. The user's file with the problem runs successfully without an infinite loop. Warning messages are also now produced to alert the user to this zero value in their input file. In addition, some code cleanup was done in the get input routine of the variable speed DX coil as part of this work. No changes needed to either the documentation or the IDD. No differences in the program results after the change as anticipated.

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.

• Title of PR should be user-synopsis style (clearly understandable in a standalone changelog context)
• Label the PR with at least one of: Defect, Refactoring, NewFeature, Performance, and/or DoNoPublish
• Pull requests that impact EnergyPlus code must also include unit tests to cover enhancement or defect repair
• 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
• 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.

• Perform a Code Review on GitHub
• If branch is behind develop, merge develop and build locally to check for side effects of the merge
• If defect, verify by running develop branch and reproducing defect, then running PR and reproducing fix
• If feature, test running new feature, try creative ways to break it
• CI status: all green or justified
• Check that performance is not impacted (CI Linux results include performance check)
• Run Unit Test(s) locally
• Check any new function arguments for performance impacts
• Verify IDF naming conventions and styles, memos and notes and defaults
• If new idf included, locally check the err file and other outputs

[mjwitte]

I'm not 100% certain that this is the correct fix. My understanding of the variable sped coils is that the various capacities are the different speeds are intended to be scaled by the top-level Gross Rated Total Cooling Capacity At Selected Nominal Speed Level. That's how the I/O Ref describes it:

Based on that I would expect a zero input here to give zero capacity at all speeds. When it's dividing by zero, is it zero/zero there? A warning about zero capacity is still useful.

Also, do you recall which Psych function is going into an infinite loop? Seems that should be protected somehow, if there's a way without slowing down the psych function too much. That could be a separate unit test that throws the same bad values at the psych function?

[mjwitte]

@RKStrand Is this the only substantive change? A walkthrough highlighting the real changes would be helpful.

[RKStrand]

Yes, that is the only real change--a call to this new routine to sort out the issue as I saw it.

[RKStrand]

@mjwitte First off, my apologies for a description that was unclear at best and probably confused the issue. So, let me backtrack and correct that...

A user had a file that included a variable speed coil. The coil had 10 different speeds. From my analysis, the ten different speed definitions made sense. What did not make a whole lot of sense was that the total cooling capacity was set to zero rather than autosized or set to the same capacity as the maximum speed capacity. When that happened, if I'm remembering correctly, either the top speed capacity got reset or one of the calculations was using the total capacity to calculate the fraction. That's where the divide by zero came in. I'm trying to track that down again now. The fix I proposed simply resetting the total capacity when it was less than or equal to zero but not Autosize to the highest speed capacity. I guess it could be set to autosize as an alternative. The rest of the changes in the routine were cosmetic/modernization. While well intended, I can see how it might mask the point of this fix.

So, without the fix, E+ got stuck in an infinite loop (more on that in a minute). If you change the 0 total cooling capacity to autosize or the same as the highest speed capacity, it runs just fine. If the total capacity is zero though, a divide by zero occurs and rather than giving a divide by zero right on the spot, E+ continues to move along until it gets stuck in the variable speed coil simulation in an infinite loop. I said psychrometric routine earlier because when I paused in debugger that's where it is. But that's not really where it gets stuck--the psychrometric routine is just being called by the variable speed coil. So, I don't think there is anything wrong with the psychrometric routine (sorry for making that confusing). It's actually bogged down in a sizing routine that is part of the init.

Does that help any? Do you still have concerns about this solution?

[mjwitte]

@RKStrand Thanks for the additional details. I still hold that the input in question which is zero should be king and force the entire coil to zero. @rraustad any thoughts here?

[RKStrand]

This will likely get discussed today in the iteration call, but I thought I would bring this up again. @rraustad, what are your thoughts on this potential fix and comments by @mjwitte?

[rraustad]

@RKStrand for 1) I think we should allow any component to have 0 capacity or flow to allow the user to eliminate it from the simulation, and 2) if the autosized cooling load = 0 then the coil capacity will also = 0. I see coils sizing to 0 quite often, especially heating coils.

[rraustad]

@RKStrand the desription implies that it was the user input for rated capacity that was set to 0, and only at the highest speed. Whether that is a mistake or intentional, for this corner case, it should be handled. How is another question. Reduce to a 9 spd coil? Set all operating capacities to 0? I think we should avoid changing the users input.

[rraustad]

Either the user wanted a 0 coil capacity or completely missed this input. This coil should have 0 capacity.

10,                                     !- Nominal Speed Level {dimensionless}
0,                       !- Gross Rated Total Cooling Capacity At Selected Nominal Speed Level {W}

If any of these inputs are 0 then the capacity at that speed will also be 0 and is related to this discussion. This input provides a multiplier of total capacity at speed n so no divide by 0 issue. Is there a check of reference speed n capacity > reference speed n-1 capacity?

N16, \field Speed 1 Reference Unit Gross Rated Total Cooling Capacity
    \note Total cooling capacity not accounting for the effect of supply air fan heat
    \units W
    \type real
    \minimum 0
    \required-field

N88, \field Speed 10 Reference Unit Gross Rated Total Cooling Capacity
    \note Total cooling capacity not accounting for the effect of supply air fan heat
    \units W
    \type real
    \minimum 0

[RKStrand]

@rraustad @mjwitte So, based on what I am hearing from both of you, there is a desire to allow the user to easily eliminate a coil from the simulation by setting the total capacity to 0 which would override all of the remaining input. In the case of the user input file, the zero overall capacity would eliminate all 10 of the speeds and the coil would never operate. I will back out the current solution that sets the capacity to the capacity of the highest speed and implement a new solution that will do what you have asked. Not sure this will make the cutoff for V24.1, but I don't think that's a big deal at this point if it doesn't make it.

[Myoldmopar]

CI was totally green here. But it looks like it may need a bit more time to wrap things up. I'm going to push this to after release for now, but am open to squeezing it in still if it becomes all ready.

@RKStrand thank you for the refactor/cleanup here as well! I would ask that you put the bug fix in a standalone commit that I can inspect/checkout/test, and then any cleanup commits you want to make after, or before, that.

I won't do a deep review today, but if this is all ready sometime next week, we may still get it in for 24.1.

[RKStrand]

Closing this pull request which is superseded by Pull Request #10470

#10470