-
Notifications
You must be signed in to change notification settings - Fork 19
handle CGDims properly in serial and MPI #969
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
d2c3fd2 to
bb37b7c
Compare
timspainNERSC
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think a smarter structure for the dimensions might be useful, rather than changing the function signature of ModelArray::dimensions depending on how the model is compiled.
| * @param dim The per-dimension size to be set. | ||
| */ | ||
| #ifdef USE_MPI | ||
| static void setDimensions(Type, const MultiDim&, const MultiDim&); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I must admit that I don't like the idea of having a different function signature serial and MPI versions of the code. I wonder if a better solution might be changing the result of ModelArray::dimensions from a plain MultiDim to a vector of not scalars but local-global pairs in the MPI case.
| globalDims[0] = metadata.getGlobalExtentX(); | ||
| globalDims[1] = metadata.getGlobalExtentY(); | ||
| localDims[0] = metadata.getLocalExtentX(); | ||
| localDims[1] = metadata.getLocalExtentY(); | ||
| ModelArray::setDimensions(ModelArray::Type::CG, globalDims, localDims); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, the metadata class could have a function to generate the correct form of the combined local/global dims.
| // Reset dimensions so it is possible to check if they | ||
| // are read correctly from refeence file | ||
| #ifdef USE_MPI | ||
| ModelArray::setDimensions(ModelArray::Type::H, { 1, 1 }, { 1, 1 }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would become ```
ModelArray::setDimensions(ModelArray::Type::H, { { 1, 1 }, { 1, 1 } });
Handle CGDims properly in serial and MPI
TODO
rebase on latest develop to fix failing mac ci once PR ci: fix mac ci workflow to detect venv #970 is mergedFix failing tests for PR #879
ModelArray::setDimensionsto support MPI local and global dimssetDimensionsfunction into the tests and codeBackground
The underlying problem is that the
x_cgandy_cgdimensions are wrong in the initial model file. For example, when I run theconfig_june23.cfgit uses input fileinit_25km_NH.nc. In there the dimensions are listed as follows:They should be
y_cg = 243andx_cg = 309, forCGDegree=2.These get read in here in
ParaGridIO::getModelStateand then set on line 137.nextsimdg/core/src/ParaGridIO.cpp
Lines 81 to 137 in ca3dddb
The
localLengthis then overridden later inBBMDynamics.cppon line 125:nextsimdg/core/src/modules/DynamicsModule/BBMDynamics.cpp
Line 125 in ca3dddb
Note the function
ModelArray::setDimensionsonly sets thelocalLengthand not theglobalLengthand that's why you needed to make the changes here:nextsimdg/core/src/ParaGridIO.cpp
Lines 266 to 267 in ca3dddb
I think this logic needs to be re-written to use the
ModelMetadatasingleton so that it will work for MPI and serial code. Then we need to also modify functionModelArray::setDimensionsor create a new one that will also set theglobalLength.