-
Notifications
You must be signed in to change notification settings - Fork 139
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 Cgrid-related fixes for nuopc/cmeps #728
add Cgrid-related fixes for nuopc/cmeps #728
Conversation
* Isotopes for CICE (CICE-Consortium#423) Co-authored-by: apcraig <[email protected]> Co-authored-by: David Bailey <[email protected]> Co-authored-by: Elizabeth Hunke <[email protected]>
Update CICE for coupling with UFS
…l temperature and density are satisfied
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 <[email protected]>
This reverts commit e70d1ab.
Update OpenMP directives as needed including validation via new omp_suite. Fixed OpenMP in dynamics. Refactored eap puny/pi lookups to improve scalar performance Update Tsfc implementation to make sure land blocks don't set Tsfc to freezing temp Update for sea bed stress calculations
* baselines pass with these extra halo updates removed
* compiling in debug mode using -init=snan,arrays requires initialization of variables
There seems to be a bug in the implementation which github Actions is picking up.
Can you fix this and retest please. thanks! |
Also, it would be great if some of the initialization were moved to other parts of the code. The C-grid forcing/history terms should be initialized in cicecore/cicedynB/general/ice_flux.F90:init_history_dyn. An "if (grid_ice == "CD" .or. grid_ice == "C")" can be added there. The iceEmask and iceNmask can stay in ice_dyn_shared.F90 but need to be initialized to false there. |
I checkout out Denise's code and ran an ice-only (D compset) test with grid_ice == 'B'. It is still hanging with threading. I am going to try turning off threading. |
@apcraig Sorry about the sloppy setting of the mask variables as c0. I will fix that. Also, I did originally have the changes in the ice_flux routine; I only moved them because I saw that uvelE/N are initialized in ice_dyn_shared. So, other than the mask variables (logical, .false. on initialization) all of the others should be moved to the history initialization ? |
@DeniseWorthen, no problems. I think the velocity can stay where it is as well. I think we should have the C grid variables initialized, as much as we can, in the same places where the B-grid equivalents are initialized. So init_dyn should have velocities (already there) and iceemask/icenmask near iceumask. I think the other variables go in ice_flux.F90. Let me know if there are questions. Just trying to keep some consistency. thanks! |
This version works with threading off for me! So, there is something likely bad with an OMP loop in the cap. Still hunting. |
@dabail10, I'll wait for your approval to merge. |
I still have some threading issues to sort out, but I don't think we should hold this up. |
I did find a threading bug in ice_import_export.F90 where inst_pres_heigh_lowest needs to be private. I don't think this is the problem though. I wonder though, we don't have the "SCHEDULE" thing in the cap. Does this matter?
|
The SCHEDULE only affects performance, it won't impact usage. It was a big pain to debug the OpenMP when I went thru the exercise earlier this year in CICE. One thing I often did is turn off all OpenMP loops in a suspicious subroutine or file. Then I would turn them on one or a few at a time and see if I could identify which one(s) were causing problems. Then I would focus debugging that OpenMP loop. Let me know if I can help. I will merge this now. |
* replace save_init with step_prep in CICE_RunMod * fixes for cgrid repro * remove added haloupdates * baselines pass with these extra halo updates removed * change F->S for ocean velocities and tilts * fix debug failure when grid_ice=C * compiling in debug mode using -init=snan,arrays requires initialization of variables * respond to review comments * remove inserted whitespace for uvelE,N and vvelE,N Co-authored-by: apcraig <[email protected]> Co-authored-by: David Bailey <[email protected]> Co-authored-by: Elizabeth Hunke <[email protected]> Co-authored-by: Mariana Vertenstein <[email protected]> Co-authored-by: Minsuk Ji <[email protected]> Co-authored-by: Tony Craig <[email protected]> Co-authored-by: Philippe Blain <[email protected]>
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
Updates drivers/nuopc/cmeps for C-grid code changes. See C-grid coupled validation #721
@DeniseWorthen
@apcraig
@dabail10
See below
This PR will change baselines even when the C-grid option is not exercised because of the change in
cicecore/cicedynB/dynamics/ice_dyn_evp.F90
:These lines determine whether the variables are averaged as though they were fluxes ("F") vs state variables ("S"). The option "S" is correct for these terms (ocean velocities and surface tilts). The pre-Cgrid code is equivalent to averaging the terms as if they were fluxes ("F"). Using the coupled application of UWM, retaining these four lines as "F" gave B4B answers.
A change is also made to
cicecore/cicedynB/dynamics/ice_dyn_shared.F90
to initialize E/N variables.