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

Refactor MeshBlock Partitioning #1119

Merged
merged 37 commits into from
Jun 26, 2024
Merged

Conversation

lroberts36
Copy link
Collaborator

@lroberts36 lroberts36 commented Jun 18, 2024

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

  • Code passes cpplint
  • New features are documented.
  • Adds a test for any bugs fixed. Adds tests for new features.
  • Code is formatted
  • Changes are summarized in CHANGELOG.md
  • Change is breaking (API, behavior, ...)
    • Change is additionally added to CHANGELOG.md in the breaking section
    • PR is marked as breaking
    • Short summary API changes at the top of the PR (plus optionally with an automated update/fix script)
  • CI has been triggered on Darwin for performance regression tests.
  • Docs build
  • (@lanl.gov employees) Update copyright on changed files

@lroberts36 lroberts36 changed the title WIP: Refactor MeshBlock Partitioning Refactor MeshBlock Partitioning Jun 19, 2024
@lroberts36 lroberts36 requested a review from bprather June 20, 2024 16:44
Copy link
Collaborator

@Yurlungur Yurlungur left a comment

Choose a reason for hiding this comment

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

Overall I like this. Two big picture questions:

  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)?
  2. It's a bit weird that Get is sometimes Get and sometimes GetOrAdd. Also weird that a partition index must always requested.

src/bvals/comms/bvals_utils.hpp Show resolved Hide resolved
src/driver/driver.cpp Show resolved Hide resolved
src/mesh/meshblock.hpp Outdated Show resolved Hide resolved
src/mesh/mesh.hpp Show resolved Hide resolved
src/mesh/mesh.hpp Outdated Show resolved Hide resolved
src/interface/mesh_data.hpp Show resolved Hide resolved
@lroberts36
Copy link
Collaborator Author

lroberts36 commented Jun 24, 2024

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
Copy link
Collaborator

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.

👍

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
Copy link
Collaborator Author

  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.

Comment on lines 114 to 111
if (c->first != "base") {
// Remove anything that doesn't contain the label base
if (c->first.find("base") == std::string::npos) {
Copy link
Collaborator Author

@lroberts36 lroberts36 Jun 25, 2024

Choose a reason for hiding this comment

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

This change made the tests break...

Copy link
Collaborator

@pdmullen pdmullen left a comment

Choose a reason for hiding this comment

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

LGTM! Tested downstream (without iterative tasklists, without changing GetOrAdd -> pmesh->Add) and all looked good.

example/advection/advection_driver.cpp Show resolved Hide resolved
example/advection/advection_driver.cpp Show resolved Hide resolved
example/advection/advection_driver.cpp Show resolved Hide resolved
src/driver/driver.cpp Show resolved Hide resolved
src/mesh/mesh-amr_loadbalance.cpp Outdated Show resolved Hide resolved
src/mesh/mesh.cpp Outdated Show resolved Hide resolved
@lroberts36 lroberts36 enabled auto-merge June 25, 2024 16:56
@lroberts36 lroberts36 disabled auto-merge June 26, 2024 00:19
@lroberts36 lroberts36 enabled auto-merge June 26, 2024 00:19
@lroberts36 lroberts36 merged commit f7a1908 into develop Jun 26, 2024
50 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Formalizing MeshBlock partitions for creating MeshData
3 participants