-
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
Add new people schedule columns to EIO/initialization summary to address #10314 #10437
Conversation
…port columns to use it.
This is impacting every EIO file that has lighting or internal equipment, in other words, all of them. But It is simply renaming two columns and adding six new columns to the internal load reports so that is expected. I spot checked a bunch of files and they are doing as expected and nothing more. The 2.7% decrease in speed does not make sense to me. @Myoldmopar is that possibly due to something else going on? This only impacts the routine that populates the EIO file from internal games. Based on this, I think this is ready to review. Since this is changing output it is on track for 24.2 so it is not critical this week. |
The documentation updates should be done soon. |
src/EnergyPlus/ScheduleManager.cc
Outdated
break; | ||
case DayTypeGroup::WeekEndHoliday: | ||
dayTypeFilter = {true, false, false, false, false, false, true, true, false, false, false, false}; | ||
break; |
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.
Is there something else needed here to include holidays?
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.
The first true is Sunday, then five falses are Monday to Friday, then true for Saturday and then true for Holiday.
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 see, my comment shows Sunday twice. I should probably verify which one is really Sunday :-)
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.
The other thing I noticed is that this dayTypeFilter array is a constexpr for each day type. It would be nice to set these up at compile time and somehow use the correct array in the loop below.
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 fixed the comment based on the enumeration:
EnergyPlus/src/EnergyPlus/ScheduleManager.hh
Lines 73 to 90 in 8955374
enum class DayType | |
{ | |
Invalid = -1, | |
Dummy = 0, | |
Sunday = 1, | |
Monday, | |
Tuesday, | |
Wednesday, | |
Thursday, | |
Friday, | |
Saturday, | |
Holiday, | |
SummerDesignDay, | |
WinterDesignDay, | |
CustomDay1, | |
CustomDay2, | |
Num | |
}; |
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 changed to using std::array so I can make these constexpr.
This does look like a change only to output reporting yet there are other diffs. For example, FCfactor data is not reported, which looks correct for 5ZoneAirCooledWithSpaceHeatBalance and 5ZoneAirCooledWithSpaces, but what change caused that diff?
and the actual input file location object info is reported now, not the weather file used (../testfiles/CMakeLists.txt). Maybe because this branch is behind develop? I would think if we report the input file location, then we should also report the weather file used?
and a small diff radiant to convective decay curve data (0.30 vs 0.31) which may be the reason for the zone and component sizing diffs.
|
@rraustad I'm going to remerge and try it again since I only added documentation changes last time and time before that I was only showing the EIO diffs. |
src/EnergyPlus/ScheduleManager.cc
Outdated
if (dayTypeFilter[jType - 1]) { | ||
auto &daySch = state.dataScheduleMgr->DaySchedule; | ||
MinValue = min(MinValue, minval(daySch(weekSch.DaySchedulePointer(jType)).TSValue)); | ||
MaxValue = max(MaxValue, maxval(daySch(weekSch.DaySchedulePointer(jType)).TSValue)); |
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 here:
auto &daySch = state.dataScheduleMgr->DaySchedule(weekSch.DaySchedulePointer(jType));
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
Noticing an issue when the day type (e.g., SummerDesignDay vs WinterDesignDay) is called out in different "For" groups. Using 5ZoneAirCooledWithSpaceHeatBalance.idf.
The data is correct (i.e., the minimum for all design days is a 0.02 multiplier), however, I am wondering if internal gains on a summer design day vs winter design day are important enough for a dedicated column? Here the minimum for a winter design day should match Weekends/Holidays column using a 0.2 multiplier. Develop results as a comparison for reviewer: |
And wondering if CustomDay1 and 2 need their own columns? |
I pulled develop and the unit tests run successfully. |
I considered it but decided that they were not used much (at least as far as I could tell). |
Interesting idea so you are suggesting separate min and max for summer design day and for winter design day. Another two columns. I can do that if you think it would be useful. I would guess for many modelers the design day values for these are constant so the min for the winter and the max for the summer will probably show the values they care about. But if you think it will be useful, I can add them. |
@JasonGlazer I'm not sure if it is useful or not. @mjwitte ? |
src/EnergyPlus/ScheduleManager.cc
Outdated
ShowFatalError(state, "getScheduleMinMaxByDayType called with ScheduleIndex out of range"); | ||
} else if (ScheduleIndex > 0) { | ||
std::array<bool, maxDayTypes> dayTypeFilter; | ||
std::fill(dayTypeFilter.begin(), dayTypeFilter.end(), false); |
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.
Is this initialization (4517) really necessary if you are going to fill this array anyway with one of the below? Also, ScheduleIndex < 1 (line 4513) is after testing for ScheduleIndex == -1 (4507) and == 0 (4510). Is it even possible for a schedule index to be less than -1? if not, then ScheduleIndex < 1 at line 4515 is not needed (not sure that fatal is needed either but it's safe). Ideally, if the fatal were not there then if (ScheduleIndex > 0)
could be first in the IF ladder since it is most common. This is probably way too picky for something that has limited use for the new table columns.
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 thanks for the careful review. I think I addressed your suggestions with the latest comit.
I looked through all the warnings and the EIOs changed as expected. I spot checked a bunch. This is ready for review. |
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 looks good now.
"Convected,Sensible Fraction Calculation,Activity level,ASHRAE 55 Warnings,Carbon Dioxide Generation Rate," | ||
"Minimum Number of People for All Day Types,Maximum Number of People for All Day Types," | ||
"Minimum Number of People for Weekdays, Maximum Number of People for Weekdays, " | ||
"Minimum Number of People for Weekends/Holidays, Maximum Number of People for Weekends /Holidays," |
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.
false, false, false, false, false, false, false, false, false, true, false, false}; | ||
constexpr std::array<bool, maxDayTypes> dayTypeFilterNone = { | ||
false, false, false, false, false, false, false, false, false, false, false, false}; | ||
if (ScheduleIndex > 0 && ScheduleIndex <= state.dataScheduleMgr->NumSchedules) { |
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 am trying to convince myself that it is even possible for ScheduleIndex > NumSchedules. Not all functions in ScheduleManager use this check (i.e., ScheduleIndex <= NumSchedules). The only way to get an index is to call GetScheduleIndex, which uses FindItemInList and will either return a valid index or 0. I'm not convinced yet so nothing to do here. While investigating this I noticed in GetScheduleIndex that WeekSchedule(Schedule().WeekSchedulePointer()).Used
and WeekSchedule(Schedule().WeekSchedulePointer()).DaySchedulePointer().Used
are set but never used anywhere.
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.
Used
variables are never used. Alanis would be proud of that irony.
// store for the next call of the same schedule | ||
curSch.MaxByDayType[curDayTypeGroup] = MaxValue; | ||
curSch.MinByDayType[curDayTypeGroup] = MinValue; | ||
curSch.MaxMinByDayTypeSet[curDayTypeGroup] = true; |
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 another loop over people schedules in DetermineSystemPopulationDiversity. Not sure if this could help there or if when checking those people schedules these 3 values could be set to save even more simulation time.
|
||
People Internal Gains,~ SPACE1-1 PEOPLE 1, OCCUPY-1, SPACE1-1, 99.16, 11.0, 11.0, 0.111, 9.015, 0.300, 0.700, AutoCalculate, ACTSCHD, No, 3.8200E-008, 0, 11 | ||
People Internal Gains Nominal, SPACE1-1 PEOPLE 1, OCCUPY-1, SPACE1-1, 90.00, 10.0, 10.0, 0.111, 9.009, 0.300, 0.700, 0.550, ACTSCHD, No, 3.8200E-008, 0.0, 10.0, 0.8, 4.0, 0.6, 2.5, 9.5, 10.0, 0.0, 0.5, |
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.
Correction. The word "Nominal" is added to the eio title and was already included in the html using develop. So nothing to do here.
This numeric field is the calculated maximum amount of lighting in Watts based on the calculated lighting level (above) * the maximum value (annual) for the lights schedule. It may be useful in diagnosing errors that occur during simulation. | ||
\subsubsection{Field: Minimum Lighting Level for Weekdays \{W\}}\label{field-minimum-lighting-level-for-weekdays-w} | ||
|
||
This numeric field is the calculated minimum amount of lighting in Watts based on the calculated lighting level (above) * the minimum value for the lights schedule for weekday day types (Monday through Friday). |
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.
Noting that the wording from here forward is different from the description of people.
based on the number of people field
based on the calculated lighting level (above)
|
||
std::tie(schMin, schMax) = getScheduleMinMaxByDayType(*state, index, DayTypeGroup::WinterDesignDay); | ||
EXPECT_EQ(0.16, schMin); | ||
EXPECT_EQ(0.25, schMax); |
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 took me a minute to see the 2 WinterDesignDay in the HIGHLOW02 schedule. Same for other schedule types. Good unit test.
Pulled develop and unit tests run to completion. |
I think because there was [decent_ci_skip] in the last commit there are no results to review. |
Sorry, I didn't realize that adding [decent_ci_skip] would make accessing those previous CI results more difficult. |
Yes, the only diffs are shown in the eio file for internal gains information. For 1ZoneDataCenterCRAC_wApproachTemp the schedule is always 1 so the new data for min/max internal gains for all day types is the same as the peak value of 50000 W.
|
@JasonGlazer are there any other changes planned? @Myoldmopar this is ready to go if no other changes planned. |
@rraustad thanks for the careful review. I have no other changes planned so this is ready to go. |
src/EnergyPlus/ScheduleManager.cc
Outdated
for (int jType = 1; jType <= maxDayTypes; ++jType) { | ||
if (dayTypeFilter[jType - 1]) { | ||
auto &daySch = state.dataScheduleMgr->DaySchedule(weekSch.DaySchedulePointer(jType)); | ||
MinValue = min(MinValue, minval(daySch.TSValue)); | ||
MaxValue = max(MaxValue, maxval(daySch.TSValue)); | ||
} |
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.
Rather than cycle through the entire schedule for every daytype group, why not cycle through once and store the max/min for each day type first? Then cycle through those to sort into the DayTypeGroups.
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 I did some optimization of this loop that should greatly improve it's performance although it is not exactly using the approach you suggested.
This is running slightly faster than develop now! |
All the warnings are EIO diffs. I spot checked them again and saw nothing that we weren't expecting. I think this is ready for a final review and (hopefully) merge. |
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.
No problem here.
false, false, false, false, false, false, false, false, false, true, false, false}; | ||
constexpr std::array<bool, maxDayTypes> dayTypeFilterNone = { | ||
false, false, false, false, false, false, false, false, false, false, false, false}; | ||
if (ScheduleIndex > 0 && ScheduleIndex <= state.dataScheduleMgr->NumSchedules) { |
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.
Used
variables are never used. Alanis would be proud of that irony.
@@ -69,6 +69,7 @@ struct EnergyPlusData; | |||
namespace ScheduleManager { | |||
|
|||
constexpr int ScheduleAlwaysOn = -1; | |||
constexpr int ScheduleAlwaysOff = 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.
Much better than a magic number used everywhere, nice.
std::array<bool, static_cast<int>(DayType::Num)> MaxMinByDayTypeSet{ | ||
false}; // minimum and maximum values by daytype have been stored for this schedule | ||
std::array<Real64, static_cast<int>(DayType::Num)> MinByDayType{0.0}; // minimum values by daytype for this schedule | ||
std::array<Real64, static_cast<int>(DayType::Num)> MaxByDayType{0.0}; // maximum values by daytype for this schedule |
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.
Multiple arrays allocated to the same size hints that they may be nicer as a single struct. Not a big deal since there are just 3 arrays, but if it grew to more, it might be worthwhile.
EIO diffs as expected, but builds and tests happily locally. I think this is ready to go, I'm going to merge. Thanks @JasonGlazer ! |
Pull request overview
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.
Reviewer
This will not be exhaustively relevant to every PR.