Conversation
bpbond
left a comment
There was a problem hiding this comment.
Hi @kdorheim apologies for my slow response on this.
Interestingly, this (varying physical constants in the codebase) is something that E3SM is working on right now, too!
Some comments for you to consider; happy to discuss more. Thanks for tackling this.
| trackingDate=9999 ; year to start tracking (only carbon currently) | ||
| do_spinup=1 ; if 1, spin up model before running (default=1) | ||
| max_spinup=2000 ; maximum steps allowed for spinup (default=2000) | ||
| atmosphere_mol=1.727e20 ; number of moles in the atmosphere, mols |
There was a problem hiding this comment.
Hmm, why make this a parameter? Is it uncertain, or something we expect users to change?
| // Wigley et al, 2002 (https://doi.org/10.1175/1520-0442(2002)015%3C2690:RFDTRG%3E2.0.CO;2) | ||
|
|
||
| // Unit conversion realted terms | ||
| #define PG_C_TO_TG_CH4 (1000.0 * 16.04 / 12.01) // should this be #define or would it make sense to do something like constexpr? |
There was a problem hiding this comment.
I think #define is fine although tagging @pralitp for his $0.02
|
|
||
| // Determine the conversion factor used to convert from Tg CH4 --> ppbv | ||
| double atm_mols = core->getAtmMols(); | ||
| const double Tg_to_mol = 1 * 1e12 * (1/16.04); // 1 Tg CH4 --> moles of CH4 |
There was a problem hiding this comment.
Whichever you choose, though, be consistent. I would use a mix of #define and const without a good reason.
There was a problem hiding this comment.
#define with all-caps names seems clearer to me, and makes it super easy to see constants in equations.
Right now when we are converting emissions --> ppX for various concentration types, they are inconsistent with one another. If we use a consistent value for the number of moles of CO2 (simpleNbox), CH4, N2O, and halocarbons in the atmosphere, we can be consistent across the hector components.
Right now I have just implemented this change to the CH4 component because I have some questions for @bpbond