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

Make unsatzone_flow_layer less convoluted #443

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Huite
Copy link
Contributor

@Huite Huite commented Jul 26, 2024

I was going through this function, and it took me very long to get it.

    sum_ast = 0.0
    # first transfer soil water > maximum soil water capacity layer (iteration is not required because of steady theta (usd))
    st = kv_z * min(pow(usd / l_sat, c), 1.0)
    st_sat = max(0.0, usd - l_sat)
    usd -= min(st, st_sat)
    sum_ast = sum_ast + min(st, st_sat)

sum_ast is initialized as zero, so it's equal to min(st, st_sat).

min(st, st_sat) has to be computed only once; might as well be bound directly to sum_ast.

Next is this bit:

    ast = max(min(st - min(st, st_sat), usd), 0.0)
    # number of iterations (to reduce "overshooting") based on fixed maximum change in soil water per iteration step (0.2 mm / model timestep)
    its = Int(cld(ast, 0.2))

ast is not the best name for max(min(st - min(st, st_sat), usd), 0.0), since it's only used to determine the number of iterations an not actually transferred. Hence my suggestion for remainder.

Additionally, max(min(st - min(st, st_sat), usd), 0.0) is unnecessarily complicated. min(st - sum_ast, usd) suffices, and is clearer.

In case you don't see it the equivalence immediately given the context (it took me long enough...):

  • usd is always positive (otherwise function would have returned early).
  • st - sum_ast is always positive, as sum_ast <= st due to min(st, st_sat).
  • Hence the outer max(..., 0.0) can be removed.
  • We still need to check versus usd, as st hasn't been compared yet.

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

Successfully merging this pull request may close these issues.

1 participant