Merge stochastic physics (SPPT only) into GSL's develop branch#239
Merge stochastic physics (SPPT only) into GSL's develop branch#239gsketefian wants to merge 218 commits into
Conversation
…he stochastic_physics submodule itself will be added in a separate commit.
…'t updated during latest merge of gsl/develop.
…he gsl/MPAS_stoch_physics in the stochastic_physics repo.
…teger seed workaround.
… smoke) from "output" to "output_smoke" (and file from "history..." to "history_smoke...". This is necessary because the stream name "output" is already taken by the main output stream, and having the same name for two different output streams apparently causes SMIOL I/O errors during the forecast (and incorrect/corrupted history*.nc files).
…) no stale stream_list.atmosphere files exist in the two default_inputs directories (one under core_atmosphere and the other immediately under the MPAS-Model top-level directory), and (2) for the atmosphere core, all files (stream-related as well as namelist) in the top-level directory are backed up before new such files are copied from the default_inputs directory back up a level into the top-level directory. Previously, existing (and thus possibly outdated) stream_list.atmosphere.* files in the top-level directory were not being replaced by newer ones in default_inputs, and that was causing unexpected (and wrong) behavior. Probably a similar fix is needed for the init_atmsophere core and maybe even other ones.
…ll as its output file) so it doesn't conflict with the default output stream.
…tochastic_physics's master branh will gradually happen.
…/MPAS_stoch_physics_try_merge_stoch_master
…tochastic_physics code know that the dycore being used is MPAS.
…ill instead be defined in the Makefile for stochastic_physics only (in a separate commit into the stochastic_physics repo) since it is only needed by the stochastic_physics submodule.
…e that is now added in the Makefile in stochastic_physics).
…s the TEMPO tables.
…n building with the stand-alone MPAS-Model, stochastic_physics uses "dgemm" instead of "esmf_dgemm" to perform matrix multiplication.
…mpi-gaeac6" for building the stand-alone MPAS-Model with stochastic_physics on Gaea C6 because it turns out that the existing "ifort_icx" target can be used for this purpose -- as long as a small modification (addition of LAPACK_LIBS) is made to it, which we also do here.
|
@gsketefian Can you sync this branch with the noaa/develop branch? |
| COMMAND mpas_streams_gen ${CMAKE_CURRENT_BINARY_DIR}/Registry_processed.xml streams.${ARG_CORE} stream_list.${ARG_CORE}. listed | ||
| COMMENT "CORE ${ARG_CORE}: Generate Streams" | ||
| DEPENDS mpas_streams_gen Registry_processed.xml) | ||
| DEPENDS mpas_streams_gen ${CMAKE_CURRENT_BINARY_DIR}/Registry_processed.xml) |
There was a problem hiding this comment.
Lines 217-234:
Did we get any troubles without these newly added ${CMAKE_CURRENT_BINARY_DIR} etc?
If not, I would suggest removing all the newly added ${CMAKE_CURRENT_BINARY_DIR} etc to keep sync'ed with the upstream NCAR's MPAS-Dev/MPAS-Model as much as possible.
There was a problem hiding this comment.
@guoqing-noaa It took some time to remember why I made these changes. The short answer to your question is that, yes, we did run into trouble without these explicit path additions.
The error does not show up if you're only doing cmake builds. It shows up when you first do a build with the make system that NCAR has (i.e. using the Makefiles that are part of the repo) and THEN do a build with the cmake system. Although this is not what most people would be doing (they'd use either make or cmake), I for one would prefer having make and cmake builds that don't interfere with each other.
I'll describe what happens when building core_init_atmosphere, but the same applies to core_atmosphere.
Assume you first run a successful make build. This will generate the following file (among many others):
.../MPAS-Model/src/core_init_atmosphere/Registry_processed.xml
The way MPAS_Functions.cmake is currently written (i.e. with relative paths) causes the DEPENDS section of the "Parse Registry" build rule, which is given by
add_custom_command(
OUTPUT ${ARG_INCLUDES}
COMMAND mpas_parse_${ARG_CORE} Registry_processed.xml ${CPP_EXTRA_FLAGS}
COMMENT "CORE ${ARG_CORE}: Parse Registry"
DEPENDS mpas_parse_${ARG_CORE} Registry_processed.xml)
to erroneously detect the Registry_processed.xml file in .../MPAS-Model/src/core_init_atmosphere created by the make build. That's because in the DEPENDS, only a relative path is given (just Registry_processed.xml), and apparently cmake's behavior for DEPENDS is to first try to resolve this with respect to the source directory CMAKE_CURRENT_SOURCE_DIR, given by
CMAKE_CURRENT_SOURCE_DIR = .../MPAS-Model/src/core_init_atmosphere/
and only if that fails to then try to resolve it with respect to the current cmake build (binary) directory CMAKE_CURRENT_BINARY_DIR, given by
CMAKE_CURRENT_BINARY_DIR = .../MPAS-Model/build/src/core_init_atmosphere/
Since the initial successful make build creates the file ${CMAKE_CURRENT_SOURCE_DIR}/Registry_processed.xml, when one then runs the cmake build the DEPENDS in the "Parse Registry" rule is satisfied, which means cmake does not establish a dependency of the "Parse Registry" rule on the "Pre-Process Registry" rule that is given by
add_custom_command(
OUTPUT Registry_processed.xml
COMMAND ${CPP_EXECUTABLE} -E -P ${CPP_EXTRA_FLAGS} ${CMAKE_CURRENT_SOURCE_DIR}/Registry.xml > Registry_processed.xml
COMMENT "CORE ${ARG_CORE}: Pre-Process Registry"
DEPENDS Registry.xml)
Thus, "Parse Registry" runs immediately without waiting for "Pre-Process Registry" to complete. Since "Pre-Process Registry" hasn't yet completed, its output file .../MPAS-Model/build/src/core_init_atmosphere/Registry_processed.xml doesn't yet exist [note that cmake behavior for the COMMAND (and OUTPUT) sections of add_custom_command() is to resolve relative paths using CMAKE_CURRENT_BINARY_DIR, so the "Pre-Process Registry" rule above will place the Registry_processed.xml file it generates in CMAKE_CURRENT_BINARY_DIR, which in this case is .../MPAS-Model/build/src/core_init_atmosphere/]. Since that's the file that "Parse Registry" is looking for as input and it is missing, "Parse Registry" fails with a segfault.
The fix here is to make sure that the DEPENDS in "Parse Registry" checks for Registry_processed.xml ONLY in CMAKE_CURRENT_BINARY_DIR (and never in CMAKE_CURRENT_SOURCE_DIR). The straightforward way to do that is to specify the absolute path in the DEPENDS by adding CMAKE_CURRENT_BINARY_DIR in front of Registry_processed.xml. That's one of the changes made in this PR.
More generally, to get a more robust build configuration that avoids relying on cmake's internal rules for resolving relative paths (which can be complex and obscure), it is best to just use absolute paths in all parts of these add_custom_command() commands. That's what the changes here are doing.
There was a problem hiding this comment.
@gsketefian Thanks a lot for finding out the original intention and providing the details of this change!
I did get the same errors if I did the "make" build first and then did the "cmake" build without cleaning.
However, if we run make clean CORE=atmosphere and/or make clean CORE=init_atmosphere before doing "cmake", we are good.
Now it goes to different preferences by different views. Usually, we will let cmake find the correct path/file without explicitly specifying ${CMAKE_CURRENT_BINARY_DIR}. This is consistent with the upstream NCAR repo.
If users does "make" first and then gets errors at doing "cmake", we can just suggest these users do clean first, just like we usually do before lauching a new round of make.
There was a problem hiding this comment.
@guoqing-noaa After discussing this with @clark-evans during our DTC meeting, we decided to revert these changes and stay with the "make clean" approach.
…piling, not linking.
I merged |
|
@gsketefian would it be possible to get a status update on this PR as soon as you have the chance? I see the new HRRRv5 + stochastic physics test is still failing, which I presume is part of the "still have a couple of issues to work through" from a few weeks ago. Once this is ready on your end and passing all of the tests, we should get it merged soon. |
@clark-evans That error is a new one. I worked through all the errors a couple of weeks ago, so hopefully this is a one-off that won't happen again since it occurs on this I need to check the PR comments again, but I think the only thing I have left to do (as we discussed during our last MPAS Stochastic Physics meeting) is to (1) undo the cmake-related changes in MPAS-Model and (2) remove the |
…ply] since ierr is always set to 0 and does not actually provide any information on errors.
…ructures to play nice.
…_init_atmosphere is built first.
Looks like the tests are all now passing. Thanks for catching the reason why the test failed; I neglected to look at the output log before making my comment. Thanks for the other changes. I think we're now ready for final review. I'll nudge that forward today. |
|
@AndersJensen-NOAA @XiaSun-Atmos could you please review before tomorrow's code discussion meeting? Thanks! |
| ( cd $(ROOT_DIR)/default_inputs; for FILE in `ls -1`; do \ | ||
| if [ -e ../$$FILE ]; then \ | ||
| N=1; \ | ||
| while [ -e "../$$FILE.old$$(printf '%03d' $$N)" ]; do N=$$((N+1)); done; \ | ||
| mv "../$$FILE" "../$$FILE.old$$(printf '%03d' $$N)"; \ | ||
| fi; \ | ||
| cp $$FILE ../.; \ | ||
| done ) |
…g, which stand for longitude_fourier and latitude_legendre) from the registry since this array is only for debugging.
…to ensure it never fails - for debugging purposes.
…utput" stream (since we removed it from the Registry.xml).
…chastic physics by removing stoch_pattern_gg from the stream_list file for the "output" stream).
|
@clark-evans I made a couple more changes (and bug fixes) to address the reviewer comments. The PR is now ready for review. |
This PR enables use of the SPPT scheme of stochastic physics into MPAS-Model. This is addressed in Issue #204.
Main changes:
stochastic_physicscode as a submodule.Makefiles, registryxmlfiles, and fortran files, and GitHub workflow files to allow the SPPT stochastic scheme to be used with MPAS.stochastic_physicscode as a submodule. There is a companion PR into the authoritativestochastic_physicsrepo here.This PR adds a new GitHub Actions CI workflow (
run_mpas_stoch.yml) that runs two tests of MPAS-Model on a winter case using GFS ICs/LBCs and with SPPT enabled. The first test is the "convection_permitting" physics suite, and the second is with the "hrrrv5" suite. These two tests use the same grid, static file, ICs, LBCs, etc as the existing CI test(s) for the winter case with GFS ICs/LBCs, etc. These tests require the setup (namelist, streams, etc) files located in the directoriesMandatory Questions
MPAS-Model/testing_and_setup/ufs-community/cases/ufscommunity.[convection_permitting|hrrrv5].gfs.winter.stoch, which are under version control.Priority Reviewers