Add 3D gather/scatter capability - #154
Conversation
…the same fypp used for COLLAPSE=2
awnawab
left a comment
There was a problem hiding this comment.
Thanks a lot for this development @pmarguinaud 🙏 We don't use the gather-scatter functionality, so we would support replacing the existing gather-scatter with gather-scatter-collapse1. Please proceed however you see fit.
I've left a few comments otherwise. I would prefer to restrict the fypp mapping to the rank-suff matrix, and for further templating moving the loop inside the files themselves. Since this is purely a stylistic consideration, I'll leave the final call up to you. But it would be nice to have a green CI before we merge.
| #ifdef OMPGPU | ||
| !$OMP TARGET | ||
| #else | ||
| !$ACC SERIAL PRESENT (ID_FILTERED) COPYIN (YL_FGS) IF (JPASS == 2) |
There was a problem hiding this comment.
Compilation is failing for this test in the OpenACC builds. That would be nice to fix before this can be merged. My best guess would be a missing CONTIGUOUS attribute on ID_FILTERED.
There was a problem hiding this comment.
What version of nvhpc do you use ?
There was a problem hiding this comment.
It is quite old. Maybe we should upgrade to a 2026 release ?
I have tried 24.5 on ECMWF cluster, but I cannot reproduce the problem:
ec:/perm/sor/field_api/build
There was a problem hiding this comment.
Yes I agree, but a lot of our downstream CI also uses nvhpc 24.5 and that's not trivial to update (unlike here), so I'd like to keep the hpc ci at 24.5 (we are testing a newer 25.9 in the github CI already) until our downstream CI has been updated.
I'm happy to checkout your branch and try and reproduce the problem myself, but I'm moving house over the next week so I'll probably try this on Monday the 27th (I hope that's ok, sorry for the inconvenience).
There was a problem hiding this comment.
Thank you, I will be waiting.
There was a problem hiding this comment.
I managed to reproduce the issue offline, and it's a compiler bug related to the !$ACC SERIAL IF directive. The cleanest workaround to me seems to be to move the condition to Fortran rather than in the openacc pragma. If you agree I can push a commit here with the workaround.
There was a problem hiding this comment.
Yes I agree, please proceed
|
|
||
| CALL FGS%SCATTER () | ||
|
|
||
| DO JBLK = JBLKL, JBLKU |
There was a problem hiding this comment.
We're failing somewhere here in the cray builds. Since we don't use the gather scatter functionality, I would be ok with this test being excluded from the cray CI run (with something like a conditional ctest -E collapse2_lastdim.
| #! granted to it by virtue of its status as an intergovernmental organisation | ||
| #! nor does it submit to any jurisdiction. | ||
|
|
||
| MODULE FIELD_${RANK}$${SUFF}$_GATHER_COLLAPSE${COLLAPSE}$_MODULE |
There was a problem hiding this comment.
Do we really need a separate module per derived type here? If possible, I would prefer to keep the fypp expansion to the rank-suff matrix, and a loop in here for the different values of COLLAPSE. This is effectively what we are already doing in many places for WRAPPER, OWNER types in the same module. You could then simply add the relevant files to the ranksuff_srcs and non_ranksuff_srcs lists.
…ion is larger than NFLEVG. This can happen when processing arrays with extra levels such as : REAL (KIND=JPRB) :: ZFLD (NPROMA, 0:NFLEVG)
|
The openacc tests are passing now. We can ignore the lumi tests, since they are already failing on main due to what appears to be filesystem issues on lumi. EDIT: I spoke too soon, the lumi ci jobs passed on main. I think I understand the underlying issue, so I can babysit those in this PR and file a proper fix afterwards. |
awnawab
left a comment
There was a problem hiding this comment.
Thanks a lot @pmarguinaud for incorporating my earlier suggestions 🙏 Some further small changes will really help to remove a lot of redundant cmake, so it would be nice to incorporate those.
As for the failing tests, the cce seg faults appear genuine. Considering we don't rely on this functionality, I would be happy with simply marking the new *_lastdim test as expected to fail for cray builds, and then you can investigate and fix it independently from this PR (I will create a github issue to track it).
The failing rocm afar tests are related to the baseline main moving since this PR was opened (apologies, but that was an important development 😬). For PRs from forks, github uses the state of the build-hpc workflow as in main, but that requires code changes you are missing here. Another merge/rebase with main should fix the rocm afar tests, considering they were green before PR #157 was merged to main.
| string (TOLOWER ${SUFF} suff) | ||
| foreach (RANK RANGE 1 5) | ||
| foreach (FUNC IN ITEMS _shuffle _gather) | ||
| add_custom_command ( |
There was a problem hiding this comment.
I'm pretty sure we don't need this fypp expansion here. If we add _shuffle_collapse2 and _gather_collapse2, we can delete this entire fypp expansion section.
| endforeach () | ||
| endforeach () | ||
|
|
||
| add_custom_command ( |
There was a problem hiding this comment.
Same here, adding field_gathscat_collapse2_module field_gathscar_collapse2_type_module to non_ranksuff_srcs should be enough and the manual fypp expansion here can be removed.
Description
Both
FIELD_GATHSCAT(already in the Field API library) andFIELD_GATHSCAT_COLLAPSE2(introduced in this PR) implement high-level gather-scatter patterns:This pattern is essential for algorithms that need to process only a subset of grid points (e.g., convective columns, cloud-covered areas, or land points); this has already proven to improve performance quite a lot.
FIELD_GATHSCATis currently used in the shallow convection algorithm of ARPEGE.FIELD_GATHSCATis used when the selection condition is 2-D and you want to keep all intermediate dimensions (e.g., vertical levels, variables) intact in the gathered array. It works with fields of any rank ≥ 2.The purpose of
FIELD_GATHSCAT_COLLAPSE2is to be used when the selection condition is 3-D and you need to collapse the two leading spatial dimensions of arrays into one. This is typical for physics parametrizations that operate on a subset of atmospheric cells. We will use it in the AROME micro-physics.Several tests have been added to exercise the newly created code.
Tested on A100 with nvhpc/26.5 and MI300X with afar/23.2.0.
Tested in AROME, with nvhpc/26.5 on A100.
Contributor Declaration
By opening this pull request, I affirm the following: