Skip to content
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

Function documentation #3

Open
jbeaulie opened this issue Sep 14, 2017 · 3 comments
Open

Function documentation #3

jbeaulie opened this issue Sep 14, 2017 · 3 comments

Comments

@jbeaulie
Copy link
Collaborator

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
Copy link
Collaborator

kcawley commented Sep 14, 2017

@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.

@kcawley kcawley closed this as completed Sep 14, 2017
@jbeaulie
Copy link
Collaborator Author

jbeaulie commented Sep 18, 2017

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 jbeaulie reopened this Sep 18, 2017
@jbeaulie
Copy link
Collaborator Author

jbeaulie commented Oct 4, 2017

@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.

jbeaulie added a commit to jbeaulie/NEON-dissolved-gas that referenced this issue Oct 4, 2017
@jbeaulie jbeaulie changed the title def.calc.sdg and def.calc.sdg.jb functions Function documentation Oct 4, 2017
geohouse pushed a commit to geohouse/NEON-dissolved-gas that referenced this issue Aug 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants