v8.4.0 merge#246
Open
clark-evans wants to merge 252 commits into
Open
Conversation
… top-to-bottom Note: The changes in this commit mirror those in commit f232665, but this commit concerns to the code for producing LBCs rather than the code for producing ICs. The code in the init_atm_case_lbc routine for vertically interpolating relative humidity and specific humidity for LBCs assumed that first-guess levels would be given in bottom-to-top order when attempting to vertically extrapolate to model levels below the lowest first-guess level. The relevant code for relative humidity read as follows -- the code for specific humidity is similar. if (target_z < z_fg(1,iCell) .and. k < nVertLevels) relhum(k,iCell) = relhum(k+1,iCell) If first-guess levels are not given in bottom-to-top order, then z_fg(1,iCell) does not necessarily contain the height of the surface in the first-guess data, resulting in a copy of vertically interpolated relative humidity level k+1 to level k. One possible fix for this issue might be to compare target_z with sorted_arr(1,1). Since sorted_arr is always sorted in ascending order by its first index, the check to decide when to copy/extrapolate relative humidity would be independent of the order in which first-guess levels were provided. If the comparison were to be made against sorted_arr(1,1) rather than against z_fg(1,iCell), the effect would be to copy the lowest *interpolated* level downward to model levels below the first-guess ground. An alternative fix adopted in this commit is to simply delete the logic to explicitly copy/extrapolate downward, since both relative humidity and specific humidity are vertically interpolated with extrap=0, which specifies constant- value extrapolation. In this case, the lowest *first-guess* level -- rather than the lowest *interpolated* level -- is copied downward. Note: The issue being addressed by this commit is a dependence on the order in which levels are given in the input intermediate file, and not on the direction in which the first-guess model indexes its levels.
This commit updates the tag to 20250616-MPASv8.3 for the MMM-physics external in the src/core_atmosphere/Externals.cfg file to address an issue with .F90 files not being re-compiled to .o files if a .F90 file was modified. With the updated tag, compiling the atmosphere core, then making changes to any of the .F90 files in src/core_atmosphere/physics/physics_mmm/, then running 'make' again (without first cleaning) leads to the modified .F90 files correctly being re-compiled.
MPAS-Dev#1335) This merge fixes a bug in the vertical interpolation of humidity for LBCs when first-guess levels are given in top-to-bottom order. Note: The changes in this merge mirror those in merge commit d3ab76a (MPAS-Dev#936), but this merge concerns to the code for producing LBCs rather than the code for producing ICs. The code in the init_atm_case_lbc routine for vertically interpolating relative humidity and specific humidity for LBCs assumed that first-guess levels would be given in bottom-to-top order when attempting to vertically extrapolate to model levels below the lowest first-guess level. The relevant code for relative humidity read as follows -- the code for specific humidity is similar. if (target_z < z_fg(1,iCell) .and. k < nVertLevels) relhum(k,iCell) = relhum(k+1,iCell) If first-guess levels are not given in bottom-to-top order, then z_fg(1,iCell) does not necessarily contain the height of the surface in the first-guess data, resulting in a copy of vertically interpolated relative humidity level k+1 to level k. The fix adopted in this merge is to simply delete the logic to explicitly copy/extrapolate downward, since both relative humidity and specific humidity are vertically interpolated with extrap=0, which specifies constant-value extrapolation. In this case, the lowest first-guess level -- rather than the lowest interpolated level -- is copied downward. * init_atmosphere/fix_lbc_rh_extrap: Fix bug in vertical interp of humidity for LBCs when levels are given top-to-bottom
…PAS-Dev#1337) This merge updates the MMM-physics external tag to 20250616-MPASv8.3 in the src/core_atmosphere/Externals.cfg file to address an issue with .F90 files not being re-compiled to .o files if a .F90 file was modified. With the updated tag, compiling the atmosphere core, then making changes to any of the .F90 files in src/core_atmosphere/physics/physics_mmm/, then running make again (without first cleaning) leads to the modified .F90 files being correctly re-compiled. * atmosphere/update_mmm_phys_tag: Update MMM-physics tag in Externals.cfg to fix .F90 re-compilation issue
This merge corrects two issues in the MPAS-Atmosphere model: * Fix a bug in the vertical interpolation of humidity for LBCs when first-guess levels are given in top-to-bottom order. Incorrect logic in the init_atm_case_lbc routine previously assumed that first-guess levels would be given in bottom-to-top order when attempting to vertically extrapolate to model levels below the lowest first-guess level, resulting in a copy of the vertically interpolated relative humidity at level k+1 to level k if levels were given in top-to-bottom order. This bug resulted in an unreasonably low water vapor mixing ratio field (lbc_qv) in LBC files. (PR MPAS-Dev#1335) NB: This bug is present in most older releases of MPAS, and it should be possible to cherry-pick the fix onto any branch or release based on MPAS v7.0 or later. * Fix an issue with .F90 files in the src/core_atmosphere/physics/physics_mmm/ directory not being re-compiled by updating the MMM-physics external tag to acquire a new Makefile.mpas file for MMM-physics. With the updated tag, compiling the atmosphere core, then making changes to any of the .F90 files in src/core_atmosphere/physics/physics_mmm/, then running make again (without first cleaning) leads to the modified .F90 files being correctly re-compiled. (PR MPAS-Dev#1337)
This commit adds support for linking with the MUSICA-Fortran library. This capability is intended as a first step towards being able to use MUSICA chemistry components (MICM, TUV-x) in MPAS-Atmosphere. In order for MPAS to link with the MUSICA-Fortran library, the library must be detectable through 'pkg-config'. Specifying MUSICA=true on the MPAS 'make' command line will cause MPAS to link with the MUSICA library; for example: make gnu CORE=atmosphere MUSICA=true Note that, although the use of the MUSICA library will likely be within the context of the atmosphere core only, compiling any core with MUSICA=true will cause the MUSICA library to be linked with the MPAS model core executable. If the MUSICA-Fortran library has been linked successfully, the build summary will include the message MPAS was linked with the MUSICA-Fortran library version 0.10.1. (possibly with a different version number); additionally, when running the model, the log file will contain a message indicating MUSICA support as well as the MICM library version, e.g.: MUSICA support: yes - MICM version: 3.8.0 If the MUSICA-Fortran library has not been linked, the build summary will include the message MPAS was not linked with the MUSICA-Fortran library. and the log file for a core will contain the message MUSICA support: no .
This merge adds support for linking with the MUSICA-Fortran library as a first step towards being able to use MUSICA chemistry components (MICM, TUV-x) in MPAS-Atmosphere. In order for MPAS to link with the MUSICA-Fortran library, the library must be detectable through 'pkg-config'. Specifying MUSICA=true on the MPAS 'make' command line will cause MPAS to link with the MUSICA library; for example: make gnu CORE=atmosphere MUSICA=true Note that, although the use of the MUSICA library will likely be within the context of the atmosphere core only, compiling any core with MUSICA=true will cause the MUSICA library to be linked with that MPAS model core's executable. If the MUSICA-Fortran library has been linked successfully, the build summary will include the message MPAS was linked with the MUSICA-Fortran library version 0.10.1. (possibly with a different version number); additionally, when running the compiled core executable, its log file will contain a message indicating MUSICA support as well as the MICM library version, e.g.: MUSICA support: yes - MICM version: 3.8.0 * develop-add-musica-lib: Add support for linking with the MUSICA-Fortran library
This merge introduces fixes from MPAS v8.3.0, and it connects the v8.3.0 tag to future commits on 'develop'. * tag 'v8.3.0': Add 13 to the possible values for config_init_case in init_atmosphere Registry Note in description of config_lu_supersample_factor applicability to USGS Add 'MODIFIED_IGBP_MODIS_NOAH_15s' to possible_values for config_landuse_data Copy streams.init_atmosphere in the setup_run_dir.py script for MPAS-A Update version number to 8.3.0
This merge introduces fixes from MPAS v8.3.1, and it connects the v8.3.1 tag to future commits on 'develop'. * tag 'v8.3.1': Update MMM-physics tag in Externals.cfg to fix .F90 re-compilation issue Fix bug in vertical interp of humidity for LBCs when levels are given top-to-bottom Update version number to 8.3.1
…re core This commit adds a call to the mpas_initialize_vectors routine for init case 13 (CAM-MPAS 3-d grid) to compute the edgeNormalVectors, cellTangentPlane, and localVerticalUnitVectors fields, which may be useful for alternative initialization workflows for CAM-MPAS.
… (PR MPAS-Dev#1351) This merge adds the computation of the edgeNormalVectors, cellTangentPlane, and localVerticalUnitVectors fields for MPAS-A initialization case 13 (CAM-MPAS 3-d grid) through a call to the mpas_initialize_vectors routine. These fields may be useful for alternative initialization workflows for CAM-MPAS. * init_atmosphere/case_13_edgeNormalVectors: Add computation of edgeNormalVectors for case 13 in the init_atmosphere core
The c_attname array in the streaminfo_query function (in mpas_stream_inquiry.F) was allocated to the same size as the attname variable. When attname had no padding, this caused a buffer overflow when appending a C null terminator (c_null_char) to c_attname.
Tests fail when removing an input alarm from an output list or an output alarm from an input list due to the absence of proper error handling in MPAS_stream_mgr_remove_alarm in mpas_stream_manager.F. The test checks for an error when attempting to remove an alarm from the opposite list, but the error handling is missing, causing the test to fail.
…alarm Previously, some logic paths (e.g., passing the wrong direction to MPAS_stream_mgr_remove_alarm) would log an error without affecting the code's execution state. The fix can be verified by running the test core, where the previously failing stream tests now pass.
Error logging is no longer necessary in mpas_stream_list.F, as error codes and logging are now handled in mpas_stream_manager.F. The related macros have been commented out.
…o develop (PR MPAS-Dev#1354) This merge introduces error checking to the MPAS_stream_mgr_remove_alarm subroutine in mpas_stream_manager.F, ensuring that errors returned by the MPAS_stream_list_remove subroutine are correctly handled. Previously, specific logic paths (e.g., passing an incorrect direction to MPAS_stream_mgr_remove_alarm) would log an error without affecting the state of the code. This issue has been addressed by adding the necessary error handling, which now ensures the correct error codes are set and logged when errors occur. Additionally, redundant error logging in the mpas_stream_list_module (specifically in the mpas_stream_list_remove subroutine) has been removed, as the newly added error checking now handles and logs these errors correctly. The changes in this merge can be verified by running new stream alarm tests that have been added to the test core.
…velop (PR MPAS-Dev#1352) This merge corrects a buffer overflow in the streaminfo_query function in the mpas_stream_inquiry module; the streainfo_query function is invoked as the 'query' method of instances of an MPAS_streamInfo_type type. In the streaminfo_query function, the local array c_attname was allocated with the same size as the attname argument to the function, and c_attname was later set to contain a C-compatible string representation of attname using the mpas_f_to_c_string subroutine. However, the c_attname array should have been allocated with space for at least one additional character to account for the C null character that is added by the mpas_f_to_c_string routine. At present, aside from the test core, the query method of an MPAS_streamInfo_type instance is only invoked in the atm_setup_packages and atm_get_mesh_stream routines with a attname name of 'input_interval'. In practice, the under-allocated c_attname array didn't appear to cause any issues, but this under-allocation was nonetheless a bug that is corrected by allocating c_attname with len(attname)+1 characters.
Update FindPnetCDF.cmake so PnetCDF::PnetCDF_C sets INTERFACE_LINK_LIBRARIES to pnetcdf instead of only setting INTERFACE_LINK_DIRECTORIES to the parallel-netcdf library directory, ensuring proper linkage in builds.
…(PR MPAS-Dev#1356) This merge fixes CMake linking for the PnetCDF C library by updating FindPnetCDF.cmake so that PnetCDF::PnetCDF_C also sets INTERFACE_LINK_LIBRARIES to pnetcdf instead of only setting INTERFACE_LINK_DIRECTORIES to the parallel-netcdf library directory. * cmake/pnetcdf-linking-bug-in-findpnetcdf: Fix CMake linking for PnetCDF C library
Unit tests were added for the mpas_stream_list module, covering stream list creation, insertion, querying, and removal. The tests fail when inserting duplicate streams adjacent to each other, either as the first or last stream in the list. The bug is in the MPAS_stream_list_insert subroutine in the mpas_stream_list module, which does not correctly handle duplicates in these cases.
The original code allowed adjacent duplicate streams to be inserted into the list, which caused incorrect behavior when adding a duplicate stream next to an existing one. The bug was fixed by updating the insertion logic to properly reject adjacent duplicate streams. The new code checks for duplicates during insertion and prevents adding the stream if it is already in the list, even if adjacent. The mpas_test_insert_duplicate_at_begin and mpas_test_insert_duplicate_at_end tests in the mpas_stream_list test suite confirm that these changes fix the bug.
Fix bug in mpas_stream_list_insert that could unlink the head node when a duplicate stream was inserted. Moved nullify(stream % next) calls into the relevant conditional blocks to ensure new streams are only linked after passing duplicate checks. Prevents inadvertent modification of the list when duplicate insertions occur.
…er_ints The 'pointer' attribute is unnecessary for the 'inlist' argument, and requiring the inlist argument to be a pointer precluded the use of mpas_dmpar_scatter_ints for arrays that are not pointers, e.g., allocatable arrays or array constructors. Additionally, since the inlist argument is used only as the first argument to MPI_Scatterv (i.e., as the 'sendbuf' argument of MPI_Scatterv), it can be declared as an intent(in) in the mpas_dmpar_scatter_ints routine. The changes in this commit have also been found to resolve runtime errors occurring with certain compiler and MPI library combinations, specifically, nvfortran and OpenMPI 5.x.
…PAS-Dev#1361) This merge removes the pointer attribute from the 'inlist' dummy argument of the mpas_dmpar_scatter_ints routine. The pointer attribute is unnecessary for the inlist argument, and requiring the inlist argument to be a pointer precluded the use of mpas_dmpar_scatter_ints for arrays that are not pointers, e.g., allocatable arrays or array constructors. Additionally, since the inlist argument is used only as the first argument to MPI_Scatterv (i.e., as the sendbuf argument of MPI_Scatterv), it can be declared as an intent(in) argument in the mpas_dmpar_scatter_ints routine. The changes in this merge have also been found to resolve runtime errors occurring with certain compiler and MPI library combinations, specifically, nvfortran and OpenMPI 5.x. * framework/dmpar_scatter_dummy_pointer: Remove 'pointer' attribute from 'inlist' argument of mpas_dmpar_scatter_ints
…nt for the vertically (column-wise) semi-implicit acoustic/gravity-wave integration. Presently the coefficent is a single constant. We now allow this coefficient to vary with model and interface levels as a function of height zeta between two values with a transition layer between the two. This commit, to the Registry only, adds namelist control variables for specifying the lower and upper values of epssm and the lower and upper heights of the transition region for these values. The commit also includes Registry-defined arrays storing the values (1 +- epssm(z))/2 at the levels and interfaces needed in the solution procedure.
…_core.F to initialize the Registry-defined arrays storing the values (1 +- epssm(z))/2 at the levels and interfaces needed in the solution procedure. The initialization occurs at model integration start up. The vertically varying epssm is NOT active at this point; the simulation still uses the constant value from config_epssm.
…ev#1353) This merge fixes a bug in the mpas_stream_list module where adjacent duplicate streams were allowed to be inserted into a list. The issue was in the MPAS_stream_list_insert logic, which has been updated to properly reject adjacent duplicates. As part of this merge, unit tests for the mpas_stream_list_module have been added to the 'test' core. The fix for the duplicate insertion bug is tested by the mpas_test_insert_duplicate_at_begin and mpas_test_insert_duplicate_at_end tests.
This commit introduces a new include file mpas_halo_interface under src/framework in order to remove the repeated definitions of the generic interface halo_exchange_routine and replace them with an include.
…S-Dev#1359) This merge introduces a new include file under the src/framework directory, mpas_halo_interface.inc, that contains the definition of the halo_exchange_routine abstract interface. By including this file in other files, four separate definitions of the halo_exchange_routine interface have been eliminated, specifically, in: - mpas_atm_time_integration.F - mpas_atmphys_todynamics.F - mpas_atm_core.F - mpas_atm_halos.F
github-project-automation
Bot
moved this from Needs evaluation
to Ready to merge
in MPAS-A PRs to process
dustinswales
moved this from Ready to merge
to Needs evaluation
in MPAS-A PRs to process
Collaborator
Author
|
@dustinswales - after updating to the top of noaa/develop this evening, the hrrrv5 rap-summer test is failing for the feature run. It's crashing with "Segmentation fault - invalid memory reference." The Github artifact isn't getting created following the crash, so I don't think I can inspect log.atmosphere.0000.out for insight into why it's crashing. I updated the EOT PR to the top of noaa/develop and all of the tests run OK there. Thus, I'm thinking it has to be something with the model...but I'm not sure why it would fail on this specific test and not any of the others. Could there be something with the CI that I'm missing? Any thoughts? |
Collaborator
correct me if I'm wrong, but it looks like it's not being compiled with debug flags, which may be the only way to extract information to correct this problem, right? |
Collaborator
|
@clark-evans @joeolson42 It looks like the tests started failing after you added changes for MYNN-EDMF. |
Collaborator
Author
The first time around, it wasn't - it was using an older hash. That led to all 18 tests failing. I then updated the hash to match the top of noaa/develop, which leads to 17 passing and 1 failing. |
Collaborator
|
@joeolson42 Unfortunately, the HRRRv5 tests won't run in Debug mode, so we only do these in Release mode. (We should fix this IMO) @clark-evans I don't know if this will work, but you could try running the tests with Debug and see if any helpful errors/warning pop up. |
Collaborator
Author
It worked, though now the rap - winter test also fails. The backtrace appears identical between the two failing runs, and of course it would have to be with the RUC LSM (because that's the catch-all for everything): Program received signal SIGFPE: Floating-point exception - erroneous arithmetic operation. Backtrace for this error: Of these, only mpas_atmphys_driver.F and mpas_atm_core.F changed with the v8.4 code merge, and neither changed in or around the offending code. Because it runs for some cases and not others, my guess is that it's more likely an instability than something specifically associated with the merge. I'll tinker around and see what I might be able to figure out... |
Collaborator
|
@clark-evans Yeah, we turned off the debug tests do to a failure with RUC. So until that is resolved, if ever(?), using debug mode won't be helpful here. |
mdtoyNOAA
approved these changes
Jun 3, 2026
Collaborator
Author
|
There was an issue with the GFL submodule, but matching that to the current top of noaa/develop did not resolve the issue. I'm left to think it's something in one of the commits to noaa/develop since May 1st (when I'd previously updated this PR) -- not necessarily something wrong in those commits, but rather something to make the code a bit more fragile. The commits since May 1st are:
Thus, I'm thinking either #248 (as suggested by Dustin) or #252. I'll see if I can revert back to where things were yesterday afternoon and then pick-and-choose from the above commits to try to isolate the problem. |
clark-evans
force-pushed
the
merge_upstream_8.4.0
branch
from
630fe08 to
dbc4faf
Compare
* v8.3.1-2.28: Hotfix for CI tests --------- Co-authored-by: dustinswales <dswales@ucar.edu>
clark-evans
force-pushed
the
merge_upstream_8.4.0
branch
from
9970088 to
67bf1cc
Compare
clark-evans
force-pushed
the
merge_upstream_8.4.0
branch
2 times, most recently
from
cd17fc4 to
b38b07d
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR brings in the upstream v8.4 release. There were some conflicts that needed to be resolved, as detailed below, but it was a relatively clean merge. Thanks to @barlage for the instructions he put together for last year's merges.
Note: I don't think we want to squash merge this PR because we want to bring in the upstream PR history.
Full List of Conflicting Files
README.md src/core_atmosphere/CMakeLists.txt src/core_atmosphere/Registry.xml src/core_atmosphere/dynamics/mpas_atm_time_integration.F src/core_atmosphere/physics/Makefile src/core_atmosphere/physics/mpas_atmphys_init.F src/core_atmosphere/physics/mpas_atmphys_initialize_real.F src/core_atmosphere/physics/physics_wrf/Makefile src/core_atmosphere/utils/Makefile src/core_init_atmosphere/Makefile src/core_init_atmosphere/Registry.xml src/core_init_atmosphere/mpas_init_atm_cases.F src/core_landice/Registry.xml src/core_ocean/Registry.xml src/core_seaice/Registry.xml src/core_sw/Registry.xml src/core_test/Registry.xml src/driver/Makefile src/framework/Makefile src/operators/MakefileMost of the conflicts were trivial to address. The most substantial were (1) conflicts due to GSL-added physics, (2) conflicts due to NSSL-added horizontal filters, and (3) some minor conflicts due to upstream changes in how ESMF and SMIOL includes are handled. I updated our code to match the upstream changes for (3).
Mandatory Questions
Reviews
I listed the entire core development team because this is a bigger merge.