Skip to content

Generalized indices: mpp global field updates#1806

Open
J-Lentz wants to merge 18 commits intoNOAA-GFDL:mainfrom
J-Lentz:general_indices_mpp_global_field_impl
Open

Generalized indices: mpp global field updates#1806
J-Lentz wants to merge 18 commits intoNOAA-GFDL:mainfrom
J-Lentz:general_indices_mpp_global_field_impl

Conversation

@J-Lentz
Copy link
Copy Markdown
Contributor

@J-Lentz J-Lentz commented Dec 15, 2025

Description
This PR adds mpp_global_field updates for generalized indices, and causes the unit tests added in #1763 to pass. The MPP_DO_GLOBAL_FIELD_3D_ subroutine is renamed to MPP_GLOBAL_FIELD_, which now takes assumed-rank local and global arguments. Optional xdim and ydim arguments have also been added, which can be used to specify which dimensions are domain-decomposed. MPP_DO_GLOBAL_FIELD_A2A_3D_ is removed, as are the various wrapper subroutines for mpp_global_field (MPP_GLOBAL_FIELD_2D_, MPP_GLOBAL_FIELD_4D_, etc). This PR also adds mpp_domains_mod, which contains new subroutines for packing/unpacking 2D-5D arrays.

How Has This Been Tested?
Builds on C5 with intel-classic. mpp_global_field unit tests pass.

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • Any dependent changes have been merged and published in downstream modules
  • New check tests, if applicable, are included
  • make distcheck passes

Jesse Lentz and others added 8 commits September 3, 2025 15:34
Refactor the mpp_global_field unit tests to test nonstandard dimension
orders. These new tests do not currently pass, as the mpp_global_field
implementation uses hardcoded dimension orders.
Test the `mpp_*_group_update` subroutines with nonstandard dimension orders. A
separate module for shared unit test code has also been created.
Modify the interpolator tests to test all possible dimension orders.
The 2D tests in test_data_override_ongrid.F90 have been modified to test
for generalized indices support.
Update mpp_global_field unit tests to pass `xdim` and `ydim` arguments. Fix
issues with the rewritten mpp_global_field that cause build errors and test
failures.
Create a new file for `arr2vec`, `vec2arr`, and `arr_init`. Add
interfaces for these new subroutines to mpp_domains_mod.
Comment thread mpp/include/mpp_pack.fh Outdated
!* governing permissions and limitations under the License.
!***********************************************************************

!> Pack an array into a vector
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hi, here to make annoying documentation requests for the chat🤖
Could there be a bit more explanation on why packing is needed and where it's used?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I've added two sentence explanations of packing and unpacking in f42babd.

Comment thread mpp/include/mpp_pack.fh Outdated
subroutine ARR2VEC_ (arr, vec, ix, iy, is, ie, js, je)
MPP_TYPE_, intent(in) :: arr(..) !< The array to be packed
MPP_TYPE_, intent(out) :: vec(:) !< The vector to copy the data into
integer, intent(in) :: ix, iy !< Indices of the domain-decomposed dimensions
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Just repeating what was mentioned at yesterday's meeting about ix, iy being unclear. Maybe the word axes is clearer? An example would be great

Copy link
Copy Markdown
Contributor Author

@J-Lentz J-Lentz Apr 9, 2026

Choose a reason for hiding this comment

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

Changed to xdim and ydim in f42babd.

Comment thread mpp/include/mpp_pack.fh
integer, allocatable, dimension(:) :: lb, ub
integer :: n, m
integer :: i1, i2, i3, i4, i5

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

short explanation of what lb and ub?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Added in f42babd.

Comment thread mpp/include/mpp_pack.fh
ub(ix) = ie
ub(iy) = je

m = 0
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

comment this block of code

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I've added an explanation to the declaration of lb and ub, which I think explains what this block is doing.

Comment thread mpp/include/mpp_pack.fh Outdated
end subroutine ARR2VEC_

!> Unpack a vector into an array
!> @ingroup mpp_domains_mod
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Similar requests as ARR2VEC_ above

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Analogous comments added in f42babd.

Comment thread mpp/include/mpp_pack.fh

!> Initialize an assumed-rank array
!> @ingroup mpp_domains_mod
subroutine ARR_INIT_ (arr, val)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Comment on when to call this subroutine? When it's needed?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Brief explanation added in f42babd.

Comment thread mpp/include/mpp_pack.fh Outdated
MPP_TYPE_, intent(in) :: arr(..) !< The array to be packed
MPP_TYPE_, intent(out) :: vec(:) !< The vector to copy the data into
integer, intent(in) :: ix, iy !< Indices of the domain-decomposed dimensions
integer, intent(in) :: is, ie, js, je !< Starting and ending indices of the x and y dimensions
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Likewise, examples.. something like array(is:ie, js:je) for ix=1, iy=2
and array(js:je, is:ie) for ix=2, iy=1?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I've added some examples to the documentation for is, ie, js, je.

Copy link
Copy Markdown
Contributor

@uramirez8707 uramirez8707 left a comment

Choose a reason for hiding this comment

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

Minor question, but this looks to me.

Comment thread CMakeLists.txt
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why are the changes in CMakeLists necessary?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

They're not necessary. I did some back-and-forth with Gemini trying to determine the cause of a CI build failure and while none of its suggestions ended up being the actual issue, they do seem like genuine improvements, so I kept them in the PR. I can git revert CMakeLists if anyone disagrees.

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.

3 participants