ccpp_prebuild.py: add support for chunked arrays #519
Conversation
…_data with the word 'blocked' replaced by 'chunked' everywhere, no other changes)
…it, and expected failing run
…mpatibility with blocked data structures
…ring for assign tests
…n in the run phase in scripts/mkstatic.py
…active arrays are passed currently in ccpp_prebuild
| array_size = [] | ||
| dim_substrings = [] | ||
| for dim in var.dimensions: | ||
| # DH* TODO |
There was a problem hiding this comment.
This needs to be addressed in a follow-up PR, because we can't switch the ufs-weather-model and other models using potentially inactive variables to contiguous arrays without fixing the current way we handle them.
| f"horizontal dimension for {var_standard_name} is {var.dimensions}") | ||
| # *DH | ||
|
|
||
| # DH* TODO: WE CANNOT ACTIVATE USING EXPLICIT HORIZONTAL DIMENSIONS |
There was a problem hiding this comment.
Same as above, we need to fix the way we handle inactive variables to make the switch to contiguous arrays.
climbfuji
requested review from
grantfirl,
gold2718 and
mkavulich
as code owners
climbfuji
added
enhancement
ccpp_prebuild
| cdata => ccpp_data_chunks(ic) | ||
| call ccpp_physics_run(cdata, suite_name=trim(ccpp_suite), ierr=ierr) | ||
| if (ierr/=0) then | ||
| write(error_unit,'(a,i3,a)') "An error occurred in ccpp_physics_run for block", ic, ":" |
There was a problem hiding this comment.
Should "block" be "chunk" here?
There was a problem hiding this comment.
This looks fine to me from a coding point-of-view. A big question is "what methods of handling 'inactive' variables are correct, or at least consistent, with how this works"? And, by "handling" inactive variables, are you referring to both sides (i.e. host side and scheme side)?
There was a problem hiding this comment.
I'm sorry that I do not have time for a really detailed review but I don't see anything troublesome on a quick review (except that I do not really understand how this transition will work with changes to the UFS and did not understand @climbfuji's comment in mkstatic.py).
|
Waiting for RTs to succeed on ufs-community/ufs-weather-model#2066 |
|
@dustinswales @grantfirl I think we can move ahead with this PR and the associated ccpp-physics PR. For that one, we wanted to wait for the outcome of our active attribute discussion. Now that we know that we would have to make changes anyway to the ozone physics, we might as well merge this here first (make it compliant with current CCPP rules) and then update ALL of the potentially inactive (= optional) variables in a next round? |
|
@climbfuji testing is complete on ufs-wm PR #2066. Are you able to merge this ccpp-physics framework PR? |
|
I can merge both but would prefer if @dustinswales or @grantfirl did that |
…run compile jobs of regression test suite only & -mcmodel=medium gnu.cmake option (also includes #2142) (#2066) * UFSWM - Add option to `rt.sh` to run compile-only tests * FV3 - Update submodule pointer (NOAA-EMC/fv3atm#747) * ccpp-physics - Update submodule pointer (ufs-community/ccpp-physics#150) * ccpp-framework - Update submodule pointer (NCAR/ccpp-framework#519) * Update WM license and documentation logo * Update GNU.cmake: -mcmodel=medium
Description
Add support for using chunked arrays to
ccpp_prebuild.pywhile maintaining backward compatibility with the use of blocked data structures.This PR adds support (and a test) for using chunked arrays as an alternative to blocked data structures to
ccpp_prebuild.py. The idea behind this is that we can convert the ufs-weather-model to using contiguous arrays that are sent in chunks to the physics for (OpenMP-)parallel processing in the time integration (run) phase. The expectation is that this is at least as fast as the current implementation that uses blocked data structures (because the need to copy non-contiguous arrays into contiguous arrays and back for theinit,timestep_init,timestep_finalizeandfinalizephases is removed). There is also evidence from extensive testing with CESM by CISL that sending chunks of contiguous arrays is faster.Note. This PR does not yet make any changes for the ufs-weather-model, NEPTUNE or the SCM. It only provides the capability (and a test case that can serve as a how-to) for using contiguous arrays that can be sent to the physics in chunks in the
runphase. There are limitations with how we (withinccpp_prebuild) treat inactive (unallocated) host model data that do not allow us to switch over to contiguous arrays immediately. I left several notes in the files changed in this PR to highlight the problems. We need to find a different way to handle inactive data as a next step so that we can make the switch to contiguous arrays afterwards. This will have the positive (!) side effect that it will address concerns raised by NCO that the current approach violates operational requirements.User interface changes?: No
Issues
Fixes #520
Testing
test removed: none
test added: added
test_prebuild/test_chunked_data/*unit/system tests: all capgen unit/system tests pass (
test/run_test.sh); all ccpp_prebuild unit/system tests pass (everything intest_prebuild)end-to-end tests: full regression tests with ufs-weather-model on Hera with Intel and GNU against existing baseline; all tests pass; see also ufs-community/ufs-weather-model#2066 for final regression testing
manual testing: extensively tested on my macOS with apple-clang+gfortran
Dependencies / associated PRs
This PR is part of a set of PRs that need to be tested and merged together: