Improve testing code coverage

[apcraig]

The codecov tool is working for Icepack and CICE, (see #434, #436) and a handful of people can run the testing. We should next evaluate the codecov results and see if we can improve testing coverage. There is an equivalent issue in CICE-Consortium/Icepack#194, but I thought I'd add one specifically for CICE because the efforts may not overlap.

The codecov results for CICE can be found here,

https://codecov.io/gh/apcraig/Test_CICE_Icepack

and the current results by file are

https://codecov.io/gh/apcraig/Test_CICE_Icepack/list/3d1a95bc42877d9ab1a5ddf17d5dd255ce4d929b/

[apcraig]

There is a google doc that is trying to track some of the coverage issues

https://docs.google.com/document/d/1OMqg8u66uobqD_sbIHSM6ZXLEbv7FFRnQKSk2TH1c7M

[apcraig]

We added an io_suite to test io_binary, io_netcdf, and io_pio as well as a histall option to turn on all history fields in #447.

[apcraig]

We added coverage for ice_dyn_evp_1d, tr_aero, ncat=6, conserv_check, calc_tsfc in #450.

[apcraig]

The coverage testing moved to lcov and output can be found here, https://apcraig.github.io/.

[rallard77]

Tony, Your charts look great and are very intuitive. Rick

…

On 7/21/2020 12:38 PM, Tony Craig wrote: The coverage testing moved to lcov and output can be found here, https://apcraig.github.io/. — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub <#437 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AG63LB6U7PPDUMZCOVKGWE3R4XHBXANCNFSM4MY7NYLA>.

[eclare108213]

@CICE-Consortium/devteam
I have gone through the worst offenders in the code (modules highlighted red in the lcov output) and made notes for the things we could test better in a spreadsheet. Text in red is high priority; text in purple is BGC (which needs a concerted effort all by itself). I've assigned many of you to take a look at and (maybe) address particular items in this list (negotiable - let me know if you won't be able to do this). Options include adding a test, choosing not to test, deleting the unexercised code, etc.

When you drill down into a module, many of the red lines in the code are continuation lines which do appear to be exercised even though they are red in this output. Many others are calls to the warning package in cases where the code fails, and I'm happy those are rarely accessed. Others are diagnostics for debugging, which we might consider removing or formalizing. The items I put in the spreadsheet are generally code options or capabilities that we could consider testing.

I'll go through the yellow- and green-highlighted modules eventually (or you can volunteer!). Please let me know if you have questions/concerns.

[phil-blain]

@eclare108213 it is on my list to test the implicit solver more thoroughly, and under more configurations. So I'm OK with all of the items you've assigned @JFLemieux73 and me in that spreadsheet. I don't know when I'll get to it then - most likely Q1 2121 EDIT that's very pessimistic lol Q1 2021 is more optimistic :P.

[eclare108213]

I have scanned the ‘verbose’ output in the log files for base_suite in my fork/master. Here are physically inconsistent settings with suggestions for improving them. I haven't throughly studied @apcraig 's code coverage spreadsheet yet, and I'm still waiting for my base_suite run from #533 to clear the queue, so I haven't looked at the timeseries output yet -- this means that we might choose different values than my initial suggestions below.

gbox80_1x1_boxslotcyl
kridge = -1 with tr_lvl=T and tr_pond_lvl=T. Set tr_lvl=F and tr_pond_lvl=F.

gbox128_2x2_boxadv_short
0-layer thermo with delta-Eddington radiation. Set shortwave = ccsm3 and albedo_type=constant
(Interesting that this combination runs!)

gbox128_4x2_boxdyn_short
kdyn=0 with kridge=1. This is probably okay but a little weird.
0-layer thermo with delta-Eddington radiation. Set shortwave = ccsm3 and albedo_type=constant

gbox128_4x4_boxrestore_short
0-layer thermo with delta-Eddington radiation. Set shortwave = ccsm3 and albedo_type=constant

gx3_4x2_alt03_debug_short
BL99 thermo but calc_Tsfc=F. This doesn’t make sense for our configuration, although this is what HadGEM etc does.
set l_mpond_fresh=T for this test

alt05
shortwave=delta-Eddington with albedo_type=default. Change shortwave to ccsm3. Didn’t we get rid of “default” for albedo_type?

Suggested default parameter changes:
emissivity = 0.985 (from 0.95)
nblyr = 1 (from 7)

[eclare108213]

I looked at time series plots for the alt02, alt04 and alt05 base_suite tests as part of the testing review in #533. They have some of the same problems as those described there for the 90-day gx3 and gx1 runs, many of which may just be issues with either the diagnostics or the plotting scripts. In particular, the ice salinity values in alt04 need to be formatted better on the y-axis. There are 2 lines on the plots for Antarctic surface temperature, SSS, SSH, snowfall, rainfall, congelation, area fraction, and air temperature.

There might also be issues with the test configurations themselves, as noted above for alt05. For alt02 and alt05, SST==0 even though the ocean mixed layer is turned on -- this is the main thing that needs to be understood.

[eclare108213]

I looked at time series plots for the alt02, alt04 and alt05 base_suite tests as part of the testing review in #533. They have some of the same problems as those described there for the 90-day gx3 and gx1 runs, many of which may just be issues with either the diagnostics or the plotting scripts. In particular, the ice salinity values in alt04 need to be formatted better on the y-axis. There are 2 lines on the plots for Antarctic surface temperature, SSS, SSH, snowfall, rainfall, congelation, area fraction, and air temperature.

There might also be issues with the test configurations themselves, as noted above for alt05. For alt02 and alt05, SST==0 even though the ocean mixed layer is turned on -- this is the main thing that needs to be understood.

These issues have been addressed.

Additional notes (also in the spreadsheet):

ocn_freezing_temperature
This is only called if ocn_data_type='clim', which is only available for gx3. However the data files are only fortran-unformatted and not available in our input file downloads (e.g. sss.mm.100x116.da). We could send the routine a field of constant (default) data on any grid (replacing the loop setting Tf in ice_init.F90/init_coupler_flux), or we could create new forcing data to add to our data distribution, or we could deprecate this routine and the ocean-forcing code that calls it.

get_forcing_ocn
We have some wave forcing but otherwise there is no ocean forcing in the forcing files we distribute.

debug_ice, print_state, print_points_state
print_points_state is never called. In order to test debug_ice, which calls print_state, we need to replace CICE_RunMod.F90 with CICE_RunMod.F90_debug.

writeout_finished_file
This routine should be deprecated.

zbgc
In option bgcz, try setting modal_aero=T, dEdd_algae=T for better coverage

[eclare108213]

The boxdyn test, which has kdyn=0, is poorly named. It appears to be a straight thermodynamics test with very simple parameterization choices.

[apcraig]

I am working on a number of these issues. With regard to debug_ice, I want to get rid of the CICE_RunMod.F90_debug, move the extra diagnostics into CICE_RunMod.F90 (just in the standalone model), add a run time namelist to turn it on, and leverage lonpnt and latpnt to define the points to diagnose. I will add another namelist to turn on the diagnostics after a certain number of steps (ie. check_step but maybe a different name). Does that sound reasonable? At the present time, it's all manual. Is there any reason to have different points for print_points and debug_ice? It seems like when you get into "debug_ice" mode, you just set the points to the ones you want to debug.

[apcraig]

I am testing modal_aero in bgcz. But there is a hardwired file in ice_forcing_bgcz.F90,

       optics_file =  &
        '/usr/projects/climate/njeffery/DATA/CAM/snicar/snicar_optics_5bnd_mam_c140303.nc'

I can move that out to namelist, but we need the file. @njeffery, can you provide us with that file?

[eclare108213]

Here is the SNICAR file.

snicar_optics_5bnd_snow_and_aerosols.nc.zip

[eclare108213]

@apcraig, it would be good to combine the RunMod files into one, and turn them on using the debug flag. I kept them separate for computational efficiency purposes, but that's not necessary for the stand-alone version anymore.

For the points used in debugging, we need to be able to enter a particular (i,j,iblock) combination. The print_points locations are entered based on lat/lon and the nearest grid cell is found and printed. Also, there are two of them -- they don't have to be NH and SH but that's how they've always been set up. It would be fine to allow a user to choose the points by either method, but think about whether only one point is being debugged and the code prints 2 points... Lat/lon is more intuitive for general "sanity" diagnostics, but for debugging it's better to be able to specify the exact point being queried (and not all run configurations use sensible lat/lon values).

[apcraig]

Here is the SNICAR file.

snicar_optics_5bnd_snow_and_aerosols.nc.zip

I will give it a try, thanks.

[apcraig]

For the points used in debugging, we need to be able to enter a particular (i,j,iblock) combination. The

On this point, do we really want to specify a local i, j, iblck, ntask value? If someone uses a different decomposition or pe count, that all changes. Would we want a global i,j instead of lon/lat as a way to specify the grid point to be analyzed? How does one normally decide what point they want to analyze in more detail?

[eclare108213]

It's only intended for very detailed debugging, e.g. when a run blows up, it's usually only in one or maybe a few grid points, and sometimes it's not clear what's causing it to blow up -- it can develop gradually -- and sometimes I need to debug in a parallel configuration. Sometimes it takes a little debugging to figure out what the particular grid cell is, but then I can specify it and re-run, watching how the problem develops over time. Often it helps me narrow down the problematic process model (thermo vertical or itd, evp, ridging, radiation, etc) so that I can look more closely there, and/or it narrows down the time period I need to pay attention to (making it easier to start a debugging session in totalview from a restart file, for instance). This approach usually works well except when the problem is imported by advection (and then totalview is very helpful).

[apcraig]

OK, sounds fine. So you want a set of namelist inputs where the point can be specified with (ilocal,jlocal,iblk,ntask) rather than (iglobal,jglobal)? Either way is fine by me. I'll take care of it.

[eclare108213]

Yes, I'd prefer to have the local point info rather than global - it's more accessible when you're elbow-deep in the code.

[eclare108213]

Plus, sometimes we're debugging halo cells which aren't accessible as global indices, right?

[apcraig]

Good point about halo cells.

[apcraig]

Here is the SNICAR file.

snicar_optics_5bnd_snow_and_aerosols.nc.zip

CICE is looking for a variable named bcint_enh_mam_cice which should be (3,10,8) with 3 spectral intervals (as defined in icepack). The new file has a variable called modalBCabsorptionParameter5band but it has 5 spectral intervals so is (5,10,8). How should this be handled in the model? Do we read the first 3 of 5 bands? Do we change icepack spectral bands from 3 to 5? Is modalBCabsorptionParameter5band the right field name for the bcenh variable in CICE?

[eclare108213]

@njeffery, could you clarify re the input file for modal aerosols? I haven't started upgrading for the SNICAR radiation changes yet -- that will follow the snow work -- so if this code difference is part of that, maybe we should wait and not worry about it yet.

[njeffery]

@eclare108213 : The above file is for use with the new snicar-ad option on.
For the modal-aerosol scheme with 3 spectral intervals, I believe this is the correct file:
snicar_optics_5bnd_mam_c160322_mpas_cice.nc.gz

There's also a standard optics file for use without the modal option and no Snicar-AD:
standard_optics_mpas_cice.nc.gz

[apcraig]

I've been messing around with the modal_aerosols and dedd_algae options the last couple days. There are a number of bigger issues other than the missing dataset. It looks like nbtrcr_sw was not being set within icepack properly (it was always zero). The other thing is that trcrn_sw was also not being allocated properly, it was size 0, but since nbtrcr_sw was also 0 in icepack, there wasn't really anything happening with this code. The only reason I started tracking this down is that the model was hanging at the end of the test runs with dedd_algae on. I've been debugging the last day or two. I was hoping to identify and quickly fix a problem, but the problems seem a little more systemic. I will make a few small changes to fix some the things I found, but the model will still fail with dedd_algae on.

There are probably a number of reasons this happened. It looks like the shift to dynamic memory allocation and new icepack interfaces didn't properly address the implementation for the bgc sw on some level. In addition, the fact that we didn't have a test for it allowed it to unravel a bit. We do have a test with zbgc, but just not with modal_aerosol and dedd_algae on. The zbgc test we're running does work, but I don't know that we've ever validated the science results. Maybe we need to do that too.

I will include this info in my next PR.

[apcraig]

PR #602 addressed a number of issues including

gbox80_1x1_boxslotcyl
kridge = -1 with tr_lvl=T and tr_pond_lvl=T. Set tr_lvl=F and tr_pond_lvl=F.

gbox128_2x2_boxadv_short
0-layer thermo with delta-Eddington radiation. Set shortwave = ccsm3 and albedo_type=constant
(Interesting that this combination runs!)

gbox128_4x2_boxdyn_short
kdyn=0 with kridge=1. This is probably okay but a little weird.
0-layer thermo with delta-Eddington radiation. Set shortwave = ccsm3 and albedo_type=constant

gbox128_4x4_boxrestore_short
0-layer thermo with delta-Eddington radiation. Set shortwave = ccsm3 and albedo_type=constant

gx3_4x2_alt03_debug_short
BL99 thermo but calc_Tsfc=F. This doesn’t make sense for our configuration, although this is what HadGEM etc does.
set l_mpond_fresh=T for this test

alt05
shortwave=delta-Eddington with albedo_type=default. Change shortwave to ccsm3. Didn’t we get rid of “default” for albedo_type?

debug_ice, print_state, print_points_state
print_points_state is never called. In order to test debug_ice, which calls print_state, we need to replace CICE_RunMod.F90 with CICE_RunMod.F90_debug.

writeout_finished_file
This routine should be deprecated.

The boxdyn test, which has kdyn=0, is poorly named. It appears to be a straight thermodynamics test with very simple parameterization choices.

I think the outstanding issues noted in this PR are

ocn_freezing_temperature
This is only called if ocn_data_type='clim', which is only available for gx3. However the data files are only fortran-unformatted and not available in our input file downloads (e.g. sss.mm.100x116.da). We could send the routine a field of constant (default) data on any grid (replacing the loop setting Tf in ice_init.F90/init_coupler_flux), or we could create new forcing data to add to our data distribution, or we could deprecate this routine and the ocean-forcing code that calls it.

get_forcing_ocn
We have some wave forcing but otherwise there is no ocean forcing in the forcing files we distribute.

zbgc
In option bgcz, try setting modal_aero=T, dEdd_algae=T for better coverage

[eclare108213]

@apcraig thanks for your thorough work on the code coverage! Regarding the outstanding issues:

ocn_freezing_temperature
This is only called if ocn_data_type='clim', which is only available for gx3. However the data files are only fortran-unformatted and not available in our input file downloads (e.g. sss.mm.100x116.da). We could send the routine a field of constant (default) data on any grid (replacing the loop setting Tf in ice_init.F90/init_coupler_flux), or we could create new forcing data to add to our data distribution, or we could deprecate this routine and the ocean-forcing code that calls it.

I would be fine with deprecating it when we clean up the forcing routines (if not earlier). It only calls icepack_sea_freezing_temperature, which is also called elsewhere in the code and so hopefully is covered by existing tests.

get_forcing_ocn
We have some wave forcing but otherwise there is no ocean forcing in the forcing files we distribute.

This is fine for now. We have the flexibility to add other forcing files if needed. Technically, the BGC data files are ocean forcing but they aren't called/read from here.

zbgc
In option bgcz, try setting modal_aero=T, dEdd_algae=T for better coverage

We will have to merge all of the changes to the BGC code that have been made in E3SM into Icepack and can add these tests as part of that work.