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

Added new capability in ice_forcing.F90 to test with JRA55 atmospheri… #350

Merged
merged 17 commits into from
Sep 19, 2019

Conversation

rallard77
Copy link
Contributor

@rallard77 rallard77 commented Aug 14, 2019

…c forcing on gx1 grid.

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:
    Added new capability to perform tests with JRA55 atmospheric forcing on gx1 grid for years 2005-2009.
  • Developer(s):
    Richard Allard, Julie Crout, Matt Turner
  • 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.
    repo = https://github.com/rallard77/CICE.git
    #bran = jra55
    #hash = eff67a2
    #hshs = eff67a2
    #hshu = Elizabeth Hunke [email protected]
    #hshd = Thu Aug 1 17:36:22 2019 -0600
    #date = 2019-08-13
    #time = 14:28:39
    #mach = conrad
    #user = allard
    #vers = CICE_6.0.1
    ...skipping most of the log file
    PASS conrad_gnu_restart_gx1_8x1_jra55 build
    PASS conrad_gnu_restart_gx1_8x1_jra55 initialrun
    PASS conrad_gnu_restart_gx1_8x1_jra55 run 94.60 16.34 47.64
    PASS conrad_gnu_restart_gx1_8x1_jra55 test
    #---
    COPY conrad_gnu_smoke_gx3_1x1_diag1_run1day build
    PASS conrad_gnu_smoke_gx3_1x1_diag1_run1day run 9.00 1.52 4.49
    PASS conrad_gnu_smoke_gx3_1x1_diag1_run1day test
    #---
    COPY conrad_gnu_restart_gbox128_8x1_diag1 build
    PASS conrad_gnu_restart_gbox128_8x1_diag1 initialrun
    PASS conrad_gnu_restart_gbox128_8x1_diag1 run 30.07 8.70 15.00
    PASS conrad_gnu_restart_gbox128_8x1_diag1 test
    #---
    COPY conrad_gnu_restart_gx3_4x2_debug_diag1_run5day build
    PASS conrad_gnu_restart_gx3_4x2_debug_diag1_run5day initialrun
    PASS conrad_gnu_restart_gx3_4x2_debug_diag1_run5day run 50.30 22.29 14.04
    PASS conrad_gnu_restart_gx3_4x2_debug_diag1_run5day test
    #-------

#totl = 708
#pass = 708
#fail = 0
#pend = 0

ENTER INFORMATION HERE 
  • 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
    • [X 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:
    2 new subroutines added to ice_forcing.F90: JRA55_files and JRA55_data. In Ice-in, atm_data_type is set to "JRA55" to use this option. I have 5 .nc files for each year of forcing (2005-2009). Unlike COREII, we use shortwave and do not need a cloud factor. Forcing is at 3-hr intervals for all needed fields including precipitation. Testing was performed on conrad XC40 using quick_suite and base_suite. All tests passed. 1 new test case added to base_suite.ts which runs a 10-day restart with JRA55 for 2005. Added abort syntax in JRA55_data to ensure that days_per_year is set to 366 for leap year. The data is expected to be located in ./gx1/forcing/JRA55

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.

This looks good. Just a few questions.

It looks like a test was removed from the base suite, was this intended?

The "if" section around line 1380 of ice_forcing.F90 looks a little odd. Is this correct mixing logic for atm_data_type and atm_data_format?

  if (trim(atm_data_type) == 'hadgem') then ! netcdf
     i = index(data_file,'.nc') - 5
     tmpname = data_file
     write(data_file,'(a,i4.4,a)') tmpname(1:i), yr, '.nc'
  elseif (trim(atm_data_format) == 'nc') then ! netcdf
     i = index(data_file,'.nc') - 5
     tmpname = data_file
     write(data_file,'(a,i4.4,a)') tmpname(1:i), yr, '.nc'
  else                                     ! LANL/NCAR naming convention
     i = index(data_file,'.dat') - 5
     tmpname = data_file
     write(data_file,'(a,i4.4,a)') tmpname(1:i), yr, '.dat'
  endif

Have the new data files been put on ncar ftp so others can test? Have the data files been tested from a formal CICE input data area? I don't think I see the input files on conrad in the shared input data area.

What is the plan for updating documentation. Could this be done with this PR?

@dabail10
Copy link
Contributor

I'm working on this now. I am in the process of setting up the data tar files on our ftp. I am trying a test on cheyenne for a year. One quirky thing I have noticed is that it is ignoring year_init/fyear_init and starting from 2025 when I use year_init = 2005. I assume this is because it is using the istep1 and time from the initial file which is year Jan 1st year 21. Shouldn't the code be ignoring this on an initial run?

@dabail10
Copy link
Contributor

Just found the use_restart_time flag.

@dabail10
Copy link
Contributor

Dumb question. Does year_init have to line up with fyear_init to read in the forcing?

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.

The code changes are working fine here. I would recommend adding the namelist flag:

use_restart_time = .false.

to the test. I am working on setting up the forcing tar files. This will be required if we add the JRA55 set_nml option as a test in Travis-CI.

@apcraig
Copy link
Contributor

apcraig commented Aug 22, 2019

@rallard77, looks like there are a couple things pending then we can possibly merge this.

  • review the changes in base_suite.ts, it looks like a test was removed when you added your test. Can you fix that.
  • please add "use_restart_time = .false." in the set_nml.jra55 file
  • has the documentation been adequately updated?

While that is going on, I will try to independently download the data from the ftp site and run a test on another machine just to make sure the data is available. Thanks.

@dabail10
Copy link
Contributor

I have updated all of the datasets on our ftp to include the JRA55 forcing. Note that these are really big and the file sizes have increased dramatically.

CICE_data_all.tar.gz
CICE_data_new.tar.gz
CICE_data_forcing_gx1_200?.tar.gz

Also, should we put use_restart_time = .false. in the default ice_in? Does this impact continue runs?

Dave

@apcraig
Copy link
Contributor

apcraig commented Aug 22, 2019

I don't know much about the use_restart_time flag. Did you need it to get the case to run? By default, it's true and we do not have any tests that change it to false, so we are not exercising it.

@dabail10
Copy link
Contributor

It runs fine. Just that the date of the history and restart files are off by 20 years from year_init.

Dave

@apcraig
Copy link
Contributor

apcraig commented Aug 22, 2019

I downloaded the "new" CICE tar file to conrad, dropped it in the inputdata area, and ran conrad_intel_restart_gx1_16x2_jra55 successfully.

I guess I think we should do a bit more research on the "use_restart_time" flag before switching it for this case. So I guess I feel there are still two minor questions before merging @rallard77

  • add the removed test in base_suite.ts if appropriate
  • let us know whether the documentation is adequate at this point and if more is needed whether you propose to defer.

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.

This looks good, though I agree that a few additional things need to be done before the PR is merged:

  • Definitely reinstate the bgc test in the base suite.

  • We do need more documentation about this case, to at least describe what the forcing fields are, what dates they cover, and a citation for the original data set. This can be added in a separate PR but I'd prefer it all go in together.

The calendar related flags in CICE were made to be very flexible, perhaps too flexible -- this is something that can be changed when the time manager is overhauled. Generally speaking, year_init and fyear do not need to be the same, but in this case I think they should be the same. If use_restart_time is true, then I believe it overrides the other flags, except possibly the istep0 value (which I think gets added to whatever the others produce), so it should be false. Note that ycycle=1 indicates that only 1 year of forcing data is being used.

What appears to be going on here is that the new test option mimics the set_nml.gx1 file, which is basically just a smoke test with 24 time steps, and the forcing and simulation dates might not match -- something we hadn't noticed. A full 1- or 5-year test with this data would need additional namelist flags as in set_nml.gx1prod and/or set_nml.qc (but with values appropriate for this data set). It's probably better to set everything appropriately in the set_nml.jra55 file except keep the run short for a smoke test.

  • I suggest setting the flags so that the dates on the history and restart files match the dates inherent to the data for longer runs.

This should probably be done for the COREII forcing too, but not in this PR. At some point when we're comfortable with the JRA55 data set, maybe we can switch the QC to this data and retire the COREII data or at least separate it out from the JRA55 download. This will require some longer simulations to fully test and add sample output.

@rallard77
Copy link
Contributor Author

rallard77 commented Aug 22, 2019 via email

@apcraig
Copy link
Contributor

apcraig commented Aug 22, 2019

I support the idea that the setup should be correct for a long run even if we are just carrying out short test runs.

@rallard77
Copy link
Contributor Author

rallard77 commented Aug 22, 2019 via email

@duvivier
Copy link
Contributor

I just emailed @rallard77 but we also need to update the CICE wiki with information about the new forcing data:
https://github.com/CICE-Consortium/CICE/wiki/CICE-Input-Data

Mainly the "Updated Files" section as well as the directory structures and sizes. @dabail10 do you want to handle this since you put them together or should I?

@dabail10
Copy link
Contributor

I can update the wiki.

@dabail10
Copy link
Contributor

Updated the forcing wiki. Let me know if you expected more than this?

@rallard77
Copy link
Contributor Author

rallard77 commented Aug 28, 2019 via email

@apcraig
Copy link
Contributor

apcraig commented Sep 16, 2019

Thanks @mattdturner. I guess I suggest that you create a new option, set_nml.jra55_2008, for the 2008 start date leap year test. I would then setup a test that was "smoke, jra55_2008, 3 months, medium"? That way we clear March 1 for sure. I think this is really mostly a test of the leap year capability with the JRA dataset?

With the current test, "smoke gx1 8x1 jra55,short", this is not using the leap year calendar, is that correct? I would modify this to be a restart test and I would increase the pe count (you are running gx1) to something 10x-32x higher? Does that make sense? I haven't quite followed all the details of all the discussions, but my sense is we probably want a restart test here and to be able to run qc if needed. Then separately, we want to have a leap year test with the leap year calendar.

Are we thinking that eventually we'll drop the COREII dataset testing? Or are we going to continue to support both? If JRA is going to be our primary forcing dataset, we might want to add bgc gx1 tests with JRA forcing too.

One final issue I'm noticing, the set_nml.gx1 sets the COREII forcing dataset then set_nml.jra55 switches that to JRA55. Ideally, we'd clean this up a bit, maybe renaming gx1 to gx1_coreii and adding gx1_jra55. or maybe removing the coreii stuff from set_nml.gx1 and then adding a set_nml.coreii. But I think we can defer this for the time being for backwards compatibility. I assume you've confirmed that by using gx1 and jra55, the tests are being setup with the jra forcing correctly.

@mattdturner
Copy link
Contributor

I guess I suggest that you create a new option, set_nml.jra55_2008, for the 2008 start date leap year test. I would then setup a test that was "smoke, jra55_2008, 3 months, medium"? That way we clear March 1 for sure. I think this is really mostly a test of the leap year capability with the JRA dataset?

Correct, the smoke test is basically just to test the use_leap_year option with the JRA55 dataset.

With the current test, "smoke gx1 8x1 jra55,short", this is not using the leap year calendar, is that correct?

Yes

Are we thinking that eventually we'll drop the COREII dataset testing? Or are we going to continue to support both?

My understanding is that, eventually, JRA55 would replace COREII. I could be wrong though.

I've pushed changes that add a new jra55_2008 namelist option, and a new run90day option. It also changes the tests in base_suite.ts to do a 90 day smoke test for jra55_2008, and a JRA55 smoke test.

I am currently running base_suite to make sure I didn't break anything, and will post the results once all simulations finish.

@mattdturner
Copy link
Contributor

So far, all tests have passed. There are still 2 tests that I am waiting for (conrad_intel_restart_gx1_40x4_droundrobin_medium and conrad_intel_restart_tx1_40x4_dsectrobin_medium), but everything else has completed and passed. Since those 2 tests shouldn't be affected by the changes that were made any differently than the other tests, I'm comfortable saying that there will be no failures in the base suite.

I don't think any documentation needs to be updated for the recent changes I made. I have no idea if the JRA55 documentation is sufficient (I'm not suggesting that it isn't, either). Somebody more familiar with JRA55 would need to make that conclusion.

@@ -338,6 +338,7 @@ Table of namelist options
"","``restore_ice``", "true/false", "restore ice state along lateral boundaries", ""
"\*","``atm_data_type``", "``default``", "constant values defined in the code", ""
"","", "``LYq``", "AOMIP/Large-Yeager forcing data", ""
"","", "``JRA55``", "JRA55 forcing data", ""
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's add the citation here. You want to do it with the following format :cite:Tsujino18 (see below for box2001 as an example).

You will also have to add the citation at the very en of the following file (right after Roberts18: /doc/source/master_list.bib

@Article{Tsujino18,
author = "H. Tsujino and S. Urakawa and R.J. Small and W.M. Kim and S.G. Yeager and et al.",
title = "{JRA‐55 based surface dataset for driving ocean–sea‐ice models (JRA55‐do)}",
journal = OM,
year = {2018},
url = {http://dx.doi.org/10.1016/j.ocemod.2018.07.002}
}

Copy link
Contributor

Choose a reason for hiding this comment

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

I added the citation.

Copy link
Contributor

@duvivier duvivier left a comment

Choose a reason for hiding this comment

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

See comments in ug_case_settings.rst

@duvivier
Copy link
Contributor

@rallard77 @apcraig @eclare108213 @dabail10

I have gone through the CICE Wiki (https://github.com/CICE-Consortium/CICE/wiki/CICE-Input-Data) to add the JRA55 information in the Input Data section. Please check that the citations are correct for JRA55 and COREII.

Additionally, I noticed that the yearly forcing didn't include JRA55, so I'm remaking the forcing files and will put them in the FTP asap. I also removed the other "new" files since they were from a year ago and we say on the wiki that after 6 months we no longer consider the data to be "new".

Otherwise I've worked with @rallard77 on the documentation through the repo and minus a few small details we should be ready to go here soon.

@duvivier
Copy link
Contributor

I just updated the wiki pages with the individual year files and sizes of files. I think we're all good to go on that end of the documentation. Let me know if anyone sees an issue there.

So just the addition of the citation into the rst files in the main repo.

@mattdturner
Copy link
Contributor

So just the addition of the citation into the rst files in the main repo.

The citations should have been added to the rst files already. So it sounds like this PR might be good to go

@duvivier
Copy link
Contributor

So just the addition of the citation into the rst files in the main repo.

The citations should have been added to the rst files already. So it sounds like this PR might be good to go

@mattdturner Yep, I see them in there now. Looks good to me.

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.

I just had another look at the code modifications and I don't have any concerns at this point. Unless there are outstanding issues still to be done that I'm not aware of or @eclare108213 has some comments, I think we can merge.

@eclare108213
Copy link
Contributor

This looks fine to me except for the conflicting file.
Not for this PR but for the release, can one of you who has done a run with this data set (preferably using a standard test from the test suite) make some plots similar to those with COREII, for the sample output wiki page? Thanks everyone! This is a big step for us.

@duvivier
Copy link
Contributor

@mattdturner it looks like the conflict is with the updated documentation file. This is one I changed yesterday to add the reference for COREII. I think if you just do a "git pull upstream master" to the development fork (assuming the master branch is your upstream) that will take care of it.

@apcraig
Copy link
Contributor

apcraig commented Sep 19, 2019

I just edited the conflicted doc file in the PR. Sometimes you can do that with a conflict. I committed the change thru the github interface. That correction is now being checked with travis and readthedocs. Once those both pass, we should be able to merge.

@duvivier
Copy link
Contributor

@apcraig It looks like the documentation build is good, so as soon as Travis passes I think we can merge.

@apcraig apcraig merged commit 3d1a340 into CICE-Consortium:master Sep 19, 2019
@rallard77
Copy link
Contributor Author

rallard77 commented Sep 19, 2019 via email

@rallard77
Copy link
Contributor Author

rallard77 commented Sep 20, 2019 via email

@duvivier
Copy link
Contributor

@rallard77 I don't see attached plots. Maybe email them directly to me?

@eclare108213
Copy link
Contributor

Or drag and drop into a comment box directly on this github page.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants