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.

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

Add support for partitioning with PetscSF #14736

Merged
merged 2 commits into from Feb 9, 2023

Conversation

stefanozampini
Copy link
Contributor

@stefanozampini stefanozampini commented Jan 26, 2023

This implements a backend for partitioning based on the PetscSF object https://arxiv.org/abs/2102.13018

It comes in two different flavors:

  • PETScWrappers::CommunicationPattern is an implementation of Utilities::MPI::NoncontiguousPartitioner
  • PETScWrappers::Partitioner is an implementation of Utilities::MPI::Partitioner

There are a few differences wrt the dealII classes:

  • API based: PETSc does not need explicit specification of the communication channels, and the user passing MPI_Requests and temporary storage
  • the import_from_ghosted does not explicitly zero the ghost data after communication is performed and its signature is thus ArrayView<const ...> (I don't see what is the point of zeroing the ghost data within the internal API when users can do that themselves if needed)

Support for device memory can be easily added by just adding an extra template argument on the memory type of the ArrayView arguments, since PetscSF already support device memory pointers

Tests are copy-pasted from already existing tests.

@bangerth
Copy link
Member

Ah, the famous star-forest finally makes an appearance :-)

Copy link
Member

@bangerth bangerth left a comment

Choose a reason for hiding this comment

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

Here are some comment.

I don't know my way around the partitioners very well. It would be nice if @kronbichler or @peterrum would take another look at this, but I think most of it is pretty clear at least as far as the interface is concerned. Thanks for adding all of these tests!

include/deal.II/lac/petsc_communication_pattern.h Outdated Show resolved Hide resolved
include/deal.II/lac/petsc_communication_pattern.h Outdated Show resolved Hide resolved
include/deal.II/lac/petsc_communication_pattern.h Outdated Show resolved Hide resolved
source/lac/petsc_communication_pattern.cc Show resolved Hide resolved
Comment on lines +65 to +68
const PetscInt *idxs;
PetscInt n;
IS is = want.make_petsc_is(communicator);
AssertPETSc(ISGetLocalSize(is, &n));
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand this code. The IS object so created just gets a bunch of indices on every process, but no indication which of these are locally owned. How does it determine which ones of these are local?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

PetscSFSetGraphLayout(sf, layout, n, nullptr, PETSC_OWN_POINTER, idxs)) does this for you.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using IS is superfluous here (I could have used a std::vector<PetscInt> instead), it is only to test the new functionality in IndexSet

Copy link
Member

Choose a reason for hiding this comment

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

Still, I don't know what is happening in the three lines 66-68. You create an index set from a bunch of indices, and then ISGetLocalSize returns how many of these are stored on the current process: https://petsc.org/main/docs/manualpages/IS/ISGetLocalSize/ How does it know? Does make_petsc_is() internally partition indices into locally owned and ghost ones?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the local size corresponds to the number of indices stored in the array, independent of their actual distribution.
IS does not know anything about global indices distribution, it is merely a class around an array of indices.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So the indices returned are passed as "global indices I would like to receive" to the PetscSFSetGraphLayout call

source/lac/petsc_communication_pattern.cc Outdated Show resolved Hide resolved
source/lac/petsc_communication_pattern.cc Outdated Show resolved Hide resolved
source/lac/petsc_communication_pattern.cc Outdated Show resolved Hide resolved
source/lac/petsc_communication_pattern.cc Show resolved Hide resolved
source/lac/petsc_communication_pattern.cc Show resolved Hide resolved
@bangerth
Copy link
Member

Let me know once you've got it in a state where you want me to take another look!

@stefanozampini
Copy link
Contributor Author

@bangerth I think I have addressed your comments. Please take a look.
I have decided to implement get_mpi_communicator with a static variable for the moment, since otherwise I would have to change all the implementations of the partitioners. I will do all the get_mpi_communicator changes in a single PR

@stefanozampini
Copy link
Contributor Author

/rebuild

Copy link
Member

@bangerth bangerth left a comment

Choose a reason for hiding this comment

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

My apologies for not getting to this faster. This looks good now, many thanks!

Out of curiosity, why did you implement this instead of just using the existing classes that do the same thing? Are your classes substantially faster?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants