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 JRA55 forcing for gx3 grid; modified ice-forcing.F90 to now han… #408

Merged
merged 9 commits into from
Feb 28, 2020

Conversation

rallard77
Copy link
Contributor

…dle both the gx1 and gx3 grid. JRA55 forcing files named were changed to include gx1 and gx3

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

  • [X ] Short (1 sentence) summary of your PR:
    Code modifications to ice_forcing.F90 now support both running gx1 and gx3 test cases with JRA55 for the years 2005-2009.
  • [X ] Developer(s):
    Rick Allard and Dave Hebert
  • [X ] Suggest PR reviewers from list in the column to the right.
    Tony Craig, Dave Bailey, Alice DuVivier
  • [X ] Please copy the PR test results link or provide a summary of testing completed below.
    Performed tests with all 4 compilers on gordon for quick_suite and bass_suite. All 816 tests passed.
    I can provide the "results.log" file if needed.
  • How much do the PR code changes differ from the unmodified code?
    • [X ] 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?
    • [X ] Yes but just additional tests for gx1 test cases with JRA55
    • 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.)
    • [X ] Yes
    • No, does the documentation need to be updated at a later time?
      • Yes
      • No
  • [X ] Please provide any additional information or relevant details below:
    Filenames have been changed to "JRA55_gx3_03hr_forcing_2006.nc ", "JRA55_gx1_03hr_forcing_2006.nc" etc. I need to ftp them to the proper directory. I will be updating the documentation.

…dle both the gx1 and gx3 grid. JRA55 forcing files named were changed to include gx1 and gx3
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 have noted a few changes inline. We also need to put the new input data on ftp and I'd like to run some tests on an independent machine.

@@ -91,7 +91,20 @@ cat >> ${jobfile} << EOFB
#PBS -l walltime=${batchtime}
EOFB

else if (${ICE_MACHINE} =~ thunder* || ${ICE_MACHINE} =~ gordon* || ${ICE_MACHINE} =~ conrad* || ${ICE_MACHINE} =~ gaffney* || ${ICE_MACHINE} =~ koehr*) then
else if (${ICE_MACHINE} =~ gordon*) then
if (${runlength} > 0) set queue = "standard"
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not a good idea. It will always overwrite any queue set by the user. We should either change the default queue on gordon (and conrad) to standard or the user can set --queue standard on the cice.setup command line. I always do that when I'm running a test suite.

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 reverted to original cice.batch.csh configuration. I will use the --queue option for testing on gordon and conrad.

@@ -9,7 +9,7 @@ maskhalo_dyn = .true.
maskhalo_remap = .true.
maskhalo_bound = .true.
fyear_init = 2008
atm_data_dir = 'ICE_MACHINE_INPUTDATA/CICE_data/forcing/gx1/JRA55'
atm_data_dir = '/p/work1/allard/CICE_data/forcing/gx1/JRA55'
Copy link
Contributor

Choose a reason for hiding this comment

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

What's happening here? We cannot have things pointing to a user space. We should make sure there are no other dirs set like this.

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 made changes to remove references to /p/work1/allard; files now point to "ICE_MACHINE_INPUTDATA"

maskhalo_remap = .true.
maskhalo_bound = .true.
fyear_init = 2005
atm_data_dir = '/p/work1/allard/CICE_data/forcing/gx1/JRA55'
Copy link
Contributor

Choose a reason for hiding this comment

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

Needs to point to ICE_MACHINE_INPUTDATA

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changes made.

maskhalo_remap = .true.
maskhalo_bound = .true.
fyear_init = 2005
atm_data_dir = '/p/work1/allard/CICE_data/forcing/gx3/JRA55'
Copy link
Contributor

Choose a reason for hiding this comment

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

Same. There seem to be others I won't point out.

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 believe I have fixed all the files so they all should point to "ICE_MACHINE_INPUTDATA"

… path, reverted to previous cice.batch.csh script; will use --queue option in seting up test suites.
@rallard77
Copy link
Contributor Author

Per Tony's feedback, I updated the set_nml_jra55 file with correct path of ICE_MACHINE_INPUTDATA
I placed the jra55 gx3 files on ftp://ftp.cgd.ucar.edu/incoming
The jra55 gx1 forcing files will need to renamed from
JRA55_03hr_forcing_2005.nc to JRA55_03hr_forcing_2005.nc etc. (for years 2005-2009)

@apcraig
Copy link
Contributor

apcraig commented Feb 25, 2020

@dabail10 @duvivier what is the status of the new input datasets? Can I grab them off ftp and give this a test?

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.

There are also some changes to the wave implementation code. Do we know why that happened? Was there a pull from master?

Have you tested the latest changes with data dropped into inputdata? If you need me to move the data, let me know.

@@ -9,7 +9,7 @@ maskhalo_dyn = .true.
maskhalo_remap = .true.
maskhalo_bound = .true.
fyear_init = 2008
atm_data_dir = 'ICE_MACHINE_INPUTDATA/CICE_data/forcing/gx1/JRA55'
atm_data_dir = '/ICE_MACHINE_INPUTDATA/CICE_data/forcing/gx1/JRA55'
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like there is a typo here (extra slash in front).

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 did not make any changes to the wave implementation code. It is possible I pulled from master. Can you put the jra55 forcing data on gordon at /p/work1/RASM_data etc.?
Then I can re-run the test suites

@dabail10
Copy link
Contributor

Datasets are staged.

@apcraig
Copy link
Contributor

apcraig commented Feb 25, 2020

I will put the datasets on gordon and conrad today.

One other thing that's been bothering me a little is that we have different set_nml files for different grids. We should be able to find a way to do "-g gx3 -s JRA55" or "-g gx1 -s JRA55" and have it work, rather than have to do "-g gx3 -s JRA55_gx3". This works for COREII but that's because it's the default forcing. Here we are changing the forcing and the grid and I have not quite figured out a way to implement this as I'd like. We may have to add some new features to the scripts to be able to parse the grid while we are setting the -s flags. Anyway, something to think about. If anyone has any thoughts, that'd be great.

@apcraig
Copy link
Contributor

apcraig commented Feb 25, 2020

The input data is now on gordon, conrad, onyx, gaffney, and koehr. @rallard77, give it a try. I will do the same. thanks!

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.

The other thing is that I can confirm the wave changes are not part of the PR.


uwind_file = &
trim(atm_data_dir)//'/8XDAILY/JRA55_03hr_forcing_2005.nc'
trim(atm_data_dir)//'/8XDAILY/JRA55_gx1_03hr_forcing_2005.nc'
Copy link
Contributor

Choose a reason for hiding this comment

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

This line is a problem. My testing is failing. Here you have assume the gx1 datasets have new names, but all we've done is add gx3 datasets. The general plan is to never rename of delete any datasets. We can make a second copy of the datasets with the gx1 name in them, but then we'll have two copies of that data in our input data area. We need to be able to support versions of the code before this change and then after. My recommendation is that we revert to the prior naming convention.

@rallard77
Copy link
Contributor Author

Ran quick_suite and base_suite with pgi compiler on gordon. All tests passed. This version is using Icepack v1.2.1 with a modification to ice_forcing.F90 to use the original naming convention for the JRA55 gx1 forcing files.

@apcraig
Copy link
Contributor

apcraig commented Feb 27, 2020

The icepack update is causing problems. You are actually reverting to an earlier version than what's on the current CICE master by one hash. Unless there is a reason to commit the icepack update, you should generally not do so. It's fine to pull from consortium master, update icepack, and test. But do not do a "git add icepack" unless it's required. I understand it's difficult and confusing to keep a development branch updated to master especially when icepack may have also been updated as a submodule and when you are dealing with both the consortium master and possibly the state of your fork master. What I suggest at this point is that you go to your sandbox and do

cd icepack
git checkout 7259556
cd ../
git add icepack
git commit -m "update icepack to current version in consortium master"
git push origin jra55_update

Hopefully, that will fix things enough. If not, you may have to revert to the icepack version that was originally on the branch or create a new branch. Lets see what happens. Thanks.

@apcraig
Copy link
Contributor

apcraig commented Feb 27, 2020

Thanks Rick, it still shows a change in Icepack, but it's pointing to the same version as on the current master, so shouldn't do anything when merged. I will run some tests and hopefully we can get this merged today or tomorrow.

One other thing, how about documentation. At the very least, the namelist options have to be updated in the user guide? Anything else?

@rallard77
Copy link
Contributor Author

rallard77 commented Feb 27, 2020 via email

@duvivier
Copy link
Contributor

I'll update the namelist changes in the user guide today. I don't think anything else needs to be updated. Rick

We need to update the CICE wiki about data availability with this PR.

@phil-blain
Copy link
Member

Thanks Rick, it still shows a change in Icepack, but it's pointing to the same version as on the current master, so shouldn't do anything when merged.

Interestingly, the same changes to icepack appear on the git command line, depending on the options used:

# first clone the consortium master and fetch Rick's branch
$ git clone --recurse-submodules https://github.com/CICE-Consortium/CICE/
$ git remote add rick https://github.com/rallard77/CICE/
$ git fetch rick jra55_update
$ git checkout jra55_update
# then compare:
$ git diff --stat master...jra55_update
 cicecore/cicedynB/general/ice_forcing.F90            | 51 ++++++++++++++++++++++++++++++++++++++-------------
 configuration/scripts/options/set_nml.jra55          |  2 +-
 configuration/scripts/options/set_nml.jra55_gx1      | 17 +++++++++++++++++
 configuration/scripts/options/set_nml.jra55_gx1_2008 | 17 +++++++++++++++++
 configuration/scripts/options/set_nml.jra55_gx3      | 17 +++++++++++++++++
 configuration/scripts/options/set_nml.jra55_gx3_2008 | 17 +++++++++++++++++
 configuration/scripts/tests/base_suite.ts            |  6 ++++--
 doc/source/user_guide/ug_case_settings.rst           |  3 ++-
 icepack                                              |  2 +-
 9 files changed, 114 insertions(+), 18 deletions(-)
# show the changes to the icepack commit
$ git  diff -p --submodule=short master...jra55_update -- icepack
diff --git a/icepack b/icepack
index 2fa887f..7259556 160000
--- a/icepack
+++ b/icepack
@@ -1 +1 @@
-Subproject commit 2fa887f6445652eb7ea67637c061f299df2a1f66
+Subproject commit 7259556232cebf37f4c9b42b4ba0b866a07d196b

These are the same modifications that are shown in the "Files" tab of the PR. (If you hover over "27 files" on the line "Submodule icepack updated 27 files", the link will be CICE-Consortium/Icepack@2fa887f...7259556, i.e. the commits listed above.

The three-dot syntax master...jra55_update syntax means "all changes made on jra55_update since it diverged from master."
We can check that the submodule is currently at the same commit on both branches:

$ git rev-parse jra55_update:icepack
7259556232cebf37f4c9b42b4ba0b866a07d196b
$ git rev-parse master:icepack
7259556232cebf37f4c9b42b4ba0b866a07d196b

If we use the two-dot syntax for git-diff (which means "all changes on both branches since they diverged"), then icepack does not appear, as expected:

 $ git diff --stat master..jra55_update
 cicecore/cicedynB/dynamics/ice_dyn_shared.F90        | 14 ++++++--------
 cicecore/cicedynB/general/ice_forcing.F90            | 51 ++++++++++++++++++++++++++++++++++++++-------------
 cicecore/cicedynB/general/ice_init.F90               | 15 ++-------------
 cicecore/drivers/direct/hadgem3/CICE.F90             |  4 ++--
 cicecore/drivers/mct/cesm1/CICE_copyright.txt        |  4 ++--
 cicecore/drivers/nuopc/cmeps/CICE_copyright.txt      |  4 ++--
 cicecore/drivers/nuopc/dmi/CICE.F90                  |  4 ++--
 cicecore/drivers/standalone/cice/CICE.F90            |  4 ++--
 cicecore/version.txt                                 |  2 +-
 configuration/scripts/ice_in                         |  3 ---
 configuration/scripts/options/set_nml.jra55          |  2 +-
 configuration/scripts/options/set_nml.jra55_gx1      | 17 +++++++++++++++++
 configuration/scripts/options/set_nml.jra55_gx1_2008 | 17 +++++++++++++++++
 configuration/scripts/options/set_nml.jra55_gx3      | 17 +++++++++++++++++
 configuration/scripts/options/set_nml.jra55_gx3_2008 | 17 +++++++++++++++++
 configuration/scripts/tests/base_suite.ts            |  6 ++++--
 doc/source/conf.py                                   |  6 +++---
 doc/source/intro/copyright.rst                       |  2 +-
 doc/source/science_guide/sg_dynamics.rst             |  2 +-
 doc/source/user_guide/ug_case_settings.rst           |  6 ++----
 20 files changed, 137 insertions(+), 60 deletions(-)

If we take a look at the commit graph, we can understand what happens:

$ git log --oneline --decorate --graph master jra55_update $(git merge-base jra55_update master)^!
* a5a9d22 (HEAD -> jra55_update, rick/jra55_update) updated user_guide to account for JRA55 options for both the gx1 and gx3 grid
* 4fad132 update icepack to current version in consortium master
* 85e6ba9 Updated icepack to version 1.2.1
* d41796f Modified ice_forcing.F90 to revert to using original naming convention for JRA55 forcing files for gx1 grid
* 3953f8d corrected typo in set_nml.jra55_2008 file
* 3a3a2f5 Updated set_nml.jra55* files to reflect correct ICE_MACHINE_INPUTDATA path, reverted to previous cice.batch.csh script; will use --queue option in seting up test suites.
* ce51d33 Added JRA55 forcing for gx3 grid; modified ice-forcing.F90 to now handle both the gx1 and gx3 grid. JRA55 forcing files named were changed to include gx1 and gx3
| * f3aec72 (origin/master, origin/HEAD, master) Add additional basal stress parameters to the dynamics namelist (#404)
| * 7e11a34 update copyright year, update internal version number in preparation for minor release, update icepack (#409)
|/  
* 2e1c3b7 Fix errors in cice.setup script and add travis tests to catch this sooner (#406)

The branch jra55_update was based on 2e1c3b7, where icepack is at 2fa887f6445652eb7ea67637c061f299df2a1f66:

git rev-parse 2e1c3b7:icepack
2fa887f6445652eb7ea67637c061f299df2a1f66

So that is why icepack appears as modified in the pull request "Files" tab, and with git diff master...jra55_update: the icepack commit in branch jra55_update is different to the one on which this branch was started, so the three dot syntax (and the "Files" tab of the PR) shows it as modified. It kind of unintuitive for a submodule, but makes sense for files: for code review we want to see changes introduced by the commits in this branch, and we don't care if master has advanced since the branch was created.

Note that icepack would not appear as changed in the PR if the branch jra55_update had been created after icepack was updated on master.

@apcraig
Copy link
Contributor

apcraig commented Feb 27, 2020

Thanks @phil-blain for filling in some of the details. I agree with everything you've noted. I think the option once icepack was updated in this PR was to either revert back to the original version (2fa887f) or to update to the current version on master (725955), and I suggested moving forward to 725955. My real preference would have been to go back to 2fa887f. That way the PR would show no diffs, but I felt it might be better just to move forward. The other problem with setting it to 725955 is that if the master changes again, this PR could revert it backwards. Maybe we should have a general policy that if icepack is accidently updated (which has happened a number of times to different people), that we should just require them to set it back to the original version when the branch was created.

But there are also questions about what happens when you pull the consortium master onto your branch and the submodule has changed. I still find the logic in these PR diffs confusing and have learned to deal with them as best as I can. Even in this PR, you see some "wave" code differences in the PR that are NOT in the branch and do not show up when you merge the code into the master manually (I have done that in my sandbox), but that still show up because of something being out of sync on the branch relative to the master.

In the end, the simplest way to move things forward if a branch is a little stale seems to be to create a new branch off current master, merge the old branch onto it, and create a new PR. I hate to ask folks to do this because then the PR has to be redone. It's not a huge time cost, but it's certainly inconvenient on several levels including losing all the discussion in the current PR. I wish there were some better tools or processes that would make all this easier. We continue to run into these issues with PRs.

@phil-blain
Copy link
Member

@apcraig the changes to the get_wave_spec subroutine (https://github.com/CICE-Consortium/CICE/pull/408/files#diff-9db2d4f2e4d315c7a5c0594d11342ad0L5157-L5178) are in this branch:
see ce51d33 (the first commit in this PR)
and

git show ce51d33 cicecore/cicedynB/general/ice_forcing.F90

I just tried merging this branch into master in my sandbox (with git merge) and the get_wave_spec subroutine is modified : after the merge, I get:

git -P diff HEAD^1 HEAD cicecore/cicedynB/general/ice_forcing.F90
diff --git a/cicecore/cicedynB/general/ice_forcing.F90 b/cicecore/cicedynB/general/ice_forcing.F90
index 8e3e147..2c5cedd 100755
--- a/cicecore/cicedynB/general/ice_forcing.F90
+++ b/cicecore/cicedynB/general/ice_forcing.F90
@@ -117,7 +117,7 @@ module ice_forcing
          ocn_data_format, & ! 'bin'=binary or 'nc'=netcdf
          atm_data_type, & ! 'default', 'monthly', 'ncar', 
                           ! 'LYq' or 'hadgem' or 'oned' or
-                          ! 'JRA55'
+                          ! 'JRA55_gx1' or 'JRA55_gx3'
          bgc_data_type, & ! 'default', 'clim'
          ocn_data_type, & ! 'default', 'clim', 'ncar', 'oned',
                           ! 'hadgem_sst' or 'hadgem_sst_uvocn'
@@ -241,8 +241,8 @@ subroutine init_forcing_atmo
             file=__FILE__, line=__LINE__)
       endif
 
-      if (use_leap_years .and. (trim(atm_data_type) /= 'JRA55' .and. &
-                                trim(atm_data_type) /= 'default' .and. &
+      if (use_leap_years .and. (trim(atm_data_type) /= 'JRA55_gx1' .and. &
+                                trim(atm_data_type) /= 'JRA55_gx3' .and. &
                                 trim(atm_data_type) /= 'hycom' .and. &
                                 trim(atm_data_type) /= 'box2001')) then
          write(nu_diag,*) 'use_leap_years option is currently only supported for'
@@ -259,8 +259,10 @@ subroutine init_forcing_atmo
          call NCAR_files(fyear)
       elseif (trim(atm_data_type) == 'LYq') then
          call LY_files(fyear)
-      elseif (trim(atm_data_type) == 'JRA55') then
-         call JRA55_files(fyear)
+      elseif (trim(atm_data_type) == 'JRA55_gx1') then
+         call JRA55_gx1_files(fyear)
+      elseif (trim(atm_data_type) == 'JRA55_gx3') then
+         call JRA55_gx3_files(fyear)
       elseif (trim(atm_data_type) == 'hadgem') then
          call hadgem_files(fyear)
       elseif (trim(atm_data_type) == 'monthly') then
@@ -556,7 +558,9 @@ subroutine get_forcing_atmo
          call ncar_data
       elseif (trim(atm_data_type) == 'LYq') then
          call LY_data
-      elseif (trim(atm_data_type) == 'JRA55') then
+      elseif (trim(atm_data_type) == 'JRA55_gx1') then
+         call JRA55_data(fyear)
+      elseif (trim(atm_data_type) == 'JRA55_gx3') then
          call JRA55_data(fyear)
       elseif (trim(atm_data_type) == 'hadgem') then
          call hadgem_data
@@ -1395,7 +1399,11 @@ subroutine file_year (data_file, yr)
          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_type) == 'JRA55') then ! netcdf
+      elseif (trim(atm_data_type) == 'JRA55_gx1') 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_type) == 'JRA55_gx3') then ! netcdf
          i = index(data_file,'.nc') - 5
          tmpname = data_file
          write(data_file,'(a,i4.4,a)') tmpname(1:i), yr, '.nc'
@@ -2025,12 +2033,12 @@ subroutine LY_files (yr)
       endif                     ! master_task
 
       end subroutine LY_files
-      subroutine JRA55_files(yr)
+      subroutine JRA55_gx1_files(yr)
 !
       integer (kind=int_kind), intent(in) :: &
            yr                   ! current forcing year
 
-      character(len=*), parameter :: subname = '(JRA55_files)'
+      character(len=*), parameter :: subname = '(JRA55_gx1_files)'
 
       uwind_file = &
            trim(atm_data_dir)//'/8XDAILY/JRA55_03hr_forcing_2005.nc'
@@ -2040,8 +2048,23 @@ subroutine JRA55_files(yr)
          write (nu_diag,*) 'Atmospheric data files:'
          write (nu_diag,*) trim(uwind_file)
     endif
-      end subroutine JRA55_files
+      end subroutine JRA55_gx1_files
+      subroutine JRA55_gx3_files(yr)
+!
+      integer (kind=int_kind), intent(in) :: &
+           yr                   ! current forcing year
+
+      character(len=*), parameter :: subname = '(JRA55_gx3_files)'
 
+      uwind_file = &
+           trim(atm_data_dir)//'/8XDAILY/JRA55_gx3_03hr_forcing_2005.nc'
+      call file_year(uwind_file,yr)
+  if (my_task == master_task) then
+         write (nu_diag,*) ' '
+         write (nu_diag,*) 'Atmospheric data files:'
+         write (nu_diag,*) trim(uwind_file)
+    endif
+      end subroutine JRA55_gx3_files
 !=======================================================================
 !
 ! read Large and Yeager atmospheric data
@@ -5157,7 +5180,6 @@ subroutine get_wave_spec
       if (icepack_warnings_aborted()) call abort_ice(error_message=subname, &
          file=__FILE__, line=__LINE__)
 
-      ! if no wave data is provided, wave_spectrum is zero everywhere
       wave_spectrum(:,:,:,:) = c0
       wave_spec_dir = ocn_data_dir
       dbug = .false.
@@ -5165,14 +5187,17 @@ subroutine get_wave_spec
       ! wave spectrum and frequencies
       if (wave_spec) then
       ! get hardwired frequency bin info and a dummy wave spectrum profile
-      ! the latter is used if wave_spec_type == profile
          call icepack_init_wave(nfreq,                 &
                                 wave_spectrum_profile, &
                                 wavefreq, dwavefreq)
 
+         ! default, for testing only
+         do k = 1, nfreq
+            wave_spectrum(:,:,k,:) = wave_spectrum_profile(k)
+         enddo
 
          ! read more realistic data from a file
-         if ((trim(wave_spec_type) == 'constant').OR.(trim(wave_spec_type) == 'random')) then
+         if (trim(wave_spec_type) == 'file') then
          if (trim(wave_spec_file(1:4)) == 'unkn') then
             call abort_ice (subname//'ERROR: wave_spec_file '//trim(wave_spec_file))
          else

This has the effect or reverting the changes to cicecore/cicedynB/general/ice_forcing.F90 in commit e18b650 (PR #394), which I don't think is intended.

@apcraig
Copy link
Contributor

apcraig commented Feb 27, 2020

I am finding the same thing, just updating the PR as @phil-blain comments came in.

It looks like the wave code changes that are showing up in this PR are actually in the code and in my testing on conrad, they are causing a bunch of fsd12 tests to fail. It looks like the code mod in e18b650 (PR #394) is being reverted in this PR in ice_forcing.F90. Again, not sure how this happened. Maybe some files were copied from one sandbox to another at some point with different bases?

@phil-blain, do you have an idea how this happened or how to best fix it. At this point, I'd recommend a new branch. What do you think?

@phil-blain
Copy link
Member

phil-blain commented Feb 27, 2020

I think the option once icepack was updated in this PR was to either revert back to the original version (2fa887f) or to update to the current version on master (725955), and I suggested moving forward to 725955. My real preference would have been to go back to 2fa887f. That way the PR would show no diffs, but I felt it might be better just to move forward.

I think it would have been less confusing to revert to the original (2fa887f). Then git diff master...jra55_update --stat would not show any changes to icepack (and neither would the "Files" tab of the PR). The merge result would have been the same (regarding Icepack.)

There is a third option: use interactive rebasing to remove the commits that introduced icepack changes, and force push the branch:

git checkout jra55_update
git rebase -i --keep-base master
# $EDITOR opens with this content:
pick ce51d33 Added JRA55 forcing for gx3 grid; modified ice-forcing.F90 to now handle both the gx1 and gx3 grid. JRA55 forcing files named were changed to include gx1 and gx3
pick 3a3a2f5 Updated set_nml.jra55* files to reflect correct ICE_MACHINE_INPUTDATA path, reverted to previous cice.batch.csh script; will use --queue option in seting up test suites.
pick 3953f8d corrected typo in set_nml.jra55_2008 file
pick d41796f Modified ice_forcing.F90 to revert to using original naming convention for JRA55 forcing files for gx1 grid
pick 85e6ba9 Updated icepack to version 1.2.1
pick 4fad132 update icepack to current version in consortium master
pick a5a9d22 updated user_guide to account for JRA55 options for both the gx1 and gx3 grid
# remove the lines corresponding to the commits that change icepack (or change "pick" to "drop" or "d":
pick ce51d33 Added JRA55 forcing for gx3 grid; modified ice-forcing.F90 to now handle both the gx1 and gx3 grid. JRA55 forcing files named were changed to include gx1 and gx3
pick 3a3a2f5 Updated set_nml.jra55* files to reflect correct ICE_MACHINE_INPUTDATA path, reverted to previous cice.batch.csh script; will use --queue option in seting up test suites.
pick 3953f8d corrected typo in set_nml.jra55_2008 file
pick d41796f Modified ice_forcing.F90 to revert to using original naming convention for JRA55 forcing files for gx1 grid
pick a5a9d22 updated user_guide to account for JRA55 options for both the gx1 and gx3 grid
# save 
# rebase should happen automatically without conflicts
git push --force-with-lease origin jra55_update

This will simply drop the two commits that changed icepack (and rewrite the last commit of the branch with an updated parent). The --keep-base option makes sure that we don't change the base-commit of the branch. It is equivalent to doing

git rebase -i $(git merge-base HEAD master)

@phil-blain
Copy link
Member

phil-blain commented Feb 27, 2020

I am finding the same thing, just updating the PR as @phil-blain comments came in.

It looks like the wave code changes that are showing up in this PR are actually in the code and in my testing on conrad, they are causing a bunch of fsd12 tests to fail. It looks like the code mod in e18b650 (PR #394) is being reverted in this PR in ice_forcing.F90. Again, not sure how this happened. Maybe some files were copied from one sandbox to another at some point with different bases?

@phil-blain, do you have an idea how this happened or how to best fix it. At this point, I'd recommend a new branch. What do you think?

@apcraig Regarding how this happened: yes, probably some manual copy of files from different clones, without using Git commands.

Regarding how to best fix it: no need for a new branch. Again, Rick could use interactive rebasing to rewrite the offending commit (ce51d33) and remove the modifications to get_wave_spec. This is a little involved. An easier alternative is to simply add a commit that returns the subroutine to its original state. This can be done manually, or even better, using Git:

git log -1 -L :get_wave_spec:cicecore/cicedynB/general/ice_forcing.F90 e18b650 | git apply
git add cicecore/cicedynB/general/ice_forcing.F90
git commit -m "Revert changes to get_wave_spec in ice_forcing.F90"
git push

@rallard77
Copy link
Contributor Author

rallard77 commented Feb 27, 2020 via email

@apcraig
Copy link
Contributor

apcraig commented Feb 27, 2020

Thanks @rallard77. While we're at it, how about also reverting the icepack back to 2fa887f. That would be

cd icepack
git checkout 2fa887f
cd ../
git add icepack
git commit -m "revert icepack to 2fa887f"
git push origin jra55_update

Then we'll review again and I'll test. thanks!

@apcraig
Copy link
Contributor

apcraig commented Feb 27, 2020

The PR now seems to have what we expect, so that's great. I'll create another sandbox and run another suite. I think we are getting close.... thanks.

@rallard77
Copy link
Contributor Author

rallard77 commented Feb 27, 2020 via email

@apcraig
Copy link
Contributor

apcraig commented Feb 27, 2020

By the way, it's pretty crazy you can actually do this:

git log -1 -L :get_wave_spec:cicecore/cicedynB/general/ice_forcing.F90 e18b650 | git apply

@apcraig
Copy link
Contributor

apcraig commented Feb 27, 2020

@phil-blain is our git star!

@phil-blain
Copy link
Member

It is pretty cool! It works because of our .gitattributes file and the embedded logic in Git that knows about Fortran.

@apcraig
Copy link
Contributor

apcraig commented Feb 28, 2020

I think this is ready to merge. Full test results from conrad are here. Bit-for-bit with the prior results except where the testnames changed due to renaming of the "sets" files. In those cases, I have manually diffed the 4 log files for the smoke tests of each compiler of the "jra55_gx1" cases versus the prior "jra55" cases and they are bit-for-bit. So I feel I have also confirmed the jra55 gx1 cases are also bit-for-bit with the baselines.

I think we can merge this. If anyone else has any comments or concerns, let me know. Otherwise, I'll try to merge by Friday midday.

@eclare108213
Copy link
Contributor

Merging is fine with me, let's just make sure that the updates to the wiki are done, as @duvivier notes above.

@rallard77
Copy link
Contributor Author

rallard77 commented Feb 28, 2020 via email

@apcraig
Copy link
Contributor

apcraig commented Feb 28, 2020

@rallard77, you may not have permission to edit. @duvivier or I will take care of it.

@duvivier
Copy link
Contributor

@rallard77
I sent a long email the other night. Can you just confirm that the files I bolded in that are the only ones that need to be added?

@apcraig
Copy link
Contributor

apcraig commented Feb 28, 2020

I owe you a reply from your email the other day @duvivier, but I think the new files for this PR are just the JRA55/8XDAILY files under the gx3 directory. I don't think we are supporting tx1 + JRA55 yet. We have a tx1 test case, but I'm not sure what the forcing is. It doesn't seem to be JRA55, maybe internal forcing? The ww3 file is probably stuck where it is due to backwards compatibility.

I want to merge this PR this afternoon. If the wiki is not updated before that, I can try to update the wiki this evening.

@rallard77
Copy link
Contributor Author

rallard77 commented Feb 28, 2020 via email

@duvivier
Copy link
Contributor

I know, it was a long email. I will update the wiki this afternoon so you should feel free to merge.

I was wondering when the tx1 JRA55 files had been added, since I didn't remember seeing that PR go through, but it's good to know that they have not been officially added, so I will not include them in any tarballs.

Finally, with respect to the WW3 file. I get the backward compatibility issue. Could we possibly add a new directory with that file in it and then in future versions post that this will chance (i.e. give people warning) and then switch to that format. This is the sort of future planning I'd like to set the foundation for.

Thanks!

@duvivier
Copy link
Contributor

@apcraig I think maybe we should chat on the phone (Monday?) about the files. I think you'll have good insight about how best to organize them.

@apcraig
Copy link
Contributor

apcraig commented Feb 28, 2020

Happy to chat sometime. With the ww3 file, I think another option is to move it and then create a link from the current location. We can delete the link in the future if we feel it's reasonable. That file location is part of our last release, so that's a little sticking point. We probably need to preserve it for at least a while in some form.

@apcraig apcraig merged commit e81f710 into CICE-Consortium:master Feb 28, 2020
@duvivier
Copy link
Contributor

I updated the wiki. The wiki may need adjustments as we figure out the forcing file details, but this PR should be good to go from my end.

phil-blain added a commit to phil-blain/CICE that referenced this pull request Mar 11, 2024
In e81f710 (Added JRA55 forcing for gx3 grid; modified ice-forcing.F90
to now han… (CICE-Consortium#408), 2020-02-28), ice_forcing::init_forcing_atmo was
modified to allow 'atm_data_type=JRA55_gx3' to be used with
'use_leap_years', but by mistake it removed 'default' from the allowed
values of 'atm_data_type' when 'use_leap_years' is used.

Put it back so that atm_data_type=default and use_leap_years can be used
together again.
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.

6 participants