Skip to content
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.

Sign up for GitHub

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

Jump to bottom

WIP: MPI-ify the dynamics #663

Open
wants to merge 1 commit into
base: slicer
Choose a base branch
from
Open

WIP: MPI-ify the dynamics #663

wants to merge 1 commit into from

Conversation

TomMelt
Copy link
Contributor

@TomMelt TomMelt commented Aug 23, 2024 •
edited
Loading

Fixes #120

Change Description

To parallelize the dynamics we first need the halo region information for each process.

This information is located in the metadata file produced by the domain decomp tool.

This PR updates ModelMetadata::getPartitionMetadata with all the necessary information on halo regions.

Test Description

This PR adds two new tests to check the functionality of the getPartitionMetadata method and the halo exchange.

  • ModelMetadata_test.cpp - test getPartitionMetadata can correctly load metadata from a partition metadata file into the appropriate data structure e.g., neighbourExtents, neighbourHaloStarts etc.
  • HaloExchange_test.cpp - check that we can do basic halo exchange by making use of the new slicing utilities e.g., ModelArraySlice Add halo exchange logic #744

Pre-Request Checklist

  • Add methods to read halo exchange info into metadata class
  • Add halo exchange info to DGModelArray class
  • Add exchange logic to DGModelArray.hpp
  • Add option of in-/out-flows in the halo? or should this be separate?

@TomMelt TomMelt self-assigned this Aug 23, 2024
@TomMelt TomMelt added enhancement New feature or request ICCS Tasks or reviews for the ICCS team labels Aug 23, 2024
@TomMelt TomMelt added this to the 3 Stand-alone model milestone Aug 23, 2024
@TomMelt TomMelt force-pushed the melt-mpi-metadata branch 2 times, most recently from fdb3f12 to a382d9f Compare November 7, 2024 17:04
@TomMelt TomMelt changed the base branch from develop to slicer November 25, 2024 12:20
@nextsimhub nextsimhub deleted a comment from coderabbitai bot Nov 25, 2024
@TomMelt
feat: added metadata info on neighbours
63e6f77 
add metadata info for periodic and non-periodic neighbours i.e., halo
sizes, starting indices etc.

add an associated unit test to check values are read correctly from
file.
@TomMelt TomMelt marked this pull request as ready for review December 4, 2024 12:55
jwallwork23
jwallwork23 requested changes Dec 4, 2024
View reviewed changes
Copy link
Contributor

@jwallwork23 jwallwork23 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, thanks @TomMelt. I drew a diagram and convinced myself that the numbers in the CDL files are consistent with the expected values in the tests. I have a couple of suggestions, the main one being a bit of refactoring to avoid duplication.

core/test/ModelMetadata_test.cpp
Comment on lines +71 to +100
// this metadata should be identical to the Closed Boundary version so we check it again
if (test_rank == 0) {
REQUIRE(metadata.neighbourRanks[ModelMetadata::LEFT].size() == 0);
REQUIRE(metadata.neighbourRanks[ModelMetadata::RIGHT] == std::vector<int> { 2 });
REQUIRE(metadata.neighbourExtents[ModelMetadata::RIGHT] == std::vector<int> { 4 });
REQUIRE(metadata.neighbourHaloStarts[ModelMetadata::RIGHT] == std::vector<int> { 0 });
REQUIRE(metadata.neighbourRanks[ModelMetadata::BOTTOM].size() == 0);
REQUIRE(metadata.neighbourRanks[ModelMetadata::TOP] == std::vector<int> { 1 });
REQUIRE(metadata.neighbourExtents[ModelMetadata::TOP] == std::vector<int> { 7 });
REQUIRE(metadata.neighbourHaloStarts[ModelMetadata::TOP] == std::vector<int> { 0 });
} else if (test_rank == 1) {
REQUIRE(metadata.neighbourRanks[ModelMetadata::LEFT].size() == 0);
REQUIRE(metadata.neighbourRanks[ModelMetadata::RIGHT] == std::vector<int> { 2 });
REQUIRE(metadata.neighbourExtents[ModelMetadata::RIGHT] == std::vector<int> { 5 });
REQUIRE(metadata.neighbourHaloStarts[ModelMetadata::RIGHT] == std::vector<int> { 12 });
REQUIRE(metadata.neighbourRanks[ModelMetadata::BOTTOM] == std::vector<int> { 0 });
REQUIRE(metadata.neighbourExtents[ModelMetadata::BOTTOM] == std::vector<int> { 7 });
REQUIRE(metadata.neighbourHaloStarts[ModelMetadata::BOTTOM] == std::vector<int> { 21 });
REQUIRE(metadata.neighbourRanks[ModelMetadata::TOP].size() == 0);
} else if (test_rank == 2) {
REQUIRE(metadata.neighbourRanks[ModelMetadata::LEFT] == std::vector<int> { 0, 1 });
REQUIRE(metadata.neighbourExtents[ModelMetadata::LEFT] == std::vector<int> { 4, 5 });
REQUIRE(metadata.neighbourHaloStarts[ModelMetadata::LEFT] == std::vector<int> { 6, 6 });
REQUIRE(metadata.neighbourRanks[ModelMetadata::RIGHT].size() == 0);
REQUIRE(metadata.neighbourRanks[ModelMetadata::BOTTOM].size() == 0);
REQUIRE(metadata.neighbourRanks[ModelMetadata::TOP].size() == 0);
} else {
std::cerr << "only valid for 3 ranks" << std::endl;
exit(1);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this common part of the tests be pulled out into a function that's called in both places? It'd avoid duplication and reduce the likelihood of human errors when copy/pasting and reviewing.

core/test/ModelMetadata_test.cpp

// this metadata is specific to the periodic boundary conditions
if (test_rank == 0) {
// clang-format off
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm guessing this is because of line length? Might be sufficient to put using namespace std; at the top.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or define a typedef (or using)

core/test/ModelMetadata_test.cpp
Comment on lines +14 to +16
const std::string filename = testFilesDir + "/paraGrid_test.nc";
const std::string diagFile = "paraGrid_diag.nc";
const std::string dateString = "2000-01-01T00:00:00Z";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unused?

timspainNERSC
timspainNERSC requested changes Dec 5, 2024
View reviewed changes
Copy link
Collaborator

@timspainNERSC timspainNERSC left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple of minor edits.

core/src/ModelMetadata.cpp
Comment on lines +50 to +51
size_t nStart {}; // start point in metadata arrays
size_t count {}; // number of elements to read from metadata arrays
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Initialize numeric types using = 0, if indeed that is necessary (nStart and count seem to be set to other values before being read from).

core/test/ModelMetadata_test.cpp

// this metadata is specific to the periodic boundary conditions
if (test_rank == 0) {
// clang-format off
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or define a typedef (or using)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@timspainNERSC timspainNERSC timspainNERSC requested changes

@jwallwork23 jwallwork23 jwallwork23 requested changes

@einola einola Awaiting requested review from einola

Assignees

@TomMelt TomMelt

Labels
enhancement New feature or request ICCS Tasks or reviews for the ICCS team
Projects
neXtSIM_DG overview
Status: Review required
Milestone
3 Stand-alone model
Development

Successfully merging this pull request may close these issues.

MPI parallelization of dynamics
3 participants
@TomMelt @jwallwork23 @timspainNERSC