Skip to content

Commit f53d8c6

Browse files
authored
Fix Halo constructor for DGVectorHolder (#1065)
# Fix `Halo` constructor for `DGVectorHolder` Fixes #1064 ### Task List - [x] Linked an issue above that captures the requirements of this PR - [x] Defined the tests that specify a complete and functioning change - [x] Implemented the source code change that satisfies the tests - [x] Commented all code so that it can be understood without additional context - [x] No new warnings are generated or they are mentioned below - [x] The documentation has been updated (or an issue has been created to do so) - [x] Relevant labels (e.g., enhancement, bug) have been applied to this PR - [x] This change conforms to the conventions described in the README --- # Change Description After simplifying #1005 I accidentally introduced a bug by making the `DGVectorHolder` constructor directly call the `DGVector` one i.e., https://github.com/nextsimhub/nextsimdg/blob/2165b00b858f7c15139b8c5a3ed581c942a568d8/core/src/include/Halo.hpp#L59 This is incorrect and leads to a segmentation fault. I have now modified the constructor to use the Member Initializer List, which will ensure the object is stored correctly https://github.com/nextsimhub/nextsimdg/blob/6f337fb12867865fb1a1842c64f8949098b6395b/core/src/include/Halo.hpp#L60-L64 I added a test to check it works correctly, and I have fixed a small typo in the naming of `initializeHaloMetadata` (which was `intializeHaloMetadata` before) --- # Test Description I added `MPI_TEST_CASE("DGVectorHolder", 3)` to `HaloExchangeCB_test.cpp` to check that halo exchange works correctly for `DGVectorHolder`
2 parents 2165b00 + 6f337fb commit f53d8c6

File tree

3 files changed

+62
-7
lines changed

3 files changed

+62
-7
lines changed

core/src/Halo.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -50,7 +50,7 @@ void Halo::setSpatialDims()
5050
}
5151
}
5252

53-
void Halo::intializeHaloMetadata()
53+
void Halo::initializeHaloMetadata()
5454
{
5555
// number of halo cells (should be general for any halo width)
5656
if (not isCG) {

core/src/include/Halo.hpp

Lines changed: 11 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,8 @@
77
*
88
* All functionality for halo exchange between MPI ranks is contained in this class.
99
*
10-
* Halo supports the main data structures of NextSim e.g., ModelArray, DGVector and CGVector.
10+
* Halo supports the main data structures of NextSim e.g., ModelArray, DGVector, DGVectorHolder and
11+
* CGVector.
1112
*
1213
* The halos are exchange via one-sided MPI communication using Remote Memory Access (RMA).
1314
*/
@@ -49,14 +50,18 @@ class Halo {
4950
m_numComps = ma.nComponents();
5051
isVertex = ma.getType() == ModelArray::Type::VERTEX;
5152
setSpatialDims();
52-
intializeHaloMetadata();
53+
initializeHaloMetadata();
5354
}
5455

5556
/*!
5657
* @brief Constructs a halo object from DGVectorHolder
5758
* @param dgvh DGVectorHolder object to create halo from
5859
*/
59-
template <int N> Halo(DGVectorHolder<N>& dgvh) { Halo(static_cast<DGVector<N>&>(dgvh)); }
60+
template <int N>
61+
Halo(DGVectorHolder<N>& dgvh)
62+
: Halo(static_cast<DGVector<N>&>(dgvh))
63+
{
64+
}
6065

6166
/*!
6267
* @brief Constructs a halo object from DGVector
@@ -66,7 +71,7 @@ class Halo {
6671
{
6772
m_numComps = N;
6873
setSpatialDims();
69-
intializeHaloMetadata();
74+
initializeHaloMetadata();
7075
}
7176

7277
/*!
@@ -80,7 +85,7 @@ class Halo {
8085
CGdegree = N;
8186
nCells = CGdegree;
8287
setSpatialDims();
83-
intializeHaloMetadata();
88+
initializeHaloMetadata();
8489
}
8590

8691
static const size_t haloWidth = HALOWIDTH; // how many cells wide is the halo region
@@ -128,7 +133,7 @@ class Halo {
128133
* - Defines edge lengths
129134
* - Sets up inner and outer slices for Bottom, Right, Top, Left edges
130135
*/
131-
void intializeHaloMetadata();
136+
void initializeHaloMetadata();
132137

133138
/**
134139
* @brief Return true if the provided edge is vertical (LEFT or RIGHT).

core/test/HaloExchangeCB_test.cpp

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@
88
* - VertexField
99
* - DGField
1010
* - DGVector
11+
* - DGVectorHolder
1112
* - CGVector
1213
*/
1314

@@ -19,6 +20,7 @@
1920
#include "include/DGModelArray.hpp"
2021
#include "include/Halo.hpp"
2122
#include "include/Interpolations.hpp"
23+
#include "include/dgVectorHolder.hpp"
2224

2325
namespace Nextsim {
2426

@@ -220,6 +222,54 @@ MPI_TEST_CASE("DGVector", 3)
220222
verifyTestData(testData, localNx, localNy, offsetX, offsetY, nx, ny);
221223
}
222224

225+
MPI_TEST_CASE("DGVectorHolder", 3)
226+
{
227+
auto& modelMPI = ModelMPI::getInstance();
228+
auto& metadata = ModelMetadata::getInstance();
229+
230+
const size_t nx = metadata.getGlobalExtentX();
231+
const size_t ny = metadata.getGlobalExtentY();
232+
const size_t localNx = metadata.getLocalExtentX() + 2 * Halo::haloWidth;
233+
const size_t localNy = metadata.getLocalExtentY() + 2 * Halo::haloWidth;
234+
const size_t offsetX = metadata.getLocalCornerX();
235+
const size_t offsetY = metadata.getLocalCornerY();
236+
237+
ModelArray::setDimension(ModelArray::Dimension::X, nx, localNx, offsetX);
238+
ModelArray::setDimension(ModelArray::Dimension::Y, ny, localNy, offsetY);
239+
ModelArray::setNComponents(ModelArray::Type::DG, DG);
240+
241+
ParametricMesh smesh(CARTESIAN);
242+
smesh.nx = localNx;
243+
smesh.ny = localNy;
244+
smesh.nelements = localNx * localNy;
245+
smesh.nnodes = (localNx + 1) * (localNy + 1);
246+
smesh.vertices.resize(smesh.nnodes, Eigen::NoChange);
247+
for (size_t i = 0; i < localNx + 1; ++i) {
248+
for (size_t j = 0; j < localNy + 1; ++j) {
249+
smesh.vertices(i * (localNy + 1) + j, 0) = i;
250+
smesh.vertices(i * (localNy + 1) + j, 1) = j;
251+
}
252+
}
253+
254+
// create example DGVector
255+
DGVector<DG> testData(smesh);
256+
testData.zero();
257+
258+
DGVectorHolder<DG> testHolder;
259+
testHolder = DGVectorHolder<DG>(testData);
260+
261+
// create halo for testData model array
262+
Halo halo(testHolder);
263+
264+
initializeTestData(static_cast<DGVector<DG>&>(testHolder), localNx, localNy, offsetX, offsetY);
265+
266+
// exchange halos
267+
halo.exchangeHalos(static_cast<DGVector<DG>&>(testHolder));
268+
269+
verifyTestData(
270+
static_cast<DGVector<DG>&>(testHolder), localNx, localNy, offsetX, offsetY, nx, ny);
271+
}
272+
223273
MPI_TEST_CASE("CGVector", 3)
224274
{
225275
auto& modelMPI = ModelMPI::getInstance();

0 commit comments

Comments
 (0)