Saltflux option for CICE by dabail10 · Pull Request #799 · CICE-Consortium/CICE

dabail10 · 2022-12-02T16:32:25Z

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

Fix an ACC directive in ice_dyn_evp_1d.F90 (issue OPENACC code in ice_dyn_evp_1d.F90 #783)
Fix tmpstr. Issue tmpstr not defined in ice_init.F90 #798
Remove the CESM1_PIO stuff in io_pio2/ice_restart.F90. This is not necessary now that we are not using landblock elimination. (Issue Add CESM1_PIO for fill value check #675)
Put an ifdef around the OpenMP initialization code in ice_init.F90. Not sure what is going on here yet. (Issue OpenMP Initialization in ice_grid.F90 #800 )
I have added an option (wrapped in #CESMCOUPLED) to initialize ice in the 'default' case based on Tair instead of sst. When coupling in CESM, the SST has not been received from the ocean on initialization, but it has received Tair.

apcraig

I think we need to do some standalone CICE testing to make sure we understand what the results are going to be. I think the log comparisons are going to fail because of the new diagnostics even if it's all bit-for-bit. I can help with this if needed.

apcraig · 2022-12-02T17:18:35Z

+      ! north/south salt
+      call total_salt (work2)
+      totsn = global_sum(work2, distrb_info, field_loc_center, tarean)
+      totss = global_sum(work2, distrb_info, field_loc_center, tareas)


Looks like you could reuse work1 here rather than add new array, work2.

Maybe. I thought they needed to be independent.

In this code, we do not need work2, but in the print_points code we do. The array work2 is used in a number of places, so it is not really saving us memory.

OK, it just looks like work2 was added in init_mass_diags just for the new salt diagnostic. work2 must exist separately in print_points and elsewhere.

apcraig · 2022-12-02T17:19:49Z

        highfreq,       natmiter,        atmiter_conv,  calc_dragio,    &
        ustar_min,      emissivity,      iceruf,        iceruf_ocn,     &
        fbot_xfer_type, update_ocn_f,    l_mpond_fresh, tfrz_option,    &
+        saltflux_option, ice_ref_salinity, &


ice_ref_salinity could be shifted left one space, align &.

apcraig · 2022-12-02T17:20:34Z

            write(nu_diag,*) subname//' WARNING:   For consistency, set tfrz_option = mushy'
         endif
      endif
+      if (ktherm == 1 .and. trim(saltflux_option) /= 'constant') then


should this be an error or just a warning that nobody will see?

I'm basically going with the example of tfrz_option here. It is not advisable to use ktherm = 1 and saltflux_option /= 'constant', but maybe people could do it if they wanted.

fine by me, was just asking.

apcraig · 2022-12-02T17:27:14Z

+              ice_IOUnitsInUse(nu_diag) = .true. ! reserve unit nu_diag
+#ifdef CESMCOUPLED
+         ! CESM can have negative unit numbers.
+         if (nu_diag < 0) nu_diag_set = .true.


Had a quick look at "nu_diag_set" usage and don't love it overall. Can we put #ifdef CESMCOUPLED" around it everywhere, so it's clear it has nothing to do with CICE.

including in the declaration in ice_fileunits.F90.

or could we get rid of nu_diag_set and use CESMCOUPLED cpp to "turn off" whatever CICE is doing to set/release nu_diag? With CESMCOUPLED, if we can assume nu_diag is set by the coupler layer, we shouldn't need "nu_diag_set".

This is fine.

apcraig · 2022-12-02T17:29:33Z

         call release_fileunit(nu_hdr)
-         if (nu_diag /= ice_stdout) call release_fileunit(nu_diag)
+         ! CESM can have negative unit numbers
+         if (nu_diag > 0 .and. nu_diag /= ice_stdout) call release_fileunit(nu_diag)


Maybe rather than change this logic, it should just be

#ifndef CESMCOUPLED
if (nu_diag /= ice_stdout) call release_fileunit(nu_diag)
#endif

We still need to make sure not to release the negative file units here. However, I am fine with wrapping in a CESMCOUPLED.

Overall, I think it would be nice if the unit number management in CICE is not altered by the coupled models. If we could come up with a few rules like

the coupler layer will always set nu_diag but no other unit numbers

when coupled, nu_diag should not be set or released

can use CESMCOUPLED cpp as needed to change nu_diag implementation

then it should be easy to clean up the unit number management, really just nu_diag, in CICE and make it a lot clearer. Does that make sense?

dabail10 · 2022-12-02T19:25:54Z

I think this is a good idea.

apcraig · 2022-12-02T19:26:41Z

Github Actions indicates the code is not compiling. Recommend standalone testing.

dabail10 · 2022-12-02T19:28:55Z

I fixed the issue with nu_diag_set in ice_init.F90. Should compile now. Although, I don't think it will actually run, because it requires the new icepack submodule?

apcraig · 2022-12-02T19:29:24Z

I was just noticing that it needs the updated Icepack.

dabail10 · 2022-12-02T19:29:26Z

I will do some standalone testing though.

apcraig · 2022-12-06T01:52:22Z

You should be able to update Icepack on your branch and push to update the PR.

How about also changing the name of the test from salt to saltflux. Thanks.

apcraig · 2022-12-06T17:25:07Z

I think the Icepack update is incorrect. GHActions is also failing. You could try the following and see if it helps,

mv icepack icepack.old
git submodule update --init
cd icepack
git pull origin main
git log (should be #8637a2d)
cd ../
git add icepack
git commit -a "update icepack"
git push ...

dabail10 · 2022-12-06T17:54:17Z

Hmm. I thought I had the right one. I will check it.

dabail10 · 2022-12-06T17:58:23Z

Sorry. I was pointing to ESCOMP instead of Consortium main for Icepack.

dabail10 · 2022-12-06T18:00:33Z

I just synced my ESCOMP main with Consortium main. I do like this option in github now.

dabail10 · 2022-12-06T18:11:37Z

Ok. Updating icepack is creating some conflicts with the current Icepack hash.

apcraig · 2022-12-06T18:23:59Z

I think Icepack is OK now. Will run some tests just to be sure.

dabail10 · 2022-12-06T18:33:18Z

There were some weird conflicts in CICE, but I was able to resolve these.

apcraig · 2022-12-07T04:01:10Z

Full test suite on cheyenne is all bit-for-bit, 3 compilers.

dabail10 added 17 commits October 24, 2022 10:17

Bring Consortium main into CESM

c489483

Comment out OMP initialization

9db5da4

Update wave-ice coupling in NUOPC/CMEPS driver

490b259

Update to Consortium main

0d23ccd

Update saltflux option to be constant and use ice_ref_salinity

675670d

Fix a couple inconsistencies with CICE Consortium master

758d2ac

Fix some issues

853b6ec

Fix some merge issues

df51c68

Fix some merge issues

b13b728

Fix ACC directive

9bb0281

Bug fixes with eap initialization

33ea462

Fix evp initialization

842d5c6

Add warning when ktherm=1 and saltflux_option not constant

fbf9d59

Comment out Tair initialization

d88e71e

Add CESMCOUPLED around OMP initialization

ba6fe8a

CESM File unit handling

71cf0fb

Change icepack hash for PR

df5573f

dabail10 requested review from apcraig and eclare108213 December 2, 2022 16:32

apcraig reviewed Dec 2, 2022

View reviewed changes

dabail10 added 2 commits December 2, 2022 10:55

Some cleanup things

4f8cf7e

Move nu_diag_set to CESMCOUPLED region

8b1c232

Put Tair initialization in a CESMCOUPLED

f19faa7

New saltflux tests

3363e83

Update icepack for saltflux option

34dbeed

Update icepack

6201214

Merge branch 'main' of https://github.com/ESCOMP/CICE into cesm_updates2

38103c5

apcraig approved these changes Dec 7, 2022

View reviewed changes

apcraig merged commit 4befa1e into CICE-Consortium:main Dec 7, 2022

eclare108213 added the Type: Bug label Dec 7, 2022

This was referenced Dec 7, 2022

OPENACC code in ice_dyn_evp_1d.F90 #783

Closed

tmpstr not defined in ice_init.F90 #798

Closed

Add CESM1_PIO for fill value check #675

Merged

Add true salt flux computation. #706

Closed

dabail10 deleted the cesm_updates2 branch December 7, 2022 16:47

DeniseWorthen mentioned this pull request Jan 6, 2023

Update to latest Consortium/main NOAA-EMC/CICE#55

Closed

Conversation

dabail10 commented Dec 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR checklist

Uh oh!

apcraig left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

apcraig Dec 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dabail10 commented Dec 2, 2022

Uh oh!

apcraig commented Dec 2, 2022

Uh oh!

dabail10 commented Dec 2, 2022

Uh oh!

apcraig commented Dec 2, 2022

Uh oh!

dabail10 commented Dec 2, 2022

Uh oh!

apcraig commented Dec 6, 2022

Uh oh!

apcraig commented Dec 6, 2022

Uh oh!

dabail10 commented Dec 6, 2022

Uh oh!

dabail10 commented Dec 6, 2022

Uh oh!

dabail10 commented Dec 6, 2022

Uh oh!

dabail10 commented Dec 6, 2022

Uh oh!

apcraig commented Dec 6, 2022

Uh oh!

dabail10 commented Dec 6, 2022

Uh oh!

apcraig commented Dec 7, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

dabail10 commented Dec 2, 2022 •

edited

Loading

apcraig Dec 2, 2022 •

edited

Loading