ZoneAirMassFlowConservation object can crash when unconditioned zones are included

rraustad commented 1 year ago

Issue overview

A customer file showed some issues with the ZoneAirMassFlowConservation model. A defect file was created from 5ZoneAirCooled_ZoneAirMassFlowBalance by reordering the zone objects. The ZoneAirMassFlowConservation field Infiltration Balancing Zones was changed to AllZones to match the customer file. The ZoneMixing object were set to 0 flow/zone to also match the customer file.

The first issue with the was that the one unconditioned zone was not the first zone listed in the input file. For this reason, the difference in the zone order and the order of the ZoneReOrder array caused calculations to be performed on an unconditioned zone. In this figure line 4307 continues if the zone is uncontrolled. However, when ZoneAirMassFlowConservation is used (line 4309), ZoneNum1 is not the zone acted on, ZoneNum is the correct zone number.

For this reason, line 4307 was moved down to line 4309, and ZoneNum1 changed to ZoneNum.

This corrected the crash later in this function where zoneEquipConfig.ZoneNode = 0.

        auto &zoneNode = Node(zoneEquipConfig.ZoneNode);
        zoneNode.MassFlowRate = TotInletAirMassFlowRate;
        zoneNode.MassFlowRateMax = TotInletAirMassFlowRateMax;
        zoneNode.MassFlowRateMaxAvail = TotInletAirMassFlowRateMaxAvail;
        zoneNode.MassFlowRateMin = TotInletAirMassFlowRateMin;
        zoneNode.MassFlowRateMinAvail = TotInletAirMassFlowRateMinAvail;

The next issue is if the ZoneMixing object flow rates are set to 0 a Nan is encountered in the simulation.

The reason being that the ZoneMixingReceivingFr array is only set if there is a mixing flow rate. Since the user set the flow rate to 0 this variable was never initialized.

This issue was corrected by initializing the ZoneMixingReceivingFr array after allocation (line 3102).

and also initializing ZoneMixingReceivingFr each iteration (line 4836) such that a value of ZoneMixingFlowSum = x > 0, followed by the next iterations value of ZoneMixingFlowSum = 0, would update ZoneMixingReceivingFr when ZoneMixingFlowSum = 0.0.

After these changes the defect file runs successfully.

************* EnergyPlus Completed Successfully-- 1 Warning; 0 Severe Errors; Elapsed Time=00hr 00min 19.98sec

Issue and correction courtesy of Trane North America.

Details

Some additional details for this issue (if relevant):

Platform (Operating system, version)
Version of EnergyPlus (if using an intermediate build, include SHA)
Unmethours link or helpdesk ticket number

Checklist

Add to this list or remove from it as applicable. This is a simple templated set of guidelines.

[x] Defect file added (5ZoneAirCooled_ZoneAirMassFlowBalance_AllZones.idf.txt)
[ ] Ticket added to Pivotal for defect (development team task)
[ ] Pull request created (the pull request will have additional tasks related to reviewing changes that fix this defect)

RKStrand commented 1 year ago

@rraustad I have verified that the file does crash in develop where .ZoneNode = 0 as noted by Trane. While avoiding the case where this is zero also allows it to run to completion (skipping the auto &zoneNode block when this->ZoneNode = 0), I do not think that is the best idea because it still yields a severe error regarding a flow mismatch. In other words, the "easy" solution ignores a flow rather than assigns things correctly. The solution offered by Trane does eliminate the crash and I believe preserves the functionality and flow (no severe errors). I am working to get this implemented and a unit test defined. I have assigned this to myself. If someone is already working on this, please let me know asap so that I don't duplicate effort.

rraustad commented 1 year ago

I looked at these loops again to see what ZoneReOrder was supposed to mean and it seems, since these are separate loops, that this reordering scheme might? have a problem. The first loop checks for IsOnlySourceZone && IsSourceAndReceivingZone, and then the next 2 loops check for this same thing individually. What is happening to Loop2 as these loops are executed?

// zone mass conservation calculation order starts with receiving zones
// and then proceeds to source zones
int Loop2 = 0;
for (int ZoneNum = 1; ZoneNum <= state.dataGlobal->NumOfZones; ++ZoneNum) {
    if (!state.dataHeatBal->MassConservation(ZoneNum).IsOnlySourceZone &&
        !state.dataHeatBal->MassConservation(ZoneNum).IsSourceAndReceivingZone) {
        Loop2 += 1;
        state.dataHeatBalFanSys->ZoneReOrder(Loop2) = ZoneNum;
    }
}
for (int ZoneNum = 1; ZoneNum <= state.dataGlobal->NumOfZones; ++ZoneNum) {
    if (state.dataHeatBal->MassConservation(ZoneNum).IsSourceAndReceivingZone) {
        Loop2 += 1;
        state.dataHeatBalFanSys->ZoneReOrder(Loop2) = ZoneNum;
    }
}
for (int ZoneNum = 1; ZoneNum <= state.dataGlobal->NumOfZones; ++ZoneNum) {
    if (state.dataHeatBal->MassConservation(ZoneNum).IsOnlySourceZone) {
        Loop2 += 1;
        state.dataHeatBalFanSys->ZoneReOrder(Loop2) = ZoneNum;
    }
}

RKStrand commented 1 year ago

If I am understanding this code correctly, it seems like when it does the zone mass balances it has to re-order the sequence in which it looks at the zones to make sure everything is taken into account in the mass balances. Since they are impacted by mixing and different zones have different mixing, it seems to make sense that zones that serve as source and receiving would have to be looked at first, followed by ones that are only source zones. So, Loop2 is just to reorganize which order this mass balance happens and it's got to start at zero and build up from there. ZoneReOrder seems to just be a list of zone pointers in the order in which they will need to be simulated rather than the order in which they appear in the input file. But it's possible that I'm not understanding this or what you are trying to say. Does any of this help or did I miss the mark?

rraustad commented 1 year ago

I missed the ! on the first loop's conditionals. So this reorder stacks the zones in a specific order. And then the code shown in the description needs to act on that new order correctly. I haven't looked at this much more than that.

RKStrand commented 1 year ago

🤣🤣🤣 Actually, I totally missed that too and was still trying to figure out why it was doing that. Seemed to make no sense. Now it does. So, it does a mass balance on zones that don't do any mixing (or receive only?) first, then zones that both send and receive, and finally those that are source only. Kinda makes sense, maybe. Mixing only affects the receiving zone from an energy standpoint. The initials BAN show up in the code...is that Bereket?

rraustad commented 1 year ago

BAN = @Nigusse

Nigusse commented 1 year ago

@RKStrand Yes, the zone sequence re-order was intentional for the mass balance calculation. We have zones that serve as a source only, a receiving only, and both source and receiving. In order to balance the mass flow, iterative adjustment is done at the source zones, and the receiving zones are passive; they just get what the source zone provides. This is the main reason that I wanted the zones reordered.

RKStrand commented 1 year ago

@Nigusse Thanks for your comment. That makes sense based on what I saw in the code. If you have any concerns about the change here, feel free to comment. Thanks!

NREL / EnergyPlus