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 saltflux option. #418

Merged
merged 13 commits into from
Dec 6, 2022
Merged

Add saltflux option. #418

merged 13 commits into from
Dec 6, 2022

Conversation

dabail10
Copy link
Contributor

@dabail10 dabail10 commented Dec 1, 2022

For detailed information about submitting Pull Requests (PRs) to the CICE-Consortium,
please refer to: https://github.com/CICE-Consortium/About-Us/wiki/Resource-Index#information-for-developers

PR checklist

  • Short (1 sentence) summary of your PR:
    This adds the option to do a "constant" salinity reference for the salt and fresh water fluxes versus a 'prognostic' option that uses the mushy-layer prognostic salinity.
  • Developer(s):
    dabail10 (D. Bailey)
  • 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.
    Bfb with aux_cice suite in CESM.
    https://github.com/CICE-Consortium/Test-Results/wiki/icepack_by_hash_forks#b3b48bf7a7b27c6cf4d89261151bbee0f15a525d
  • 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:
    There is an associated CICE tag to go with this.

@apcraig
Copy link
Contributor

apcraig commented Dec 2, 2022

Please do standalone Icepack testing including regression testing.

@dabail10
Copy link
Contributor Author

dabail10 commented Dec 5, 2022

I did the Icepack regression testing with intel, pgi, and gnu on cheyenne. Everything is bfb.

Copy link
Contributor

@apcraig apcraig left a comment

Choose a reason for hiding this comment

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

Looks good. Do we need to add a test for saltflux_option = prognostic in Icepack and/or CICE?

@dabail10
Copy link
Contributor Author

dabail10 commented Dec 5, 2022

I agree. I should probably add tests for both.

@apcraig
Copy link
Contributor

apcraig commented Dec 5, 2022

Sounds good. Maybe we can merge the Icepack PR today and the CICE PR in the next day or two then I can start testing for the release. Thanks!

@apcraig
Copy link
Contributor

apcraig commented Dec 5, 2022

How about if we call the salt test "saltprog" instead of just "salt"? Does the new test pass?

@dabail10
Copy link
Contributor Author

dabail10 commented Dec 5, 2022

If we are doing a longer name I would prefer saltflux.

@dabail10
Copy link
Contributor Author

dabail10 commented Dec 5, 2022

The test passes in CICE with the updated Icepack submodule.

@apcraig
Copy link
Contributor

apcraig commented Dec 5, 2022

saltflux is OK too. Or saltfluxprog. Can you also do a quick run with standalone Icepack for the new test. Lets just make sure it runs and passes. Then we can merge. Thanks!

@dabail10
Copy link
Contributor Author

dabail10 commented Dec 5, 2022

Did quick_suite on intel.

./icepack.setup --suite quick_suite --mach cheyenne --env intel --testid saltflux4 -s saltflux

10 measured results of 10 total results
10 of 10 tests PASSED
0 of 10 tests PENDING
0 of 10 tests MISSING data
0 of 10 tests FAILED

@apcraig
Copy link
Contributor

apcraig commented Dec 5, 2022

The new test is in the base_suite.

@apcraig
Copy link
Contributor

apcraig commented Dec 5, 2022

OK, I see you ran quick_suite with -s saltflux. I guess that's OK too.

@apcraig
Copy link
Contributor

apcraig commented Dec 6, 2022

You need to add set_nml.saltflux to your push.

@dabail10
Copy link
Contributor Author

dabail10 commented Dec 6, 2022

Whoops. Should be there now.

@apcraig apcraig merged commit 8637a2d into CICE-Consortium:main Dec 6, 2022
@dabail10 dabail10 deleted the saltflux_option2 branch December 6, 2022 18:01
@dabail10 dabail10 mentioned this pull request Jan 6, 2023
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