Add parallel I/O output system#713
Conversation
4c009fd to
b226a95
Compare
beefeab to
0d53f58
Compare
Co-authored-by: Lachlan Whyborn <lachlan.s.whyborn@gmail.com>
This is done to allow access to private components of derived types in child submodules. See this bug report for more details: https://community.intel.com/t5/Intel-Fortran-Compiler/Intel-oneAPI-bug-with-submodules/td-p/1347530
0d53f58 to
cfb572c
Compare
cfb572c to
458d67d
Compare
458d67d to
df516f5
Compare
|
Hi @abhaasgoyal @Whyborn, thank you for your time in reviewing these PRs! This one is now ready for review, and no rush of course, it's the heaviest one so far! As an introduction, it might be useful to read these developer documentation pages:
Benchcab is running currently - I had to make a small change to achieve bitwise compatibility, I will update the status soon. Thank you again! |
Whyborn
left a comment
There was a problem hiding this comment.
Done a first pass. My main issue at the moment is with the structure of the output_variable_t definition. The current structure is amenable with what already existed within CABLE, but I don't see it being amenable to the planned user output API. The current structure will lead to a combinatory explosion of the number of output variables defined, e.g. a variable with 2 allowed reductions would require 15 output_variable_t, for each combination of reduction (3 including no reduction) and aggregation method (5). These would be allocated regardless of whether the variable is written or not.
I think it would make more sense to have a variable definition, which has only one instance per internal variable. It would contain a reference to the target variable, dimensionality and attributes for the variable. Then we use this and the information supplied by the user's output definition to only define the output variables that are actually going to be written.
| real(kind=real64), dimension(:,:,:), pointer :: source_data => null() | ||
| end type aggregator_real64_3d_t | ||
|
|
||
| interface new_aggregator |
There was a problem hiding this comment.
What's the reason for separating the creation of a new aggregator into new and init phases? Looking at the code, I don't see anything that would stop an aggregator being initialised with new_aggregator(source_data=..., method=...)? Within the aggregator_mod itself the handling of each bit may be functionally separate, but from the "user" POV it seems logical to group this into a single call.
There was a problem hiding this comment.
This is a sort of temporary file right? To replicate the current output behaviour on main, while the new API for defining the output is developed?
| real :: scale_by = 1.0 | ||
| !* A multiplicative factor to apply to the native diagnostic values when | ||
| ! writing output. | ||
| real :: divide_by = 1.0 |
There was a problem hiding this comment.
Why provide this if we already provide a scale_by? If they want a division, they can just supply 1 / divide_by (and it's a tiny bit more efficient to do a division once then apply multiplications, than to do repeated divisions)
| character(256) :: value !! Value of the attribute | ||
| end type | ||
|
|
||
| type, public :: cable_output_variable_t |
There was a problem hiding this comment.
I feel like it would make our lives easier in the long run to separate the definitions of output variables, output parameters and restart variables, rather than handle them in one big blob and separate via logical flags on the type. I'm guessing this was something you thought about- what made you decide to do it this way?
| character(64), parameter :: NATIVE_DIM_NAME_PATCH = "patch_native" | ||
| character(64), parameter :: NATIVE_DIM_NAME_PATCH_GLOBAL = "patch_global_native" | ||
| character(64), parameter :: NATIVE_DIM_NAME_PATCH_GRID_CELL = "patch_grid_cell_native" | ||
| character(64), parameter :: NATIVE_DIM_NAME_LAND = "land_native" | ||
| character(64), parameter :: NATIVE_DIM_NAME_LAND_GLOBAL = "land_global_native" |
There was a problem hiding this comment.
What is the point of these parameters? I might be missing something, but I didn't see anywhere that using these parameters was functionally different to just using the associated strings, and the strings are already intuitive enough names so I don't think it adds clarity.
This change brings in a new output system based on the parallel I/O infrastructure introduced in #706, and is a direct replacement of the previous output module used for offline CABLE. The main motivation behind this is to add MPI support to the serial offline driver, and eventually, to replace the legacy MPI implementation (#358). This also makes progress towards the proposed output redesign (#715) by introducing the underlying data structures needed for its implementation.
The new output system brings in the following enhancements:
This change should be brought in after #712.
Type of change
Please delete options that are not relevant.
Checklist
Testing
CABLE benchcab runs tested using
ifort2021.10.0.Please add a reviewer when ready for review.
📚 Documentation preview 📚: https://cable--713.org.readthedocs.build/en/713/