BGC update

[apcraig]

PR checklist

• Short (1 sentence) summary of your PR:
  This is the BGC update
• Developer(s):
  njeffery, eclare108213, apcraig
• Suggest PR reviewers from list in the column to the right.
• Please copy the PR test results link or provide a summary of testing completed below.
  Full test suite on Derecho with Icepack and CICE on all support compilers produces expected results on Sept 12.
• How much do the PR code changes differ from the unmodified code?
  • bit for bit
  • different at roundoff level
  • more substantial for BGC
• Does this PR create or have dependencies on CICE or any other models?
  • Yes, will require an update to CICE due to interface changes
  • No
• Does this PR add any new test cases?
  • Yes, remove skl tests, add zaero tests
  • No
• Is the documentation being updated? ("Documentation" includes information on the wiki or in the .rst files from doc/source/, which are used to create the online technical docs at https://readthedocs.org/projects/cice-consortium-cice/.)
  • Yes
  • No, does the documentation need to be updated at a later time?
    • Yes
    • No
• Please document the changes in detail, including why the changes are made. This will become part of the PR commit log.

This is a significant update in the BGC including refactoring Icepack interfaces.

• Deprecate skl BGC but leave code alone for now hoping we get help from the community to validate the latest code.
• Add BGC parameters to icepack_parameters.F90
• Update BGC physics, consistent with Adds icepack BGC  E3SM-Project/E3SM#6457. Remove redundant arguments in BGC interfaces.
  • icepack_aerosol.F90
    • revised subroutine update_snow_bgc
  • icepack_algae.F90
    • revised subroutine zbio
    • revised subroutine z_biogeochemistry
    • revised subroutine algal_dyn
    • add subroutine bgc_carbon_sum
  • icepack_brine.F90
    • revise subroutine prepare_hbrine
    • revise subroutine update_hbrine
  • icepack_mechred.F90
    • add mbio calculation to subroutine ridge_shift
    • add flux_bio calculation to subroutine ridge_ice
  • icepack_therm_itd.F90
    • update calculation of dvssl and dvint in subroutine lateral_melt
  • icepack_zbgc.F90
    • lots of stuff
  • icepack_zbgc_shared.F90
    • lots of stuff
• Remove redundant arguments in non-BGC interfaces.
  • icepack_atmo.F90
  • icepack_fsd.F90
  • icepack_isotope.F90
  • icepack_itd.F90
  • icepack_meltpond_topo.F90
  • icepack_mushy_physics.F90
  • icepack_snow.F90
  • icepack_therm_bl99.F90
  • icepack_therm_mushy.F90
  • icepack_therm_shared.F90
  • icepack_therm_vertical.F90
  • icepack_tracers.F90
  • icepack_wavefracspec.F90
• Generalize merge_fluxes to make all arguments optional
• Fix bug in subroutine snow_redist computation of hsn_new when nslyr=1
• Update the Icepack driver consistent with Icepack interface changes
• Update zbgc initialization in the Icepack driver, move bgc parameter intiialization to icepack_init_parameters, update icepack_init_zbgc call.
• Update some calls to icepack_warnings_setabort to add file and line number.
• Update warning package to improve OpenMP robustness. Fixes potential race conditions that show up when lots of output is produced.
• Modified congel implementation in subroutine thickness_changes in icepack_therm_vertical.F90 to recover bit-for-bit results. New congel and new bgc implementation were bit-for-bit independently, but when combining the changes, the intel compiler (with -O2) introduces non bit-for-bit changes (roundoff).
• Update bgc namelist defaults and settings in icepack_in
• Update bgc tracer sizing in set_env files
• Update testing, remove skl tests, add zaero tests.

We still need to

• review bgc configurations including namelist settings
• update documentation as needed
• review bgc test suite
• decide what to do with skl
• validate results
• add zaerosol tests

Closes #180

[eclare108213]

This is looking good. A few notes on the documentation (one not directly related to this PR):

• Could the reference to CICE.v5 in the second sentence of section 2.9.1.1 be updated to the current doc? Those equation numbers in the v5 documentation don’t make sense — something didn’t get translated correctly. The problem is that the equations being referenced are in the CICE docs, not the Icepack docs (equations 4 and 5 in section 2.3 Tracers). I recommend removing the parenthetical “(eq. 15 and 16…)”. Section 2.4 Tracers in the Icepack doc should probably reference section 2.3 Tracers in the CICE doc, and vice versa.
• also add the notice that skl is being deprecated at the beginning of section 2.9.4
• There’s a space after the hyphen in brine-volume, in the 4th paragraph of section 2.9.4.2
• The Johnson95 and Edwards2012 citations aren’t being rendered correctly in the doc
• The Nquota math variables look weird, but I guess they’re what’s intended.

What is the status with respect to the to-do list at the top of this PR?

[eclare108213]

Note that this is a bug fix (which I thought we had already fixed!).

[apcraig]

This has always been a fix on this branch. It hasn't been checked in to the trunk yet, but will with the bgc.

[eclare108213]

why are the icepack_warnings.F90 changes included in the BGC PR?

[apcraig]

There were some openmp issues when lots of output was being written (ie. icepack bgc debugging). This was causing problems so I worked on it as part of the bgc branch and it was pulled into the E3SM version. In particular, this is not related to bgc, but it became part of the bgc work.

[apcraig]

I have updated the three bgc parameters and fixed some of the documentation problems. With regard to the boxes on the PR page, I think most are done. I will do some final testing as we approach the merge, but testing is now working well. Will we want a new validation run, three bgc parameters have changed values? Finally, while skl is currently deprecated on the branch, do we want to continue to work on that?

[apcraig]

I ran a full test suite last night on Derecho with 5 or 6 compilers for Icepack and CICE. The test results look good. So, I think we're close and this is where we are

• Testing is looking good
• Documentation has been updated. I think it's ready or at least close?
• We are going to deprecate skl with the possibly to revive later
• We have done a long validation and had a very cursory look, it seems OK. This was done before the latest updates of three namelist settings. Do we want to redo the 8 year bgc gx1 test?
• The zaerosol test is generating conservation errors and has NOT been added to the tested configurations. Do we need to do more debugging/testing? Do we want to add the test even if it's not working perfectly with the hope that it might be fixed in the future? Do we want to defer on the zaerosol test? Ideally, someone would debug the results before we merge, but if we don't do that, then I still favor adding that test to the test suite to keep it on the radar. I would also add an issue.

[njeffery]

@apcraig & @eclare108213 : I'll do some testing of the zaerosol only case from the E3SM side and see if I can reproduce the conservation errors. chrysalis is slow, though. I don't know that this PR should wait.

[apcraig]

Thanks @njeffery. With the zaerosol test, does the snow need to be on or off or maybe either?

[njeffery]

Both should work. I'm testing with the snow on.

[apcraig]

I have added zaero configuration and tests. I ran with both snow on and off, both run to completion in Icepack and CICE but all zaero tests generate conservation errors. But the new tests are running and have been added which I think is good.

[njeffery]

@apcraig : Great! thanks for adding those tests.

My E3SM ocean-ice test case with zaeros active but no sea ice bgc has gone 4 years without any error messages. When do the error messages first appear in Icepack? I'm wondering if there is an initial condition problem for the Icepack test cases.

[apcraig]

OK, I think this could be ready to merge.

• I have read thru the bgc documentation quickly and fixed a couple final formatting things.
• The skl is deprecated with hopes of revival in the future.
• The zaero tests have been added to Icepack and CICE with and without snow turned on. All zaero tests run to completion with conservation errors. Those conservation errors will need to be addressed at some point, we can create an Issue for it.
• A full test suite was run on Derecho with 5 and 6 compilers for CICE and Icepack respectively on Sept 12. Test results are as expected. All tests are bit-for-bit except bgc and congel. A small number of tests do fail but that's consistent with what is happening on the main trunk. These failed tests are understood (mostly hdf5 and nvhpc compiler).

@njeffery, @eclare108213, please approve this and the CICE PR if you think they are ready to merge.

[apcraig]

@njeffery. The conservation errors appear at every timestep in Icepack and CICE with the zaero configuration. I did a test last week where I set the zaero forcing to 0 by changing

    faero_atm(:,:,1,:) = 1.e-12_dbl_kind ! kg/m^2 s                                                                             
    faero_atm(:,:,2,:) = 1.e-13_dbl_kind
faero_atm(:,:,3,:) = 1.e-14_dbl_kind
    faero_atm(:,:,4,:) = 1.e-14_dbl_kind
    faero_atm(:,:,5,:) = 1.e-14_dbl_kind
    faero_atm(:,:,6,:) = 1.e-14_dbl_kind

to

    faero_atm = 0.0_dbl_kind

The conservation errors went away. Of coarse, the aerosol tracer remained identically zero throughout as well, so maybe not very useful.

Could it be that something about HOW I'm running the cases is incorrect. I will send output from those cases via email separately.

[njeffery]

Approved based on ocean-ice multiyear testing of Icepack bgc and zaerosols in E3SM and reading of the documentation changes.

[apcraig]

The conservation errors have been fixed. I needed to run nblyr=7 instead of 1. That has been checked in and the zaero cases are now running with NO conservation errors.

[njeffery]

Awesome!

[eclare108213]

Awesome, thanks for all the work on this, @njeffery @apcraig