Check list of state variables for required ones

[kdraeder]

Description:

Users can specify in model_nml which variables should be in the state vector.
They are currently free to leave out variables that are required for some calculations.
Filter writes no useful message about this problem when it fails, which results in a long debugging process.
This fix adds a function call after model_nml has been read and parsed to check whether required variables are in the state, and exit with a helpful message when one is missing.
The lists of required variables are different for cam-fv and cam-se, and may change in the future.

Fixes issue

fixes #662

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• Documentation update

Documentation changes needed?

• My change requires a change to the documentation.
  • I have updated the documentation accordingly.

Tests

I ran both cam-fv and cam-se using model_nml:state_variables which contained

1. all required variables
2. all except for PS
3. all except for CLDICE (cam-se only; dry mass calculation)

These were not set up using the usual setup_ scripts. They use the "assimilation-only" test set up
which is (partly?) in /glade/derecho/scratch/raeder/Test_cam-se_dry_mass.

Checklist for merging

• Updated changelog entry
• Documentation updated
• Update conf.py

Checklist for release

• Merge into main
• Create release from the main branch with appropriate tag
• Delete feature-branch

Testing Datasets

• Dataset needed for testing available upon request
• Dataset download instructions included
• No dataset needed

[hkershaw-brown]

Hi Kevin,

Functionally the code looks to be doing the checks for the required state variables. At the moment you code seems a bit over complicated, I've put some comments on simplifying it.

You have some debugging statements, that will only print on task 0. I'd advocated for not putting these debugging statements in (see comment on the ~3 places these debug statements were added)

The change in value for the istatus (16 vs 26) - it would be good to know what this change was for.

Do you have a cam-se case I can grab on Derecho that I can add to our test cases? If not I can spin one up.

Cheers,
Helen

[hkershaw-brown]

functionally this looks to be doing what is should, but it seems a little over complicated.

Why do you have 10 var_names, but only 4 names, and 6 blank strings?

Why is PS in the var_names array?
You could check for PS (always required), then loop though your sometimes required variables.
That way

i = 1
i = i+1
if (dry_mass_vertical_coordinate .and. precise_dry_mass_get_close) then
do while (var_names(i) .ne. '')
  i = i + 1
enddo
endif

becomes

check for PS
if (dry_mass_vertical_coordinate .and. precise_dry_mass_get_close) then
  do i = 1, 3
  enddo
endif

[kdraeder]

My intent was to write generic and flexible code; more fields may need to be added to var_names in the future and it would be nice if the only required change is to var_names, and not finding where var_names is dimensioned and then used, and changing the sizes there. Although size(var_names) coud be used for the loop.
A more comprehensive fix in the future might involve var_names being passed in, so the code I wrote would be more easily adapted to that.
But if the priority is short code to solve the immediate problem, then I'll do that.

[hkershaw-brown]

I don't think you need a data statement & and i variable here
You only have 'PS' so you could call

varid = get_varid_from_varname(domain_id, 'PS')

[kdraeder]

I was making it consistent with the cam-se version to make comparisons easier, and to keep the flexibility for adding fields to the list in the future.
If that's misguided I'll simplify the cam-fv version.

[hkershaw-brown]

Hi Kevin, just checking in on this cam-se pull request and the review comments. Is your plan to work on this?

[kdraeder]

I do plan to finish this after reviewing 740 for Jeff.
I lost momentum on it during the AGU scramble.
Thanks for all of your comments!

[kdraeder]

The change of istatus values from {16,17} to {26,27} in get_se_four_nonstate_values was useful to me because {16,17} are already used in get_se_four_state_values, so I couldn't see which subroutine was returning a non-0 value. It doesn't seem to break anything or add code, so I left it in to save time in the future.