-
Notifications
You must be signed in to change notification settings - Fork 393
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
Add Output Variables to Ideal Air loads #8302
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.
This will need a few minor cleanups. But it should be ready after that.
@@ -486,6 +490,16 @@ \subsubsection{Outputs}\label{outputs-043} | |||
|
|||
The volume flow rate of the supply air stream in m3/s using the standard density. The standard density is determined for dry air at the standard barometric pressure for the location's elevation and a temperature of 20.0ºC. The standard density does not vary over time. | |||
|
|||
\paragraph{Zone Ideal Loads Supply Air Temperature {[}C{]}}\label{zone-ideal-loads-supply-air-temperature-c} | |||
|
|||
The dry bulb temperature of the supply air stream in ºC. |
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.
We should use the \SI
package for units.
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.
Shall I change all of the title lines (like line 493) to use the \SI package? Or just in the text portion, like line 495?
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.
Just change the in-text units. In this case, just move to \si{\celsius}
(or whatever it ends up being). If you see others in this file, feel free to change them.
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.
Got it, thanks!
|
||
\paragraph{Zone Ideal Loads Supply Air Humidity Ratio {[}kgWater/kgDryAir{]}}\label{zone-ideal-loads-supply-air-humidity-ratio-kgWaterkgDryAir} | ||
|
||
The humidity ratio of the supply air stream in kgWater/kgDryAir. |
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.
These units are fine, though 🙈
@@ -2785,6 +2802,8 @@ namespace PurchasedAirManager { | |||
PurchAir(PurchAirNum).OutdoorAirVolFlowRateStdRho = OAMassFlowRate / StdRhoAir; | |||
PurchAir(PurchAirNum).SupplyAirMassFlowRate = SupplyMassFlowRate; | |||
PurchAir(PurchAirNum).SupplyAirVolFlowRateStdRho = SupplyMassFlowRate / StdRhoAir; | |||
PurchAir(PurchAirNum).ZoneIdealLoadsSupplyAirTemperature = Node(InNodeNum).Temp; |
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.
This is just passing the inlet temp and hum-rat through. I believe we want to use the already existing local variables SupplyTemp
and SupplyHumRat
to set these values.
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.
Or would it work to just move SupplyTemp
and SupplyHumRat
from local into the PurchAir
struct?
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.
And while you're in here, MixedAirTemp
and MixedAirHumRat
would also be useful outputs.
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.
That's fine with me.
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!
@@ -247,6 +247,9 @@ namespace PurchasedAirManager { | |||
int ZonePtr; // pointer to a zone served by an Ideal load air system | |||
int HVACSizingIndex; // index of a HVAC Sizing object for an Ideal load air system | |||
|
|||
Real64 ZoneIdealLoadsSupplyAirTemperature; // ZoneIdealLoadsSupplyAirTemperature [C] |
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'd shorten these to just SupplyAirTemp
and SupplyHumRat
.
|
||
\paragraph{Zone Ideal Loads Mixed Air Humidity Ratio {[}kgWater/kgDryAir{]}}\label{zone-ideal-loads-supply-air-humidity-ratio-kgWaterkgDryAir} | ||
|
||
The humidity ratio of the mixed air stream after mixing the recirculation air stream and outdoor air stream (if present) in kgWater/kgDryAir. |
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.
New addition. Does it look ok?
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.
Yes, this looks reasonable. I thought about adding "entering the zone inlet node" to be more specific but with the change for zone/spaces coming it's probably better at this point to leave this generic.
|
||
\paragraph{Field: Minimum Cooling Supply Air Humidity Ratio}\label{field-minimum-cooling-supply-air-humidity-ratio-000} | ||
|
||
The minimum humidity ratio (kg of water per kg of dry air) of the cool supply air. The default is 0.0077 kgWater/kgDryAir which corresponds to a 10C (50F) dew point. | ||
The minimum humidity ratio (kg of water per kg of dry air) of the cool supply air. The default is 0.0077 kgWater/kgDryAir which corresponds to a 10\si{\degreeCelsius} (50F) dew point. | ||
|
||
\paragraph{Field: Heating Limit}\label{field-heating-limit-000} | ||
|
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.
Mostly changing the tex code to use the siunitx package.
Output:Variable,*,Zone Ideal Loads Mixed Air Humidity Ratio,Detailed; | ||
|
||
Output:Variable,*,System Node Mass Flow Rate,Detailed; | ||
|
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.
Now the results appear in the test file output. Should I change the reporting frequency to hourly? Also, the effects are better visible for an annual simulation.
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.
Yes, I would make the reporting frequency consistent - looks like everything else is hourly.
@@ -2080,6 +2106,8 @@ namespace PurchasedAirManager { | |||
PurchAir(PurchAirNum).MinOAMassFlowRate = 0.0; | |||
PurchAir(PurchAirNum).TimeEconoActive = 0.0; | |||
PurchAir(PurchAirNum).TimeHtRecActive = 0.0; | |||
PurchAir(PurchAirNum).SupplyTemp = 0.0; | |||
PurchAir(PurchAirNum).SupplyHumRat = 0.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.
Not sure if they need to be reset, and if 0.0 is ok.
@@ -175,8 +175,6 @@ namespace PurchasedAirManager { | |||
Real64 SupplyAirMassFlowRate; // Supply air mass flow rate [kg/s] | |||
Real64 SupplyAirVolFlowRateStdRho; // supply air volume flow using standard density [m3/s] | |||
// Intermediate results | |||
Real64 FinalMixedAirTemp; // Dry-bulb temperature of the mixed air, saved for system ventilation load reporting [C] | |||
Real64 FinalMixedAirHumRat; // Humidity ratio of the mixed air, saved for system ventilation load reporting [kgWater/kgDryAir] | |||
Real64 HtRecSenOutput; // Sensible heating/cooling rate from heat recovery (<0 means cooling) [W] |
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.
Removed them since I felt the new mixedairtemp and mixedairhumrat were doing the same thing?
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.
And it appears that FinalMixedAirTemp
and FinalMixedAirHumRat
weren't being used anywhere, so find to replace them here.
@jmythms I'm not sure what the deal is with the regressions. Maybe we hit an API limit? (FYI @Myoldmopar ) Regardless of that, there are still some build warnings that will need to be addressed. Let me know if you need help figuring those out. |
@mjwitte May I get a review on this PR please? I added a new unit test for the new output variables. |
@@ -274,8 +276,7 @@ namespace PurchasedAirManager { | |||
EMSValueMassFlowRate(0.0), EMSOverrideOAMdotOn(false), EMSValueOAMassFlowRate(0.0), | |||
EMSOverrideSupplyTempOn(false), EMSValueSupplyTemp(0.0), EMSOverrideSupplyHumRatOn(false), | |||
EMSValueSupplyHumRat(0.0), MinOAMassFlowRate(0.0), | |||
OutdoorAirMassFlowRate(0.0), OutdoorAirVolFlowRateStdRho(0.0), SupplyAirMassFlowRate(0.0), SupplyAirVolFlowRateStdRho(0.0), | |||
FinalMixedAirTemp(0.0), FinalMixedAirHumRat(0.0), HtRecSenOutput(0.0), HtRecLatOutput(0.0), OASenOutput(0.0), OALatOutput(0.0), | |||
OutdoorAirMassFlowRate(0.0), OutdoorAirVolFlowRateStdRho(0.0), SupplyAirMassFlowRate(0.0), SupplyAirVolFlowRateStdRho(0.0), HtRecSenOutput(0.0), HtRecLatOutput(0.0), OASenOutput(0.0), OALatOutput(0.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.
Not sure why this line didn't wrap earlier. It's much longer than all the rest. Was Clang format used?
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.
@rraustad I just checked this, it was disabled in my IDE. Shall I do one more commit after reformatting?
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.
@Myoldmopar is it worth another commit for Clang formatting the hh file? I don't see any other changes needed, but I haven't tested the example file yet. Unit tests run clean locally.
@mjwitte I have reviewed and this looks good. I would expect 0 diff's with this change so as long as CI shows that it should be ready to merge (save any last minute edits). Also, these new report variables, do those need to be included in a new report variable summary for users? |
@rraustad We don't normally list new outputs in the output changes doc, maybe we should, but I wouldn't hold up this one. Did you reiew I/O Ref pdf to make sure all the changes are ok? Feel free to complete this review and merge. |
OK, I'll build the docs and run the example file. I see MTD and big table diff's on HVACTemplate-5ZonePurchAir. @jmythms did you review those diff's? |
The diffs were generated because I asked for the new variables in the IDF file. The MTD diffs are changes in meter numbers, screenshot attached below. Big table diffs are changes to the input fields, which were created from asking for these new variables: |
This output reports the outdoor air temperature. The outdoor air temperature is inherited from the associated outdoor air node in each timestep. The output is the average over the reporting period. | ||
|
||
\paragraph{Zone Hybrid Unitary HVAC Supply Air Humidity Ratio [kgWater/kgDryAir]} | ||
This output reports the supply air humidity ratio. For each timestep the value is calculated as a supply air mass weighted average of the supply air humidity ratio for each of the settings selected for the time step. For example, if the system operates for half of the timestep with supply air mass flow rate of 1 kg/s and supply air humidity ratio of 0.016 kg/kg, and for half of the timestep at 2 kg/s and 0.010 kg/kg, the supply air humidity ratio calculated for the time step would be 0.012 °C. The output is the average over the reporting period. | ||
This output reports the supply air humidity ratio. For each timestep the value is calculated as a supply air mass weighted average of the supply air humidity ratio for each of the settings selected for the time step. For example, if the system operates for half of the timestep with supply air mass flow rate of 1 kg/s and supply air humidity ratio of 0.016 kg/kg, and for half of the timestep at 2 kg/s and 0.010 kg/kg, the supply air humidity ratio calculated for the time step would be 0.012\si{\degreeCelsius}. The output is the average over the reporting period. | ||
|
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 is an error here in the units (i.e., 0.012 C). The existing units were converted correctly, however, the units should be kgWater/kgDryAir. Also, the previous sentence uses kg/kg, these units could also be updated to be consistent. This is the only issue I found with the doc changes.
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.
@jmythms there is a correction needed in the doc. This gives you the opportunity to Clang format the hh as needed (and cc? but I didn't see anything there). Give me a little time for more review before you make a commit.
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.
That was a stupid mistake on my part. I will make the changes, and add the reformatted "src/EnergyPlus/PurchasedAirManager.hh" file in with that commit.
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.
@jmythms I have finished my review. Go ahead and make your changes and commit. |
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.
Reviewed code, docs and unit test. With the next commit this is ready for merge.
@mjwitte @Myoldmopar this is ready to go after this next commit. I will merge when I can, just acknowledging my review and that this is approved and good to go. |
Thank you so much for the review! |
@jmythms also check off the boxes in the Pull request overview above under Pull request author. I could do that but I would like you to start doing that for your PRs. It's a reminder check against our work so we don't forget anything. |
I have checked it for this PR and will start doing it for the other ones. The requested changes were made. Thanks again. I also accidentally clang-formatted the "ScheduleManager.hh" file in the 863baf5 commit, did not revert it since I thought leaving it formatted would be a good thing. Just wanted to let you know. |
@jmythms, a tip moving forward. I want you to notice how much has changed when you clang format a file. I am glad I did not have to review the code with these format changes included with yours. The reviewer would have a very difficult time finding the actual code changes. Either format only the code you touch (in MSVS highlight the code, select Edit on the menu, then Advanced, then format selection) or clang format the file after reviewers have a chance to look at your changes. Just a tip to make the reviewers life easier. Anyway, I think this is ready now and I will run the unit tests one more time and then this can be merged. |
Unit tests run clean locally as expected. |
There is another diff here, the SolarShadingTestGPU example file but I think that has caused this same diff for some time. |
@jmythms did you have a pivotal ticket for this? I can't find one. |
Pull request overview
The documentation has also been changed.
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.