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

Python 3 bugfix in ccpp-framework, ESMF 8.1.0bs27, cleanup rt_utils.sh, fix RRTMGP regression tests #201

Merged
merged 15 commits into from
Sep 15, 2020
Merged

Python 3 bugfix in ccpp-framework, ESMF 8.1.0bs27, cleanup rt_utils.sh, fix RRTMGP regression tests #201

merged 15 commits into from
Sep 15, 2020

Conversation

climbfuji
Copy link
Collaborator

@climbfuji climbfuji commented Sep 11, 2020 •
edited
Loading

Description

This PR:

  • contains a submodule pointer update for fv3atm for a Python 3 bugfix in ccpp-framework
  • uses the correct Python interpreter to run ccpp_prebuild.py from CMakeLists.txt
  • removes machine dependent logic from tests/rt_utils.sh
  • updates ESMF from 8.1.0bs21 to 8.1.0bs27 on all tier-1 platforms, and all tier-2 systems that I am responsible for
  • fixes the RRTGMP regression tests (these tests were running the original RRTMG schemes due to typos in the regression test files); however, since these currently crash (@dustinswales is working on it), they are temporarily turned off in tests/rt.conf and tests/rt_gnu.conf
  • minor updates of pull request template
  • turn on INLINE_POST for hera.gnu, cheyenne.intel and cheyenne.gnu in gnumake build (they are already on in the cmake build)

The update from ESMF 8.1.0bs21 to ESMF 8.1.0bs27 (unexpectedly) changes the results of the regression tests. No changes in the actual results, but in the metadata of the phyf*.{nc,nemsio} and dynf*.{nc,nemsio} files:

  • order of global and variable attributes
  • the time variable (counting hourse since forecast initialization) sometimes - but not always, seems to depend on the regridding option - changes from double to float

Testing

  • full regression tests passed for new baseline (20200914) on hera.intel, hera.gnu, cheyenne.intel, cheyenne.gnu, orion.intel, wcoss_cray, wcoss_dell_p3
  • code compiles successfully on jet.intel when compile_cmake.sh is used; however, when using rt.sh, there is a problem with line 215 in FV3/ccpp/physics/CMakeLists.txt; investigation ongoing, will be fixed in a forthcoming commit
  • code compiles and runs on macOS with clang+gfortran and gcc+gfortran
    STILL TODO/CONFIRM
  • manual tests will be run on gaea.intel, stampede.intel
  • tests should also be run on the new wcoss platform (which shares the PBS scheduler logic with cheyenne in tests/rt_utils.sh)

Dependencies

If testing this branch requires non-default branches in other repositories, list them.
Those branches should have matching names (ideally)

Do PRs in upstream repositories need to be merged first?
If so add the "waiting for other repos" label and list the upstream PRs

This was referenced Sep 11, 2020
Merged
Merged
@climbfuji
Copy link
Collaborator Author

Note: temporarily added tests/rt_rrtmgp.conf (by mistake), will remove before the PR is merged.

MinsukJi-NOAA
MinsukJi-NOAA reviewed Sep 11, 2020
View reviewed changes
.github/pull_request_template.md Outdated
## Testing

How were these changes tested?
What compilers / HPCs was it tested with?
Are the changes covered by regression tests? (If not, why? Do new tests need to be added?)


Have regression tests and unit tests (utests) been run? On which platforms and with which compilers?
Copy link
Contributor

Choose a reason for hiding this comment

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

Utests can be run on Hera, Orion, Dell with Intel compilers (I had some trouble with ecFlow on Cray)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Should I add this information to the PR template?

Copy link
Contributor

Choose a reason for hiding this comment

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

Since regression tests are supported on all platforms (tier 1, 2, etc.), maybe it would be helpful to add that utests are only supported on tier 1 (Cray will have to come later)? e.g. unit tests (utests on tier 1 platform)

@climbfuji
Copy link
Collaborator Author

@MinsukJi-NOAA thanks for the docker/ci updates, the automated tests all passed

@climbfuji
Copy link
Collaborator Author

@junwang-noaa @DusanJovic-NOAA this PR is ready from my side. Waiting for CI tests to complete.

@junwang-noaa junwang-noaa merged commit 60d3ae2 into ufs-community:develop Sep 15, 2020
pjpegion pushed a commit to NOAA-PSL/ufs-weather-model.p7b that referenced this pull request Jul 20, 2021
@climbfuji @mark-a-potts
jedi-ufs with ccpp-physics submodule update (contains ufs-community#201…
babf6dc 
…) (ufs-community#211)

* updated CMakeLists.txt
* Changes for JEDI linking/control
* Update .gitmodules and submodule pointer for ccpp-physics for code review and testing
* Revert change to .gitmodules and update submodule pointer for ccpp-physics

Co-authored-by: Mark Potts <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DusanJovic-NOAA DusanJovic-NOAA DusanJovic-NOAA left review comments

@junwang-noaa junwang-noaa junwang-noaa approved these changes

@MinsukJi-NOAA MinsukJi-NOAA MinsukJi-NOAA approved these changes

@theurich theurich Awaiting requested review from theurich

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

Successfully merging this pull request may close these issues.
4 participants
@climbfuji @junwang-noaa @DusanJovic-NOAA @MinsukJi-NOAA