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

Merge MPAS-Dev::develop into release-stable #8

Merged
merged 385 commits into from
Jan 11, 2021

Conversation

jjguerrette
Copy link

Merge latest changes from upstream repository. Works with mpas-jedi branch of the same name. See that corresponding PR for more information on mpas-bundle behavior.

There are a lot of commits in this PR due to it having been a long time since such a merge was conducted. The conflicts between the two branches are dealt with in commits 5beb733 and after. Some small code changes from the early days of MPAS-JEDI development is removed, because it no longer serves a purpose.

After this PR is merged, release-stable will be nearly up to date with MPAS-Dev::develop with the following exceptions for code that is only in the JCSDA-internal fork:

  • convective/cloud diagnostics for PANDA-C
  • 2-stream I/O with a static file shared across all dates
  • adjoint of some halo communications used for static B
  • bug fixes in core_init_atmosphere for initializing rho_zz and xice
  • optional cmake build process and dependence on the jedi-cmake repository

Closes #7

mgduda and others added 30 commits April 23, 2019 13:41
…olation

This commit adds three new fields to the Registry.xml file,
bdyMask{Cell,Edge,Vertex}, plus new code to ensure that pixels of static
data that are mapped to a cell actually belong to that cell. This latter change
is needed for limited-area meshes, since the remapping of source pixels for
terrain, land use, soil category, monthly albedo, monthly vegetation fraction,
and maximum snow albedo using a path-based search does not guarantee that
the pixel actually lies in the cell where the search terminates (in particular,
in the case of pixels outside of a limited-area mesh, where the search
terminates at the boundary cell closest to, but not containing, the pixel).

If the bdyMask{Cell,Edge,Vertex} fields are not present in the input file, they
will receive a default value of zero, which will effectively deactivate the new
logic added in this commit, giving results as in prior versions of the code.

At present, a bdyMaskCell value of 7 (via the parameter nBdyLayers) is assumed
to represent the value for cells on the outermost edge of a limited-area mesh;
however, if regional meshes make use of different numbers of relaxation cell
layers or specified cell layers, this will need to be generalized.

Eventually, some of the new functions in this commit should be considered
for migration to mpas_geometry_utils.F.
When applying terrain smoothing on limited-area meshes, we now set the terrain
for non-existent neighbor cells to the same value as the terrain in the cell
where the terrain filter is being applied.
Similarly to what was done for terrain in the previous commit, update
the smoothing of coordinate surfaces so that references to non-existent
neighbors yield the same value of hx as in the cell itself.

This commit also removes some commented-out, unused logic in the coordinate
surface smoothing code.
This commit adds logic to set the index of non-existent cells -- those on
the exterior side of a boundary edge in a limited-area mesh -- to the same
as the cell that does exist. This is done only to avoid bad values in
the computed z-metric terms zb and zb3 for boundary edges.

The exact value assumed in the computation of zb and zb3 for edges on
the outermost boundary of a limited-area mesh should not matter, since
these edges are specified-zone edges that border specified-zone cells.
…Dev#204)

This merge enables the production of static fields and vertical grids for
limited-area meshes in the init_atmosphere core.

* init_atmosphere/regional_static:
  Avoid unpredictable values in metric terms zb, zb3 for boundary edges
  Modify coordinate surface smoothing to work with limited-area meshes
  Modify terrain smoothing to work with limited-area meshes
  Improvements to init_atmosphere core to enable regional static interpolation
This commit creates an `ISO_C_BINDING` interface for `read_geogrid.c` file.
As well, it contains a number of other changes to `read_geogrid.c` and its
corresponding callers: `mpas_init_atm_gwd.F` and `mpas_init_atm_static.F`.

Per the ISO_C_BINDING interface, the arguments of `read_geogrid.c` were changed
from pointers, to variables to remove the need to for dereferencing. As well,
the `int len` argument was removed. Instead of prepending a null character at
the end of `fname` with C, one can create a null-terminated c string with
`mpas_c_to_f_string` routine.

In `mpas_init_atm_gwd.F` and `mpas_init_atm_static.F`, a number of changes have
occurred to rarray. First, rarray has been set to `pointer` and `contiguous`.
Instead of passing the Fortran array by reference to C, we now pass the C
address via: `rarray_ptr = c_loc(rarray)`, where `rarray_ptr` is of `type
(c_ptr)` in Fortran.

Also contains fix (from PR review) to the read_geogrid interface variable
fname.
This commit creates an `ISO_C_BINDING` interface for the `pool_hash.c` file. To
handle the interface between Fortran and C, a new function (`pool_hash(...)`
was created in `mpas_pool_routines.F` to act as a wrapper for the `pool_hash.c`
function (`c_pool_hash(...)).

The wrapper function converts the Fortran types into corresponding C types and
calls `c_pool_hash`.

The `len` argument for `c_pool_hash` has been removed and has been
replaced with `strlen(key)` accordingly. Note that this works now because of
the use of `mpas_f_to_c_string` function module, which prepends a null
character onto `key`.

As well note, that if it is desired to create a hash that is greater then
`strlen(key)`, the length will need to be re-added in another commit.

Also contains the fix (from the PR review) to loop until the null character
within `key` instead of using `strlen`.
This commit creates two new `ISO_C_BINDING` interfaces for the functions found
in `random_id.c`: `seed_random()` and `gen_random()`.
This commit adds an ISO_C_BINDING, 2003 standard compilent
interface to two C functions: `compute_ev_2` and `compute_ev_3`
within mpas_ocn_okubo_weiss.F. As well, it removes the `ifdef
UNDERSCORE` in mpas_ocn_okubo_weiss_egienvalues.c.
This commit removes references to -DUNDERSCORE in the top level Makefile as
well as the ACME and CESM Makefile.
This commit adds a new initialization case, case 9, to produce lateral boundary
conditions data for regional MPAS-Atmosphere simulations. At present, the new
routine, init_atm_case_lbc, is just a placeholder that is called from the main
init_atm_setup_case routine when config_init_case == 9 in the namelist file.
The init_atm_case_lbc routine is called in a time loop from config_start_time
to config_stop_time every config_fg_interval seconds.

This commit also updates the description of the config_init_case namelist option
in the init_atmosphere core's Registry.xml file to document the existence of
init case 9.
This commit defines an 'lbc' output stream for writing model LBC files,
an 'lbcs' package to control the writing of the 'lbc' stream and the allocation
of the 'lbc_state' variables, and a new var_struct defining the fields
that are needed in MPAS-A LBC files.

Code has been added to properly set up packages when init case 9 is run,
and appropriate calls to the MPAS stream mananger for the 'lbc' stream
have also been added.
The init_atm_case_lbc routine now produces valid lbc_u, lbc_w, lbc_theta,
lbc_rho, and lbc_{qc,qv,qr} fields. The implementation of the routine borrows
from the init_atm_case_gfs routine, which produces similar fields for
the model initial conditions. In future, it may be possible to factor out
duplicated code between init_atm_case_lbc and init_atm_case_gfs.
If the config_fg_interval value specified in the namelist.init_atmosphere
file doesn't match the output_interval specified for the 'lbc' stream
when producing LBCs, stop the init_atmosphere core, as there's generally
no good reason for not having these intervals match.
This merge creates ISO_C_BINDING interfaces for C functions in the atmosphere and
ocean core to eliminate the need for the -DUNDERSCORE name mangling logic.

It also updates each C source file to be more standards compliant, for example, by
passing in arrays using the c_loc ISO_C_BINDING function (see, e.g.,
read_geogrid.c). Where necessary, the mpas_f_to_c_string routine from
mpas_c_interfacing is used to create null-terminated strings for C, which
eliminates the need to pass in the length of a string in some circumstances
(specifically, read_geogrid.c and pool_hash.c).

Lastly, many arguments to C routines are passed by value to avoid the need to
deference them in C.

*** Conflicts: There were quite a few conflicts in mpas_init_atm_static.F, and
    resolving them involved removing the old code for interpolating VARSSO and
    pre-computed GWDO fields, and adding c_loc and mpas_f_to_c_string calls for
    MODIS-based max snow albedo, monthly albedo, and monthly green fraction.

* framework/dunderscore:
  Remove references to -DUNDERSCORE within Makefiles
  ISO_C_BINDING interface for computer_ev_2/3 for Ocean
  ISO_C_BINDING Interface for random_id.c
  ISO_C_BINDING update for pool hash
  ISO_C_BINDING Interface for read_geogrid.c

Conflicts:
	src/Makefile.in.CESM
	src/core_init_atmosphere/mpas_init_atm_static.F
There were about a dozen write statements in the MPAS-A dynamics that were
used for debugging, but can now be safely removed.
This commit removes print statements guarded by an if-test on "debug_regional",
and it also removes the debugging routine atm_bdy_check_values.
…imulations

The new namelist option, config_apply_lbcs, defaults to .false. and resides in
the new &limited_area namelist group. When set to .true., this option activates
the code that was previously activated by the 'regional_mpas' parameter at
the top of the atm_time_integration module.
…truct

The 'limited_area' package is active whenever the config_apply_lbcs namelist
option is true. This package controls the reading of the 'lbc_in' stream
as well as the allocation of all variables in the 'lbc' var_struct.
…hysics suite

The scalars are reset along the domain boundaries individually (i.e., for qv, qc,
etc.) after scalar transport and after microphysics. Previously, there was no
logic to detect which scalars where actually active and which were not, so it was
necessary to comment or un-comment, e.g., the handling of nr and ni, depending
on which physics suite was being run.

Now, there is a check to see that the index of each scalar is valid before setting
that scalar along the domain boundaries.

Also, it appears that the last argument to mpas_atm_get_bdy_state in the previously
commented-out calls to mpas_atm_get_bdy_state for nr and ni had an incorrect final
argument in the call to mpas_atm_get_bdy_state: this argument should be 'dt' rather
than 'rk_timestep(rk_step)'.
The 'xtime' field was previously being read by the 'lbc_in' stream, but
this is not necessary and in some cases may actually be problematic.
While the 'xtime' field must be present in the LBC files to allow the stream
manager to seek to the correct LBC update file at any given point in
the model simulation, we don't actually need to read the 'xtime' field.
In cases where the time in the LBC file being read does not exactly match
the current simulation time -- because we don't request an exact time match in
the stream manager read calls in mpas_atm_update_bdy_tend -- the xtime field
that was read in would overwrite the model xtime field with an incorrect time.
The 'xtime' field is set to the correct simulation time in the atm_timestep
routine in each timestep, but this happens only after most of the physics
(all non-microphysics schemes, currently) are called.
When computing rho_zz, we use array syntax to divide rho(:,:) by zz(:,:).
The value of zz in the "garbage cell" is generally not predictable, and
this can lead to a divide by zero FPE. To avoid this potential FPE, we
now set zz(:,nCells+1) = 1.0 in mpas_atm_update_bdy_tend. In principle
this should be completely safe, since no other code should rely on
the garbage element for a field taking on any particular value.
…constituents

When the mpas_atm_get_bdy_state routine is called for scalar constituents
(e.g., qv), the calls to mpas_pool_get_array will not find the LBC field in
the tend and state pools as a standard field, leading to warning messages when
the code is compiled in debug mode. These messages are expected and harmless,
so this commit adds calls to set the error mode for the pool routines to
MPAS_POOL_SILENT only for these calls to mpas_pool_get_array.
…efile

The module_sf_noah_seaice.o file depends on module_sf_noahlsm.mod. Occasionally,
parallel builds of MPAS-Atmosphere would fail due to this missing dependency
in src/core_atmoshere/physics/physics_wrf/Makefile. This commit adds a dependency
on module_sf_noahlsm.o for module_sf_noah_seaice.o.
…n boundary

This commit adds a new function, blend_bdy_terrain, that combines the first-
guess terrain field from an intermediate file with the terrain field produced
by the init_atmosphere core's "static interpolation" step along the boundary
cells of regional domains. The new function is called only when a new namelist
option, config_blend_bdy_terrain in the &vertical_grid namelist group, is true.

Terrain blending happens before terrain smoothing and vertical grid generation,
but after static interpolation. The first-guess terrain is taken to be
the SOILGHT field found in an intermediate file described by config_met_prefix
and config_start_time.

This commit also removes the deactivated code in the real-data test case setup
for using the first-guess terrain everwhere.
…ified_physics (PR MPAS-Dev#223)

This merge adds a missing dependency for module_sf_noah_seaice.o in physics_wrf/Makefile.

The module_sf_noah_seaice.o file depends on module_sf_noahlsm.mod. Occasionally,
parallel builds of MPAS-Atmosphere would fail due to this missing dependency
in src/core_atmoshere/physics/physics_wrf/Makefile. This commit adds a dependency
on module_sf_noahlsm.o for module_sf_noah_seaice.o.

* atmosphere/noah_makefile_dependency:
  Add missing dependency for module_sf_noah_seaice.o in physics_wrf/Makefile
This merge fixes a bug in the PIO compatibility check in the top-level Makefile,
wherein the PGI compilers failed to build any of the test programs due to
the order in which arguments were provided in the compilation command. The simple
fix is to move the source file to the front of the compiler arguments.

* framework/pio_pgi_fix:
  PIO Version Compatibility - PGI Bug Fix
…the logical

  config_o3climatology from false to true.

  This change forces the RRTMG long- and short-wave radiation codes to use the
  monthly-mean global climatological ozone data also used in the CAM radiation
  code, instead of a single vertical profile of ozone, one for winter and one
  for summer.

  When config_o3climatology is true, the three-dimensional ozone data is updated
  every day using temporal interpolation of the input global climatological data
  on fixed presssure levels. The three-dimensional data is interpolated to the
  time-varying pressure levels before calls to the RRTMG radiation codes.
Several loops to enforce LBCs over boundary edges needed to cover owned and halo
edges to ensure correct results with multiple MPI tasks, and one OpenMP barrier
in the code to adjust scalars along boundaries needed to be activated to ensure
correct results with more than one OpenMP thread.

The dycore now gives bit-identical results for limited-area domains any
MPI task count and any OpenMP thread count, provided compiler flags like
"-fp-model precise" are added.
…v6.3 (PR MPAS-Dev#183)

This merge sets the use of CAM climatological O3 by default, rather than using
a single profile.

This change forces the RRTMG long- and short-wave radiation codes to use
the monthly-mean global climatological ozone data also used in the CAM radiation
code, instead of a single vertical profile of ozone, one for winter and one
for summer.

When config_o3climatology is true, the three-dimensional ozone data is updated
every day using temporal interpolation of the input global climatological data
on fixed presssure levels. The three-dimensional data is interpolated to
the time-varying pressure levels before calls to the RRTMG radiation codes.

* atmosphere/unified_physics.o3_climatology:
  In ./src/core_atmosphere/Registry.xml, switch the default value of logical ...
Copy link

@snyderdad snyderdad left a comment

Choose a reason for hiding this comment

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

I've not looked through the many individual commits here, but I think we have to update to what they have in develop branch of MPAS-Dev.

@jjguerrette can you confirm that with this PR MPAS-Model will now be up to date with develop branch of MPAS-Dev?

@jjguerrette
Copy link
Author

@jjguerrette can you confirm that with this PR MPAS-Model will now be up to date with develop branch of MPAS-Dev?

Yes, that is what is achieved in commit 5beb733 and after. If you look at the list of commits in the PR, those commits are almost at the bottom of the list. Every commit before that one are the changes made to MPAS-Dev::develop since we performed a similar merge.

I'm not sure if we should do a "squash and merge" or a regular merge. A regular merge would allow for the MPAS-Dev history to be combined with ours, but it is not our usual practice.

@svahl991
Copy link
Collaborator

I'm not sure if we should do a "squash and merge" or a regular merge. A regular merge would allow for the MPAS-Dev history to be combined with ours, but it is not our usual practice.

My opinion is we should do whatever makes our life easiest. In most other repos, that is "squash and merge". That may not be the case with this repo.

Copy link
Collaborator

@svahl991 svahl991 left a comment

Choose a reason for hiding this comment

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

Did not actually review the code changes since they have been externally approved and what is important here is that we stay up-to-date. Since the automated tests say all the ctests pass, that is good enough for me.

@jjguerrette
Copy link
Author

Should I run a short-range cycling experiment to make sure we get similar results?

Copy link
Collaborator

@liujake liujake left a comment

Choose a reason for hiding this comment

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

Do we expect some result changes from this merge with a lot of commits? Maybe better for us to do squash and merge but keeping all those original commit messages in case the results have some bad changes and need to debug.

@svahl991
Copy link
Collaborator

Should I run a short-range cycling experiment to make sure we get similar results?

It would be interesting to know if the "compare" ctests were passing with exact matches or just falling within the accepted tolerances. I'm not sure if there's a simple way to tell that. If the results are matching exactly for gnu-openmpi, then I think a cycling test is probably not needed.

It would normally be fairly simple for me to force a run of the automated cycling test and we could compare cycling results that way. But right now that test isn't working. (JCSDA-internal/mpas-jedi#464)

@jjguerrette
Copy link
Author

That makes sense Steve. I can build and compare with a zero tolerance for the non-forecast applications. If I go to try to do a cycling test, I may run into the same error message that you see in the the automated test. Conversely, it might help your investigation of the error if I try it.

@snyderdad
Copy link

Yes, please.

@snyderdad
Copy link

I pinged Michael Duda about other pending mods listed in this issue. Plan is to start an issue in MPAS-Dev containing that list, then link to PRs as they made and check off items as they are merged.

@jjguerrette
Copy link
Author

Full month cycling yields numerically identical results. See here: https://github.com/JCSDA-internal/mpas-jedi/pull/456#issuecomment-757007159.

Ready to merge pending remaining clang tolerance failure for AD in linear getvalues test.

@jjguerrette jjguerrette merged commit 31ca214 into release-stable Jan 11, 2021
@snyderdad
Copy link

adjoint of some halo communications used for static B

Refers to https://github.com/JCSDA-internal/mpas-jedi/pull/192?

@jjguerrette
Copy link
Author

adjoint of some halo communications used for static B

Refers to JCSDA-internal/mpas-jedi#192?

I think it is this commit actually: b22c340

PR was in my fork here: jjguerrette#10

Here is the feature branch: https://github.com/jjguerrette/MPAS-Model/tree/feature/add_sfvp

@jjguerrette jjguerrette deleted the feature/merge_upstream branch February 17, 2021 22:30
climbfuji pushed a commit that referenced this pull request Aug 26, 2022
Merge latest changes from upstream repository.  Works with mpas-jedi branch of the same name.  See that corresponding PR for more information on mpas-bundle behavior.

There are a lot of commits in this PR due to it having been a long time since such a merge was conducted.  The conflicts between the two branches are dealt with in commits 5beb733 and after.  Some small code changes from the early days of MPAS-JEDI development is removed, because it no longer serves a purpose.

After this PR is merged, `release-stable` will be nearly up to date with MPAS-Dev::develop with the following exceptions for code that is only in the JCSDA-internal fork:
- convective/cloud diagnostics for PANDA-C
- 2-stream I/O with a static file shared across all dates
- adjoint of some halo communications used for static B
- bug fixes in core_init_atmosphere for initializing `rho_zz` and `xice`
- optional cmake build process and dependence on the `jedi-cmake` repository

Closes #7
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.

Merge latest upstream develop into release-stable