parthenon-hpc-lab / parthenon

Parthenon AMR infrastructure
https://parthenon-hpc-lab.github.io/parthenon/
Other
105 stars 33 forks source link

Refactor `MeshBlock` Partitioning #1119

Closed lroberts36 closed 1 week ago

lroberts36 commented 2 weeks ago

PR Summary

This PR adds an object BlockListPartition that stores a BlockList_t and a little more information about a partition of blocks. We then have the analogy BlockListPartition is to MeshData as MeshBlock is to MeshBlockData. Then instead of doing block list partitioning inside of DataCollection, partitioning is moved to Mesh (which at least to me is a more reasonable location for this to live) and methods for getting the partitions are added to Mesh. MeshData gains an Initialize method that takes a BlockListPartition. As a result, all incarnations of DataCollection<MeshData>::Add are made to "just work" for MeshData when it is passed either another MeshData object or a BlockListPartition object. DataCollection<MeshData>::GetOrAdd is deprecated (although still available, it just calls Add internally). I have also removed GetOrAdd from Parthenon internals and from the examples.

This nearly gets us to working multigrid for multiple partitions, aside from the task bug described in #1125 (at least I hope).

This should close issue #1097.

PR Checklist

lroberts36 commented 1 week ago

Overall I like this. Two big picture questions:

Does this now preclude different partitioning for different parts of a task list? Does it also preclude packs over different subsets of blocks (for example different refinement levels)?

No, you can build BlockListPartitions however you might wish and multiple partitions could contain partially overlapping block lists. This PR provides no functionality for anything beyond what the preceding code did in DataCollection for partitioning, it just moves it to Mesh. That being said, different functionality could be added to mesh or downstream codes could build BlockListPartitions themselves based on whatever criteria they desire. The main change here is that partitioning is decoupled from DataCollection.

It's a bit weird that Get is sometimes Get and sometimes GetOrAdd. Also weird that a partition index must always requested.

DataCollection<T>::GetOrAdd was only supported previously for T = MeshData and it took a partition index (since partitioning was done internally in DataCollection). This PR deprecates that function, but I did not remove it since it would break just about every code downstream (and the old version is what requires a partition index, which I agree is not very robust). DataCollection<T>::Get should just get a container only if it exists.

What is confusing is that DataCollection<T>::Add really gets or adds the requested container (depending on if it is already in the map). I would support changing the name, but that would be a breaking change since Add is used for MeshBlockData in downstream codes.

Yurlungur commented 1 week ago

No, you can build BlockListPartitions however you might wish and multiple partitions could contain partially overlapping block lists. This PR provides no functionality for anything beyond what the preceding code did in DataCollection for partitioning, it just moves it to Mesh. That being said, different functionality could be added to mesh or downstream codes could build BlockListPartitions themselves based on whatever criteria they desire. The main change here is that partitioning is decoupled from DataCollection.

:+1:

What is confusing is that DataCollection<T>::Add really gets or adds the requested container (depending on if it is already in the map). I would support changing the name, but that would be a breaking change since Add is used for MeshBlockData in downstream codes.

Yeah I feel ambivalent about this. I think changing the name would be better but not sure it's worth breaking backwards compatibility. Maybe we table that change for a later PR and poll if people think it's worth doing.

lroberts36 commented 1 week ago
  1. Does this now preclude different partitioning for different parts of a task list? Does it also preclude packs over different subsets of blocks (for example different refinement levels)?

I wasn't thinking about the map key when I responded to this earlier, which did rely on the partition id. I now made the key just contain a list of the gids of all blocks in the partition. This should allow arbitrary partitioning without having to make any changes to Mesh.