Move ZeroSourceSumHATsurf in DataHeatBalance to ChilledCeilingPanelSimple

rraustad commented 1 year ago

Issue overview

Followup to #9921. The variable ZeroSourceSumHATsurf stores the sum of zone (or space?) surfaces radiant heat. This methodology is used for all radiant heaters. The discussion is in regard to more than 1 radiant heater in the same zone and how this zone surface data summation is used within each radiant model. The ElectricBaseBoard branch #9921 moved this variable to be a member of the electric baseboard instead of being indexed to the zone where the baseboard was installed. There is uncertainty of how this information should be used within a radiant heat transfer model.

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.

[ ] Defect file added (list location of defect file here)
[ ] 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'm assigning this to myself since a lot (all?) of these instances relate to stuff that was worked on by UIUC. I will take a look at this sometime in the future and get back to you.

rraustad commented 1 year ago

@RKStrand perfect. Thank you.

RKStrand commented 1 year ago

@rraustad Looking at this issue this morning...trying to wrap my head around what was done/discussed for #9921 and what is being used in the code for all of these radiant models (it's been a while since I've had to think about this issue). So, if this post seems a bit odd, please note that I'm trying to understand the issue and not question anything that was done in #9921. If I am understanding the issue correctly, the ZeroSource terms being in the zone heat balance data structure was problematic and it was moved the electric baseboard for that model only (and not the several other models that use this variable). It seems like the issue is that when there is more than one baseboard there is a problem, if I am understanding this correctly? If I have pieced that together correctly, then I think I understand the issue and how this is a problem. There is a hidden assumption in the ZeroSource method is that there is only one radiant system in the zone. The point of that variable, as I am sure you have already figured out, is to account for the fact that radiant systems will impact surfaces within the space which in turn impacts the convection from the surfaces. This has to be included in how much of the load is met by that system. Otherwise, there is a concern that the full impact of the radiant system will not be captured and LoadMet will not be accurate, leading to improper conditioning (missing the setpoint). While having more than one radiant system in a zone isn't all that common, in those cases where more than one radiant system type is in the zone, the LoadMet is not being calculated correctly because ZeroSource is not being updated to include what the other systems are doing. So, this isn't a violation of the 1st Law or anything, more of a controls possibly not working properly issue. Is this an accurate summary of the issue that you noticed? Or am I off on a tangent?

mjwitte commented 1 year ago

@RKStrand That might be useful if the various radiant systems were indeed sharing the same ZeroSourceSumHATsurf values, but they are not. ChilledCeilingPanel is the only equipment that saves this in state.dataHeatBal->Zone(ZoneNum).ZeroSourceSumHATsurf.

All of the others (including the baseboard in #9921) were/are storing it in their respective namespaces as an array by zone e.g. state.dataHighTempRadSys->ZeroSourceSumHATsurf.dimension(state.dataGlobal->NumOfZones, 0.0); The motivation for the change in #9921 was this:

state.dataElectBaseboardRad->ZeroSourceSumHATsurf was an array by zone. When a baseboard is called, this was updated for the zone holding that baseboard

if (state.dataGlobal->BeginTimeStepFlag && FirstHVACIteration) {
            ZeroSourceSumHATsurf(ControlledZoneNum) = state.dataHeatBal->Zone(ControlledZoneNum).sumHATsurf(state);

Looking at this code, it seemed unnecessary to have an array by zone, since the elements for zones without a baseboard of that type would never be touched. So we decided to move it into the baseboard struct.

Given this is updated by each piece of equipment at BeginTimeStepFlag && FirstHVACIteration) it seems appropriate for each piece of equipment to save it's own value, because this will include the impact of any equipment that already operated in this zone, correct? So, it's dependent on the order of the equipment list. If everyone touched a shared value like state.dataHeatBal->Zone(ZoneNum).ZeroSourceSumHATsurf then it would just keep getting higher and higher and be incorrect if the first piece of equipment was called again in a later iteration.

So it seems appropriate for each piece of radiant equipment to save it's own value rather than use a shared place. Of course, this will only include whatever the prior equipment output was during the first iteration. If that changes, then later equipment will still be using the first iteration result, but that's probably the best we can do.

p.s. Swimming pool is actually saving a full zone array for each swimming pool. this->ZeroSourceSumHATsurf.allocate(state.dataGlobal->NumOfZones);

RKStrand commented 1 year ago

@mjwitte Thanks for that clarification. I think I better understand the issue now. I agree that even after that this is still all based on the first iteration result for the ZeroSource variables. I'll think about that some more, but right now, the only solution that I can think of would be extremely messy (recalling the surface heat balances a bunch more times by running the heat balance once, each iteration, for a particular system being off and then again if it turns on). That might be a bridge too far.

At a minimum, the chilled ceiling panel needs to be fixed so that it isn't using something out of dataHeatBal. Beyond that, I'll also need to think more about whether the swimming pool variant is an improvement or not. With ZeroSource being calculated once per zone at the first iteration (if I'm remembering correctly), I'm not sure that it needs to have a full array of zones on every pool. In fact, it doesn't even need to be an array since it's only going to use the value for the zone that the pool is in. Not very consistent implementation on my part... 😬😳

RKStrand commented 1 year ago

@rraustad @mjwitte After looking at the code and thinking through this some more, I am in full agreement with what was done for #9921. That is the correct solution and to address this issue, I will implement this in the other systems/equipment that use this ZeroSource term. So, all of these will get pushed to local and will only save the value for the zone in question (not an array of all zones which makes no sense).

NREL / EnergyPlus