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.

Sign up for GitHub

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

Jump to bottom

Use JRA55 for tx1 grid #435

Merged
merged 2 commits into from
May 12, 2020
Merged

Use JRA55 for tx1 grid #435

merged 2 commits into from
May 12, 2020

Conversation

dabail10
Copy link
Contributor

@dabail10 dabail10 commented Apr 14, 2020 •
edited
Loading

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:
    Change the default forcing for the tx1 grid to JRA55. This should finally address issue one-degree tripole support #186
  • Developer(s):
    dabail10 (D. Bailey)
  • 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.
    I have run a one-year simulation using this forcing on izumi. Needs to be run on other machines.
  • How much do the PR code changes differ from the unmodified code?
    • bit for bit
    • different at roundoff level
    • more substantial
      This will change answers for the tx1 tests.
  • 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:
    You will need to set up the tx1 JRA55 forcing manually for testing. Once it is fine, then we can put it in Zenodo. The path and files are as follows:

ICE_MACHINE_INPUTDATA/CICE_data/forcing/tx1/JRA55/8XDAILY/
JRA55_03hr_forcing_tx1_2005.nc
JRA55_03hr_forcing_tx1_2006.nc
JRA55_03hr_forcing_tx1_2007.nc
JRA55_03hr_forcing_tx1_2008.nc
JRA55_03hr_forcing_tx1_2009.nc

This is triggered now by -s tx1.

@dabail10 dabail10 requested review from daveh150 and rallard77 April 14, 2020 19:31
@apcraig
Copy link
Contributor

apcraig commented Apr 14, 2020

I have a couple questions and comments.

  • What forcing were we using for the tx1 before this update? And are we no longer going to use/test that forcing in the future?
  • Do we need to add any new tests?
  • You do not need to specifically add -s tx1. That is automatically picked up when you set -g tx1.
  • Could we get rid of set_nml.jra55 and set_nml.fra55_2008. I think those are obsolete and should have been removed when the jra55 gx1 was added. I know it has nothing to do with tx1, but would be helpful.

thanks.

@dabail10
Copy link
Contributor Author

Previously the atm_data_type was set to 'clim'.

I think the tx1 test should capture this?

I agree with removing the other set_nml options.

@rallard77
Copy link
Contributor

rallard77 commented Apr 14, 2020 via email

@apcraig
Copy link
Contributor

apcraig commented Apr 14, 2020

Previously the atm_data_type was set to 'clim'.

I think the tx1 test should capture this?

The way I read the changes is that the tx1 grid has changed defaults from clim to JRA55 forcing. If you want to support clim by default and JRA55 as an option, then you want to reset the set_nml.tx1 changes and create a new file, set_nml.jra55_tx1 that would turn off clim and turn on JRA55. Then -g tx1 would create a clim forced case and -g tx1 -s jra55_tx1 would create a case with jra55 forcing. That would also be consistent with the jra55 forcing for gx3 and gx1. By default, it's not jra55 but the -s jra55_${grid} turns it on.

@dabail10
Copy link
Contributor Author

The default for gx1 is COREII and the default for gx3 is NCAR_bulk. So, when we set up the tx1 originally, we didn't have forcing, hence the 'clim' option was used. I feel like we should set the default to JRA55 for tx1. The tx1 is not used for science.

@apcraig
Copy link
Contributor

apcraig commented Apr 14, 2020

I don't have an opinion about whether the default should be clim or jra55 or whether we should support just one or the other. I just wanted to be sure we had a clear plan.

@eclare108213
Copy link
Contributor

My inclination would be to do the tx1 scripts (i.e. -s) analogously to how gx3 and gx1 are done. Keeping clim as the default is okay -- do we have any other tests that use clim? On the other hand, variable forcing is a better test of the triple grid, which is a really good argument for making JRA55 the default for tx1.

@dabail10
Copy link
Contributor Author

Looks like no for atm_data_type = 'clim'. We do test atm_data_type = 'default'.
set_nml.alt01:atm_data_type = 'default'
set_nml.box2001:atm_data_type = 'box2001'
set_nml.gbox128:atm_data_type = 'box2001'
set_nml.gbox80:atm_data_type = 'box2001'
set_nml.gx1:atm_data_type = 'LYq'
set_nml.gx3:atm_data_type = 'ncar'
set_nml.jra55:atm_data_type = 'JRA55_gx1'
set_nml.jra55_2008:atm_data_type = 'JRA55'
set_nml.jra55_gx1:atm_data_type = 'JRA55_gx1'
set_nml.jra55_gx1_2008:atm_data_type = 'JRA55_gx1'
set_nml.jra55_gx3:atm_data_type = 'JRA55_gx3'
set_nml.jra55_gx3_2008:atm_data_type = 'JRA55_gx3'

Looks like there is not actually an atm_data_type = 'clim' option. This is for ocn_data_type and bgc_data_type only. So, we would need to change 'clim' to 'default' if anything. I would prefer to use JRA55 myself.

Dave

@eclare108213
Copy link
Contributor

Thanks @dabail10. So 'clim' in set_nml.tx1 is a bug? It's probably defaulting to 'default'... I tend to agree, JRA55 would be a better choice for this test.

@dabail10
Copy link
Contributor Author

That is correct. 'clim' is a bug.

@dabail10
Copy link
Contributor Author

@rallard77 and @daveh150 have you had a chance to test this out?

@rallard77
Copy link
Contributor

rallard77 commented Apr 29, 2020 via email

@dabail10
Copy link
Contributor Author

That sounds good. I would like one of you or @daveh150 to approve the code changes for this.

@rallard77
Copy link
Contributor

rallard77 commented Apr 29, 2020 via email

@dabail10
Copy link
Contributor Author

That's what this PR is about. It's the code changes needed to read the JRA55 tx1 forcing. Can you make sure this is consistent with the code you have?

@eclare108213
Copy link
Contributor

I'm a little confused about where this PR stands. I believe this PR contains all of the code needed to read and test the JRA-55 tx1 data, but it sounds like @rallard77 needed to change the code in order to test the PR. Is that true? Or are you just saying that you tested the code in this branch (which has the changes)? Regardless, we need to make sure that the code with the PR modifications was tested and then @rallard77 or @daveh150, if you would, please approve the PR, if your output looks like it is running correctly. Thank you!

@rallard77
Copy link
Contributor

rallard77 commented May 12, 2020 via email

@eclare108213 eclare108213 self-assigned this May 12, 2020
@eclare108213 eclare108213 merged commit 6ddb5e4 into CICE-Consortium:master May 12, 2020
@eclare108213 eclare108213 mentioned this pull request May 20, 2021
Closed
@phil-blain phil-blain mentioned this pull request Sep 18, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rallard77 rallard77 rallard77 approved these changes

@eclare108213 eclare108213 eclare108213 approved these changes

@daveh150 daveh150 Awaiting requested review from daveh150

Assignees

@eclare108213 eclare108213

Labels
Forcing Priority: High Testing Type: Feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@dabail10 @apcraig @rallard77 @eclare108213