C-grid Code Cleanup

[apcraig]

key:
needs to be done in cice-consortium/main before cgrid fork
likely changes answers on B grid

style and consistency

• variable names TUNE consistency
  - caps for TUNE (i.e. Tbu)
  - do we want to add U or T to existing variables?
  - do we want to change tmask to maskT? tarea to areaT? etc
• there is a mix of if (grid_ice == ‘CD’) then and select case (trim(grid_ice)). Do this consistently.
• rename strocn[x,y]T because it's different units from other strocn[x,y] terms (divides by ai for coupling output). Better yet, move this to different location associate with output.
• comment the code to explain that 00, 10, 01, 11 are i+0,j+0,i+1 and j+1
• set capping variable based on namelist (or other, i.e. kdyn), make name more descriptive
• join iblk loops in ice_grid.F90 (and review loop structure everywhere), careful about answer changes
• remove unused grid options in history file (i.e. ustr3Dz)
• white space, alignments, comments, etc
• iceumask, icenmask, iceemask are logicals in ice_flux.F90. icetmask is integer. Make consistent?
• reneame cicedynB to cicedyn and create soft link

bugs

• uocn, vocn is on the T grid and never averaged to U grid (except for hadgem forcing)
• kinetic energy diagnostic mixes T and U variables for B grid #734
• atm_data_dir = '/glade/u/home/tcraig/cice_data/' in ice_in, needs to be 'unknown'
• change ice_step_therm1 to compute uvel, vvel at cell centers using new averaging routine #735
• change to_ugrid calculation of mass, aice to use T2US instead of T2UF (check and fix everywhere, eventually) #736
• variable drag coefficients are computed on T-cell centers, need to be moved to velocity locations #737
• Principal stress calculation does not work for C grid  #707

better approaches

• consolidate deltamin variables and update value of deltamin
• change visc_method default to avg_zeta #738
• update dynamics driver, (see Dynamics control logic and implementation #619)
• new computation of dx and dy leveraging HTS, HTW (see Grid lengths and averaging #653)
• change grid_type = 'setmask' to use kmt_type instead (NUOPC / CMEPS) (see Change grid_type = 'setmask' to use kmt_type instead (NUOPC / CMEPS) #805)
• check whether ice strength is calculated at the correct location. Move to after last subcycle? #739
• move pre-transport shift of (uvel,vvel) from U to N,E points into a subroutine that can be called from the ends of eap, vp, evp
• clean up geometry/forcing/ic settings primarily related to box configurations. it's currently a function of ice_in, grid_type, atm/ocn/ice_data_type, kmt_type, boundary_type, close_boundaries. Want to be able to define the geometry, mask, initial condition, and forcing separately and cleanly.
  • allow close_boundaries to work for all rectgrid geometries
  • make ew_boundary_type independent of geometry
  • create consistent approach for defining masks, kmt vs ice_data_type
  • separate geometry and ice initial condition in namelist (split ice_data_type?)
  • review kmt_type and kmt_file interactions with other namelist

testing

• review box2001 (and other box configs) forcing T vs U
• -s gbox128 runs in parallel using box2001 atm and ice data types (quick suite), but -s box2001 does not run in parallel
• remove set_env.box2001
• Noise in boxwall test, does it still exist, C-CD TEST: boxwallblockp5 noise #689
• refactor boxslotcyl test (pull loop and ice velocity grid shifts into subroutine)
• clean up set_nml.gbox* files, remove *_data_type and move those settings to specific box set_nml test. Probably also need a set_nml.boxgeneric to define generic forcing for some of the tests.
• increase coverage of dynamics options (capping, visc_method, etc)
• extend unit testing, Extend unit testing  #696
• create variable-resolution box configuration for testing #740

documentation

• fix width of command line examples in box2001 and other testing documentation
• provide C-grid results to community as part of next release #741

[phil-blain]

• use trim(character string) in conditional or case statements, e.g. if (grid_system == 'CD') then

since grid_system comes from the namelist, we could trim it in ice_init and not have to call trim all over the code, no ?

[apcraig]

I don't think trim has any impact except in write statements. The length of a string is defined in it's declaration. The other thing is when Fortran is doing string compare, I believe it pads the two strings to the same length with spaces and compares those. So (trim(grid_system) == 'CD') would behave exactly the same as (grid_system == 'CD') regardless of the length of grid_system. It could be that one or the other performs better, but I suspect that will have little impact on the overall code performance. Doing something like grid_system = trim(grid_system) has no impact because while trim(grid_system) returns a string with the last character non-space and appropriate length, as soon as you do grid_system=trim(grid_system), grid_system is padded with blanks again. From the fortran handbook, 7.3.1.2

When the operands are both of type character, the shorter one is padded on the right with blank padding characters until the operands are of equal length. Then, the operands are compared one character at a time in order, starting from the leftmost character of each operand until the corresponding characters differ. ... The operands are equal if both are of zero length or all corresponding characters are equal, including the padding characters. Note that the padding character is the Fortran blank (3.1.1) when the operands are of default character type ...

So, we may want to unify our string compares in the model, it's unlikely to have any impact on performance or result.

[dabail10]

I was wondering about the grid_type versus kmt_type for setmask. The option setmask is for prescribed ice or single column mode. It kind of replaces the old latlon option. So, maybe we need a different flag?

[eclare108213]

@apcraig Most of this list has been done -- are there items here that probably won't be done before the next release, and could be moved into a new issue to be addressed later? The important one in my opinion is the last one -- providing sample output.

[apcraig]

I agree, I think we've done the most important tasks on this list and the most important thing prior to release is validation at this point. I think we could release now-ish once we're happy with the results. I will have a look at the deltamin and dynamics driver refactor although we've had some refactoring on the dynamics driver already. But I think we envision a more general refactor at some point. We may need to create a couple new issues when we close #660. We should probably do that during the release period.

[apcraig]

@dabail10 has requested we complete #707. I'll also try to take a look at that.

[apcraig]

For deltamin cleanup, we currently have

  configuration/scripts/ice_in:    deltaminEVP     = 1e-11
  configuration/scripts/ice_in:    deltaminVP      = 2e-9

I assume we want a single deltamin variable? It would be set to 1e-11 by default in the code and in namelist and it would change to 2e-9 in namelist when kdyn=3 is set in the options? And nothing inside the code would modify deltamin depending on kdyn. Also, are we happy with the current values or does that also need to change?

@JFLemieux73 @phil-blain @eclare108213 @dabail10

[eclare108213]

Re deltamin, see #712:

It was decided to keep the same default parameters and to add an advice in the documentation to increase ndte for high resolution applications (see pull request #722).

deltaminEVP and deltaminVP are also documented well, so I think this item is okay as-is and can be checked off. @JFLemieux73 do you agree?

[apcraig]

With release of CICE6.1.4, most tasks are complete. Outstanding tasks have been given their own issue. Closing now.