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

Update gx1 initial condition, Icepack, tests, and version. Fix hmix default value #586

Merged
merged 9 commits into from
Apr 6, 2021

Conversation

apcraig
Copy link
Contributor

@apcraig apcraig commented Mar 28, 2021

PR checklist

  • Short (1 sentence) summary of your PR:
    Update gx1 initial condition, Icepack, tests, and version number. Fix hmix default value CESM ocean forcing - hmix division by zero #585
  • Developer(s):
    apcraig, dabail10
  • 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.
    tests are bit-for-bit except gx1 with new initial condition. debug tests with gx1 fail due to Nans in ic file. https://github.com/CICE-Consortium/Test-Results/wiki/cice_by_hash_forks#c0364f4f6924009f9d949d6c0484e832c1c5a9c6
  • 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 Icepack 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/. 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:

Also

apcraig added 5 commits March 26, 2021 17:26
- update set_nml.gx1prod to match current production system
- add apr 1 test case for gx1
- update landice tests, use gx1
- fix hmix default CICE-Consortium#585
- delete old code in ice_forcing.F90 (should have been done in earlier PR)
endif ! dbug

end subroutine Jra55_data_old

!=======================================================================
!
! AOMIP shortwave forcing
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a note, mainly as a reminder: We are making JRA the default, but the older forcing options are still available. Those will need to be deprecated via our process (not in this release).

@@ -2,6 +2,9 @@
days_per_year = 365
use_leap_years = .false.
year_init = 1997
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't year_init match the new forcing data? maybe it doesn't matter, as long as it's properly set in the -s options. I see below that fyear_init=2005, but I thought that one wasn't used now. It's a little confusing, would be better to be consistent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I saw this and wondered the same things. I was reluctant to make this change as I didn't understand the importance or history. The other thing is that it will change all the gx3 results as well in our regression test. But I agree 100% that it would be nice if all of this were consistent. Are there other defaults that need to change. Should use_leap_years be always true, for instance? I can make this change as part of this PR, but it will change answers.

@@ -4394,7 +4153,7 @@ subroutine ocn_data_ncar(dt)
do j = 1, ny_block
do i = 1, nx_block
if (n == 2) sss (i,j,:) = c0
if (n == 3) hmix (i,j,:) = c0
if (n == 3) hmix (i,j,:) = c20
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this just temporary because of NaNs in the file? If not, then it would be better to set this as a parameter at the top of the module, to make it more explicit. 'default mixed layer depth'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this avoids a divide by zero. I assume there are also some science reasons for setting it to 20. I can create a parameter in the code as suggested and will do that later today. I think that's a good idea.

Copy link
Contributor

@TillRasmussen TillRasmussen Apr 6, 2021

Choose a reason for hiding this comment

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

This is a way of avoiding divison by zero. @dabail10 suggested max(c20,work1(i,j,iblk)). I dont have a preference. C20 is set to match the only other fixed setting in subroutine init_coupler_flux.

Update CI wget implementation CICE-Consortium#588
Create variable in ice_forcing.F90 called mixed_layer_depth_default and use it instead of c20
Fix bug in hourly output, created in time manager update CICE-Consortium#589
Set start year for all runs to 2005 and turn leap years on by default
Update documentation in history output section to add information about hist_avg namelist, noted in CICE-Consortium#566
@apcraig
Copy link
Contributor Author

apcraig commented Apr 2, 2021

This is ready to merge. I ran a full test suite on cheyenne this morning, https://github.com/CICE-Consortium/Test-Results/wiki/cice_by_hash_forks#4dc3bf15908deffbd68dd400fc0513a6bae7b81f and everything looks good.

It would be nice to merge this afternoon to leverage automated weekend testing and to carry out release testing this weekend as well. Someone should review and approve. I'm happy to make additional changes as needed too.

Copy link
Contributor

@eclare108213 eclare108213 left a comment

Choose a reason for hiding this comment

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

Sorry I didn't notice this was waiting for a review earlier. A few questions/comments to consider, but it looks good.

@@ -4518,7 +4280,7 @@ subroutine ocn_data_oned
ss_tlty(:,:,:) = c0
frzmlt (:,:,:) = c0 ! freezing/melting potential (W/m^2)
qdp (:,:,:) = c0 ! deep ocean heat flux (W/m^2)
Copy link
Contributor

Choose a reason for hiding this comment

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

We need to fix the ocn_data_oned subroutine when we clean up the forcing module. sss probably shouldn't be hardwired either. We can discuss whether to get rid of this option completely (and just use Icepack) or keep it as an additional test for comparing with Icepack's test, or keep it to have netcdf available in a column configuration.

cicecore/cicedynB/general/ice_forcing.F90 Outdated Show resolved Hide resolved
@@ -0,0 +1,6 @@
year_init = 2005
month_init = 9
day_init = 2
Copy link
Contributor

Choose a reason for hiding this comment

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

does this test work, starting Sep 2?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes it does. It works just fine. In fact, sec_init = 7200 as well in this setup in case you didn't notice and it all works fine. It runs 5 days to 2005-09-07-07200. The output is also as expected in the sense that things like daily history averages are fine except on the first and last day. If one started on 2005-09-07-07200, you'd probably want to run 22 hours first (which you can do with npt_unit='h' and npt=22) to get on a daily boundary, and then start running days or months after that.

Copy link
Contributor

Choose a reason for hiding this comment

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

cool, thanks

Copy link
Contributor

@dabail10 dabail10 left a comment

Choose a reason for hiding this comment

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

This looks good to me. I do wonder about setting a mixed-layer depth default. Particularly as I did a fill, so there should be valid values everywhere.

@apcraig apcraig merged commit afc3a58 into CICE-Consortium:master Apr 6, 2021
@apcraig apcraig deleted the gx1ic branch August 17, 2022 20:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants