Function documentation - Githubissues

NEONScience / NEON-dissolved-gas

Repository for a code package that calculates dissolved gas concentrations from air and equilibrated water data, from NEON's Dissolved gases in surface water (DP1.20097.001) data product.

GNU Affero General Public License v3.0

4 stars 8 forks source link

Function documentation #3

Open jbeaulie opened 7 years ago

jbeaulie commented 7 years ago

def.calc.sdg is a beautiful function. Very concise and elegant. My only comment is that the equation is so elegant that it is difficult to unravel the underlying logic. Please consider providing a detailed description of the logic behind the function.

As an example/alternative, see def.calc.sdg.jb. Here I took the def.calc.sdg function and broke it down into a series of smaller calculations, along with a lot of text documenting the overall approach. The results from def.calc.sdg.jb are nearly identical to that of def.calc.sdg (see demo.R for details).

Please consider presenting def.calc.sdg in a format that is a bit more accessible to the reader. I suppose one alternative is to provide a detailed derivation of the function in the User's Manual, while leaving the function as is.

kcawley commented 7 years ago

@jbeaulie I just synced a version of the function with a commented out step-by-step calculation drawing from the .jb function, but using the flow of the documentation that comes with the data download. Also, updated the read me with the equations.

jbeaulie commented 7 years ago

I really like the combination of the step-by-step in the function, combined with more details in the read me. I have a few more comments here.

cPressConv: I suggest that we can improve the name and description of this constant. Please see testingAndDev\jbCals\cPressConv.docx for details. In the document I state that we divide ppmv by 1000000 (equivalent to multiply by cPressConv) to convert ppmv to parts, but I was fairly certain 'parts' isn't actually a unit. I believe the correct terminology is that we convert ppmv to mole fraction.
I think we can expand the read me to include more details. Please see testingAndDev\jbCals\readMeComments.docx for proposed text/equations.
I suggest that we reference the step-by-step equations in the function script to the relevant portion of the read me document. Please see testingAndDev\jbCals\def.calc.sdg.jb.V2.R for additional annotation.

jbeaulie commented 7 years ago

@kcawley

Please see new 'pull request' for minor changes to README.Rmd. Please see below for additional comments on README.Rmd.

eq_1.png: "H2O" in denominator of first quotient should be subscript.
'eq' was occasionally subscripted in the text, but not in the equations. I removed subscripting from text. Alternatively, you could add subscript to equations. Doesn't matter, just want to be consistent.
To be consistent with the comments in def.calc.sdg.R, I repalced the term 'headspace gas' with 'reference gas' when discussing the gas used for the headspace equilibration.