Correct Thermal Comfort Clothing Value Reporting Error #11121
Conversation
The current clothing value was not being reported properly for situations when the user had clothing insulation scheduled. This was an issue for several for several models. This corrects the problem and the output value is properly assigned rather than always zero.
The Pierce model also did not have any code to set the clothing value reporting variable. This corrects this. Also a unit test verifying the fix was added. Finally, this variable was officially added to the IO Ref and two missing input fields were documented as well.
|
Appears that the two fields that had "missing" desriptions were not missing but rather misplaced. Moved the information to the correct place and added some degree symbols that were missing throughout this section.
|
|
Myoldmopar
left a comment
There was a problem hiding this comment.
I'm a little surprised with the move in the docs. Is the placement correct? It might be and I'm just missing something. Anyway, the functional changes are great so if you can confirm the doc comment, we can get this merged.
| is defined as the longest duration (number of hours), starting from the | ||
| beginning time of the risk period (e.g., the start time of a power outage), to | ||
| not above a certain temperature threshold (specified in this field). If not | ||
| specified, 86F (or 30C) will be used as the default temperature threshold. |
There was a problem hiding this comment.
These were moved/removed? I didn't expect that from the PR text. Could just be a groggy start for me though :)
There was a problem hiding this comment.
Yes, they were moved because the order did not match the IDD before. In the current IDD, these are placed at the end (N7, N8) of the object syntax. I moved them to match the IDD.
| is defined as the longest duration (number of hours), starting from the | ||
| beginning time of the risk period (e.g., the start time of a power outage) to | ||
| not above a certain temperature threshold (specified in this field). If not | ||
| specified, 86$^\circ$F (or 30$^\circ$C) will be used as the default temperature threshold. |
There was a problem hiding this comment.
So these were moved down here? It kinda feels a bit odd given the paragraph text above saying the IDF text follows.
There was a problem hiding this comment.
My bad. I moved that paragraph and made another commit. Should be better now.
| switch (people.clothingType) { | ||
| case DataHeatBalance::ClothingType::InsulationSchedule: { | ||
| state.dataThermalComforts->CloUnit = people.clothingSched->getCurrentVal(); | ||
| comfort.ClothingValue = state.dataThermalComforts->CloUnit; |
There was a problem hiding this comment.
Got it, so every scheduled block you need to update it.
Docs fix--when sections moved, a paragraph that leads into the next section was not accounted for.
|
Myoldmopar
left a comment
There was a problem hiding this comment.
Thanks for addressing my comments, @RKStrand. I'm doing a quick build locally. Looks like there is an unused variable, which is now properly causing a build error. So I'll fix that, push, and let CI give me one sanity check.
|
|
Happy again with my unused variable fixed. Merging this. Thanks @RKStrand |
Pull request overview
Description of the purpose of this PR
A user discovered that when selecting scheduled clothing values that the reported value for the Fanger model was always zero. There were various missing assignments that caused this to be the case for several thermal comfort models, always when the clothing value was schedule driven only. This did not affect the results because internally the calculation variables were updated properly. It only affected the report value. This issue has been corrected and now the clothing value is reported properly. A unit test created for this fix verifies this. The variable in question was actually not even listed in the Input Output Reference even though the code clearly made this a valid report variable. The documentation was updated to include this variable as an option. In addition, while working on this defect, it was discovered that two input fields for the People object were not documented in the Input Output Reference. This was also corrected. Finally, an IDF in the test suite was modified to request the report variable that was corrected since no IDF files in the test suite requested it.
Pull Request Author
Reviewer