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
H2 effects on CH4 lifetime #758
base: dev-h2
Are you sure you want to change the base?
H2 effects on CH4 lifetime #758
Changes from 4 commits
2203297
a46e14e
ec1511f
42479b7
8ed3f3e
95b3196
7058e56
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.
?
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.
Do you mind correcting the spelling of "coefficient" here and in previous lines?
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.
follow up questions: double vs unitval? Do we care? When should we use one over the other?
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 all of the coefficients are read in via ini file/R.... I would assume that we would want to be consistent with out treatment of them but also don't think that any of these coefficients have units that are easy to interpret
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.
If it's used strictly within the component, and pretty isolated, then sometimes unitval is overkill -- feel free to use a double imho. If it's coming from user (INI or R) then having explicit units attached can be important. Does that help?
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.
Same spelling note; also, is there a reference/source for this value? If so please note it.
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.
Was there some derivation needed for this, or was this coefficient in the right format from the original source? (And how similar were the coefficients for other emissions to the ones we are using 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.
Okay there was a derivation that was needed for this parameter, which will be described in detail in the manuscript associated with this work although that is a TBD, so I have included a TODO-H2 tag which is linked to the H2 project as a reminder to address this issue before merging into main or anything.
@ssmithClimate Could you elaborate on this a bit more? (And how similar were the coefficients for other emissions to the ones we are using now?) I am not sure if I understand what you are asking here. It is also unclear where the original coefficients came from @ssmithClimate do you know the source of those?
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.
D_COEFFICIENT_H2
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.
@bpbond do we have strong feelings on the use of e as a variable name?
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.
switch to f, we would like to be consistent with the notation above
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.
Single letter variables seem overly cryptic to me. Perhaps something like this:
toh = toh_ch4 + toh_nox ... + toch_h2
(or capitalize if that is your style).
Also, suggest putting a comment as such above:
// toh is a sum of coefficients in an exponential representation of OH lifetime
double toh = 0.0;
[Just so that readers are clear without reading the next page of code what this is.]
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.
+1 to the point about single-letter variables.