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

Add dorebin and docleanup optional arguments to icepack_step_ridge #503

Merged
merged 3 commits into from
Oct 24, 2024

Conversation

apcraig
Copy link
Contributor

@apcraig apcraig commented Oct 2, 2024

PR checklist

  • Short (1 sentence) summary of your PR:
    Add dorebin and docleanup optional arguments to icepack_step_ridge
  • Developer(s):
    kshedstrom, 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.
    Tested Icepack and CICE on Derecho without the optional arguments, with the optional arguments set to true and with the optional arguments set to false. Icepack remained bit-for-bit in all cases (probably too few gridcells), CICE changed answers when the optional arguments were set to false. Otherwise results are bit-for-bit, but will do more testing before merging. Ran full test suites on Derecho with intel and gnu for Icepack and CICE just before merging, results look fine.
  • How much do the PR code changes differ from the unmodified code?
    • bit for bit
    • 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 document the changes in detail, including why the changes are made. This will become part of the PR commit log.

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.

Based on https://github.com/ESMG/Icepack/tree/optional_cleanup

Update interface document

@apcraig
Copy link
Contributor Author

apcraig commented Oct 2, 2024

This is a draft PR based on https://github.com/ESMG/Icepack/tree/optional_cleanup. See #478.

Do we need new namelist (in Icepack and CICE) to allow users to turn the features on and off or will this just be hardcoded in the calls when needed?

I am totally open to using different argument names if anyone wants something different.

@eclare108213
Copy link
Contributor

Do we need new namelist (in Icepack and CICE) to allow users to turn the features on and off or will this just be hardcoded in the calls when needed?

Since we generally would not recommend these flags be set to false, I do not think they should be exposed in namelist.

@eclare108213
Copy link
Contributor

Since it's pertinent, please also remove the reference to a boundary call mentioned here. It's around line 742 in ice_itd.F90. I looked in the old svn repository -- that comment is ANCIENT (more than 20 years).

@kshedstrom
Copy link

I don't understand it, but I can reproduce my old answers if I add yet a third optional argument to icepack_step_ridge. If the third one is false, I call ridge_ice without the opening and closing arguments, forcing it to call ridge_prep. I thought I was doing all the same computations on the SIS2 side, but something is still different. If chinook ever comes back, I'll have a better shot at debugging it.

@kshedstrom
Copy link

I figured it out - it's all in the order of operations. I approve this PR.

@apcraig
Copy link
Contributor Author

apcraig commented Oct 17, 2024

@kshedstrom Great to hear you figured out what's going on. So do you need both of these optional arguments to make things work? Do you understand why?

@kshedstrom
Copy link

No, I only need the one, but I can get three different answers by having both. I haven't talked to the GFDL folks about this and maybe down the road we'll be able to pick which answer we want, but so far we've only tested it in long runs without any itd_cleanup at all.

if (present(dorebin)) then
ldorebin = dorebin
else
ldorebin = .true.
Copy link
Contributor

Choose a reason for hiding this comment

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

ldorebin does not appear to be used in this subroutine

@kshedstrom
Copy link

It is passed through to cleanup_itd.

Copy link
Contributor

@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.

Ah, now I see ldorebin in the cleanup_itd argument list. Thanks for the clarification. I'm still a bit hesitant to approve these changes without understanding what it is about the driver that requires them. I'm worried about nonphysical results that might not be noticed or accounted for. But I will approve, since these changes seem important for the GFDL model, and because these options are buried in the code where others are less likely to thoughtlessly experiment with them. If they turn out to be helpful for other groups, then we can expose them in namelist. I highly recommend the GFDL model's developers look closely at how their model handles ice thicknesses that lie outside of their thickness bins, especially considering @kshedstrom's comment that

I only need the one, but I can get three different answers by having both.

@apcraig
Copy link
Contributor Author

apcraig commented Oct 23, 2024

@kshedstrom @eclare108213 I am a little concerned that we don't fully understand why we need these changes and whether the science results are valid. I'm willing to merge this mostly because the feature is pretty well hidden, although the new arguments will appear in our Icepack interface documentation. @kshedstrom please confirm again that you want these changes as currently implemented in the PR to be merged and that you will continue to validate and report back any results to us. Thanks.

@kshedstrom
Copy link

Yes, this is indeed what I want, at least for now. In the MOM6 world having the ability to reproduce old answers can be important and we need one flag for that. I will let you know about the rest as I learn more.

@apcraig apcraig marked this pull request as ready for review October 24, 2024 19:03
@apcraig
Copy link
Contributor Author

apcraig commented Oct 24, 2024

I ran full test suites on Derecho with intel and gnu for Icepack and CICE, tests look good.

@apcraig apcraig merged commit 6da5668 into CICE-Consortium:main Oct 24, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ridge_ice no longer public
3 participants