Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Closing this for now. Will open a new PR when this is complete. |
|
This fixes a collection of HVACTemplate issues. It's ready for review. |
| CALL SetIfBlank(fldValStart + vsMinOutsideFlowOff, 'autosize') | ||
| CALL SetIfBlank(fldValStart + vsMinOutsideControlOff, 'ProportionalMinimum') | ||
| CALL SetIfBlank(fldValStart + vsEconomizerKindOff, 'None') | ||
| CALL SetIfBlank(fldValStart + vsEconomizerKindOff, 'NoEconomizer') |
There was a problem hiding this comment.
I can understand some of the changes here, but I don't have a feel for the full context of each change, or the impact/side effects. Perhaps you could comment on some of them, but I also understand that these have stemmed from the vastly improved testing structure set up for the new Python expand objects tool. So I am fine accepting them if you feel these are satisfying the issues identified over there.
There was a problem hiding this comment.
@Myoldmopar There are more details in #9018 . Here the default wasn't even a valid choice for this object.
| CALL AddToObjFld('Name', base + usAirHandlerNameOff,' Economizer Supply Air Temp Manager') | ||
| CALL AddToObjStr('Control Variable','Temperature') | ||
| CALL AddToObjStr('minimum supply air temperature {C}','13') | ||
| CALL AddToObjFld('minimum supply air temperature {C}',base + usCoolDesignSupplyTempOff,'') |
There was a problem hiding this comment.
So the magic "13" is gone in this line, what about the magic "45" in the following line?
mjwitte
left a comment
mjwitte
left a comment
There was a problem hiding this comment.
@Myoldmopar Walkthru.
| CALL SetIfBlank(fldValStart + vsMinOutsideFlowOff, 'autosize') | ||
| CALL SetIfBlank(fldValStart + vsMinOutsideControlOff, 'ProportionalMinimum') | ||
| CALL SetIfBlank(fldValStart + vsEconomizerKindOff, 'None') | ||
| CALL SetIfBlank(fldValStart + vsEconomizerKindOff, 'NoEconomizer') |
There was a problem hiding this comment.
@Myoldmopar There are more details in #9018 . Here the default wasn't even a valid choice for this object.
| CALL SetIfBlank(fldValStart + cvsCoolSetPtAtODBHiOff, '12.8') | ||
| CALL SetIfBlank(fldValStart + cvsCoolResetODBHiOff, '23.3') | ||
| CALL SetIfBlank(fldValStart + cvsHeatCoilKindOff, 'None') | ||
| CALL SetIfBlank(fldValStart + cvsHeatCoilKindOff, 'HotWater') |
There was a problem hiding this comment.
Incorrect default fixed here.
| ! check if there is no heating coil and heating design setpoint is > preheat design setpoint | ||
| IF (heatCoilType == ctNone) THEN | ||
| IF(StringToReal(FldVal(base + vsHeatSetPtDesignOff)) .GT. StringToReal(FldVal(base + vsPreheatSetPtConstantOff))) THEN | ||
| IF((preHeatCoilType .NE. ctNone) .AND. (StringToReal(FldVal(base + vsHeatSetPtDesignOff)) .GT. StringToReal(FldVal(base + vsPreheatSetPtConstantOff)))) THEN |
There was a problem hiding this comment.
This warning is only useful it there is a preheat coil.
| !AIR LOOP EQUIPMENT LIST ~ line 552 | ||
| CALL CreateNewObj('AirLoopHVAC:OutdoorAirSystem:EquipmentList') | ||
| CALL AddToObjFld('Name', base + vsAirHandlerNameOff,' OA System Equipment') | ||
| IF (heatRecovery .EQ. htrecSens) THEN |
There was a problem hiding this comment.
Fix the order of components to match node connections.
| IF (isCoolStPtSchedBlank .AND. (coolSetPtReset == csprNone)) THEN | ||
| IF ((heatCoilType .NE. ctNone) .AND. isHeatStPtSchedBlank .AND. (heatSetPtReset == hsprNone)) THEN | ||
| IF (FldVal(base + pvavsHeatSetPtDesignOff) .GT. FldVal(base + pvavsCoolSetPtDesignOff)) THEN | ||
| IF (StringToReal(FldVal(base + pvavsHeatSetPtDesignOff)) .GT. StringToReal(FldVal(base + pvavsCoolSetPtDesignOff))) THEN |
There was a problem hiding this comment.
Fix GT to be on numbers, not strings - duh.
| ELSE | ||
| IF (.NOT. isDehumidifyNone) THEN | ||
| ! Dehumidification humidistat | ||
| IF (.NOT. isHumidifierNone .OR. .NOT. isDehumidifyNone) THEN |
There was a problem hiding this comment.
Fix logic for adding humidistat(s) - only if needed.
| CALL AddToObjStr('Control Variable','Temperature') | ||
| CALL AddToObjStr('minimum supply air temperature {C}','13') | ||
| CALL AddToObjFld('minimum supply air temperature {C}',base + usCoolDesignSupplyTempOff,'') | ||
| CALL AddToObjStr('maximum supply air temperature {C}','45') |
There was a problem hiding this comment.
This is the cooling setpoint manager, so keep the fixed upper limit. Basically, this allows the cooling setpoint to go high enough to turn off the cooling coil if there's not cooling load.
| ELSE | ||
| CALL AddToObjStr('Economizer Maximum Limit Dewpoint Temperature (C)',' ') | ||
| ENDIF | ||
| CALL AddToObjFld('Economizer Maximum Limit Dewpoint Temperature (C)',base + usEconoUpDewLimitOff, ' ') |
There was a problem hiding this comment.
Dew point limit should always be used if input, regardless of economizer type (per documentation).
| CALL AddToObjFld('Rated Air Flow Rate {m3/s}', base + uhpsSupplyHeatFlowRateOff,'') | ||
| CALL AddToObjStr('Rated Air Flow Rate {m3/s}', 'autosize') |
There was a problem hiding this comment.
Component flow rates/capacities should always be 'autosize'. User values flow into the sizing objects.
| !AIR LOOP EQUIPMENT LIST ~ line 552 | ||
| CALL CreateNewObj('AirLoopHVAC:OutdoorAirSystem:EquipmentList') | ||
| CALL AddToObjFld('Name', base + ddsAirHandlerNameOff,' OA System Equipment') | ||
| IF (heatRecovery .EQ. htrecSens) THEN |
There was a problem hiding this comment.
Again - fix order of OA components to match node flow order.
|
Thanks for the walkthrough, it all makes sense. This is a highly valuable set of changes, and again, I am really excited about the robustness improvements to ExpandObjects as an unexpected outcome from the Python conversion. I'm going to merge this in. |
8 participants
Pull request overview
Pull Request Author
Add to this list or remove from it as applicable. This is a simple templated set of guidelines.
Reviewer
This will not be exhaustively relevant to every PR.