Noah-MP Miguez-Macho and Fan

[TimLahmers]

Description

This PR includes support for the Miguez-Macho and Fan Groundwater scheme (Miguez-Macho et al. 2007). This scheme allows 2-way sub-surface routing in the Noah-MP v.4.0.1 LSM in serial and parallel processor configurations. A halo size of at least 3 grid points is recommended for stable results in parallel. The Miguez-Macho and Fan scheme is not recommended for grid resolutions higher than 4-km due to assumptions for the channel exfiltration (QRF) calculation becoming unrealistic at high resolutions.

Resolves #1037

Requires PR #1407 to support ISRIC soils

Testcase

LDT: /discover/nobackup/tlahmers/SHARE/MMF_Testcases/LDT_run
LIS: /discover/nobackup/tlahmers/SHARE/MMF_Testcases/LIS_run

[sudarsan3010]

So, the MMF scheme in Noah-MP is now ready to use? Is it already merged with the master?
I am waiting from a very long time to use this sub-surface scheme.

[dmocko]

I note that "dtbl" is not used anywhere else in the code (including within the "phys" sub-directory).

[TimLahmers]

@dmocko This might be an artifact from earlier code changes, before I took over the MMF coding. If this doesn't appear anywhere else in the code or affect other LIS components, I am open to removing it.

[dmocko]

This line should be changed, so 20 is not hard-coded. Please change to number of vegetation classes.

[TimLahmers]

@dmocko Consistent with the change proposed below in the NoahMP401_setup.F90 module, could we change the hard-coded value to LIS_rc%nsurfacetypes? Similarly, we could use LIS_rc%nsoiltypes for the number of soils.

[dmocko]

Yes, I believe that this will work.

[dmocko]

I don't think "17" should be hard-coded here. Instead, please set to the water type within LIS, so it covers all vegetation classifications that might be used.

[TimLahmers]

@dmocko Something similar also came up for another version of MMF (the GWS project that Wanshu and I started). Is something like this what you are looking for? a3a6114

[dmocko]

Sort of. I was thinking more about "LIS_rc%waterclass", which is read in from WATERCLASS from the lis_input file.

[dmocko]

Hi @TimLahmers and @jvgeiger

I'm finally getting around to reviewing this PR for MMF. My apologies for taking so long to finish it.
May all my jobs this week sit on the queue for an extra 12 hours as penance!

I made a few specific comments about code above, but more generally most of these changes look good to me.

I do wonder, though - What is the difference between the variables “LIS_rc%nsurfacetypes” and “LIS_rc%nvegtypes”?
In NoahMP401_setup.F90, Lines 370 and 386 go from 1 to LIS_rc%nsurfacetypes. Should this be LIS_rc%nvegtypes?

As a test, I ran in the NLDAS-2 domain using both the latest main/master branch and using the code in this PR.
For this test, I am using the TOPWATER groundwater scheme - in other words, I am NOT using the MMF scheme.
All of the variables - EXCEPT for RelSMC - produced identical output (as expected) between the two runs.
Note that these test runs both use the UMD vegetation classification, which show that results could be affected
by the hard-coding described above.

@TimLahmers - Can you please take a look at this output field?

This is RelSMC from the main branch:

This is RelSMC from the code for this PR:

This is the difference between the two fields:

There are two issues:

1. In the run with the MMF code, there's a strip of undefined values from DC south through VA and NC.
2. In the run with the MMF code, there are values for the urban type, while from the main code, these values are undefined. They should be undefined according to the code. The difference field shows that outside of these two areas (DC to VA to NC strip - and - urban points), the values are identical between these two runs.

[dmocko]

Hi @TimLahmers - this is another hard-coded value that should be generalized.

Can you please change the "13" to LIS_rc%urbanclass ?

[dmocko]

@TimLahmers - One other question about hard-coding the ice values in lines 1029-1032.

Hard-coding these values will run the MMF groundwater for all glacier points in LIS.

However, the code itself seems to check if it is a glacier/ice point, and possibly not run these points:

    WHERE(XLAND-1.5.LT.0..AND.XICE.LT. XICE_THRESHOLD.AND.IVGTYP.NE.ISICE)
         LANDMASK=1
    ELSEWHERE
         LANDMASK=-1
    ENDWHERE

Can you please comment on why you are making MMF run for all glacier/ice points?

[TimLahmers]

@dmocko I had a chance to check on this today. It looks like the lines related to glacier points are copied from HRLDAS. Is there a reason we shouldn't run MMF for glacier/ice points? If there is a scientific justification for not running this way, we could raise this at the next Noah-MP telecon.

[dmocko]

Once @TimLahmers changes the urban type from my most recent comment, can you (Tim) please re-run your testcase?

I now get identical results from the RELSMC output, so that issue is resolved. Thanks!

Otherwise, I am satisfied with the code, and after the urban type fix and Tim re-running the testcase, we are ready
for @jvgeiger to finish his review. Jim will likely remove many commented-out lines (including the comments about hard-coded values for vegetation and soil types, as well as debugging print statements). Then he can compare to Tim's updated testcase output with the latest code.

[TimLahmers]

@dmocko I just re-ran the test case, and it is in the same directory as before: /discover/nobackup/projects/ualbmap21/tlahmers/MMF_DemoCases_062023/LIS_MMF_CONUSvalid_RELSMCbug/OUTPUT.SIMGM.CONUS_RelSMC_Valid

The code change has been committed.

[dmocko]

Many thanks for @TimLahmers for re-running the testcase a couple times over the past couple weeks to address minor
issues with the testcase (but not with the code).

The latest testcases are here:
LDT: /discover/nobackup/tlahmers/SHARE/MMF_Testcases/LDT_run
LIS: /discover/nobackup/tlahmers/SHARE/MMF_Testcases/LIS_run

Note that this PR is only for LIS, so you only need to run/use the LIS testcase.

However, just for completeness, I successfully replicated both the LIS and the LDT testcase with identical results.

Further, I re-tested other Noah-MP-4.0.1 output variables using the new code but with SIMGM groundwater, and found
that no other variables are affected (again, identical results). So I confirm that this PR will not affect non-MMF output.

Finally, I did another complete code review, and believe that this PR is ready to merge - once @jvgeiger cleans up the
code comments to his satisfaction.

@jvgeiger - Please finish up and merge this PR once you have the time. Thanks!

[dmocko]

@jvgeiger - as long as you are cleaning up code, can you also remove the "NS" from this line before the PR is merged?

https://github.com/NASA-LIS/LISF/blob/master/lis/configs/lis.config.adoc?plain=1#L8306

Also @TimLahmers - what is the recommended number of "Halo size" that should be used? We should also specify that
halos must be turned on when using MMF option 5 (probably put it as a new sentence within lines 8293 to 8297 above).

[TimLahmers]

@dmocko @jvgeiger All of the tests we performed with MMF used a halo size of 3. Analysis from the earlier development stages (right after we fixed the bugs with the gather/scatter routines with halos) showed minimal round-off errors with that configuration compared to serial runs.