Merge CICE-Consortium/Icepack main to E3SM-Project/Icepack main #35
Conversation
Co-authored-by: David Bailey <[email protected]> Co-authored-by: Elizabeth Hunke <[email protected]>
* Add .readthedocs.yaml Copy CICE's readthedocs configuration as of CICE-Consortium/CICE@06282a53 (Update version to 6.4.2 (#864), 2023-09-08). * doc: mention code variables relating to 'calc_dragio' At the end of the "Ocean" section, we mention that 'dragio' can be computed from 'iceruf_ocn' and 'thickness_ocn_layer1', but do not mention these code variables, nor the 'calc_dragio' setting used to activate that computation. Mention the code variables next to their mathematical symbol, for more clarity.
…ters (#465) All parameters of icepack_parameters::icepack_query_parameters are intent(out), so it does not make sense to recompute constants at the end of that subroutine since no constants are changed. This superfluous call dates back to the introduction of icepack_query_parameters (then called icepack_query_constants) in 7646f2a (Migrate icepack_constants out of the columnphysics/constants directory..., 2017-10-04). Remove that call.
* Remove older "_old" tfrz_options, added for backwards compatibility Modify tfrz_option logic in icepack_sea_freezing_temperature to trap unsupported values * Update Icepack set_nml settings, remove _old from tfrz_option
* Clarify documentation associated with namelist inputs Alphabetize cpl_frazil namelist documentation * Update documentation
* Update 5-band dEdd to support test data Add an interpolation method in icepack_shortwave.F90 for rsnw Refactor portions of icepack_shortwave to improve robustness Add snicar and snicartest test cases * Update the 5-band snicar shortwave rsnw_snicar_tab interpolation This changes answers but QC tests pass. The new implementation checks whether the rsnw_snicar_tab is an array of integer values with deltas of value 1 (and sets the rsnw_snicar_chk variable). If so, it uses a shortcut to find the rsnw_snicar_tab bounds for interpolation, otherwise it calls the shortwave_search routine. In testing, the shortcut did not change performance much, but that could change in the future. * Clean up debug output * Refactor the logic to control rsnw table interpolation in the shortwave. Set a character string, rsnw_datatype, at initialization.
…ation (#476) * correct ks units, clean up comments, big fix for BL99 enthalpy calculation * make it pretty
Update pull request template to ask for additional information about the PR. This information can and will be used during the squash merge process to produce more useful commit logs. Update Macros.conda_macos to remove ffpe-trap=invalid from debug flags. This flag causes nf90_create to fail with the latest version of the conda environment. This happens when debug flags are on and a test with netcdf is turned on. I believe this is a compiler error.
* Add gnuplot to the conda environment This fix allows the timeseries.csh script to be used for Icepack standalone runs.
|
I am testing this using the E3SM-Polar-Developer script. |
|
Running QC test on the following directories: |
|
Baseline E3SM: E3SM with updated Icepack |
|
Here are the stats comparing the 5-year QC runs. |
There was a problem hiding this comment.
We have worked further on the high resolution version of E3SM and determined that the model is also crashing with a data sea ice model, thereby eliminating anything in this PR as a potential cause of model instability. In consultation with the HR team and with @eclare108213, I would like to suggest we close this PR so as not to complicate @njeffery 's work that would automatically see this change brought into E3SM as part of the BGC tag.
|
The contents of this PR will likely NOT be brought in with @njeffery's BGC mods. I think it's best to keep them separate, since they are both somewhat large collections of changes and their intent is different. The argument for not merging this PR yet is so that it's easier to compare Nicole's code / simulations to existing fully coupled (water cycle) runs. If the control runs will not be BFB due to other changes/bug fixes, then I we could go ahead with this PR. Then @njeffery would need to rebase and retest -- that's extra work that we'd prefer to avoid. Are non-BFB changes going into master now? |
|
Also, there really isn't any risk/cost to merge this. If the plan is to keep the E3SM-Project/Icepack main synced up with the Consortium/Icepack main, this PR is just doing that. We could probably create a github action that would do it automatically once a week and it would have no impact on any other development or plans. Merging this PR has no impact on the bgc branch or whether bgc is updated to the current main. That is a totally independent step. It also has no impact on the Icepack hash that E3SM points to. That hash can be on any branch. At some points, it could be a hash from main, but never has to be. All that assumes the plan is for the E3SM-Project/Icepack main to match the Consortium/Icepack main exactly always and for that update to happen periodically. |
|
That's true, E3SM will not point to this new hash. But I think this branch (main) is where we had planned to force-push changes from the Consortium in order to keep the history clean, so I don't think we would actually merge this PR anyhow. Mostly I don't want to confuse the order in which things happen, relative to the BGC merge. If we're going to make these changes in E3SM first, then let's do it following the established process, which I think means force-pushing to E3SM Icepack main and then opening another PR into E3SM itself, linking the new Icepack hash, and moving the documentation above into that PR. But that's harder for Nicole to deal with, so I'm waiting. It's good to know that my tests above succeeded cleanly. I do not know whether Rob et al will want us to update the Icepack link in E3SM separately for the resync and for the BGC, or if we could PR them together - my guess is that they'll need to be done separately. |
|
I'm not sure I understand the plan. The current E3SM Icepack main is up to date with Consortium from a few months ago. I think we forced pushed then, so the history is identical, just E3SM is a bit behind. This PR just does this push from Consortium main to E3SM main, we don't need to force push, just don't squash merge and the mains will remain synced up with identical history. That is taken care of with this PR. As I said, we could probably automate this merge weekly if we wanted, don't need to force push as long as nobody pushed onto E3SM main when they shouldn't. For bgc, we may want or have to fix the history, but that would NOT have to happen from the head of main and what's on main wouldn't matter. We would find the appropriate hash to branch from on main (or elsewhere), create a branch from it, then merge the bgc changes onto it. When bgc is ready, the hash on that branch would be used in E3SM and then could be merged to Consortium main. We cannot and should not require the head of main in E3SM to be somehow associated with some current version of Icepack that's useful in E3SM. There may be hashes on main that are particularly useful wrt E3SM development, we can know/discover what those are quite easily. But the head of main should not be part of the equation of that. We should never be force pushing again in general. Only if someone commits something to main that shouldn't be there. We should probably clarify the process again, seems like there is some confusion. |
|
Okay, we need to sort through this. It's is already on the agenda for the Icepack meeting tomorrow: https://acme-climate.atlassian.net/wiki/spaces/ICE/pages/4254990453/2024-04-17+Icepack+Merge+Meeting+notes |
* add tutorial documentation * add tutorial * add tutorial * add tutorial * add tutorial * add tutorial * Minor clarifications and grammatical updates. --------- Co-authored-by: Elizabeth Hunke <[email protected]>
Update Copyright to 2024 in code and LICENSE file
Add run_testoutput script to generate release plots for Icepack
Documentation is being update based on the Consortium workshop in 2024. The "adding a tracer" section has been updated as has the tutorial section related to adding a fluff tracer. An example set of code diffs for the tutorial is also includes as reference.
Port to NCAR's cluster, casper using the HTC nodes using standard cpus. This will support testing and development on another piece of hardware other than derecho and izumi and provides a good option for smaller test cases. Added intel, inteloneapi, and gnu compiler options. The -check debug option is very sensitive in this version of Inteloneapi, so it has been turned off.
Add congel_freeze namelist and add 'one-step' option to the default 'two-step' option. Namelist flag congel_freeze chooses which formulation to use. The original is ‘two-step’, since only the mushy boundary moves in the first step and the phase change happens in the next step. Plante et al. (‘one-step’) moves the boundary and performs the phase change in a single time step. Maintain 'two-step' as default for now.
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 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. --------- Co-authored-by: Elizabeth Hunke <[email protected]> Co-authored-by: David Bailey <[email protected]> Co-authored-by: Nicole Jeffery <[email protected]>
Update inteloneapi, cray, intel, nvhpc modules/compiler. Changes answers for inteloneapi and cray. Switch default Derecho queue from main to develop to support quicker turn around and lower costs. Develop is a shared queue. Error in the snow physics with the cray compiler needs further assessment, but suggest we merge this now as is. That error does not present itself in CICE with snow physics with the same cray compiler.
* Update Derecho Cray, rename subroutines, minor cleanup Update Derecho Cray, add -Rp to -O2 standard optimization to address SAME_TBS compiler bug that is triggered in limited cases. Reported issue to NCAR. -O2 -Rp changes answers relative to -O2. Rename two public Icepack subroutines, but maintain backward compatibility. Update the calls in Icepack driver, but also confirmed it works fine with the old names as well as in CICE with the old names. Closes #255 use icepack_therm_shared , only: icepack_init_thermo => icepack_init_salinity use icepack_therm_shared , only: icepack_init_trcr => icepack_init_enthalpy Add a check and abort for negative values in the sqrt in computation of Tin in function calculate_Tin_from_qin. Closes #482 Refactor calls to icepack_aggregate to make them consistent. This was part of the testing for the Derecho Cray bug, and decided to keep the implementation. Update comments associated with floeshape constant attribution. Change from Steele to Rothrock 1984. Closes #479 Clean up some of the variable declarations in subroutine set_state_var and module icedrv_state, merge multiple lines to one line and shift to assumed shape arrays where appropriate. Clean up implementation error in icedrv_restart.F90, subroutine restartfile. This subroutine was using a parameter, ntrcr, directly from icepack_tracers. Switched that to a call to icepack_query_tracer_sizes. Update documentation of kice noting it's use with BL99 and MU71. See #447. Generate updated interface documentation (./icepack.setup --docintfc)
) Add dorebin and docleanup optional arguments to icepack_step_ridge to support NOT calling cleanup_itd and NOT calling rebin in the ridge_ice. Closes #478 Rename limit_aice_in argument in ridge_ice to limit_aice. This argument is not used in Icepack or CICE at the moment. Update interface document Based on https://github.com/ESMG/Icepack/tree/optional_cleanup
This is a bug fix plus some rearranging for conservation in CICE. This impacts FSD cases as well as non-FSD cases.
There are two main aspects to the bug fix:
Initial ice and snow volume values at the beginning of the lateral melt routine are now used for updating the snow and ice enthalpy as well other tracers for lateral melting. This is answer changing in all cases!
The other fix is to move the computation of vi0new_lat (lateral growth from FSD) outside of the subroutine fsd_lateral_growth and a check to make sure the category vi0new (vin0new) does not exceed the total vi0new. This is a warning and it recomputes to ensure conservation. This is only answer changing in FSD cases.
There is also a change in definition of floe_area_c, so that it is precisely half way between floe_area_l and floe_area_h. The original computation of floe_area_c based on floe_rad_c biases the area towards the lower value.
Update the fsd subroutine arguments (floe_rad_c, floe_rad_l, floe_binwidth, c_fsd_range) to store static data inside Icepack when computed in Icepack rather than pass them back and forth. Provide an ability to pass out the values from Icepack as optional arguments.
Move the computation of rsiden and get rid of fside which is no longer needed.
Clean up some comments and indentation
Update swccsm3 test, set ktherm=1
Update interface documentation
There is an associated CICE tag to go with this.
This is the icepack part of making both dsnow and dsnown optional. Later I can add some diagnostics for these in CICE.
Update interface documentation Clean up redundant dsnown line in icepack_therm_vertical, see #506.
This is a simple PR created directly from the repositories.