Bugfix DNRA and introduce max_limiter #413
Conversation
jmaerz
added
bug
There was a problem hiding this comment.
@jmaerz - thanks, a few comments from my side, but nothing major.
I support introducing this bugfix also in the release-1.6 branch and make a new tag.
hamocc/mo_param_bgc.F90
Outdated
| @@ -152,6 +153,7 @@ module mo_param_bgc | |||
| real, parameter :: c14_t_half = 5700.*365. ! Half life of 14C [days] | |||
|
|
|||
| ! Extended nitrogen cycle | |||
| real, parameter :: max_limiter = 0.9999 ! maximum in concentrations that can consumed at once | |||
There was a problem hiding this comment.
| real, parameter :: max_limiter = 0.9999 ! maximum in concentrations that can consumed at once | |
| real, parameter :: max_limiter = 0.9999 ! maximum in concentrations that can be consumed at once |
hamocc/mo_ocprod.F90
Outdated
| @@ -137,6 +137,7 @@ subroutine ocprod(kpie,kpje,kpke,kbnd,pdlxp,pdlyp,pddpo,omask,ptho,pi_ph,psao,pp | |||
| real :: wpocd,wcald,wopald,wdustd,dagg | |||
| real :: wcal,wdust,wopal,wpoc | |||
| real :: o2lim ! O2 limitation of ammonification (POC remin) | |||
| real, parameter :: tiny_num = 1e-25 | |||
There was a problem hiding this comment.
Do you need to define this parameter? If you just need a small number, there is the intrinsic function "tiny" for which tiny(0.0) will return the smallest positive real number.
See e.g. https://fortran-lang.org/en/learn/intrinsics/model/#tiny
There was a problem hiding this comment.
Good question, thanks - I exchanged the value by the epsilon function now.
There was a problem hiding this comment.
| real, parameter :: tiny_num = 1e-25 | |
| nh4uptfrac = anh4lim/(nlim+epsilon(1.)) |
Why not replace the tiny_num parameter? Now you have to look in the variable definition list to see that tiny_num = epsilon(1.)
There was a problem hiding this comment.
That didn't come out as I intended. Point is, I'm not sure you need a tiny_num variable at all.
There was a problem hiding this comment.
I'll/should go through the code again exchanging those numbers, which I'll do.
There was a problem hiding this comment.
I now went for your suggestion in mo_ocprod, while I introduced a parameter for the extended nitrogen cycle routines - I feel that this could be done generally throughout the code to define a small number that is generally used, but maybe not for now. Done.
|
Hi @TomasTorsvik and @JorgSchwinger , I now checked the results of a 20a run in comparison to the former run and see very minor differences for the water column, while, of course, the sediment bug becomes fixed. Hence, I would suggest to push this fix. To inform further: I intend to experiment with one more process-implementation for the sediment (small change in how the rates are formulated), which in addition may come in to the sediment N-cycle (but this is beyond the present PR). |
* Bugfix DNRA and introduce max_limiter
* Bugfix DNRA and introduce max_limiter * Equivalent to PR #413, applied to the `release-1.6` branch Co-authored-by: jmaerz <[email protected]>
This PR only affects runs with the extended nitrogen cycle being switched on. It fixes a bug in the sediment DNRA pathway. In addition, I introduce a limiter (
max_limiter) to avoid coming too close to available concentrations in one time step (avoiding turning values negative due to precision issues). Further: minor clean-up.I am currently running a test by applying those changes to a BLOM v1.6.2 setup.
@JorgSchwinger and @TomasTorsvik, if possible and if tests are giving positive results, I would consider to request to also cherry pick this fix to the v.1.6.2 tag - leading to a bugfixed v1.6.3 which I can then use in my setups - I assume that this would be feasible since the N-cycle was anyway optional in v1.6.x