Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Fix the Thermal Comfort CEN 15251 Running Mean Temperature Calculation #8175
Fix the Thermal Comfort CEN 15251 Running Mean Temperature Calculation #8175
Changes from 3 commits
e62502d
da1fdc8
047e3d1
e334b84
440c0ae
a33eb9f
e27106c
eeed712
33d8a62
50c183d
dcd6271
0613cd4
c8232b8
d500677
73ed5e6
b27d75a
7c08bf9
ce2b14c
5bed1ff
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 still a good change.
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.
So I guess the
- 1
wasn't needed because we already offset DayOfYear by 1 a couple lines up?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 not sure about the original intent, but I think this could be one explanation.
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.
24 * (calcStartDay - 1)
is a conversion to hours. Why exactly was this changed when calcStartDay wasn't changed?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.
Because it was wrong by offsetting one excessive day here. This was also an issue with the ASHRAE comfort calculation function above, which was fixed in #6474.
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, this code is wrapping the epw to 24*calcStartDay as the number of hours last year and +1 is the first hour of this year. If you pulled develop this would not show as a diff. Never mind that right now.
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 whole thing appears to be built around an assumption that the weather file has hourly data, which makes me feel funny. This PR isn't changing it, just handling this part better, and that is acceptable here.
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.
Using the new dynamically determined days in year is great.
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'm willing to accept this as the corrected start by shifting a day.