-
Notifications
You must be signed in to change notification settings - Fork 423
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Thermal Comfort Model Documentation Corrections #6317
Thermal Comfort Model Documentation Corrections #6317
Conversation
This commit corrects many problems with the thermal comfort model documentation sections for the Fanger, Pierce, and KSU models as requested by the defect description. Equations were rechecked and corrected when discrepancies were found. Now, everything lines up with what is in the code. Lots of changes for the better.
@Myoldmopar @michael-okeefe @mjwitte Would one of you be willing to review this? Thanks, Rick |
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.
Looks pretty good. I didn't build it to look at the output PDF -- I would recommend doing that if you haven't already to make sure everything is looking OK. Otherwise, just one minor glitch found with some unicode characters that should be a quick fix.
|
||
HeatFlow = (CoreTemp-SkinTemp)*(5.28 + 1.163*SkinBloodFlow) | ||
|
||
Where | ||
where 5.28 is the average body tissue conductance in W/m\(^{2}\)•°C and 1.163 is the thermal capacity of blood in W•h/L•°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.
Looks like some of the unicode characters may have gotten scrambled up. Maybe:
W/m\(^2 \mdot \) °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.
Sorry that should be:
W/m\(^2 \cdot \) °C
I've been working with mass flow rates too much. \cdot
is center-dot. You could also use \bullet
if you wanted to match the original Word closer.
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.
@michael-okeefe Thanks for the review--I'll fix that as soon as possible.
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.
@michael-okeefe By the way, I have been building the PDF locally on my computer to make sure that things are formatted correctly. So, hopefully once it's up in GitHub things should be good to go other than any typos, etc.
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.
Awesome. Sounds 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.
@RKStrand This still needs to be addressed but once that is fixed, this can be merged in.
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 Yes, I will correct this very soon along with equation you mentioned and get this checked back in. Will keep you posted...
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.
@michael-okeefe @Myoldmopar Ok, this issue is fixed now. Changed the degrees C to K...because despite many attempts I couldn't be both of the degree symbols to show up. Weird but using just K works and is 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.
Yeah, LaTeX is definitely a black art. In the past, I've used:
$^{o}$C
or $^{\circ}$C
to get the "degree" symbol to print although there are more eloquent ways. K
is fine with me although I'll defer to Edwin.
Great work!
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 a whole bunch of cleanup, thanks for that.
Only a couple real comments:
- There are several places in your changes where there had previously been units labeled on variables, and I think those just went away. It's probably worth either putting the units back in the text around the variable or putting a little table/list showing the units for different terms. Or just let me know why you think they aren't needed and we can debate.
- I started marking the equations that were put in the docs "from the source code", because they look real funny in the docs here. I thought there would only be one or two. I realized after seeing more that it may be a bigger ask to clean them all up. Can you just comment back on whether it is valuable to leave them in there like they currently are?
I'll mark my review as a "comment only" for now, not approve or reject, so we can just chat about it before deciding whether to merge or not.
|
||
The metabolic rate (H/A\(_{Du}\)), is a measure of the internal heat production rate of an occupant (H) w/hr. in per unit of ``Dubois'' body surface area (A\(_{Du}\)) in units of m\(^{2}\). The DuBois body surface area is given by : | ||
The thermal resistance of the clothing (I\(_{cl}\)) is measured in units of ``clo.'' The 1985 ASHRAE Handbook of Fundamentals (ASHRAE 1985) suggests multiplying the summation of the individual clothing items clo value by a factor of 0.82 to determine the overall clo value for clothing ensembles. |
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.
In general, I'd suggest in the future using dollar signs to drop into and out of inline math mode. Using the escaped parentheses is more bulky, and difficult to read, especially when there are other parentheses around. No need to change all these here, I know they are currently all over the docs, just a suggestion for the future.
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 Thanks for the suggestion--I will definitely keep that in mind in any future documentation work. I admit that I don't know all the ins and outs of what is best--I was just trying to figure out how to get everything to look correct and I borrowed stuff from what was already there. Apparently I used the "bad" examples!
\midrule | ||
\endfirsthead | ||
|
||
\caption[]{General Nomenclature list for Thermal Comfort Models} \tabularnewline | ||
\toprule | ||
Mathematical variable & Description & Units ~ & Range & FORTRAN variable \tabularnewline | ||
Mathematical variable & Description & Units ~ & Range & Source Code Variable \tabularnewline |
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.
👍
|
||
One more consideration is important in dealing with thermal comfort - the effect of asymmetrical heating or cooling. This could occur when there is a draft or when there is a radiant flux incident on a person (which is what is of primary interest to us here). Fanger (1967) noted that the human regulatory system is quite tolerant of asymmetrical radiant flux. A reasonable upper limit on the difference in mean radiant temperature (T\(_{r}\)) from one direction to the opposing direction is 15°C. (ASHRAE 1984). This limit is lower if there is a high air velocity in the zone. | ||
|
||
% table 78 | ||
\begin{longtable}[c]{p{1.2in}p{2.0in}p{0.8in}p{0.8in}p{1.2in}} | ||
\caption{General Nomenclature list for Thermal Comfort Models \label{table:general-nomenclature-list-for-thermal-comfort}} \tabularnewline | ||
\toprule | ||
Mathematical variable & Description & Units ~ & Range & FORTRAN variable \tabularnewline | ||
Mathematical variable & Description & Units ~ & Range & Source Code Variable \tabularnewline |
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.
👍
\(L = {Q_{res}} + {Q_{dry}} + {E_{sk}} + W\) ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ W/m\(^{2}\) | ||
\begin{equation} | ||
M = L | ||
\end{equation} |
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 it worth putting these units in this section somewhere?
|
||
Total evaporative heat loss from the skin, E\(_{sk}\), includes evaporation of water produced by regulatory sweating, E\(_{rsw}\), and evaporation of water vapor that diffuses through the skin surface, E\(_{diff}\). | ||
~~~~~~~~~~~~~~~~~~~~~~~/(CloCond + CloBodyRat*(Hc+Hr)) |
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 supposed to be attached to the equation two lines up? If so, can you clean up the connection? Or just make it a real equation and not a source code representation?
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 Well, here's what I can say about this. When I delete the blank line between these two lines, it merges them back together like there is no paragraph marker between them and it messes up the spacing upon building the Eng Ref on my machine. Same thing with PMVET, PMVSET, and DISC equations elsewhere in this section. So, I left them...or is there another way to do this?
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's fine. I mean, really, the right thing to do, is to put them in actual equation format, instead of just pasted in code equations. Then LaTeX can handle this formatting stuff and we don't have to keep things lined up just right. But I get the original intent of this section and other parts of the code were to show the actual equations from the source, so just leave 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.
@Myoldmopar Yah, I think for consistency and differentiation I was using equation format for actual mathematical equations and then leaving code as text. At least that was my intent. Trying to get the issue that @michael-okeefe noted sorted out. Some of these codes are a bit temperamental...but it is pretty cool that we can get all of the documentation in version control.
@Myoldmopar Thanks for the thorough review! We can definitely discuss things further though I have limited time right now. Basically, I took all the units out of those equations because the units are contained in the tables that have all of the variable names in and because my sense of getting everything to look the same was violated by all of the floating units. I converted all of those equations to actual equations and the units and equation numbers also looked awkward. So, my solution was to get rid of the units since they were already in the tables, but we can discuss that further if you don't agree with that. |
As per review comments, fixed some bad characters in the thermal comfort file. Should be good to go now.
Just happened to notice Linux was nicely failing to build the docs because of a math delimiter problem. I pulled it locally, and it was actually succeeding to build the pdf, but the math was probably wonky because there's 84 LaTeX Error: Bad math environment delimiter instances! I'm going to fix them, hijacking this branch along the way. Once I get them cleaned I'll merge this branch in @RKStrand. Thanks for your changes. |
OK, so it actually was the changes in this branch causing the bad math delimiters. Just a quick heads up here. The To fix the errors, I assumed that when I found Once I cleaned those up, there were no more bad math environment delimiter errors, so I'm calling this done and merging it. |
@Myoldmopar Ah...I didn't realize that both of those things were opening the math environment. I guess I understand why it would have an issue. Thanks for fixing that! I'll try to remember that for any future documentation work. |
Pull request overview
This pull request corrects many problems with the thermal comfort model documentation sections for the Fanger, Pierce, and KSU models as requested by the defect description. Equations were rechecked and corrected when discrepancies were found. Now, everything lines up with what is in the code. Lots of changes for the better. This was requested by GitHub Issue #2996.
Work Checklist
Add to this list or remove from it as applicable. This is a simple templated set of guidelines.
Review Checklist
This will not be exhaustively relevant to every PR.