Skip to content
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

Minor updates consistent with E3SM columnphysics modifications #10

Conversation

apcraig
Copy link
Collaborator

@apcraig apcraig commented Oct 8, 2022

PR checklist

  • Short (1 sentence) summary of your PR:
    Minor updates consistent with E3SM columnphysics modifications

  • Developer(s):
    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.
    bit-for-bit Icepack test suite on 3 compilers on cheyenne

  • How much do the PR code changes differ from the unmodified code?

    • bit for bit (but need to be careful due to change in how hi_min is set)
    • different at roundoff level
    • more substantial
  • Does this PR create or have dependencies on CICE or any other models?

    • Yes
    • No
  • Does this PR add any new test cases?

    • Yes
    • 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 provide any additional information or relevant details below:

  • Migrate hi_min to namelist+parameters and refactor implementation. In the current implementation, hi_min is set by namelist. Historically, this has been hardcoded in the model with a the value set to .01 except when kcatbound=0 and kitd/=1 where it was set to 0.1 (except if the CESMCOUPLED CPP is turned on). Users should be aware that it now needs to be set in namelist if the default value, 0.01 should be changed. In testing, when kcatbound=0 and kitd/=1, we set hi_min to 0.1 to reproduce prior results bit-for-bit and confirm this works.

  • Reduce CESMCOUPLED CPP use, now just for orbital stuff, punch list 23

  • Modify initialization of hin_max, punch list 28

  • Fix possible divide by zero, punch list 65

  • Rename icepack shr_orb_params and shr_orb_decl to icepack_orb_params, icepack_orb_decl

  • Cleanup argument list for compute_ponds_lvl, compute_ponds_cesm

- Migrate hi_min to namelist+parameters and refactor implementation
- Reduce CESMCOUPLED CPP use, now just for orbital stuff, punch list 23
- Modify initialization of hin_max, punch list 28
- Fix possible divide by zero, punch list 65
- Rename icepack shr_orb_params and shr_orb_decl to icepack_orb_params, icepack_orb_decl
- Cleanup argument list for compute_ponds_lvl, compute_ponds_cesm
Copy link
Collaborator

@eclare108213 eclare108213 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good, thanks for all the cleanup.
One thing that is a little different now is the value of hi_min for kitd/=1 and not CESMCOUPLED. This can be set in namelist so I don't think it needs to be noted in the official documentation, but it would be helpful to explain this change explicitly in the main PR comments, since the former code logic is a little obscure. (And if it was noted in the official documentation, remove it!)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants