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

Modifications for bit-for-bit with current main in CICE #14

Conversation

apcraig
Copy link
Collaborator

@apcraig apcraig commented Oct 25, 2022

PR checklist

  • Short (1 sentence) summary of your PR:
    Modifications for bit-for-bit with current main in CICE

  • 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.
    This recovers bit-for-bit in CICE, will need CICE mods, still testing

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

  • Add "_old" tfrz_options to recover bit-for-bit for some cases

  • Move hi_min namelist from forcing_nml to tracer_nml

- Add "_old" tfrz_options to recover bit-for-bit for some cases
- Move hi_min namelist from forcing_nml to tracer_nml
@apcraig apcraig requested a review from eclare108213 October 25, 2022 17:38
@apcraig
Copy link
Collaborator Author

apcraig commented Oct 25, 2022

@dabail10 @eclare108213

I know this looks odd, but it's just temporary to recover bit-for-bit in CICE. I also need to commit some changes to the tfrz branch in CICE. We will deprecate the "_old" options at some point, but I think we need this to preserve testing for now. We can discuss more when we have a chance, but I'd like to fix this today or tomorrow and be able to test the heads of the branches again just to be sure.

Please merge this when you have a chance, so I can update CICE. There may be another Icepack PRs in the next day or so if I find other issues, but I think we're OK for the moment. Lets merge this first.

tfrz_option == "linear_salt_old" .or. &
tfrz_option == "constant_old" .or. &
tfrz_option == "minus1p8_old") then
trcrn(it) = Tocnfrz ! surface temperature
Copy link
Collaborator

Choose a reason for hiding this comment

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

Okay, but can we fix this fairly quickly? E.g. do the testing now to confirm that this indeed is what changes answers, then fix it and start using updated output for testing going forward? Maybe there are a few steps along the way, but hopefully only a few.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We can do that. But my concern is that we'll start to diverge from the results on Consortium Main. We are still bit-for-bit for all tests. If we diverge, it's going to be hard to confirm that when we sync up the Consortium and E3SM branches, that everything is working properly and we got all the merged code correctly. What we could also think about doing is modify Consortium Main so it gets the Tf modifications. Or we can let them diverge.

@eclare108213 eclare108213 merged commit 6a4533b into E3SM-Project:cice-consortium/E3SM-icepack-initial-integration Oct 25, 2022
@eclare108213
Copy link
Collaborator

I see. It's important to not let them diverge. I'd be inclined to pull over the Tf modifications, although that's a bit more work!

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