Irrigation clean-up: syntax error, derived nml packaging, protect against div by 0

[davegill]

TYPE: bug fix

KEYWORDS: irrigation, drip, statement function

SOURCE: internal

DESCRIPTION OF CHANGES:
Problem 1:
In module_irrigation.F, in subroutine sprinkler_irrigation is this line:

constants_irrigation=irr_freq*irr_daily_amount/(irr_num_hours*3600*rho(a,kms,b)*dz8w(a,kms,b)*100)

It is clear from the rest of the routine, that this is to be treated as a statement function.

1. The line is after all of the declarations and before the first executable statement.
2. The values of a, b, and kms (among others) are not yet defined.
3. The following line tries to use it as a statement function.

qr_curr(a,kms,b)=qr_curr(a,kms,b)+irrigation(a,b)*constants_irrigation*dt

However, the GNU compiler is segfaulting on the definition of the statement function,
because the variables "a" and "b" (again, among others) are not yet assigned and are likely
garbage values. The GNU compiler is segfaulting because it does not recognize this line
as a proper statement function.

Statement functions were declared obsolescent in f95, but we have them in other routines.

Solution 1:
Abandon the statement function completely. The code is actually written to not need a statement
function.

Problem 2:
The user defined integer irr_num_hours can be zero, resulting in a divide by zero.

Solution 2:
In check_a_mundo, verify that the number of hours to irrigate is >= 0. This handles all three irrigation
options in a single location.

Problem 3:
The irrigation schemes are designed to mostly be activated on the inner-most domain. As with many
physics schemes, ON vs OFF causes troubles for nested domains when packaging of variables is
involved.

Solution 3:
A derived namelist option is utilized. If any domain has an irrigation option activated, then all domains
have the packaged fields allocated.

LIST OF MODIFIED FILES:
modified: module_irrigation.F
modified: Registry/Registry.EM_COMMON
modified: share/module_check_a_mundo.F

TESTS CONDUCTED:

1. Jenkins is all PASS.
2. With no mod, with the sprinkler irrigation option activated:

 &physics
 sf_surf_irr_scheme                  = 3,     3,     3,

We get the WRF output as:

WRF NUMBER OF TILES =   1

Program received signal SIGSEGV: Segmentation fault - invalid memory reference.

Backtrace for this error:
	at /wrf/WRF/phys/module_irrigation.f90:140

3. With the mods, the code runs to completion.
4. With Registry mods for derived nml for irrigation allocation, following is also OK for 2 domains:

 &physics
 sf_surf_irr_scheme                  = 0,     3,     3,

[davegill]

@pedro-jm @weiwangncar @dudhia
Folks,
Someone familiar with the code needs to run these mods to see if they give the correct results. There is no telling what this code used to do. GNU gives segfaults, other compilers, who knows.

This needs to be tested QUICKLY.

[weiwangncar]

@arjanna Can you take a look at this PR? Thanks. If possible, can you check if this affects the results you already have. Thanks.

[davegill]

@weiwangncar @arjanna
Thanks Wei

[davegill]

Jenkins is OK

    Test Type              | Expected  | Received |  Failed
    = = = = = = = = = = = = = = = = = = = = = = = =  = = = =
    Number of Tests        : 16           10
    Number of Builds       : 41           23
    Number of Simulations  : 155           65        0
    Number of Comparisons  : 99           39        0

    Failed Simulations are: 
    None
    Which comparisons are not bit-for-bit: 
    None

[arjanna]

@arjanna Can you take a look at this PR? Thanks. If possible, can you check if this affects the results you already have. Thanks.

It is correct to move the constants_irrigation within the loop.
I would suggest adding the IF (config_flags%irr_num_hours .EQ. 0) as an error in the check_a_mundo part for the irrigation.
This because it would not make sense to run the model with such value for this config_flag (and it will save adding this same check in the other two parameterizations).

If you share the branch with me, I can do the changes. Thank you for noticing!

[davegill]

@arjanna

If you share the branch with me, I can do the changes. Thank you for noticing!

You should receive an invite. Feel free to pull the davegill:irr=3 down, and then just push right back to it with your commits.

Good idea about check_a_mundo, doing the test once.

[arjanna]

@arjanna

If you share the branch with me, I can do the changes. Thank you for noticing!

You should receive an invite. Feel free to pull the davegill:irr=3 down, and then just push right back to it with your commits.

Good idea about check_a_mundo, doing the test once.

Thank you! I will push as soon as I finish the changes

[davegill]

@arjanna @weiwangncar @dudhia
I also like that the constants_irrigation line is only in there one time. It is a better idea than what I originally had.

If we have

 &physics
 sf_surf_irr_scheme                  = 0,     0,     3,

as opposed to

 &physics
 sf_surf_irr_scheme                  = 3,     3,     3,

then we tend to see troubles with nesting, feedback, restarts, etc. We need to have the same variables available to all domains.

If it is a problem to have irrigation activated for all domains, we have a couple of options:

1. Fix the logic so that there is not a cumulative impact on the inner domains
2. Remove the additional fields from packaging (there are currently only two 2d arrays: irrigation and irr_rand_field)

We could choose option 2 now for the release. Then you can fix the logic so that in a couple of months when we release v4.2.1, we could put the packaging back in place.

[arjanna]

@arjanna @weiwangncar @dudhia
I also like that the constants_irrigation line is only in there one time. It is a better idea than what I originally had.
Thank you!
If we have

 &physics
 sf_surf_irr_scheme                  = 0,     0,     3,

as opposed to

 &physics
 sf_surf_irr_scheme                  = 3,     3,     3,

then we tend to see troubles with nesting, feedback, restarts, etc. We need to have the same variables available to all domains.

If it is a problem to have irrigation activated for all domains, we have a couple of options:

1. Fix the logic so that there is not a cumulative impact on the inner domains

This I thought was hard to do as the IRRIGATION variable will have different values and might need some special conservative regridding so that the area is conserved. But I am not sure even that would help. For example, irr=3 modifies the a variable in the microhpysics, so the effect is highly non linear.

2. Remove the additional fields from packaging (there are currently only two 2d arrays: irrigation and irr_rand_field)

We could choose option 2 now for the release. Then you can fix the logic so that in a couple of months when we release v4.2.1, we could put the packaging back in place.

With the code as it is I used already the restart option, and I never had problems (I checked if there were differences between a restart and one without, and I could see none). If we remove the fields from the restarts I am not sure that the scheme will work properly after the restart.

p.s. unfortunately, the commit I did had a problem. I am fixing it now. Sorry!

[davegill]

@arjanna @weiwangncar @dudhia
Folks,
The Registry has both irrigation and irr_rand_field both with "du" options.
The Registry has both of these fields packaged with sf_surf_irr_scheme nonzero.

If we use

&physics
 sf_surf_irr_scheme                  = 0,     0,     3,

there are definitely compilers that have troubles with nested domains for feedback, as there will not be a variable in the coarse grid to hold the information from the fine grid. We hit something similar to this trouble with cumulus schemes years ago, and more recently we had to be careful with the variables packaged for PBL schemes.

[weiwangncar]

@davegill I suppose this is where we can use a derived namelist to capture if any of the domains is using irrigation, and use it to package variables.

[davegill]

@weiwangncar @dudhia @arjanna

I suppose this is where we can use a derived namelist to capture if any of the domains is using irrigation, and use it to package variables.

Wei,
Yes, that is an excellent idea. I'll put together a PR.

[davegill]

@weiwangncar @arjanna @dudhia
Folks,
I think that we have everything in order for a real test.

1. Arianna cleaned up the statement function calls (they are removed)
2. The divide by zero is trapped for all three irrigation schemes in check_a_mundo
3. The allocation of the irrigation arrays is via a derived type, so the nested namelist is able to say

&physics
 sf_surf_irr_scheme                  = 0,     0,     3,

(of course, using either option 1 or option 2 works as well for nesting)

[davegill]

@weiwangncar @dudhia @arjanna
Folks,
We need some decision on this. Irrigation testing should not proceed without these mods.

[weiwangncar]

@davegill @arjanna As long as Arianna is ok with the change, it can be approved. What about adding a derived namelist to check for packaging?

[davegill]

@weiwangncar

What about adding a derived namelist to check for packaging?

Wei,
Yes, that is in place. This allows irrigation on only a single domain of a nested run.

[weiwangncar]

@davegill I'm ok with this PR. Is there something you need Arianna to look over?

[davegill]

@arjanna @weiwangncar @dudhia

Is there something you need Arianna to look over?

Arianna,
Please clone this fork/branch and verify that you get correct results.

[davegill]

@arjanna @weiwangncar @dudhia
Arianna,
We are ready for you to review this code. We are trying to get all of the bug fixes into the release branch for thorough testing.

[arjanna]

@arjanna @weiwangncar @dudhia
Arianna,
We are ready for you to review this code. We are trying to get all of the bug fixes into the release branch for thorough testing.

I fixed one last thing, now it gives the same results as my previous tests. Sorry for the delay!

[davegill]

@arjanna @dudhia @weiwangncar

I fixed one last thing, now it gives the same results as my previous tests.

Arianna,
Thank you for tracking this down this last problem.

Wei and Jimy,
Would you please review.

[weiwangncar]

@davegill I'm ok with this PR.