Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Increase VRF terminal unit min-field to include supp heat coil turn-off temperature #10283

Merged
merged 1 commit into from
Nov 21, 2023

Conversation

yujiex
Copy link
Collaborator

@yujiex yujiex commented Oct 27, 2023

Pull request overview

With the min-field extended to include the "Maximum Outdoor Dry-Bulb Temperature for Supplemental Heater Operation" , the default value will be filled for it regardless of whether user included it in the object.

After adding the field, even when the line "Maximum Outdoor Dry-Bulb Temperature for Supplemental Heater Operation" is not included in the TU1 object. The supplemental heating coil's turnoff temperature will be set to default 21C.

before fix

image

after fix

image

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

@yujiex yujiex added Defect Includes code to repair a defect in EnergyPlus IDDChange Code changes impact the IDD file (cannot be merged after IO freeze) labels Oct 27, 2023
@yujiex yujiex requested review from mjwitte and rraustad October 27, 2023 23:44
@yujiex yujiex self-assigned this Oct 27, 2023
@@ -42139,7 +42139,7 @@ ZoneHVAC:TerminalUnit:VariableRefrigerantFlow,
\memo AirConditioner:VariableRefrigerantFlow or
\memo AirConditioner:VariableRefrigerantFlow:FluidTemperatureControl:* system.
\memo Terminal units can be configured as zone, air loop or outside air system equipment.
\min-fields 19
\min-fields 30
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I don't like this. I would rather allow users to enter the minimum amount of information and the model accounts for the rest of the data (i.e., model accounts for defaults past min-fields). Is there a way to modify getObjectItem so that min-fields are filled? and all other objects that have min-fields issues are "fixed" by this change?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rraustad increasing min-fields is the straightforward fix for now. Users can still enter less than min-fields if desired as long as all required fields are entered, the object can be shorter than min-fields. Interfaces will likely write everything up to min-fields, but that's not a bad thing, because it documents the additional fields in the idf. Filling every default fields in getObjectItem may cause issues with certain objects that have getInput functions that rely on checking the number of fields returned from getObjectItem. So it's better to get this fix in and work on a more general solution separately.

Copy link
Contributor

@rraustad rraustad Oct 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, you've said that before. The only required-field in ZoneHVAC:TerminalUnit:VariableRefrigerantFlow is the one just before max OAT for supp heater operation. If this field (N12) was now filled with the default I would expect diffs but I guess there is no example file that trips that limit.

N11, \field Maximum Supply Air Temperature from Supplemental Heater
   \required-field
   \type real
   \units C
   \autosizable
   \default autosize
   \note Supply air temperature from the supplemental heater will not exceed this value.
N12, \field Maximum Outdoor Dry-Bulb Temperature for Supplemental Heater Operation
   \type real
   \maximum 21.0
   \default 21.0
   \units C
   \note Supplemental heater will not operate when outdoor temperature exceeds this value.
A19; \field Controlling Zone or Thermostat Location
   \type object-list
   \object-list ZoneNames
   \note Used only for AirloopHVAC equipment on a main branch and defines zone name where thermostat is located.
   \note Not required for zone equipment. Leave blank if terminal unit is used in AirLoopHVAC:OutdoorAirSystem:EquipmentList.
   \note Required when terminal unit is used on main AirloopHVAC branch and coils are not set point controlled.
   \note When terminal unit is used in air loop and is load controlled, this zone's thermostat will control operation.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The diffs are likely triggered when the main heating coil has limited capacity and when space heating relies on supplemental heating coils for a lot. In existing example files, main heating coils are usually autosized or sized generously, so this situation doesn't occur.

@Myoldmopar
Copy link
Member

Can someone refresh this for me? This is separate from, but inspired, the work where @rraustad tried to fill out all defaults in GetObjectItem, right? I'm inclined to merge this in, because it doesn't seem like it will break anything, but I don't want to jump too far ahead if it needs another thought.

@Myoldmopar
Copy link
Member

No arguments appeared in 18 hours, so I'm going to merge this. If someone thinks this should be done differently, comment on here and we can open a PR to revert/fix properly. I'm going to go ahead and drop this tiny change in for now though.

@Myoldmopar Myoldmopar merged commit fde42be into develop Nov 21, 2023
13 checks passed
@Myoldmopar Myoldmopar deleted the fixReadingSuppHeatingCutoffTemp branch November 21, 2023 16:27
@mjwitte mjwitte changed the title Increase min-field to include supp heat coil turn-off temperature Increase VRF terminal unit min-field to include supp heat coil turn-off temperature Nov 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Defect Includes code to repair a defect in EnergyPlus IDDChange Code changes impact the IDD file (cannot be merged after IO freeze)
Projects
None yet
8 participants