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 wave-ice coupling to nuopc/cmeps driver #782

Merged
merged 133 commits into from
Nov 8, 2022

Conversation

DeniseWorthen
Copy link
Contributor

@DeniseWorthen DeniseWorthen commented Nov 2, 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:

Adds wave-ice coupling capability to nuopc/cmeps driver

  • Developer(s):
    I believe the original changes were obtained from @lettie-roach during development of the new WW3 NUOPC/CMEPS cap.

  • Suggest PR reviewers from list in the column to the right. @dabail10 @lettie-roach @apcraig

  • Please copy the PR test results link or provide a summary of testing completed below.

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

    • bit for bit wave-ice coupling is optional and has no impact unless enabled
    • different at roundoff level
    • more substantial
  • Does this PR create or have dependencies on Icepack or any other models?

    • Yes
    • No
  • Does this PR update the Icepack submodule? If so, the Icepack submodule must point to a hash on Icepack's main branch.

    • 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/. A test build of the technical docs will be performed as part of the PR testing.)

    • Yes
    • No, does the documentation need to be updated at a later time?
      • Yes
      • No
  • Please provide any additional information or relevant details below:

  • The initialization of aicen and aicen_init in ice_state.F90 are required in order to run in debug mode when the intel compile option-init=snan,arrays is used.

  • At the time this was committed to the NOAA-EMC fork, a simple comparison of ice thickness and concentration with and w/o wave-ice coupling enabled was made and the results were reasonable and in-line with expected behaviour (see figure in Update CICE6 to allow WW3-CICE6 coupling; add a cdeps test using C-grid for sea ice dynamics (was #1390) ufs-community/ufs-weather-model#1381)

DeniseWorthen and others added 30 commits February 25, 2020 08:43
* Isotopes for CICE (CICE-Consortium#423)

Co-authored-by: apcraig <anthony.p.craig@gmail.com>
Co-authored-by: David Bailey <dbailey@ucar.edu>
Co-authored-by: Elizabeth Hunke <eclare@lanl.gov>
Update CICE for coupling with UFS
changes to satisfy ufsatm and cesm requirements for pot temp and density from atm
* Add atmiter_conv to CICE

* Add documentation

* trigger build the docs

Co-authored-by: David A. Bailey <dbailey@ucar.edu>
@DeniseWorthen
Copy link
Contributor Author

DeniseWorthen commented Nov 2, 2022

@lettie-roach Seeing the changes in PR #775 related to FSD history variables, is the use of aicen_init for the calculation of floe_diameter in ice_import_export correct?

@dabail10
Copy link
Contributor

dabail10 commented Nov 2, 2022

This looks good. I will have to merge this into CESM and test it out. A few of these changes were already made in our cap.

@dabail10
Copy link
Contributor

dabail10 commented Nov 2, 2022

We might need a squash merge here. Lots of history.

@@ -192,6 +192,8 @@ subroutine alloc_state
n_trcr_strata = 0
nt_strata = 0
trcr_base = c0
aicen = c0
aicen_init = c0
Copy link
Contributor

Choose a reason for hiding this comment

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

For completeness, we should probably add vsnon/vsnon_init and vicen/vicen_init.

Copy link
Contributor Author

@DeniseWorthen DeniseWorthen Nov 2, 2022

Choose a reason for hiding this comment

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

I can add these initializations. I had meant to test whether they might have fixed a similar issue I had when I looked at the tr_snow updates which were made (?) earlier this year.

@@ -25,6 +25,7 @@ module ice_comp_nuopc
use ice_calendar , only : force_restart_now, write_ic
use ice_calendar , only : idate, mday, mmonth, myear, year_init
use ice_calendar , only : msec, dt, calendar, calendar_type, nextsw_cday, istep
use ice_calendar , only : ice_calendar_noleap, ice_calendar_gregorian
Copy link
Contributor

Choose a reason for hiding this comment

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

We had made this change in our cap already.

' is overwriting tfrz_option from cice namelist '//trim(tfrz_option)
write(nu_diag,*) trim(errmsg)
call icepack_warnings_flush(nu_diag)
call icepack_init_parameters(tfrz_option_in=tfrz_option_driver)
endif
Copy link
Contributor

Choose a reason for hiding this comment

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

We also have this in our version.

@apcraig
Copy link
Contributor

apcraig commented Nov 2, 2022

Default is to squash merge. I think it's a good way to work on main. Each PR looks like a single commit. There are downsides, but minor.

@DeniseWorthen
Copy link
Contributor Author

@dabail10 For testing wave-ice coupling, for completeness you should also take a look at this bug report (NOAA-EMC/WW3#843). I have a fix going into the WW3 cap today or tomorrow, but I don't know when that change might make it to ESCOMP.

* initialize vsnon/vsnon_init and vicen/vicen_init
@eclare108213
Copy link
Contributor

@DeniseWorthen
The figures at ufs-community/ufs-weather-model#1381) look great! I see that the test runs used the floe size distribution. Rather than comment there, let me ask a couple of questions here (where everyone knows me):
Were the waves only impacting the sea ice or were sea ice changes feeding back on the waves?
What is the spatial resolution? Was that using C-grid in CICE?

@DeniseWorthen
Copy link
Contributor Author

@eclare108213 The wave model sends the wave_elevation_spectrum to CICE6; CICE6 sends the floediam and ice thickness back to WW3. WW3 uses those variables, but I'm not well-versed enough in wave-ice coupling to say exactly how they interact w/ the wave field.

The test case I ran used FV3 at C96 resolution and MOM6/CICE6 and WW3 all running on the 1-deg tripole grid. CICE6 was not using the C-grid.

@dabail10 dabail10 mentioned this pull request Nov 4, 2022
16 tasks
@dabail10
Copy link
Contributor

dabail10 commented Nov 7, 2022

Sorry to take so long. I had to manually put the changes into my version of the CAP. I will issue a PR for my changes later. I did the aux_cice testsuite and this was bfb with cheyenne intel against my baselines. I will approve this and then merge into my code.

@apcraig
Copy link
Contributor

apcraig commented Nov 7, 2022

@dabail10 @DeniseWorthen, OK to merge, right?

@dabail10
Copy link
Contributor

dabail10 commented Nov 7, 2022

Good to go.

@apcraig apcraig merged commit 251ca48 into CICE-Consortium:main Nov 8, 2022
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.

5 participants