Conversation
wwieder
left a comment
There was a problem hiding this comment.
Sorry I didn't get very far on this. We can talk more in a bit
| c = filter_methc (fc) | ||
| source(c,j,1) = -surface_layer_ch4_oxid(c) ! [mol/m3-total/s] | ||
| source(c,j,2) = -surface_layer_o2_oxid(c) ! O2 [mol/m3/s] | ||
| ! not adding error checks ... |
wwieder
left a comment
There was a problem hiding this comment.
one more comment based on our paired review.
resolving this one will make it easier to see diffs in the PR.
src/biogeochem/ch4Mod.F90
Outdated
| ! Add surface layer ch4 and convert mol CH4 to g C | ||
| do fc = 1, num_nolakec | ||
| c = filter_nolakec(fc) | ||
| !!$ l = col%landunit(c) |
There was a problem hiding this comment.
Thanks for removing the commented out code!
ekluzek
left a comment
There was a problem hiding this comment.
I just took a quick look to see what we have here. I ask a couple questions. But, I think it's cool that you were able to improve the science, by also reducing code duplication.
src/biogeochem/ch4Mod.F90
Outdated
|
|
||
| end subroutine ch4_ebul | ||
|
|
||
| !# |
There was a problem hiding this comment.
I expected this would be moving this section of code somewhere else. But, it looks like it's just removing a ton of things.
I take it that ch4_tran and ch4_tran_surface_layers must have been almost identical, so you were able to just remove the differences in the one? That's a great improvement if so.
This does make it hard to assess what those differences were though. Maybe we could do the difference by hand and add it to the PR. I might do that, if I get to it...
There was a problem hiding this comment.
Actually, there were many differences. I had kept the original routine during development, but we decided it was not needed, and thus removed it.
|
the buildnamelist change worked (i.e. no default is required for 'h2osfc' method), but I noticed that it means control%stream_fldFileName_ch4finundated (in src/cpl/share_esmf/ch4FInundatedStreamType.F90) will never be set because the namelist is not read if the 'h2osfc' method is active. That means that later, in ch4Mod, this will always be true: ch4_inst%ch4findstream%useStreams(), because UseStreams checks whether stream_fldFileName_ch4finundated) == ''. The former will be false because it is not set, nor initialized. update: adding this 2nd conditional to useStreams would ensure correct behavior for 'h2osfc': |
slevis-lmwg
left a comment
There was a problem hiding this comment.
Thank you @swensosc
seems cleaner this way, and I was able (in this quick look) to notice a few minor things that I suggested fixing this time around.
Also, would you be willing to add/commit/push the conditional that you proposed in #3893 (comment) (or in our separate emails), to provide me with a concrete starting point? If the question is whether to move the check from run-time to build, then I will feel more confident making the change after seeing your initial implementation.
src/biogeochem/ch4Mod.F90
Outdated
| ! Non-tunable constants | ||
| real(r8) :: rgasm ! J/mol.K; rgas / 1000; will be set below | ||
| real(r8), parameter :: rgasLatm = 0.0821_r8 ! L.atm/mol.K | ||
| real(r8), parameter :: psi_fc = 3.4e3 ! matric potential at field capcity (mm) |
There was a problem hiding this comment.
| real(r8), parameter :: psi_fc = 3.4e3 ! matric potential at field capcity (mm) | |
| real(r8), parameter :: psi_fc = 3.4e3_r8 ! matric potential at field capcity (mm) |
src/biogeochem/ch4Mod.F90
Outdated
| if ( .not. readv ) call endrun(msg=trim(errCode)//trim(tString)//errMsg(sourcefile, __LINE__)) | ||
| params_inst%f_ch4=tempr | ||
|
|
||
There was a problem hiding this comment.
I just noticed these. Best to take out the blank spaces here and elsewhere to keep the code neat :-)
src/biogeochem/ch4Mod.F90
Outdated
|
|
||
| integer :: nstep ! time step number | ||
| character(len=32) :: subname='ch4_tran' ! subroutine name | ||
| character(len=32) :: subname='ch4_tran_surface_layers' ! subroutine name |
There was a problem hiding this comment.
| character(len=32) :: subname='ch4_tran_surface_layers' ! subroutine name | |
| character(len=32) :: subname='ch4_tran' ! subroutine name |
src/biogeochem/ch4Mod.F90
Outdated
| end if | ||
| diffus(c,j) = (d_con_g(s,1) + d_con_g(s,2)*t_soisno_c) * 1.e-4_r8 * & | ||
| (om_frac * f_a**(10._r8/3._r8) / watsat(c,1)**2._r8 + & | ||
| (1._r8-om_frac) * eps**2._r8 * f_a**(3._r8 / bsw(c,1)) ) & |
There was a problem hiding this comment.
I'm told multiplication is much more efficient than raising exponents, so:
| (1._r8-om_frac) * eps**2._r8 * f_a**(3._r8 / bsw(c,1)) ) & | |
| (1._r8-om_frac) * eps*eps * f_a**(3._r8 / bsw(c,1)) ) & |
src/biogeochem/ch4Mod.F90
Outdated
| end if | ||
| diffus (c,j) = (d_con_g(s,1) + d_con_g(s,2)*t_soisno_c) * 1.e-4_r8 * & | ||
| (om_frac * f_a**(10._r8/3._r8) / watsat(c,j)**2._r8 + & | ||
| (1._r8-om_frac) * eps**2._r8 * f_a**(3._r8 / bsw(c,j)) ) & |
There was a problem hiding this comment.
| (1._r8-om_frac) * eps**2._r8 * f_a**(3._r8 / bsw(c,j)) ) & | |
| (1._r8-om_frac) * eps*eps * f_a**(3._r8 / bsw(c,j)) ) & |
Description of changes
biogeophysics: 1) a couple of modifications to how surface water is treated. One deals with reconciling snow / h2osfc fractional areas when they coexist. Previously fh2osfc was reduced, now reduce each proportionately to ensure a maximum sum of 1. 2) change drainage from surface water to be based on matric potential gradient instead of saturated hydraulic conductivity.
biogeochemistry: add a surface layer to the methane transport calculations. This ensures that oxidation occurs in the unsaturated column calculations when the water table is at the surface.
Specific notes
Contributors other than yourself, if any:
CTSM Issues Fixed (include github issue #):
Are answers expected to change (and if so in what way)?
Any User Interface Changes (namelist or namelist defaults changes)?
Does this create a need to change or add documentation? Did you do so?
Testing performed, if any:
(List what testing you did to show your changes worked as expected)
(This can be manual testing or running of the different test suites)
(Documentation on system testing is here: https://github.com/ESCOMP/ctsm/wiki/System-Testing-Guide)
(aux_clm on derecho for intel/gnu and izumi for intel/gnu/nag/nvhpc is the standard for tags on master)
NOTE: Be sure to check your coding style against the standard
(https://github.com/ESCOMP/ctsm/wiki/CTSM-coding-guidelines) and review
the list of common problems to watch out for
(https://github.com/ESCOMP/CTSM/wiki/List-of-common-problems).