Wetland update

[swensosc]

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).

[wwieder]

Sorry I didn't get very far on this. We can talk more in a bit

[wwieder]

few more comments

[slevis-lmwg]

@swensosc will double-check this comment.

[slevis-lmwg]

@swensosc I'm making sure whether you already checked this before I resolve the post.

[wwieder]

one more comment based on our paired review.

resolving this one will make it easier to see diffs in the PR.

[ekluzek]

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.

[swensosc]

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':
if ( trim(control%stream_fldFileName_ch4finundated) == '' .or. finundation_mtd == finundation_mtd_h2osfc)then

[slevis-lmwg]

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.

[slevis-lmwg]

let me know when the branch is ready and I'll do another CH4 calibration run.

@wwieder we need your approval of the PR, as well :-)

[slevis-lmwg]

• For sanity, I will submit aux_clm on izumi, as well, before merging and making the tag.

[billsacks]

I was just thinking about the issue that @swensosc and I fixed in 981d262 - which I believe meant that (before the fix) the mere act of writing a restart file would change answers. I realized that most of our restart tests wouldn't pick up an issue like this. That made me remember this discussion from a few months ago:

ESMCI/cime#4859

where we discussed the value of an alternative ERS test that more closely mimics the production workflow and would catch problems where writing a restart file causes a change in answers. (My understanding is that that's the main additional value of that alternative "ERS2" test.)

Seeing this issue - which seems like a very easy kind of issue to accidentally introduce in CTSM - made me wonder if we should revisit the discussion in #4859 (we had decided to remove the ERS2 test because its limited additional value didn't seem to warrant the extra complexity). However, looking back through the above test results, it looks to me like this problem was picked up in an ERI test. @slevis-lmwg am I understanding that right? I can see why an ERI test would detect this: one of the comparisons done in an ERI test is the comparison of the hybrid run with a branch run done off of that hybrid run, and one (somewhat incidental) difference between the two is that the branch run writes an extra set of restart files mid-way through. So based on that, I feel more comfortable with the current state of testing, as long as we have ERI tests covering most / all major configurations, at least in terms of configurations of restart options.

I'd be interested to hear if anyone has any thoughts on this - and will probably add it as an agenda item for an upcoming cseg meeting for further discussion.

[billsacks]

Seeing this issue - which seems like a very easy kind of issue to accidentally introduce in CTSM - made me wonder if we should revisit the discussion in #4859 (we had decided to remove the ERS2 test because its limited additional value didn't seem to warrant the extra complexity). However, looking back through the above test results, it looks to me like this problem was picked up in an ERI test. @slevis-lmwg am I understanding that right? I can see why an ERI test would detect this: one of the comparisons done in an ERI test is the comparison of the hybrid run with a branch run done off of that hybrid run, and one (somewhat incidental) difference between the two is that the branch run writes an extra set of restart files mid-way through. So based on that, I feel more comfortable with the current state of testing, as long as we have ERI tests covering most / all major configurations, at least in terms of configurations of restart options.

Answering this for myself: Yes, an ERI test detects this issue: ERI_Ld9.f10_f10_mg37.I2000Clm50BgcCru.derecho_intel.clm-default passes now, but fails if I revert 981d262.

[swensosc]

sam, please wait before merging. bill and I are looking into a possible issue in an edge case related to dynamic glacier changes.

[billsacks]

This long line should be split - I'm surprised NAG didn't complain about it. Or are longer lines (> 132 characters) now permitted? (I think Fortran 2023 increases the max line length, but I don't think we should rely on that.)

While doing that, can you check for other long lines that should similarly be split? It looks like there are some long lines here but I haven't checked the line length.

[billsacks]

To keep others in the loop:

As @swensosc noted, he and I have been working through a conservation error that still appears sometimes in some dynamic landunit cases - thanks to Sean for the careful additional testing that revealed this! Pending some additional testing by Sean, I think I have tracked it down to something that I neglected to put in the interface and/or documentation of dynColumnStateUpdaterMod: That code doesn't explicitly handle the layer thickness associated with each variable, but technically it should. Up until this point, I don't think this has caused problems because layer thicknesses are constant in time and the same between different columns (so we'd end up multiplying and dividing by the same number). But the novel thing in this new code is that layer thicknesses associated with the new methane variables change in time and space. This layer thickness therefore needs to be passed as an additional multiplying/dividing factor into the dynamic landunit column state updater.

Sean is running some tests to see if this solves the issue.

[billsacks]

These new changes look great - thank you for working through this with me, @swensosc ! I do have a couple of suggestions for some additional comments and possibly some variable renames for clarity.

[billsacks]

@slevis-lmwg - to keep you in the loop here: After @swensosc addresses my latest comments, I'd like to make an additional change that renames a couple of variables in dynColumnStateUpdaterMod.F90. This is a straightforward change but will change a lot of lines and overlap with Sean's changes, so I want to wait until he's done. At that point, I'm hopeful that this will be ready for re-testing.

I'm open to feedback from anyone on these upcoming renames: As can be seen in Sean's recent changes, there are now things like this in calls to update_column_state:

fractional_area_old = surface_layer_volume_sat_col(begc:endc), &

where the passed-in variable really isn't just fractional_area anymore, but instead is a more complete multiplier that includes thickness. I'm thinking of renaming it to something generic like extra_multiplier, but welcome any input. Then I'd add some comments explaining what that does and giving some examples.

[swensosc]

bill, I see what you are saying, but I feel that 'extra_multiplier' would be too vague. To move mass between columns, concentration is scaled by a volume. In the original code, both the gridcell area and the thickness were implicit, since their values were the same for both columns. Now, just the gridcell area is implicit, but I think it's important to keep the idea clear that a volume is what scales the concentration. I thought of changing the variable names to fractional_volume, but that maybe is too long. If you want extra_multiplier though, that is fine with me.