cice-consortium/icepack

Development repository for sea-ice column physics

icepack's Issues

diagnostic files

When an Icepack run finishes and the icepack.runlog.* file is copied back into the case/logs directory, please also copy the diagnostic output files, ice_diag.*

pond parameters

In the appropriate -s files, set rfracmax=0.7 and rfracmin=0.15 for the cesm and topo melt pond tests. For the level-ice pond scheme rfracmax=1.0.

convergence test

Use the SHEBA convergence data provided by CC Bitz to test the ridging scheme

bgc exact restart fails during icepack testing

PASS cheyenne_restart_col_1x1_bgcISPOL run-initial
FAIL cheyenne_restart_col_1x1_bgcISPOL run-restart

Bug in mushy layer?

I noticed calls like this in two_stage_solver_snow and two_stage_solver_nosnow. In the subroutine for these, it has the arguments of Tsf0, zqsn0, zqin0, zSin0, zTsn0, and zTin0.

Then just before the call to the picard solver you have:

   if (fcondtop - fsurfn < ferrmax) then

      ! solution is consistent - have solution so finish
      return

   else

      ! solution is inconsistent - resolve assuming other surface condition
      ! assume surface is cold
      fcondtop1 = fcondtop
      fsurfn1   = fsurfn


     ! reset the solution to initial values
      Tsf  = Tsf0
      zqsn = zqsn0
      zqin = zqin0
      zSin = zSin0

      ! solve the system for melting and snow
      call picard_solver(nilyr,    nslyr,    &
                         .true.,   .false.,  &
                         Tsf,      zqsn,     &
                         zqin,     zSin,     &
                         zTin,     zTsn,     &

...

So, here is a dumb question for Adrian Turner. Why do you only reset Tsf, zqsn, zqin, and zSin, but not zTsn and zTin? The NAG compiler is complaining about unused variables here. So, I just wanted to make sure they are not supposed to be used.

Dave

combine constants and parameters

Move contents of icepack_constants.F90 into icepack_parameters.F90.
Move all default settings for icepack parameters into icepack_parameters.F90.
(note that there are some driver-specific namelist settings that should remain only in icedrv*)
Check that icedrv queries icepack to get the default values, then reads namelist and updates those parameters, then (re)sets icepack's values.
Check query/init for tracers, bgc, similarly.

BGC forcing

Please add a paragraph describing the ISPOL and N-ICE cases, including forcing fields, time period, and references to the field experiments.

Fe

Dissolved and particulate iron seems to be labeled inconsistently in the code and also in the documentation. In some places it is fed, fep, others dFe, pFe. Can we make all of these references or labels or variable names consistent, or is there a real difference in them?

bgc transport eq number

There's an equation number missing in the first sentence of the "flux-corrected, positive definite transport scheme" section. [eqn:mobile_transport] does not exist in this documentation.

Missing text

Fix sentences that are missing text in the documentation - look at v5.1.2 doc.
Thermodynamics section:
"... which cannot exceed . "
"... raise its temperature to ."
New temperatures section (BL99):
"... which is held at , and ..."
New temperatures section (mushy):
"... raise the temperature of the sea ice to , including..."
Growth and melting section:
"... of liquid water at . "

PDF

Make a PDF of the documentation before the final Icepack release and add it to the master branch before the code is tagged as a release.

BGC output

Please check that the documentation about BGC output in CICE is correct in the Icepack documentation:
https://cice-consortium.github.io/Icepack/user_guide/ug_implementation.html#biogeochemistry-history-fields

zqsn inout

zqsn should be intent(inout) in icepack_therm_vertical.F90, subroutine init_vertical_profile

creating directories

When scripts create new directories, please have them first check whether the directory exists. This example came up when I tried to re-run a restart test in a previously used run directory, using a slightly different namelist configuration.

mkdir: cannot create directory `/net/scratch3/eclare/ICEPACK_RUNS/pinto_restart_col_1x1_debug.testrestart/diags_baseline': File exists

Let's keep an eye out for cases when we really do not want to re-use an existing directory, but most of the time it's probably okay for those of us doing manual shortcuts in/around the usually automated test suite.

bug in icedrv_init_column.F90

The call to icepack_init_bgc has a syntax error in the call:

  call icepack_init_zbgc ( &
     R_Si2N_in=R_Si2N, &
     R_S2N_in=R_S2N, R_Fe2C_in=R_Fe2C, R_Fe2N_in=R_Fe2N, R_C2N_in=R_C2N, &
     R_chl2N_in=R_chl2N, F_abs_chl_in=F_abs_chl, R_Fe2DON_in=R_Fe2DON, &
     R_C2N_DON_in=R_C2N_DON, &
     R_Fe2DOC_in=R_Fe2DOC, &
     chlabs_in=chlabs, alpha2max_low_in=alpha2max_low, beta2max_in=beta2max, &
     mu_max_in=mu_max, grow_Tdep_in=grow_Tdep, fr_graze_in=fr_graze, &
     mort_pre_in=mort_pre, &
     mort_Tdep_in=mort_Tdep, k_exude_in=k_exude, &
     K_Nit_in=K_Nit, K_Am_in=K_Am, K_sil_in=K_Sil, K_Fe_in=K_Fe, &
     f_don_in=f_don, kn_bac_in=kn_bac, f_don_Am_in=f_don, f_exude_in=f_exude, &

I believe it should be f_don_Am=f_don_Am here? I am fixing this in the next commit.

documentation of bgc namelist

Move zbgc namelist options into the namelist table, or create a separate table for it. This is in the User Guide now. Replace p1 with 0.1, etc, and remove _dbl_kind from doc table. Is everything in the 'BGC tuning parameters' table also in namelist?

Also, the current table in the docs looks like it was copied straight out of the code (where it sets defaults). If that is true, then some of the read values are missing _dbl_kind in the code and should be fixed.

Makefile conditionals within targets

The new makefile, after adding CCE, GNU, and PGI compilers includes a conditional within some of the targets

	@if [ "$(COMPILER)" == "cray" ] || [ "$(COMPILER)" == "pgi" ]; then \
	  mv $*.i $*.f; \
	fi

This is because the CCE and PGI compilers output a .i file when running through the preprocessor. There is likely a way with each compiler to alter the output file extension so that this branch is not required.

Gsum bug

Gsum is the cumulative area of ice and open water in a grid cell. It is occasionally 0 and causes a divzero error, though probably not in active grid cells. Fixing it may cause nonBFB results and therefore this will need to be tested in the full CICE model, once the new version of Icepack is added there.

doc wind stress option

Replace "We have ignored ice motion in u*..." with information about the new option that includes the ice velocity in the wind stress calculation (all turbulent fluxes?), and the circumstances under which it should be used. This is in the 'Atmosphere' section of the Science Guide.

which conservation equation?

In the "Mobile and stationary phases" section,
The conservation equations are given by Eq. 18 in the CICE v5 documentation with a_pnd a_i replaced by f_b v_in.
Eq. 18 does not look like the right one. Is eq. 16 in the Icepack documentation the one you were thinking of? (if the eq numbers change, it's the first equation in the 'tracers that depend on other tracers' section)

add comment about formdrag parameterization to 'known bugs'

Abort with invalid cdn_atm_ratio_n

The code aborts in merge_fluxes for sets=debug,dyn with gnu. In that case, atmbndy="constant" and cdn_atm_ratio_n is never set but it is used.

The calling sequence is something like

icepack_therm_vertical.F90, icepack_step_therm1 defines cdn_atm_ratio_n locally
icepack_step_therm1 calls icepack_atm_boundary with cdn_atm_ratio_n as argument
icepack_atm_boundary sets cdn_atm_ratio_n when atmbndy is NOT set to "constant"
icepack_step_therm1 calls merge_fluxes and cdn_atm_ratio_n is passed in and used

If someone helps me figure out what to do, I can implement and test it.

365?

In the reactions section,
Very long timescales will remove this source/sink term. The default value is currently set at 3065 days to turn off this dependency.
Is it really 3065? Looks like a typo.

cannot find entry symbol _start

NAG is giving this message because there is no "program main" anywhere. Do we want this?

2018

replace 2017 with 2018 in copyright statements everywhere

exponential underflows in dEdd shortwave

Some compilers are complaining about underflows in the shortwave calculation. These are coming from exponentials with very small arguments. The current code to get rid of those values does not avoid their being calculated in the first place:

argmax = 10.
exp_min = exp(-argmax)
f = max(exp(-A), exp_min) ! exp(-A) is what we are trying to compute

We should do this instead:

argmax = 10.
exp_min = min(argmax, A)
f = exp(-exp_min)

This type of calculation occurs in 3 places in icepack_shortwave.F90.
I expect this to change the answers (nonBFB).

Remove documentation branch

We need to remove the "documentation" branch from the Icepack repository. It looks like it is stale and was last changed in September. My guess is that it was probably added to the CICE-Consortium accidentally during PR. However, since the documentation is part of the master branch (RST and PDF) or gh-pages (HTML) I think keeping this branch will cause confusion. I think we should remove it.

Icepack output

Check the Icepack output for various cases to make sure it is what we expect

Version hardcoded in Documentation

In section 1.3, file doc/source/intro/major_updates.rst. Maybe we can just refactor a bit and call the section something like "separation of Icepack from CICE" and say this happened after CICE5 without specifying any specific versions?

Or is this really going to be an ongoing "ChangeLog" that will provide an overview of changes since the start of Icepack? If so, maybe that belongs in an appendix.

So, maybe have section 1.3 be an overview of when and why Icepack was separated from CICE. And maybe create a new appendix that summarizes changes in Icepack since CICE.5 that can be an ongoing log.

list updates to column physics since cice v5.1.2 release

2 failing BGC tests

See PR #89 for details

FAIL pinto_smoke_col_1x1_bgcISPOL_debug run
FAIL pinto_restart_col_1x1_bgcISPOL run-initial

clean up index of variables

Remove anything not in Icepack, and make sure all of the namelist options are included (bullets).

This one is extremely tedious, and I welcome all the help I can get. We could divide up the alphabet... I'll take X!

Documentation chapters

Fix overall layout of the Icepack documentation:
Table of contents should not be a chapter (but could be chapter 1 if necessary).
Chapter 1 Introduction (or chapter 2)
Chapter 2 Science Guide
Chapter 3 User Guide
Chapter 4 Developer Guide
Index of primary variables and parameters (could be chapter 6 if necessary)
Bibliography

restart diagnostics

The heading "min/max [field]" is printed to diagnostic output when restarts are read, but the min and max values are not calculated and printed. Either remove the heading or add the values.

strata

Document the set of variables used for tracking tracers on tracers

bgc restart module

Consider renaming ice_restart_column.F90 to be ice_restart_bgc.F90, and update the documentation in
https://cice-consortium.github.io/Icepack/developer_guide/dg_driver.html
appropriately.

Call to remap_zbc

This looks like a bug, but @njeffery can confirm this is the expected behaviour. So, the call to remap_bgc normally has ntrcr as the first argument. However, it is not actually used in that subroutine. The next two arguments are nlyrn (number of ice layers) and it (tracer index in top layer).

        elseif (scale_bgc .and. ktherm == 2) then
           trtmp(:) = c0
           do n = 1,ncat
              call remap_zbgc(nilyr, nilyr,    &
                              1,                          &
                              sicen(:,n),       trtmp,    &
                              0,                nblyr+1,  &
                              c1,               c1,       &
                              cgrid(2:nilyr+1),           &
                              igrid(1:nblyr+1),           &
                              sicen(1,n)                  )
              if (icepack_warnings_aborted(subname)) return

              do mm = 1,nbtrcr
              do k = 1, nblyr + 1
                 trcrn(bio_index(mm)+k-1-ntrcr_o,n) =   &
                      (trtmp(k)/sss*ocean_bio_all(bio_index_o(mm)))
                 trcrn(bio_index(mm)+nblyr+1-ntrcr_o:bio_index(mm)+nblyr+2-ntrcr_o,n) = c0 ! snow
              enddo  ! k
              enddo  ! mm
           enddo     ! n

Bug in icepack_atmo.F90

In the following code in icepack_atm_boundary:

  worku = c0
  workv = c0
  workr = c0
  if (present(uvel)) then
     worku = uvel
  endif
  if (present(uvel)) then
     worku = uvel
  endif

The second if block should be:

  if (present(vvel)) then
     workv = vvel
  endif

I have a fix coming with the next commit.

icepack_algae underflow

icepack_algae is underflowing in the second part of this line with gnu compiler and debug+bgcISPOL.

           dmobile(k) = mobile(mm)*(initcons_mobile(k)*(exp(-dt*rtau_ret( mm))-c1) + &
                             initcons_stationary(k)*(c1-exp(-dt*rtau_rel(mm))))

If anyone has any ideas, I'm happy to implement and test.

number of bgc layers?

The doc currently has this:

For zbgc with 8 vertical layers:

setenv TRBRI  1   # set to 1 for brine height tracer
setenv TRBGCS 0   # set to 1 for skeletal layer tracers
setenv TRBGCZ 1   # set to 1 for zbgc tracers
setenv NBGCLYR 7  # number of zbgc layers

Are there 8 layers when NBGCLYR is 7?

ice_brine exception

Hitting another error with bgc in ice_brine.F90,

write(99,) subname,' tcx3',-darcy_coeff, (-darcy_coeff/bphi_mindt)
call flush(99)
hbrocn_new = hbrocnexp(-darcy_coeff/bphi_mindt)

The exp is blowing up with gnu and debug flags. The last two values before blowup are

(update_hbrine) tcx3 -1.1392520136377392E-009 -2.3407096177064297E-003
(update_hbrine) tcx3 -0.56199991572839070 -3597.1318270922047

The last set of values are creating problems with exp(-3597.). I can keep adding checks for exps that are too large or too small, but if those values are not physical, then there are underlying issues that need to be fixed. Thoughts?

unused variables

Please would someone using a gnu or nag compiler produce a list of warnings with their module/line locations? The priority here is getting unused variables out of the columnphysics interface subroutine argument lists, so I'm most interested in the "Unused" warnings from the list below. But it would be good to capture all of them, to clean up at some point.

GNU Warnings
Unused variable
Creating array temporary
Label
Unused dummy argument
Unused module variable
Unused parameter
Unused PRIVATE module variable
Obsolescent feature
Conversion
DO loop
Equality comparison
Inequality comparison

Nag Warnings
Explicitly imported into
Unused dummy variable
Unused local variable
DO loop has zero iteration
Unused PARAMETER
Initialised but never used
Character data-value padded
Array section passed to old style dummy
Expression in IF construct is constant
Initialisation expression for
Expression in logical IF is constant
Set but never referenced

icepack_in default

Set icepack_in default values as in cice v5.1.2 ice_in and adjust -s options as needed for code coverage.

figure and table labels in pdf

Clean up labels for figures and tables, and make sure they are numbered in order. This appears to be an issue only in the pdf version, not in the html.

test configurations, forcing

We need a description of each of the forcing data sets, and the 'test configurations' section needs to be updated or removed from the User Guide (section 1, Implementation). This section generally needs more information and/or updating.

Remove yellowstone from machines.

I will assign myself to this one.

NBTRCR undefined

The NAG compiler reports that NBTRCR is undefined in line 297 of ice_zbgc.f90
(issue noted by DMI collaborators)

Array out of bounds failure

When running the test suite on Onyx (a Cray XC40/50 using Intel), the model fails to run to completion for case onyx_smoke_col_1x1_alt01_debug_run1year. The relevant portion of the runlog file is:

 WARNING - Zero-layer thermodynamics
  Initial forcing data year =         2015
  Final   forcing data year =         2015
forrtl: severe (408): fort: (2): Subscript #1 of the array ISWABS has value 2 which is 
greater than the upper bound of 1

Image              PC                Routine            Line        Source
icepack            000000002042E836  Unknown               Unknown  Unknown
icepack            0000000020229DC0  icepack_shortwave         653  icepack_shortwave.F90
icepack            0000000020227E2D  icepack_shortwave         257  icepack_shortwave.F90
icepack            00000000202A8662  icepack_shortwave        4134  icepack_shortwave.F90
icepack            0000000020086929  icedrv_init_colum         189  icedrv_init_column.F90
icepack            00000000200013F3  icedrv_initmod_mp         108  icedrv_InitMod.F90
icepack            0000000020002037  MAIN__                     40  icedrv_MAIN.F90
icepack            000000002000120E  Unknown               Unknown  Unknown
icepack            00000000204F27F0  Unknown               Unknown  Unknown
icepack            00000000200010F7  Unknown               Unknown  Unknown

icepack.setup checks

Add a check that --report is only available with --suite.

Remove CHECK notation in User's guide

Find and test instances of "CHECK" notation throughout the documentation (User's guide in particular) and verify we have the information we need for these sections.