ESCOMP / ESCOMP/CTSM

Change all history fields to use the new landunit_mask argument, and remove initial settings of history fields to spval

Open
#1,347 1 comment 0 reactions 0 assignees View on GitHub
blocked: dependency blocker enhancement
Dominant language
Fortran
Stars
352
Forks
361
Avg merge
2d 21h
Merged PRs (30d)
7

Description

Once we are satisfied with how we want to move ahead with history field averaging, we should change all history fields to be consistent with our plan.

Tentatively, this will involve changing all history fields to use the new landunit_mask argument and removing the initial setting of each history field to spval. This will also involve removing any uses of set_*=spval, instead using the appropriate setting for landunit_mask to serve that purpose.

In the [meeting a couple of weeks ago](https://github.com/ESCOMP/CTSM/wiki/Meeting-Notes-2021-Science#bill-and-danica-lead-a-discussion-on-grid-cell-averaging) a suggestion was raised of just starting with default-on biogeophysical variables. However, thinking about this more, I see a couple of issues with this plan:
- Only addressing default-on variables would make it likely that some default-off variables would be inconsistent with some close companions that are default-on. **So I think we should address all biogeophysical variables with similar care.**
- If we address all variables (BGC as well as biogeophysical), then we can do some other code cleanup and add some robustness, such as doing away with initial settings to spval (and any code that checks for that, e.g., in subgridAveMod) and ensuring that all history variables define a landunit_mask moving forward. **So I would suggest that we make the same changes to BGC variables.** However, we can make this process easier by assuming that the current behavior of BGC variables is correct (in contrast, we discussed giving more critical thought to the biogeophysical variables, which I believe are more likely to be incorrect); in that case, I believe it will be relatively quick to add the new landunit_mask argument to the relevant history fields.

We should test to make sure answers don't change from doing this. Doing this with confidence depends on #29 (blocked by #29).

Once we have done the above, we can – and should:
- make landunit_mask a required argument for hist_addfld
- change 'unknown' to 'all' in this metadata code (because this will now be more reliably correct): https://github.com/billsacks/ctsm/blob/8ec39e826c1e9ea08eb739b763e2fb39ec2ef816/src/main/histFileMod.F90#L5594-L5600

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.