-
Notifications
You must be signed in to change notification settings - Fork 139
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Clean up implementation following cgridDEV merge #719
Conversation
in CICE-Consortium#715 Refactor capping namelist, added capping_method variable string for namelist, options are "hibler" or "kreyscher" equivalent to 1 and 0 in old capping namelist. capping variable still exists and is set depending on capping_method setting. Update ice_in and other set_nml options. Add box2001 option for ice_data_conc. Does same things at p5, but makes decreases confusion in box2001 usage. Updated set_nml options. Remove support for ice_data_type = smallblock, bigblock, easthalf. Remove a couple tests that were using these. Capitalize last letter in variables dxt, dyt, dxe, dye, dxn, dyn, dxu, dyu -> dxT, dyT, dxE, dyE, dxN, dyN, dxU, dyU. This is the general direction we want to go, but this was done to make "dyn" look less like "dynamics". Modify some comment indentation in ice_init.F90 Reorder grid_average_X2Y calls in ice_dyn_evp.F90 for readability Remove older code in ice_dyn_shared.F90 referencing dte, dte2T, tdamp2, etc. Clean up comments in ice_history_shared.F90, ice_dyn_evp.F90, ice_transport_driver.F90, ice_flux.F90, ice_forcing.F90, ice_grid.F90 Remove commented out code in ice_dyn_shared.F90 around computation of iceumask Remove commented out code in ice_forcing.F90 related to box2001_data_ocn Update documentation. capping_method namelist table was reviewed and updated for consistency with code ice_ic change from default to internal dxt -> dxT, etc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great. I only have one change request, which unfortunately does impact several files. I hope that you wrote a script to make all of those capitalization changes at once! Thank you.
@@ -505,7 +505,7 @@ where | |||
\Delta = \left[D_D^2 + {e_f^2\over e_g^4}\left(D_T^2 + D_S^2\right)\right]^{1/2}. | |||
:label: Delta | |||
|
|||
When the deformation :math:`\Delta` tends toward zero, the viscosities tend toward infinity. To avoid this issue, :math:`\Delta` needs to be limited and is replaced by :math:`\Delta^*` in equation :eq:`zeta`. Two methods for limiting :math:`\Delta` (or for capping the viscosities) are available in the code. If the namelist parameter ``capping`` is set to 1., :math:`\Delta^*=max(\Delta, \Delta_{min})` :cite:`Hibler79` while with ``capping`` set to 0., the smoother formulation :math:`\Delta^*=(\Delta + \Delta_{min})` of :cite:`Kreyscher00` is used. | |||
When the deformation :math:`\Delta` tends toward zero, the viscosities tend toward infinity. To avoid this issue, :math:`\Delta` needs to be limited and is replaced by :math:`\Delta^*` in equation :eq:`zeta`. Two methods for limiting :math:`\Delta` (or for capping the viscosities) are available in the code. If the namelist parameter ``capping_method`` is set to ``hibler``, :math:`\Delta^*=max(\Delta, \Delta_{min})` :cite:`Hibler79` while with ``capping_method`` set to ``kreyscher``, the smoother formulation :math:`\Delta^*=(\Delta + \Delta_{min})` of :cite:`Kreyscher00` is used. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use the labels 'max' or 'sum' for capping_method instead of people's names? It's more intuitive, and I've always tried to avoid naming things after individual people except when it's unavoidable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Happy to make that change, not a problem. What strings do we want to use instead? @JFLemieux73 @eclare108213 Just let me know and I'll make the change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest 'max' instead of Hibler and 'sum' instead of Kreyscher
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed the implementation to "max" and "sum". I also added some tests (alt07) to test capping="sum" and visc_method="avg_zeta" and a different ndte. We can continue to update alt07 as we work towards better coverage of the dynamics options.
@eclare108213, just waiting for your review approval. thanks. |
* Clean up implementation following cgridDEV merge, based on comments in CICE-Consortium#715 Refactor capping namelist, added capping_method variable string for namelist, options are "hibler" or "kreyscher" equivalent to 1 and 0 in old capping namelist. capping variable still exists and is set depending on capping_method setting. Update ice_in and other set_nml options. Add box2001 option for ice_data_conc. Does same things at p5, but makes decreases confusion in box2001 usage. Updated set_nml options. Remove support for ice_data_type = smallblock, bigblock, easthalf. Remove a couple tests that were using these. Capitalize last letter in variables dxt, dyt, dxe, dye, dxn, dyn, dxu, dyu -> dxT, dyT, dxE, dyE, dxN, dyN, dxU, dyU. This is the general direction we want to go, but this was done to make "dyn" look less like "dynamics". Modify some comment indentation in ice_init.F90 Reorder grid_average_X2Y calls in ice_dyn_evp.F90 for readability Remove older code in ice_dyn_shared.F90 referencing dte, dte2T, tdamp2, etc. Clean up comments in ice_history_shared.F90, ice_dyn_evp.F90, ice_transport_driver.F90, ice_flux.F90, ice_forcing.F90, ice_grid.F90 Remove commented out code in ice_dyn_shared.F90 around computation of iceumask Remove commented out code in ice_forcing.F90 related to box2001_data_ocn Update documentation. capping_method namelist table was reviewed and updated for consistency with code ice_ic change from default to internal dxt -> dxT, etc * update andacc namelist options, comment out for now * switch capping_method options to max and sum * Add alt07 tests for new dynamics namelist options
PR checklist
Short (1 sentence) summary of your PR:
Clean up implementation following cgridDEV merge, based on comments in Merge cgridDEV branch including C grid implementation and other fixes #715
Developer(s):
apcraig, eclare108213
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.
All bit-for-bit and still working as before, Tested on Onyx with 3 compilers, https://github.com/CICE-Consortium/Test-Results/wiki/cice_by_hash_forks#078aab4844983ccbb9c9fddc1a70e060e1d99a34
How much do the PR code changes differ from the unmodified code?
Does this PR create or have dependencies on Icepack or any other models?
Does this PR add any new test cases?
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/. A test build of the technical docs will be performed as part of the PR testing.)
Please provide any additional information or relevant details below:
Refactor capping namelist, added capping_method variable string for namelist,
options are "hibler" or "kreyscher" equivalent to 1 and 0 in old
capping namelist. capping variable still exists and is set
depending on capping_method setting. Update ice_in and other set_nml options.
Add box2001 option for ice_data_conc. Does same things at p5, but makes
decreases confusion in box2001 usage. Updated set_nml options.
Remove support for ice_data_type = smallblock, bigblock, easthalf. Remove
a couple tests that were using these.
Capitalize last letter in variables dxt, dyt, dxe, dye, dxn, dyn, dxu, dyu ->
dxT, dyT, dxE, dyE, dxN, dyN, dxU, dyU. This is the general direction we
want to go, but this was done to make "dyn" look less like "dynamics".
Modify some comment indentation in ice_init.F90
Reorder grid_average_X2Y calls in ice_dyn_evp.F90 for readability
Remove older code in ice_dyn_shared.F90 referencing dte, dte2T, tdamp2, etc.
Clean up comments in ice_history_shared.F90, ice_dyn_evp.F90, ice_transport_driver.F90,
ice_flux.F90, ice_forcing.F90, ice_grid.F90
Remove commented out code in ice_dyn_shared.F90 around computation of iceumask
Remove commented out code in ice_forcing.F90 related to box2001_data_ocn
Update documentation.