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

added modified test scripts for JRA55 forcing with the gx3 grid #530

Closed
wants to merge 5 commits into from
Closed

added modified test scripts for JRA55 forcing with the gx3 grid #530

wants to merge 5 commits into from

Conversation

rallard77
Copy link
Contributor

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:
    Modified travis_suite.ts, quick_suite.ts, base_suite.ts and decomp_suite.ts to account for JRA55 on gx3 grid.

  • Developer(s):
    Rick Allard

  • Suggested reviewers Tony Craig, Dave Bailey

  • Please copy the PR test results link or provide a summary of testing completed below.
    Ran cice_setup with travis_suite, quick_suite, base_suite, decomp_suite on SGI koehr platform using intel compiler.
    Summary of test results:
    255 measured results of 255 total results
    255 of 255 tests PASSED
    0 of 255 tests PENDING
    0 of 255 tests MISSING data
    0 of 255 tests FAILED

  • 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?

    • [X ] Yes
    • No

  • Does this PR add any new test cases?

    • Yes
    • [ X] 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

  • Please provide any additional information or relevant details below:
    Documentation will be updated. However, no changes in documentation required to generate these test cases.

@apcraig
Copy link
Contributor

apcraig commented Nov 12, 2020 •
edited
Loading

The travis tests are failing because we are not downloading the jra55 data onto travis. Some changes need to be made to ~/.travis.yml. In particular, I think we need to add

# Fetch jra55_gx3 forcing data
 - "wget https://zenodo.org/record/3728364/files/CICE_data_gx3_forcing_JRA55-20200320.tar.gz &&
   tar xvfz CICE_data_gx3_forcing_JRA55-20200320.tar.gz -C ~"

One issue is that this dataset is 4GB while the NCAR bulk was 0.5 GB. Hopefully that would create any problems. If it does, we could create a smaller "travis" JRA55 gx3 dataset for travis testing that just included a subset of the data needed.

My other comment is that I'd prefer that we add the new JRA55 tests while keep the old NCAR bulk tests for the time being. That provides us with some window where both are tested and we can confirm bit-for-bit across a broader range of tests. Then in a future PR, we remove the NCAR bulk tests.

Finally, what is the impact of setting use_leap_years to false in alt01? Does this change answers?

@eclare108213
Copy link
Contributor

I agree we should keep tests for both data sets temporarily.
The NCAR bulk data does not include a leap day, and so explicitly setting use_leap_years=F here is consistent with that.

@rallard77
Copy link
Contributor Author

rallard77 commented Nov 13, 2020 via email

I have modified the test scripts for travis_suite.ts, quick_suite.ts,…
cc29597 
… base_suite.ts and decomp_suite.ts based on recommendations from Tony and Elizabeth. These tests include the original tests first, followed by the “new” jra55 gx3 tests.
@rallard77
Copy link
Contributor Author

I added new versions of .travis.yml, travis_suite.ts, quick_suite.ts, base_suite.ts and decomp_suite.ts. Based on Tony’s suggestion, I added a line in .travis.yml to go to Zenodo to wget the JRA55 gx3 forcing file (since the previous travis jra55 tests were failing). This forcing file could be subset for a smaller size and short tests. I ran the test suite for travis, quick, base, and decomp on koehr using the intel compiler. All 453 tests passed. Based on Tony’s suggestion, I added a line in .travis.yml to go to Zenodo to wget the JRA55 gx3 forcing file (since the previous travis jra55 tests were failing). This forcing file could be subset for a smaller size and short tests. I ran the test suite for travis, quick, base, and decomp on koehr using the intel compiler. All 453 tests passed.

eclare108213
eclare108213 reviewed Nov 21, 2020
View reviewed changes
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.

There seem to be a lot of extra files checked in here, that look like interim backup versions.

@apcraig
Copy link
Contributor

apcraig commented Nov 21, 2020

I see that as well. It seems to have been done with the last commit. @rallard77, can you clean that up? If you need help, let me know. I suspect you did a git add * before the commit or something like it and that picked up many of the temporary files in your directories.

@rallard77
Copy link
Contributor Author

rallard77 commented Nov 21, 2020 via email

@eclare108213
Copy link
Contributor

I ran the base_suite for this branch, test results look good. I'll try to make time series plots and compare them against master in the next few days.

@apcraig
Copy link
Contributor

apcraig commented Nov 23, 2020

I removed the extra files. I will setup a full test suite soon. One question before I do @rallard77, are the changes to cice.batch.csh what you expect? Did you intend to include those because the changes to the koehr settings doesn't look like something we want to add permanently. Also, the loft and freya diffs are showing up, but I think this is an issue with the PR diff relatively to the master. I will try to fix this depending on what you indicate and then will start testing.

@apcraig
Copy link
Contributor

apcraig commented Nov 23, 2020

Looking a bit more, it looks like you copied in an old and modified cice.batch.csh to your sandbox and then accidently committed those changes. I think we probably want to revert cice.batch.csh to what's on the current master. I'll take care of that.

@rallard77
Copy link
Contributor Author

rallard77 commented Nov 23, 2020 via email

@apcraig
Copy link
Contributor

apcraig commented Nov 23, 2020

OK, I think I have removed and reverted all the extra files now. @rallard77, please have a look at the PR and let me know if something is wrong.

There are also a number of test suite (.ts) files that have not been modified, like io_suite, nothread_suite, first_suite, etc. I'm still unclear whether the plan is to

  • move all testing to JRA55 and get rid of the old forcing
  • move nearly all testing to JRA55 and keep a minimal amount of testing with the old forcing
  • test with both JRA55 and the old forcing concurrently
  • carry out testing with a mix of JRA55 and the old forcing depending on what seems more appropriate for the given test

@rallard77
Copy link
Contributor Author

rallard77 commented Nov 23, 2020 via email

@eclare108213
Copy link
Contributor

The ultimate goal is this:

move all testing to JRA55 and get rid of the old forcing

In the meantime, we need to test both in order to compare them. Once we're satisfied that all of the tests are working correctly with JRA-55 and that it produces reasonable results in the "production" configuration, then we can deprecate the older forcing and tests.

@apcraig
Copy link
Contributor

apcraig commented Nov 23, 2020

I tested the current PR on cheyenne with 3 compilers and the results look good. https://github.com/CICE-Consortium/Test-Results/wiki/cice_by_hash_forks#b96f6cc7fc424468cbc64e713d42028fa9ae7047. The new jra55 tests don't have baselines to compare to, but everything passes and is bit-for-bit otherwise.

If we are going to get rid of the old forcing completely, then we need to update every test in all the test suites to use jra55. The current PR only covers a subset of the tests. To be complete, we should duplicate every test in every test suite with a jra55 version and test all of it. Then at a later time, we can get rid of the current default forcing. At some point, we'll also need to change the ice_in default to the jra55 forcing and get rid of the jra55 option/setting. All of this affects regression testing as test names will change. There are actually a bunch of steps we need to take to do this cleanly and correctly. When we get new initial conditions, many regressions tests will change answers yet again.

I think we need a clearer plan to move this forward. I don't think we want to do double the testing for too long on master, so we need to define the window where both the old and new may overlap. We also need to think about how many PRs we want. It could be challenging to go thru this transition on the master. I'm going to argue we want 1 PR that

  • sets the default forcing to jra55 and gets rid of all the jra55 options/settings
  • updates the initial condition
  • removes code associated with the old forcing if we really are going to completely deprecate it. Personally, I think we should keep the old forcing around for a little while, add a new setting/option to turn it on, and have a handful of tests that exercise it.

Above describes the PR I'm imagining. The steps to get there would be more complex. On the branch (say jra55_test),

  • We would double ALL the tests and test everything. Once that's all working,
  • We would make jra55 default, get rid of the jra55 options, and create a coreii and ncar_bulk option for gx1 and gx3. We would retest everything against the above step and deal with the regression comparison test names incompatibility as a one-off process to ensure it's all bit-for-bit.
  • We would update the initial condition file and retest everything.
  • We would get rid of most of the coreii and ncar_bulk tests, maybe leaving a handful in the test suite. This would take us back to the regular size test suite (instead of double).
  • The above testing would have been done on cheyenne with 3 compilers and running every test suite (which can only be done on cheyenne anyway due to pio availability). Now we run full test suites on all machines, make sure everything works, and create new baselines.
  • We then PR and merge to master.
  • We fire off the full test suites again on all machines, verify everything passes, and that the regression testing is also working on all machines. That will establish the new baselines.

By doing all the work on a branch prior to a single PR and creating the new baselines on all machines as part of the process, we reduce chances this will impact the community negatively as we go thru the transition. It's also reasonable to have two PRs, one to update the test suite and make jra55 the default and the other to update the initial condition later. I also think we need to make sure we can complete this before the next release OR we should wait until after the next release to go thru this transition.

@rallard77
Copy link
Contributor Author

rallard77 commented Nov 23, 2020 via email

@apcraig
Copy link
Contributor

apcraig commented Nov 23, 2020

Thanks @rallard77. Happy to chat next week. Unless you or anyone feels like I'm off base, I can try to work thru some of the steps on cheyenne this week. I may copy your branch to my fork and work there to update all test suites and do some testing. Will see how far I get. It probably won't hurt even if we change direction a bit next week. If I can get full double testing validated on cheyenne as well as the shift to the jra55 default in the scripts, I think that will put us in a good position to move forward rapidly.

@rallard77
Copy link
Contributor Author

rallard77 commented Nov 23, 2020 via email

@eclare108213
Copy link
Contributor

This all sounds good to me too. I would prefer that we include the new initial condition file in the PR, but we might encounter some good reasons for doing it separately as we go. Thanks @apcraig

@apcraig apcraig mentioned this pull request Nov 24, 2020
Merged
25 tasks
@apcraig
Copy link
Contributor

apcraig commented Nov 24, 2020

I'm going to close this and point to #533 which is a new PR that extends this PR to additional test suites.

@apcraig apcraig closed this Nov 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eclare108213 eclare108213 eclare108213 left review comments

@apcraig apcraig Awaiting requested review from apcraig

Assignees
No one assigned
Labels
Forcing Testing
Projects
None yet
Milestone
No milestone
Development

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