Generalized Indices: MPP_REDISTRIBUTE#1886
Conversation
abrooks1085
requested review from
bensonr,
rem1776,
uramirez8707 and
vithikashah001
as code owners
uramirez8707
left a comment
uramirez8707
left a comment
There was a problem hiding this comment.
Just minor comments regarding the new argument name. Looks good to me otherwise.
| jo = j - js + 1 | ||
| do i = is,ie | ||
| io = i - is + 1 |
There was a problem hiding this comment.
These io and jo variables appear never be used?
| logical, intent(in), optional :: complete, free | ||
| integer, intent(in), optional :: list_size | ||
| integer, intent(in), optional :: position | ||
| integer, intent(in), optional :: axis_to_dim_in(3) |
There was a problem hiding this comment.
Please document this new arguments.
Can we make it consistent with the other subroutines? I think we used dim_order.
| logical, intent(in), optional :: complete, free | ||
| integer, intent(in), optional :: list_size | ||
| integer, intent(in), optional :: position | ||
| integer, intent(in), optional :: axis_to_dim_in(3) |
There was a problem hiding this comment.
should this be 2? axis_to_dim_in(2)
| integer, intent(in), optional :: position | ||
| integer, intent(in), optional :: axis_to_dim_in(3) | ||
| MPP_TYPE_ :: field3D_in (size(field_in, 1),size(field_in, 2),size(field_in ,3)*size(field_in ,4)) | ||
| MPP_TYPE_ :: field3D_out(size(field_out,1),size(field_out,2),size(field_out,3)*size(field_out,4)) |
There was a problem hiding this comment.
Unless I'm misunderstanding something, it seems to me that this approach of fusing the third and fourth dimensions should only work with (i,j,*,*) or (j,i,*,*) dimension orders. Are these the only cases that we need to support?
There was a problem hiding this comment.
Thanks, I think you are right to question the logical equivalence here.
I was initially focused on preserving the storage layout: fusing storage dimensions 3 and 4 into a rank-3 view preserves Fortran column-major ordering and avoids a repack or transpose. However, looking more carefully at MPP_DO_REDISTRIBUTE_3D_, the routine redistributes logical x/y rectangles and carries the full logical z extent along with each communicated region. The part I missed is the fourth dimension may be fused with an intended decomposed horizontal axis.
I'm going to instead treat the fourth dimension as a list of independent 3D views, one for each field(:,:,:,l) slice. That is, I'll pass a 1D list of base addresses/pointers, where each entry points to the start of one contiguous 3D slice of the original 4D field. Each 3D slice can then be passed through the existing 3D redistribution path using axis_to_dim without copying.
This will preserves the intended logical behavior: x/y determine the communicated domain region, while the full logical z extent and each fourth-dimension slice are carried along.
I can also limit the performance impact, by making packing/unpacking loops oriented towards storage layout instead of logical layout like I have here. Using axis_to_dim, compute the logical bounds for x/y/z, map them onto storage dimensions, and traverse each 3D view in column-major storage order. That keeps the innermost loop unit-stride where possible while still preserving the logical decomposition.
Thanks again for pointing this out.
| MPP_TYPE_ :: field3D_in (size(field_in, 1),size(field_in, 2), & | ||
| & size(field_in ,3)*size(field_in,4)*size(field_in ,5)) | ||
| MPP_TYPE_ :: field3D_out(size(field_out,1),size(field_out,2), & | ||
| & size(field_out,3)*size(field_out,4)*size(field_out,5)) |
There was a problem hiding this comment.
Same as above. Unless I'm misunderstanding, it seems to me that this should only work with (i,j,*,*,*) or (j,i,*,*,*) dimension orders.
3 participants
Description
This PR adds generalized storage-layout support to MPP_REDISTRIBUTE for 2D–5D fields. The main change is to allow callers to pass an optional
axis_to_dimmapping so arrays whose logical (x,y,z) axes are stored in a different physical dimension order can be redistributed correctly. The PR modifiesmpp_do_redistribute.fh,mpp_update_domains2D.fh, and adds a regression path intest_mpp_domains.F90/test_mpp_domains2.sh.axis_to_dim_in(3)argument through theMPP_REDISTRIBUTE_*D_interfaces.axis_to_dim(logical_axis) = storage_dimension, so for example(/3,1,2/)means:xis stored in dimension 3yis stored in dimension 1zis stored in dimension 2How Has This Been Tested?
amdbox + intel
Checklist:
make distcheckpasses