Fix coil speed number equal to zero

[tanaya-mankad]

Pull request overview

• Fixes Multispeed coil is simulated when speed indicates it should be off. #10631

While integrating a new multispeed coil type, we discovered that many existing coil simulation cases do in fact allow coil operation at speed = 0, and outlet node conditions can change even when the coil should not be running.
We added a check for zero speed (to the existing check that cycling ratio is zero at the lowest speed) and in those cases we exit the calculation by setting output nodes equal to input nodes.

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

[tanaya-mankad]

@rraustad, this PR currently exposes 19 integration tests and 9 regression tests that operate with a coil speed = 0. Is there any chance you could help us isolate whether a zero-speed is intentional or not?
In our (w/@nealkruis) previous discussion about this change, you indicated that a speed of 0 could exist if the coil was "off based on sensible load, and on for latent load". This seems unlikely from the existing Unitary code, as 1) there are no variables to indicate that we're in a "sensible-only" or "latent-only" scheme, and 2) how could one isolate the humidity control from sensible temperature? I feel like the test cases that are failing are very likely the ones that need speedNum re-initialized before they run, and it's being missed and left at 0 inadvertently (I can't trace the call stack back to any place speedNum is set).

[rraustad]

If the parent object has no load, or if set point control does not need to turn on, what is the speed set to? I would think it would be 0. But maybe not, it may be speed=1 and speedRatio = 0 (but that doesn't seem intuitive). The point is to allow a call to this sim function with the coil off so that the inlet condition can be passed to the outlet, even if the inlet mass flow = 0 or constant fan is active and the inlet mass flow > 0. If you pass inlet to outlet for a parent that is off, you also have to pass inlet to outlet for all the children. When a user looks at the cooling coil speed report and they know that the coil is off (e.g., winter) and see speed = 1 they would likely think the coil is on. If speed = 1 and speedRatio = 0 means the coil is off then this change is fine. I just think that if the coil is off then speed = 0, cyclingRatio = 0 and speedRatio = 0. One or more of those variables are only non-zero if the coil is on.

[tanaya-mankad]

@rraustad, I believe that we've established that there are no use cases where speed=0 is used intentionally except as you outlined (below). The very few regression failures that are left are happening because a handful of simulation paths go through branches where coil speed in a multispeed coil is just not set, which would be a separate issue to log. Could we address this PR with this in mind?

If the parent object has no load, or if set point control does not need to turn on, what is the speed set to? I would think it would be 0. But maybe not, it may be speed=1 and speedRatio = 0 (but that doesn't seem intuitive). The point is to allow a call to this sim function with the coil off so that the inlet condition can be passed to the outlet, even if the inlet mass flow = 0 or constant fan is active and the inlet mass flow > 0. If you pass inlet to outlet for a parent that is off, you also have to pass inlet to outlet for all the children. When a user looks at the cooling coil speed report and they know that the coil is off (e.g., winter) and see speed = 1 they would likely think the coil is on. If speed = 1 and speedRatio = 0 means the coil is off then this change is fine. I just think that if the coil is off then speed = 0, cyclingRatio = 0 and speedRatio = 0. One or more of those variables are only non-zero if the coil is on.

[rraustad]

Regarding sensible vs latent control there is an input to select which type of load to activate the system. And even if you pick to operate on either, there may not be a sensible load to meet while there is a latent load to meet.

AirLoopHVAC:UnitarySystem,
  A17, \field Latent Load Control
       \type choice
       \key SensibleOnlyLoadControl
       \key LatentOnlyLoadControl
       \key LatentWithSensibleLoadControl
       \key LatentOrSensibleLoadControl

The very first check in ControlUnitarySystemOutout is the availability schedule. At this point I would think speed = 0, speedRatio = 0, and cyclingRatio = 0. I would also think the child components would also get called with these inputs. Set your test file using an always off availability schedule and step through until the coils/fan gets called and see what happens.

    if (ScheduleManager::GetCurrentScheduleValue(state, this->m_SysAvailSchedPtr) <= 0.0) {
        return;
    }

[Myoldmopar]

Is this actually ready to go? It's kinda hard to tell based on the comments. If it is, let's get some fresh CI results with develop merged in and then I'll dig in. I know the code change is tiny, but if there are side effects, I'd hate to just drop it in last minute.

[nealkruis]

@Myoldmopar we believe it's ready. From what we've discovered, we believe there are never instances where the speed is intentionally passed into the function with a value of zero. There appear to be cases in a few example files where the speed is initialized to zero, but never set to a non-zero speed for simulation.

It would be good to get @rraustad input on these cases though (either now or on a new branch and a separate PR).

[rraustad]

I don't have a problem with this. The code change catches a possible developer error (what does SpeedNum == 0 mean?) which can be fixed at a later point.

[Myoldmopar]

OK, that's fantastic to hear! Alright, I'll do a test here on this as well.

[Myoldmopar]

Looks like there were a few small diffs last time this ran, so I'll do regressions here as well.

[Myoldmopar]

Diffs locally look similar. The only big diffs were in the two EMS MultiSpeed files, which tracks enough. Let's get this merged. Thanks @tanaya-mankad and @rraustad