-
Notifications
You must be signed in to change notification settings - Fork 394
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
New Schedule for Minimum Airflow for VAV Boxes #7643
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Nigusse This looks good in general. Some comments.
\note values. This field can be used with any of the three "Zone Minimum Air Flow Input Method" | ||
\note Schedule values are fractions, 0.0 to 1.0. This field is intended for use with | ||
\note ASHRAE Standard 170. | ||
\note If this field is left blank, then the operating minimum air flow fraction value is set to 1.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Operating" is a bit confusing here. Even though the other inputs are the "design" minimum, they also are used for operation, and the existing "Minimum Air Flow Fraction Schedule Name" is primarily operational. It's only used for sizing if the other min flow fields are blank. How about "Minimum Air Flow Turndown Schedule Name" or "Minimum Air Flow Adjustment Schedule Name" or ???
Is this adjustment limited to zero to 1.0? Or would an adjustment >1.0 ever be applicable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will use "Minimum Air Flow Turndown Schedule Name" as the new field name but will use "Adjustment" in the description. I am thinking to limit to 0.0 to 1.0. I think users should use the design minimum fraction or the design minimum flow if they desire scale it up.
\note This field is used if the field Zone Minimum Air Flow Input Method is Scheduled | ||
\note Schedule values are fractions, 0.0 to 1.0. | ||
\note If the field Constant Minimum Air Flow Fraction is blank, then the average of the | ||
\note minimum and maximum schedule values is used for sizing normal-action reheat coils. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It needs to be very clear that this schedule will be multiplied by the new schedule. That's a little confusing. I suppose few users would do that, they'd likely just use the new schedule with a fixed minimum here, but one could use both schedules together.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will try to add more detailed explanation.
SingleDuct.hh | ||
Modify struct "SysDesignParams" to add two new member variables. | ||
|
||
struct SysDesignParams |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feel free to rename this confusing struct SysDesignParams
and its associated name Sys
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will take the liberty to rename the struct when I get there. Thanks.
|
||
Modify struct "DamperDesignParams" to add new new member variable | ||
|
||
struct DamperDesignParams |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here, I've always found DamperDesignParams
and Damper
to be confusing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will do.
These variable renaming were suggested during NFP review. Diffs are not expected due to these commits. |
@mjwitte you seemed to feel pretty good about this earlier on in the development. You have any lingering concerns? If not then if CI is happy once complete this can merge in. |
@Myoldmopar I took a quick look at just the IDD. That much looks good. I'll defer to you on reviewing the rest of this. |
OK, yeah it'll merge as long as Windows doesn't see anything. |
@Nigusse I tried to resolve the conflicts in this branch, but they are a little too involved. Can you please pull develop in and resolve them and push back up? Once complete and CI runs again this can go in. |
OK, will do. |
@Myoldmopar Resolved merge conflict and did cleanup. |
@Myoldmopar working to resolve confclit and will push updated branch shortly after local test. |
@Myoldmopar Sure, I am working on it. It will be done first thing in the morning. |
…resolved conflict
@Myoldmopar @mjwitte Merged develop into the branch and resolved conflicts. |
Run unit test suite locally and all passed. |
@mjwitte I am seeing audit diffs that I was not expecting. Comparing this branch to develop I see the following four output variables that must have been removed from develop for "AirTerminal:SingleDuct:ConstantVolume:NoReheat" object:
Would you please confirm? |
src/EnergyPlus/SingleDuct.cc
Outdated
SetupOutputVariable( | ||
"Zone Air Terminal Sensible Heating Rate", OutputProcessor::Unit::W, sd_airterminal(SysNum).HeatRate, "System", "Average", sd_airterminal(SysNum).SysName); | ||
SetupOutputVariable( | ||
"Zone Air Terminal Sensible Cooling Rate", OutputProcessor::Unit::W, sd_airterminal(SysNum).CoolRate, "System", "Average", sd_airterminal(SysNum).SysName); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mjwitte Thanks. I will correct in this branch and push again.
…-Min-AirFlow-For-VAV-Boxes
…-Min-AirFlow-For-VAV-Boxes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Nigusse I've pushed up a few minor text edits. I've run unit tests locally - all good. I've tested the new schedule with VAVSingleDuctReheat.idf - and it works.
There is no new example file for this. I'm not going to hold this up for that, but we need this somewhere in the testfiles. Perhaps there is an appropriate existing file to add this to (one that would correspond to applying ASHRAE Std 170).
Assuming CI comes back clean, I'll merge this soon.
@Nigusse Also - have you tested this with each of the affected terminal unit types? |
@mjwitte Yes, I have tested each combination. Also each combination has appropriate unit test. We can always add this new schedule field to selected existing test files. |
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.