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

ufs_release_1.0: cleanup work 2020/01/06 #12

Merged

Conversation

climbfuji
Copy link
Collaborator

@climbfuji climbfuji commented Jan 7, 2020

  • Cleanup .gitmodules: use https protocol everywhere, remove .git suffix
  • CMakeLists.txt: fix missing netCDF support when building ESMF, adjust formatting, cleanup old code/comments

Note: following the merge of the UFS_UTILS submodule, a new branch ufs_release_1.0 should be created in that repository from the existing release/ufs_release_1.0 branch (and the latter deleted). The .gitmodules in the umbrella repository should then be updated before the final merge.

Testing: this PR and the associated PRs listed below can be checked out for testing as follows:

git clone -b dom_cleanup_work_20191206 https://github.com/climbfuji/NCEPLIBS-1
cd NCEPLIBS-1/
git submodule sync
git submodule update --init --recursive

Since making those PRs is time consuming, especially given the number of submodules involved, I would prefer to collect any required changes to my code here instead of starting another round of 18 or more PRs.

Please do not merge any of the PRs listed below, instead let me know when you are OK with the merge and I will go through the bottom-top merge process and adjust/revert .gitmodules along the way.

@climbfuji climbfuji marked this pull request as ready for review January 7, 2020 21:02
@kgerheiser
Copy link
Contributor

They all look good to me

@climbfuji
Copy link
Collaborator Author

Update on testing: I built the libraries on Cheyenne with both Intel+MPT and GNU+MPT. For each combination, I built either the entire stack (only using the compiler, MPI and cmake modules) or just the NCEPlibs (using the versions of netCDF and ESMF in the ufs-weather-model modulefiles). I then used the two different versions per compiler to compile the ufs-weather-model and run the regression tests against a baseline created with the original (NCAR) NCEPlibs.

The results are bit for bit identical (tested for a handful of tests in 64bit and 32bit mode). However, when using the build-full-stack version, the regression tests report failures because of mismatches in the (hidden) netCDF-HDF5 headers. This is because the netCDF version used by the ufs-weather-model is older (4.6.3) and compiled against an older hdf5 version than the netCDF version that comes with the umbrella build (4.7.2). Hence, this is ok.

I did similar tests on macOS+GNU with mpich 3.3.1, and they passed.

Pending further testing on other machines and the feedback from @DusanJovic-NOAA, @mark-a-potts and @uturuncoglu, these PRs are good to go.

@climbfuji
Copy link
Collaborator Author

Someone with more knowledge of the nemsio compiler flags, please check those carefully. I see some differences in the nemsio output (cmp reports differences) on Cheyenne using Intel+MPT although the runs are b4b identical (based on the checksum printed at the end of the run).

This only happens for the Intel+MPT runs using the full-stack-build, not the one that builds only the NCEPlibs but nothing else. This doesn't make sense to me, because both version build the nemsio library in the same way.

The GNU+MPT combination seems to be ok for both full-stack-build and nceplibs-only-build.

@climbfuji
Copy link
Collaborator Author

@Hang-Lei-NOAA @mark-a-potts @kgerheiser I merged all dependent PRs tonight and checked that the code is identical to what I used for testing the development. As mentioned above, I would like to ask someone with more knowledge about the nemsio flags to check/test that the flags used in the ufs_release_1.0 branch are really ok. If not then this will be a minor change, just the NCEPLIBS-nemsio submodule and the submodule pointer for NCEPLIBS. Thanks very much in advance for your help.

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.

2 participants